diff mbox series

[v2] Handle private COMDAT function symbol reference in readonly data section

Message ID 20240131022136.572689-1-hjl.tools@gmail.com
State New
Headers show
Series [v2] Handle private COMDAT function symbol reference in readonly data section | expand

Commit Message

H.J. Lu Jan. 31, 2024, 2:21 a.m. UTC
Changes in v2:

1. Check decl non-null before dereferencing it.
2. Update PR rtl-optimization/113617 from

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113617#c14

---
For a private COMDAT function symbol reference in readonly data section,
instead of putting it in .data.rel.ro or .rodata.cst section, call
function_rodata_section to get the read-only or relocated read-only
data section associated with the function DECL so that the COMDAT
section will be used for the private COMDAT function symbol.

gcc/

	PR rtl-optimization/113617
	* varasm.cc (default_elf_select_rtx_section): Call
	function_rodata_section to get the read-only or relocated
	read-only data section for private COMDAT function symbol
	reference.

gcc/testsuite/

	PR rtl-optimization/113617
	* g++.dg/pr113617-1a.C: New test.
	* g++.dg/pr113617-1b.C: Likewise.
---
 gcc/testsuite/g++.dg/pr113617-1a.C | 145 +++++++++++++++++++++++++++++
 gcc/testsuite/g++.dg/pr113617-1b.C |   8 ++
 gcc/varasm.cc                      |  30 ++++++
 3 files changed, 183 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/pr113617-1a.C
 create mode 100644 gcc/testsuite/g++.dg/pr113617-1b.C

Comments

Jakub Jelinek Jan. 31, 2024, 4:30 p.m. UTC | #1
On Tue, Jan 30, 2024 at 06:21:36PM -0800, H.J. Lu wrote:
> Changes in v2:
> 
> 1. Check decl non-null before dereferencing it.
> 2. Update PR rtl-optimization/113617 from

Thanks for updating the testcase.

