On Wed, Jan 31, 2024 at 8:30 AM Jakub Jelinek <ja...@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.
