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

[Jakub Jelinek]

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