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

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

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.