On Wed, Jan 31, 2024 at 10:11 AM Jakub Jelinek <ja...@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.
From 3c8b9ad67383d645e19746720deb6e8f020fccd0 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.to...@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