> --- a/gcc/varasm.cc
> +++ b/gcc/varasm.cc
> @@ -7459,16 +7459,46 @@ default_elf_select_rtx_section (machine_mode mode, rtx x,
>  {
>    int reloc = compute_reloc_for_rtx (x);
>  
> +  tree decl = nullptr;
> +
> +  /* If it is a private COMDAT function symbol reference, call
> +     function_rodata_section for the read-only or relocated read-only
> +     data section associated with function DECL so that the COMDAT
> +     section will be used for the private COMDAT function symbol.  */
> +  if (HAVE_COMDAT_GROUP)
> +    {
> +      if (GET_CODE (x) == CONST
> +	  && GET_CODE (XEXP (x, 0)) == PLUS
> +	  && CONST_INT_P (XEXP (XEXP (x, 0), 1)))
> +	x = XEXP (XEXP (x, 0), 0);
> +
> +      if (GET_CODE (x) == SYMBOL_REF)
> +	{
> +	  decl = SYMBOL_REF_DECL (x);
> +	  if (decl
> +	      && (TREE_CODE (decl) != FUNCTION_DECL
> +		  || !DECL_COMDAT_GROUP (decl)
> +		  || TREE_PUBLIC (decl)))
> +	    decl = nullptr;
> +	}
> +    }
> +
>    /* ??? Handle small data here somehow.  */
>  
>    if (reloc & targetm.asm_out.reloc_rw_mask ())
>      {
> +      if (decl)
> +	return targetm.asm_out.function_rodata_section (decl, true);

As I wrote before, I still very much dislike this.
We want to refer to the
_ZN1R1BIFvvENS_1IIFPFvPvxxxEPN2N12N22N31CIN12_GLOBAL__N_11DIxEELb0EEExxxEEEE1FERNS_1HERKSI_NS_1GE
private symbol defined in
.text._ZN1R1BIFvvENS_1IIFPFvPvxxxEPN2N12N22N31CIN12_GLOBAL__N_11DIxEELb0EEExxxEEEE1FERNS_1HERKSI_NS_1GE
section in _ZN1AIxE3fooExx comdat group from some readonly data
memory, and read that from
_ZN2N12N22N31XILi1EE3booINS1_1CIN12_GLOBAL__N_11DIxEELb0EEEEEvxxxRT_ function
defined in .text._ZN2N12N22N31XILi1EE3booINS1_1CIN12_GLOBAL__N_11DIxEELb0EEEEEvxxxRT_
section in the same comdat group.

The patch puts that into
.data.rel.ro.local._ZN1R1BIFvvENS_1IIFPFvPvxxxEPN2N12N22N31CIN12_GLOBAL__N_11DIxEELb0EEExxxEEEE1FERNS_1HERKSI_NS_1GE
section in the same comdat group, but that just looks weird and for
targets which use section anchors also inefficient.

If we have a shared constant pool (otherwise the constants would be emitted
into a per-function constant pool of that
_ZN2N12N22N31XILi1EE3booINS1_1CIN12_GLOBAL__N_11DIxEELb0EEEEEvxxxRT_
function and would live in something based on that function name.
But in case it is shared, it is normally just .data.rel.ro.local or
.data.rel.ro section, shared by whatever refers to it.
These comdat private symbols are kind of exception, they can still be
shared, but have to be shared only within the containing comdat group
because it isn't valid to refer to them from other comdat groups.
So, it is ok if say two different functions in the same comdat group
actually share those MEM constants.
Thus, I think for the DECL_COMDAT_GROUP (decl) && HAVE_COMDAT_GROUP
case it would be best to make it clear in the section name that it
is a .data.rel.ro.local or .data.rel.ro section shared by everything
in the comdat group.  So, shouldn't it be just
	.section	.data.rel.ro.local,"awG",@progbits,_ZN1AIxE3fooExx,comdat
and emit that directly in this function rather than using
targetm.asm_out.function_rodata_section?
Looking at targetm.asm_out.function_rodata_section, it is
default_function_rodata_section on most targets, then on darwin,
cygwin, AIX and mcore default_no_function_rodata_section which just
returns the shared readonly_data_section (I hope those targets don't
DECL_COMDAT_GROUP (decl) && HAVE_COMDAT_GROUP, otherwise it will simply not
work) and then loongarch does some ugly magic (which is related to
jumptables and so nothing we need to care about here hopefully).

Another question is if we need to do anything about the
DECL_COMDAT_GROUP (decl) && DECL_SECTION_NAME (decl)
&& startswith (DECL_SECTION_NAME (decl), ".gnu.linkonce.t.")
case (older linkers) (i.e. when using years old GNU linkers).

	Jakub
H.J. Lu Jan. 31, 2024, 4:48 p.m. UTC | #2
On Wed, Jan 31, 2024 at 8:30 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Jan 30, 2024 at 06:21:36PM -0800, H.J. Lu wrote:
> > Changes in v2:
> >
> > 1. Check decl non-null before dereferencing it.
> > 2. Update PR rtl-optimization/113617 from
>
> Thanks for updating the testcase.
>
> > --- a/gcc/varasm.cc
> > +++ b/gcc/varasm.cc
> > @@ -7459,16 +7459,46 @@ default_elf_select_rtx_section (machine_mode mode, rtx x,
> >  {
> >    int reloc = compute_reloc_for_rtx (x);
> >
> > +  tree decl = nullptr;
> > +
> > +  /* If it is a private COMDAT function symbol reference, call
> > +     function_rodata_section for the read-only or relocated read-only
> > +     data section associated with function DECL so that the COMDAT
> > +     section will be used for the private COMDAT function symbol.  */
> > +  if (HAVE_COMDAT_GROUP)
> > +    {
> > +      if (GET_CODE (x) == CONST
> > +       && GET_CODE (XEXP (x, 0)) == PLUS
> > +       && CONST_INT_P (XEXP (XEXP (x, 0), 1)))
> > +     x = XEXP (XEXP (x, 0), 0);
> > +
> > +      if (GET_CODE (x) == SYMBOL_REF)
> > +     {
> > +       decl = SYMBOL_REF_DECL (x);
> > +       if (decl
> > +           && (TREE_CODE (decl) != FUNCTION_DECL
> > +               || !DECL_COMDAT_GROUP (decl)
> > +               || TREE_PUBLIC (decl)))
> > +         decl = nullptr;
> > +     }
> > +    }
> > +
> >    /* ??? Handle small data here somehow.  */
> >
> >    if (reloc & targetm.asm_out.reloc_rw_mask ())
> >      {
> > +      if (decl)
> > +     return targetm.asm_out.function_rodata_section (decl, true);
>
> As I wrote before, I still very much dislike this.
> We want to refer to the
> _ZN1R1BIFvvENS_1IIFPFvPvxxxEPN2N12N22N31CIN12_GLOBAL__N_11DIxEELb0EEExxxEEEE1FERNS_1HERKSI_NS_1GE
> private symbol defined in
> .text._ZN1R1BIFvvENS_1IIFPFvPvxxxEPN2N12N22N31CIN12_GLOBAL__N_11DIxEELb0EEExxxEEEE1FERNS_1HERKSI_NS_1GE
> section in _ZN1AIxE3fooExx comdat group from some readonly data
> memory, and read that from
> _ZN2N12N22N31XILi1EE3booINS1_1CIN12_GLOBAL__N_11DIxEELb0EEEEEvxxxRT_ function
> defined in .text._ZN2N12N22N31XILi1EE3booINS1_1CIN12_GLOBAL__N_11DIxEELb0EEEEEvxxxRT_
> section in the same comdat group.
>
> The patch puts that into
> .data.rel.ro.local._ZN1R1BIFvvENS_1IIFPFvPvxxxEPN2N12N22N31CIN12_GLOBAL__N_11DIxEELb0EEExxxEEEE1FERNS_1HERKSI_NS_1GE
> section in the same comdat group, but that just looks weird and for
> targets which use section anchors also inefficient.
>
> If we have a shared constant pool (otherwise the constants would be emitted
> into a per-function constant pool of that
> _ZN2N12N22N31XILi1EE3booINS1_1CIN12_GLOBAL__N_11DIxEELb0EEEEEvxxxRT_
> function and would live in something based on that function name.
> But in case it is shared, it is normally just .data.rel.ro.local or
> .data.rel.ro section, shared by whatever refers to it.
> These comdat private symbols are kind of exception, they can still be
> shared, but have to be shared only within the containing comdat group
> because it isn't valid to refer to them from other comdat groups.
> So, it is ok if say two different functions in the same comdat group
> actually share those MEM constants.
> Thus, I think for the DECL_COMDAT_GROUP (decl) && HAVE_COMDAT_GROUP
> case it would be best to make it clear in the section name that it
> is a .data.rel.ro.local or .data.rel.ro section shared by everything
> in the comdat group.  So, shouldn't it be just
>         .section        .data.rel.ro.local,"awG",@progbits,_ZN1AIxE3fooExx,comdat
> and emit that directly in this function rather than using
> targetm.asm_out.function_rodata_section?

Which function (target hook) can I use to generate

 .section        .data.rel.ro.local,"awG",@progbits,_ZN1AIxE3fooExx,comdat

> Looking at targetm.asm_out.function_rodata_section, it is
> default_function_rodata_section on most targets, then on darwin,
> cygwin, AIX and mcore default_no_function_rodata_section which just
> returns the shared readonly_data_section (I hope those targets don't
> DECL_COMDAT_GROUP (decl) && HAVE_COMDAT_GROUP, otherwise it will simply not
> work) and then loongarch does some ugly magic (which is related to
> jumptables and so nothing we need to care about here hopefully).
>
> Another question is if we need to do anything about the
> DECL_COMDAT_GROUP (decl) && DECL_SECTION_NAME (decl)
> && startswith (DECL_SECTION_NAME (decl), ".gnu.linkonce.t.")
> case (older linkers) (i.e. when using years old GNU linkers).
>

Should we support such targets? It is not easy for me to test it.

Thanks.
Jakub Jelinek Jan. 31, 2024, 5:09 p.m. UTC | #3
On Wed, Jan 31, 2024 at 08:48:33AM -0800, H.J. Lu wrote:
> Which function (target hook) can I use to generate
> 
>  .section        .data.rel.ro.local,"awG",@progbits,_ZN1AIxE3fooExx,comdat

Just
      if (decl)
	return get_section (reloc == 1
			    ? ".data.rel.ro.local" : ".data.rel.ro",
			    SECTION_WRITE | SECTION_RELRO | SECTION_LINKONCE,
			    decl);
for the first hunk and
  else if (decl)
    return get_section (".rodata", SECTION_LINKONCE, decl);
in the second case?

Haven't tried it though, maybe the get_section section conflict stuff
isn't able to handle it (and perhaps that is the reason why we simply don't
emit functions into
	.section	.text,"axG",@progbits,whatever,comdat
sections instead of
	.section	.text.function_name,"axG",@progbits,whatever,comdat
In such case, we could append something to those section names,
like const.pool (or whatever else that couldn't clash with function names,
so needs probably a dot somewhere).  Could be
.data.rel.ro.local.const.pool
.data.rel.ro.const.pool
or
.data.rel.ro.local..shared
.data.rel.ro..shared
or something similar, but .data.rel.ro.local.shared would not be ok,
because it could clash with .data.rel.ro.local section for shared function.
    
> > Another question is if we need to do anything about the
> > DECL_COMDAT_GROUP (decl) && DECL_SECTION_NAME (decl)
> > && startswith (DECL_SECTION_NAME (decl), ".gnu.linkonce.t.")
> > case (older linkers) (i.e. when using years old GNU linkers).
> >
> 
> Should we support such targets? It is not easy for me to test it.

Perhaps let's wait if somebody files an issue with such configuration.

	Jakub
H.J. Lu Jan. 31, 2024, 5:39 p.m. UTC | #4
On Wed, Jan 31, 2024 at 9:10 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Jan 31, 2024 at 08:48:33AM -0800, H.J. Lu wrote:
> > Which function (target hook) can I use to generate
> >
> >  .section        .data.rel.ro.local,"awG",@progbits,_ZN1AIxE3fooExx,comdat
>
> Just
>       if (decl)
>         return get_section (reloc == 1
>                             ? ".data.rel.ro.local" : ".data.rel.ro",
>                             SECTION_WRITE | SECTION_RELRO | SECTION_LINKONCE,
>                             decl);
> for the first hunk and
>   else if (decl)
>     return get_section (".rodata", SECTION_LINKONCE, decl);
> in the second case?
>
> Haven't tried it though, maybe the get_section section conflict stuff
> isn't able to handle it (and perhaps that is the reason why we simply don't
> emit functions into
>         .section        .text,"axG",@progbits,whatever,comdat
> sections instead of
>         .section        .text.function_name,"axG",@progbits,whatever,comdat

GNU binutils has no issues with it:

[hjl@gnu-cfl-3 tmp]$ cat x.s
.section .text,"axG",@progbits,whatever,comdat
nop
.text
nop
[hjl@gnu-cfl-3 tmp]$ gcc -c x.s
[hjl@gnu-cfl-3 tmp]$ readelf -SW x.o | grep text
  [ 2] .text             PROGBITS        0000000000000000 000048
000001 00  AX  0   0  1
  [ 5] .text             PROGBITS        0000000000000000 000049
000001 00 AXG  0   0  1
[hjl@gnu-cfl-3 tmp]$

If it doesn't work for some targets, we can use
targetm.asm_out.function_rodata_section.

> In such case, we could append something to those section names,
> like const.pool (or whatever else that couldn't clash with function names,
> so needs probably a dot somewhere).  Could be
> .data.rel.ro.local.const.pool
> .data.rel.ro.const.pool
> or
> .data.rel.ro.local..shared
> .data.rel.ro..shared
> or something similar, but .data.rel.ro.local.shared would not be ok,
> because it could clash with .data.rel.ro.local section for shared function.
>
> > > Another question is if we need to do anything about the
> > > DECL_COMDAT_GROUP (decl) && DECL_SECTION_NAME (decl)
> > > && startswith (DECL_SECTION_NAME (decl), ".gnu.linkonce.t.")
> > > case (older linkers) (i.e. when using years old GNU linkers).
> > >
> >
> > Should we support such targets? It is not easy for me to test it.
>
> Perhaps let's wait if somebody files an issue with such configuration.
>
>         Jakub
>
Jakub Jelinek Jan. 31, 2024, 6:11 p.m. UTC | #5
On Wed, Jan 31, 2024 at 09:39:12AM -0800, H.J. Lu wrote:
> GNU binutils has no issues with it:

I know, I meant gcc.
If I try the proposed:
--- gcc/varasm.cc.jj	2024-01-30 08:44:43.304175273 +0100
+++ gcc/varasm.cc	2024-01-31 18:45:57.271087170 +0100
@@ -7459,15 +7459,46 @@ default_elf_select_rtx_section (machine_
 {
   int reloc = compute_reloc_for_rtx (x);
 
+  tree decl = nullptr;
+
+  /* If it is a private COMDAT function symbol reference, call
+     function_rodata_section for the read-only or relocated read-only
+     data section associated with function DECL so that the COMDAT
+     section will be used for the private COMDAT function symbol.  */
+  if (HAVE_COMDAT_GROUP)
+    {
+      if (GET_CODE (x) == CONST
+	 && GET_CODE (XEXP (x, 0)) == PLUS
+	 && CONST_INT_P (XEXP (XEXP (x, 0), 1)))
+       x = XEXP (XEXP (x, 0), 0);
+
+      if (GET_CODE (x) == SYMBOL_REF)
+       {
+	 decl = SYMBOL_REF_DECL (x);
+	 if (decl
+	     && (TREE_CODE (decl) != FUNCTION_DECL
+		 || !DECL_COMDAT_GROUP (decl)
+		 || TREE_PUBLIC (decl)))
+	   decl = nullptr;
+       }
+    }
+
   /* ??? Handle small data here somehow.  */
 
   if (reloc & targetm.asm_out.reloc_rw_mask ())
     {
+      if (decl)
+	return get_section (reloc == 1
+			    ? ".data.rel.ro.local" : ".data.rel.ro",
+			    SECTION_WRITE | SECTION_RELRO | SECTION_LINKONCE,
+			    decl);
       if (reloc == 1)
 	return get_named_section (NULL, ".data.rel.ro.local", 1);
       else
 	return get_named_section (NULL, ".data.rel.ro", 3);
     }
+  else if (decl)
+    return get_section (".rodata", SECTION_LINKONCE, decl);
 
   return mergeable_constant_section (mode, align, 0);
 }

and append
typedef unsigned long int VV __attribute__((vector_size (2 * sizeof (long))));
VV vv;
__attribute__((noipa)) static void fn1 (void) {}
__attribute__((noipa)) static void fn2 (void) {}

void
fn3 ()
{
  VV a = { (unsigned long) &fn1, (unsigned long) &fn2 };
  vv = a;
}
to the first testcase (this is just to get a normal non-comdat
.data.rel.ro.local section referencing non-comdat non-public syumbol),
then I get the
pr113617.C:19:1: error: section type conflict with ‘static bool R::B<_R(_A ...), _F>::F(R::H&, const R::H&, R::G) [with _R = void; _F = R::I<void (*(N1::N2::N3::C<{anonymous}::D<long long int>, false>*, long long int, long long int, long long int))(void*, long long int, long long int, long long int)>; _A = {}]’
   19 | }
      | ^
In file included from pr113617.C:1:
pr113617.h:21:15: note: ‘static bool R::B<_R(_A ...), _F>::F(R::H&, const R::H&, R::G) [with _R = void; _F = R::I<void (*(N1::N2::N3::C<{anonymous}::D<long long int>, false>*, long long int, long long int, long long int))(void*, long long int, long long int, long long int)>; _A = {}]’ was declared here
   21 |   static bool F(H &, const H &, G) { return false; }
      |               ^
I feared.
So, it seems get_section handles section purely by name lookup
and isn't prepared to deal with multiple different sections
of the same name, but different comdat group.

Thus, maybe at least temporarily we need to use unique
section names here, say
.data.rel.ro.local.pool.<comdat_name>
.data.rel.ro.pool.<comdat_name>
.rodata.pool.<comdat_name>
where <comdat_name> would be the name of the comdat group, i.e.
IDENTIFIER_POINTER (DECL_COMDAT_GROUP (decl))

	Jakub
H.J. Lu Jan. 31, 2024, 7:31 p.m. UTC | #6
On Wed, Jan 31, 2024 at 10:11 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Jan 31, 2024 at 09:39:12AM -0800, H.J. Lu wrote:
> > GNU binutils has no issues with it:
>
> I know, I meant gcc.
> If I try the proposed:
> --- gcc/varasm.cc.jj    2024-01-30 08:44:43.304175273 +0100
> +++ gcc/varasm.cc       2024-01-31 18:45:57.271087170 +0100
> @@ -7459,15 +7459,46 @@ default_elf_select_rtx_section (machine_
>  {
>    int reloc = compute_reloc_for_rtx (x);
>
> +  tree decl = nullptr;
> +
> +  /* If it is a private COMDAT function symbol reference, call
> +     function_rodata_section for the read-only or relocated read-only
> +     data section associated with function DECL so that the COMDAT
> +     section will be used for the private COMDAT function symbol.  */
> +  if (HAVE_COMDAT_GROUP)
> +    {
> +      if (GET_CODE (x) == CONST
> +        && GET_CODE (XEXP (x, 0)) == PLUS
> +        && CONST_INT_P (XEXP (XEXP (x, 0), 1)))
> +       x = XEXP (XEXP (x, 0), 0);
> +
> +      if (GET_CODE (x) == SYMBOL_REF)
> +       {
> +        decl = SYMBOL_REF_DECL (x);
> +        if (decl
> +            && (TREE_CODE (decl) != FUNCTION_DECL
> +                || !DECL_COMDAT_GROUP (decl)
> +                || TREE_PUBLIC (decl)))
> +          decl = nullptr;
> +       }
> +    }
> +
>    /* ??? Handle small data here somehow.  */
>
>    if (reloc & targetm.asm_out.reloc_rw_mask ())
>      {
> +      if (decl)
> +       return get_section (reloc == 1
> +                           ? ".data.rel.ro.local" : ".data.rel.ro",
> +                           SECTION_WRITE | SECTION_RELRO | SECTION_LINKONCE,
> +                           decl);
>        if (reloc == 1)
>         return get_named_section (NULL, ".data.rel.ro.local", 1);
>        else
>         return get_named_section (NULL, ".data.rel.ro", 3);
>      }
> +  else if (decl)
> +    return get_section (".rodata", SECTION_LINKONCE, decl);
>
>    return mergeable_constant_section (mode, align, 0);
>  }
>
> and append
> typedef unsigned long int VV __attribute__((vector_size (2 * sizeof (long))));
> VV vv;
> __attribute__((noipa)) static void fn1 (void) {}
> __attribute__((noipa)) static void fn2 (void) {}
>
> void
> fn3 ()
> {
>   VV a = { (unsigned long) &fn1, (unsigned long) &fn2 };
>   vv = a;
> }
> to the first testcase (this is just to get a normal non-comdat
> .data.rel.ro.local section referencing non-comdat non-public syumbol),
> then I get the
> pr113617.C:19:1: error: section type conflict with ‘static bool R::B<_R(_A ...), _F>::F(R::H&, const R::H&, R::G) [with _R = void; _F = R::I<void (*(N1::N2::N3::C<{anonymous}::D<long long int>, false>*, long long int, long long int, long long int))(void*, long long int, long long int, long long int)>; _A = {}]’
>    19 | }
>       | ^
> In file included from pr113617.C:1:
> pr113617.h:21:15: note: ‘static bool R::B<_R(_A ...), _F>::F(R::H&, const R::H&, R::G) [with _R = void; _F = R::I<void (*(N1::N2::N3::C<{anonymous}::D<long long int>, false>*, long long int, long long int, long long int))(void*, long long int, long long int, long long int)>; _A = {}]’ was declared here
>    21 |   static bool F(H &, const H &, G) { return false; }
>       |               ^
> I feared.
> So, it seems get_section handles section purely by name lookup
> and isn't prepared to deal with multiple different sections
> of the same name, but different comdat group.
>
> Thus, maybe at least temporarily we need to use unique
> section names here, say
> .data.rel.ro.local.pool.<comdat_name>
> .data.rel.ro.pool.<comdat_name>
> .rodata.pool.<comdat_name>
> where <comdat_name> would be the name of the comdat group, i.e.
> IDENTIFIER_POINTER (DECL_COMDAT_GROUP (decl))
>
>         Jakub
>

I am testing this patch.
diff mbox series

Patch

diff --git a/gcc/testsuite/g++.dg/pr113617-1a.C b/gcc/testsuite/g++.dg/pr113617-1a.C
new file mode 100644
index 00000000000..c93f08b5068
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr113617-1a.C
@@ -0,0 +1,145 @@ 
+// { dg-do compile { target fpic } }
+// { dg-require-visibility "" }
+// { dg-options "-O2 -std=c++11 -fPIC -fvisibility=hidden -fvisibility-inlines-hidden" }
+
+namespace {
+template <int V> struct J { static constexpr int value = V; };
+template <bool V> using K = J<V>;
+using M = K<true>;
+template <int> struct L { template <typename _Tp, typename> using type = _Tp; };
+template <bool _Cond, typename _If, typename _Else> using N = typename L<_Cond>::type<_If, _Else>;
+M k;
+template <typename _Tp> struct O { using type = _Tp; };
+template <typename _Up>
+struct P : N<M ::value, O<_Up>, _Up> {};
+template <typename _Tp> struct Q { using type = typename P<_Tp>::type; };
+}
+namespace R {
+struct H;
+enum G {};
+template <typename> class S;
+struct T { using U = bool (*) (H &, const H &, G); U F; };
+template <typename, typename> class B;
+template <typename _R, typename _F, typename... _A>
+struct B<_R(_A...), _F> {
+  static bool F(H &, const H &, G) { return false; }
+  __attribute__((noipa)) static _R bar(const H &) {}
+};
+template <typename _R, typename... _A>
+struct S<_R(_A...)> : T {
+  template <typename _F> using AH = B<_R(), _F>;
+  template <typename _F> S(_F) {
+    using AG = AH<_F>;
+    barr = AG::bar;
+    F = AG::F;
+  }
+  using AF = _R (*)(const H &);
+  AF barr;
+};
+template <typename> class I;
+template <typename _F, typename... _B>
+struct I<_F(_B...)> {};
+template <typename> using W = decltype(k);
+template <int, typename _F, typename... _B> struct V {
+  typedef I<typename Q<_F>::type(typename Q<_B>::type...)> type;
+};
+template <typename _F, typename... _B>
+__attribute__((noipa)) typename V<W<_F>::value, _F, _B...>::type
+baz(_F, _B...) { return typename V<W<_F>::value, _F, _B...>::type (); }
+template <typename _Tp> struct AJ {
+  template <typename _Up> struct _Ptr { using type = _Up *; };
+  using AI = typename _Ptr<_Tp>::type;
+};
+template <typename _Tp> struct Y {
+  using AI = typename AJ<_Tp>::AI;
+  AI operator->();
+};
+}
+extern int z;
+namespace N1 {
+namespace N2 {
+namespace N3 {
+enum Z { Z1, Z2 };
+template <int> struct X {
+  template <typename _F>
+  __attribute__((noipa)) void boo(long long, long long, long long, _F &) {}
+};
+struct AC {
+  AC(int);
+  void m1(R::S<void()>);
+};
+template <typename>
+__attribute__((noipa)) void garply(void *, long long, long long, long long) {}
+template <>
+template <typename _F>
+void X<Z2>::boo(long long, long long x, long long y, _F &fi) {
+  AC pool(z);
+  for (;;) {
+    auto job = R::baz(garply<_F>, &fi, y, y, x);
+    pool.m1(job);
+  }
+}
+struct AB {
+  static AB &bleh();
+  template <typename _F>
+  void boo(long first, long x, long y, _F fi) {
+    switch (ab1) {
+    case Z1:
+      ab2->boo(first, x, y, fi);
+    case Z2:
+      ab3->boo(first, x, y, fi);
+    }
+  }
+  Z ab1;
+  R::Y<X<Z1>> ab2;
+  R::Y<X<Z2>> ab3;
+};
+template <typename, bool> struct C;
+template <typename _F> struct C<_F, false> {
+  __attribute__((noipa)) C(_F) {}
+  void boo(long first, long x, long y) {
+    auto u = AB::bleh();
+    u.boo(first, x, y, *this);
+  }
+};
+template <typename _F> struct AA { typedef C<_F, 0> type; };
+}
+}
+}
+struct AD {
+  template <typename _F>
+  static void boo(long first, long x, long y, _F f) {
+    typename N1::N2::N3::AA<_F>::type fi(f);
+    fi.boo(first, x, y);
+  }
+  template <typename _F>
+  static void boo(long first, long x, _F f) {
+    boo(first, x, 0, f);
+  }
+};
+template <typename> struct A {
+  void foo(long long, long long);
+  int *c;
+};
+namespace {
+template <typename> struct D { __attribute__((noipa)) D(int *) {} };
+}
+template <typename T>
+void A<T>::foo(long long x, long long y)
+{
+  int e;
+  D<T> d(&e);
+  AD::boo(0, y, d);
+  long p;
+  for (p = 0; p < x; p++)
+    c[p] = c[p - 1];
+}
+int z;
+long xx1;
+void corge() {
+  A<long long> a;
+  a.foo(xx1, 0);
+}
+
+// { dg-final { scan-assembler-not ".section\t\.data\.rel\.ro\.local,\"aw\"" { target { { i?86-*-linux* x86_64-*-linux* } && { ! ia32 } } } } }
+// { dg-final { scan-assembler ".section\t.data\.rel\.ro\.local\..*,\"awG\"" { target { { i?86-*-linux* x86_64-*-linux* } && { ! ia32 } } } } }
diff --git a/gcc/testsuite/g++.dg/pr113617-1b.C b/gcc/testsuite/g++.dg/pr113617-1b.C
new file mode 100644
index 00000000000..4a02bf44e72
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr113617-1b.C
@@ -0,0 +1,8 @@ 
+// { dg-do compile { target fpic } }
+// { dg-require-visibility "" }
+// { dg-options "-O2 -std=c++11 -fno-pic -fvisibility=hidden -fvisibility-inlines-hidden" }
+
+#include "pr113617-1a.C"
+
+// { dg-final { scan-assembler-not ".section\t\.rodata\.cst(4|8),\"aM\"" { target { { i?86-*-linux* x86_64-*-linux* } && { ! ia32 } } } } }
+// { dg-final { scan-assembler ".section\t\.rodata\..*,\"aG\"" { target { { i?86-*-linux* x86_64-*-linux* } && { ! ia32 } } } } }
diff --git a/gcc/varasm.cc b/gcc/varasm.cc
index fa17eff551e..c26d95b7178 100644
--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -7459,16 +7459,46 @@  default_elf_select_rtx_section (machine_mode mode, rtx x,
 {
   int reloc = compute_reloc_for_rtx (x);
 
+  tree decl = nullptr;
+
+  /* If it is a private COMDAT function symbol reference, call
+     function_rodata_section for the read-only or relocated read-only
+     data section associated with function DECL so that the COMDAT
+     section will be used for the private COMDAT function symbol.  */
+  if (HAVE_COMDAT_GROUP)
+    {
+      if (GET_CODE (x) == CONST
+	  && GET_CODE (XEXP (x, 0)) == PLUS
+	  && CONST_INT_P (XEXP (XEXP (x, 0), 1)))
+	x = XEXP (XEXP (x, 0), 0);
+
+      if (GET_CODE (x) == SYMBOL_REF)
+	{
+	  decl = SYMBOL_REF_DECL (x);
+	  if (decl
+	      && (TREE_CODE (decl) != FUNCTION_DECL
+		  || !DECL_COMDAT_GROUP (decl)
+		  || TREE_PUBLIC (decl)))
+	    decl = nullptr;
+	}
+    }
+
   /* ??? Handle small data here somehow.  */
 
   if (reloc & targetm.asm_out.reloc_rw_mask ())
     {
+      if (decl)
+	return targetm.asm_out.function_rodata_section (decl, true);
+
       if (reloc == 1)
 	return get_named_section (NULL, ".data.rel.ro.local", 1);
       else
 	return get_named_section (NULL, ".data.rel.ro", 3);
     }
 
+  if (decl)
+    return targetm.asm_out.function_rodata_section (decl, false);
+
   return mergeable_constant_section (mode, align, 0);
 }