* [PATCH v2] Handle private COMDAT function symbol reference in readonly data section
@ 2024-01-31 2:21 H.J. Lu
2024-01-31 16:30 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: H.J. Lu @ 2024-01-31 2:21 UTC (permalink / raw)
To: gcc-patches; +Cc: jakub
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
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);
}
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Handle private COMDAT function symbol reference in readonly data section
2024-01-31 2:21 [PATCH v2] Handle private COMDAT function symbol reference in readonly data section H.J. Lu
@ 2024-01-31 16:30 ` Jakub Jelinek
2024-01-31 16:48 ` H.J. Lu
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2024-01-31 16:30 UTC (permalink / raw)
To: H.J. Lu; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Handle private COMDAT function symbol reference in readonly data section
2024-01-31 16:30 ` Jakub Jelinek
@ 2024-01-31 16:48 ` H.J. Lu
2024-01-31 17:09 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: H.J. Lu @ 2024-01-31 16:48 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
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.
--
H.J.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Handle private COMDAT function symbol reference in readonly data section
2024-01-31 16:48 ` H.J. Lu
@ 2024-01-31 17:09 ` Jakub Jelinek
2024-01-31 17:39 ` H.J. Lu
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2024-01-31 17:09 UTC (permalink / raw)
To: H.J. Lu; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Handle private COMDAT function symbol reference in readonly data section
2024-01-31 17:09 ` Jakub Jelinek
@ 2024-01-31 17:39 ` H.J. Lu
2024-01-31 18:11 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: H.J. Lu @ 2024-01-31 17:39 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
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
>
--
H.J.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Handle private COMDAT function symbol reference in readonly data section
2024-01-31 17:39 ` H.J. Lu
@ 2024-01-31 18:11 ` Jakub Jelinek
2024-01-31 18:58 ` [PATCH] varasm, v3: Handle private COMDAT function symbol reference in readonly data section [PR113617] Jakub Jelinek
2024-01-31 19:31 ` [PATCH v2] Handle private COMDAT function symbol reference in readonly data section H.J. Lu
0 siblings, 2 replies; 9+ messages in thread
From: Jakub Jelinek @ 2024-01-31 18:11 UTC (permalink / raw)
To: H.J. Lu; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] varasm, v3: Handle private COMDAT function symbol reference in readonly data section [PR113617]
2024-01-31 18:11 ` Jakub Jelinek
@ 2024-01-31 18:58 ` Jakub Jelinek
2024-02-01 8:23 ` Jakub Jelinek
2024-01-31 19:31 ` [PATCH v2] Handle private COMDAT function symbol reference in readonly data section H.J. Lu
1 sibling, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2024-01-31 18:58 UTC (permalink / raw)
To: Richard Biener, Jeff Law, H.J. Lu; +Cc: gcc-patches
On Wed, Jan 31, 2024 at 07:11:20PM +0100, Jakub Jelinek 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.
> 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))
Here is an untested patch which implements that:
2024-01-31 Jakub Jelinek <jakub@redhat.com>
H.J. Lu <hjl.tools@gmail.com>
PR rtl-optimization/113617
* varasm.cc (default_elf_select_rtx_section): For
references to private symbols in comdat sections
use .data.relro.local.pool.<comdat>, .data.relro.pool.<comdat>
or .rodata.<comdat> comdat sections.
* g++.dg/other/pr113617.C: New test.
* g++.dg/other/pr113617.h: New test.
* g++.dg/other/pr113617-aux.cc: New test.
--- gcc/varasm.cc.jj 2024-01-30 08:44:43.304175273 +0100
+++ gcc/varasm.cc 2024-01-31 19:49:47.499880994 +0100
@@ -7458,17 +7458,63 @@ default_elf_select_rtx_section (machine_
unsigned HOST_WIDE_INT align)
{
int reloc = compute_reloc_for_rtx (x);
+ tree decl = nullptr;
+ const char *prefix = nullptr;
+ int flags = 0;
+
+ /* 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 (reloc == 1)
+ if (decl)
+ {
+ prefix = reloc == 1 ? ".data.rel.ro.local" : ".data.rel.ro";
+ flags = SECTION_WRITE | SECTION_RELRO;
+ }
+ else 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)
+ {
+ const char *comdat = IDENTIFIER_POINTER (DECL_COMDAT_GROUP (decl));
+ if (!prefix)
+ prefix = ".rodata";
+ size_t prefix_len = strlen (prefix);
+ size_t comdat_len = strlen (comdat);
+ size_t len = prefix_len + sizeof (".pool.") + comdat_len;
+ char *name = XALLOCAVEC (char, len);
+ memcpy (name, prefix, prefix_len);
+ memcpy (name + prefix_len, ".pool.", sizeof (".pool.") - 1);
+ memcpy (name + prefix_len + sizeof (".pool.") - 1, comdat,
+ comdat_len + 1);
+ return get_section (name, flags | SECTION_LINKONCE, decl);
+ }
+
return mergeable_constant_section (mode, align, 0);
}
--- gcc/testsuite/g++.dg/other/pr113617.C.jj 2024-01-31 19:46:30.678626083 +0100
+++ gcc/testsuite/g++.dg/other/pr113617.C 2024-01-31 19:46:26.598682981 +0100
@@ -0,0 +1,27 @@
+// PR rtl-optimization/113617
+// { dg-do link { target c++11 } }
+// { dg-options "-O2" }
+// { dg-additional-options "-fPIC" { target fpic } } */
+// { dg-additional-options "-shared" { target shared } } */
+// { dg-additional-sources pr113617-aux.cc }
+
+#include "pr113617.h"
+
+int z;
+long xx1;
+void corge() {
+ A<long long> a;
+ a.foo(xx1, 0);
+}
+
+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;
+}
--- gcc/testsuite/g++.dg/other/pr113617.h.jj 2024-01-31 19:46:33.415587908 +0100
+++ gcc/testsuite/g++.dg/other/pr113617.h 2024-01-30 13:34:07.922368904 +0100
@@ -0,0 +1,132 @@
+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];
+}
--- gcc/testsuite/g++.dg/other/pr113617-aux.cc.jj 2024-01-31 19:46:40.573488077 +0100
+++ gcc/testsuite/g++.dg/other/pr113617-aux.cc 2024-01-31 19:47:03.579167215 +0100
@@ -0,0 +1,9 @@
+// PR rtl-optimization/113617
+// { dg-do link { target { c++17 && c++14_down } } }
+
+#include "pr113617.h"
+
+void qux() {
+ A<long long> a;
+ a.foo(0, 0);
+}
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] Handle private COMDAT function symbol reference in readonly data section
2024-01-31 18:11 ` Jakub Jelinek
2024-01-31 18:58 ` [PATCH] varasm, v3: Handle private COMDAT function symbol reference in readonly data section [PR113617] Jakub Jelinek
@ 2024-01-31 19:31 ` H.J. Lu
1 sibling, 0 replies; 9+ messages in thread
From: H.J. Lu @ 2024-01-31 19:31 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3865 bytes --]
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.
--
H.J.
[-- Attachment #2: v3-0001-Handle-private-COMDAT-function-symbol-reference-i.patch --]
[-- Type: text/x-patch, Size: 9954 bytes --]
From 3c8b9ad67383d645e19746720deb6e8f020fccd0 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 26 Jan 2024 12:20:11 -0800
Subject: [PATCH v3] Handle private COMDAT function symbol reference in readonly data section
Changes in v3:
1. Add get_comdat_function_rodata_section.
2. Add a new test.
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
get_comdat_function_rodata_section to put the private COMDAT function
symbol reference in .data.rel.ro or .rodata section in the same COMDAT
group as the function DECL.
gcc/
PR rtl-optimization/113617
* varasm.cc (get_comdat_function_rodata_section): New.
(default_elf_select_rtx_section): Call
get_comdat_function_rodata_section to put the private COMDAT
function symbol reference in .data.rel.ro or .rodata section
in the same COMDAT group as the function DECL.
gcc/testsuite/
PR rtl-optimization/113617
* g++.dg/pr113617-1a.C: New test.
* g++.dg/pr113617-1b.C: Likewise.
* g++.dg/pr113617-1c.C: Likewise.
Fix
---
gcc/testsuite/g++.dg/pr113617-1a.C | 145 +++++++++++++++++++++++++++++
gcc/testsuite/g++.dg/pr113617-1b.C | 8 ++
gcc/testsuite/g++.dg/pr113617-1c.C | 17 ++++
gcc/varasm.cc | 64 +++++++++++++
4 files changed, 234 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/pr113617-1a.C
create mode 100644 gcc/testsuite/g++.dg/pr113617-1b.C
create mode 100644 gcc/testsuite/g++.dg/pr113617-1c.C
diff --git a/gcc/testsuite/g++.dg/pr113617-1a.C b/gcc/testsuite/g++.dg/pr113617-1a.C
new file mode 100644
index 00000000000..6f445d26d79
--- /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.pool._ZN1AIxE3fooExx,\"awG\",@progbits,_ZN1AIxE3fooExx,comdat" { 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..70021691504
--- /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.pool._ZN1AIxE3fooExx,\"aG\",@progbits,_ZN1AIxE3fooExx,comdat" { target { { i?86-*-linux* x86_64-*-linux* } && { ! ia32 } } } } }
diff --git a/gcc/testsuite/g++.dg/pr113617-1c.C b/gcc/testsuite/g++.dg/pr113617-1c.C
new file mode 100644
index 00000000000..35917b755dc
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr113617-1c.C
@@ -0,0 +1,17 @@
+// { dg-do compile { target fpic } }
+// { dg-require-visibility "" }
+// { dg-options "-O2 -std=c++11 -fpic -fvisibility=hidden -fvisibility-inlines-hidden" }
+
+#include "pr113617-1a.C"
+
+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;
+}
diff --git a/gcc/varasm.cc b/gcc/varasm.cc
index fa17eff551e..6e2a46e9e6c 100644
--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -7453,21 +7453,85 @@ default_select_rtx_section (machine_mode mode ATTRIBUTE_UNUSED,
return readonly_data_section;
}
+/* Return the read-only or relocated read-only data section associated
+ with the COMDAT function DECL. */
+
+static section *
+get_comdat_function_rodata_section (tree decl, int reloc)
+{
+ const char *sname;
+ unsigned int flags;
+
+ if (reloc)
+ {
+ flags = SECTION_WRITE | SECTION_RELRO | SECTION_LINKONCE;
+ if (reloc == 1)
+ sname = ".data.rel.ro.local.pool.";
+ else
+ sname = ".data.rel.ro.pool.";
+ }
+ else
+ {
+ flags = SECTION_LINKONCE;
+ sname = ".rodata.pool.";
+ }
+
+ /* Append the COMDAT name to the section name. */
+ const char *gname = IDENTIFIER_POINTER (DECL_COMDAT_GROUP (decl));
+ size_t len = strlen (sname) + strlen (gname);
+ char *name = new char[len + 1];
+ strcpy (name, sname);
+ strcat (name, gname);
+ section *s = get_section (name, flags, decl);
+ delete[] name;
+
+ return s;
+}
+
section *
default_elf_select_rtx_section (machine_mode mode, rtx x,
unsigned HOST_WIDE_INT align)
{
int reloc = compute_reloc_for_rtx (x);
+ tree decl = nullptr;
+
+ /* If it is a private COMDAT function symbol reference, call
+ get_comdat_function_rodata_section to put the private COMDAT
+ function symbol reference in .data.rel.ro or .rodata section
+ in the same COMDAT group as the function DECL. */
+ 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_comdat_function_rodata_section (decl, reloc);
+
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_comdat_function_rodata_section (decl, 0);
return mergeable_constant_section (mode, align, 0);
}
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] varasm, v3: Handle private COMDAT function symbol reference in readonly data section [PR113617]
2024-01-31 18:58 ` [PATCH] varasm, v3: Handle private COMDAT function symbol reference in readonly data section [PR113617] Jakub Jelinek
@ 2024-02-01 8:23 ` Jakub Jelinek
0 siblings, 0 replies; 9+ messages in thread
From: Jakub Jelinek @ 2024-02-01 8:23 UTC (permalink / raw)
To: Richard Biener, Jeff Law, H.J. Lu, gcc-patches
On Wed, Jan 31, 2024 at 07:58:50PM +0100, Jakub Jelinek wrote:
> On Wed, Jan 31, 2024 at 07:11:20PM +0100, Jakub Jelinek 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.
> > 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))
>
> Here is an untested patch which implements that:
>
> 2024-01-31 Jakub Jelinek <jakub@redhat.com>
> H.J. Lu <hjl.tools@gmail.com>
>
> PR rtl-optimization/113617
> * varasm.cc (default_elf_select_rtx_section): For
> references to private symbols in comdat sections
> use .data.relro.local.pool.<comdat>, .data.relro.pool.<comdat>
> or .rodata.<comdat> comdat sections.
>
> * g++.dg/other/pr113617.C: New test.
> * g++.dg/other/pr113617.h: New test.
> * g++.dg/other/pr113617-aux.cc: New test.
Bootstrapped/regtested on x86_64-linux and i686-linux successfully,
ok for trunk?
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-02-01 8:23 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-31 2:21 [PATCH v2] Handle private COMDAT function symbol reference in readonly data section H.J. Lu
2024-01-31 16:30 ` Jakub Jelinek
2024-01-31 16:48 ` H.J. Lu
2024-01-31 17:09 ` Jakub Jelinek
2024-01-31 17:39 ` H.J. Lu
2024-01-31 18:11 ` Jakub Jelinek
2024-01-31 18:58 ` [PATCH] varasm, v3: Handle private COMDAT function symbol reference in readonly data section [PR113617] Jakub Jelinek
2024-02-01 8:23 ` Jakub Jelinek
2024-01-31 19:31 ` [PATCH v2] Handle private COMDAT function symbol reference in readonly data section H.J. Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).