diff mbox series

varasm, v3: Handle private COMDAT function symbol reference in readonly data section [PR113617]

Message ID ZbqYarChjvEzdof6@tucnak
State Unresolved
Headers
Series varasm, v3: Handle private COMDAT function symbol reference in readonly data section [PR113617] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Jan. 31, 2024, 6:58 p.m. UTC 
  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.



	Jakub

Comments

Jakub Jelinek Feb. 1, 2024, 8:23 a.m. UTC | #1 
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
diff mbox series

Patch

--- 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);
+}