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

["H.J. Lu" <hjl.tools@gmail.com>]

[-- 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