On Mon, Jan 29, 2024 at 3:03 AM Jakub Jelinek <ja...@redhat.com> wrote:
>
> On Sat, Jan 27, 2024 at 07:10:55AM -0800, H.J. Lu wrote:
> > For function symbol reference in readonly data section, instead of putting
> > it in .data.rel.ro or .rodata.cst section, call function_rodata_section to
> > get the read-only or relocated read-only data section associated with the
> > function DECL so that the COMDAT section will be used for a COMDAT function
> > symbol.
>
> I have to admit I still don't understand what the linker doesn't like on
> what GCC emits and why references to the public symbols at the start of
> comdat sections are ok in .text but not in .data.rel.ro but are in .data
> or .rodata sections (or what the exact rules are, see also what we emit on
> __attribute__((noinline, noipa)) inline void foo () {}
> void bar () { foo (); } void (*p) () = foo; void (*const q) () = foo; void
> (*const *r) () = &q;
> ).
> I've always thought that the problematic references are when something
> references non-public symbols in comdat sections, especially not at their
> start, because if linker selects some comdat section(s) from some other
> TU, there is no guarantee e.g. the code is identical (just in valid program
> should behave the same) and if such reference comes from other comdat that
> is kept or from non-comdat sections, the question is what should be
> referenced.
>
> But in this case, I believe we are referencing the function at the start of
> a code comdat section.
>
> Now, in my limited understanding what the patch does is totally wrong
> for multiple reasons. On the first testcase it changes
> - .section .data.rel.ro.local,"aw"
> + .section
> .data.rel.ro.local._ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE,"awG",@progbits,_ZN26vtkStaticCellLinksTemplateIxE18ThreadedBuildLinksExxP12vtkCellArray,comdat
> .align 8
> .LC0:
> .quad
> _ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE
> Now, I believe such a .data.rel.ro.local.* section is normally
> used for .data.rel.ro.local constants from the referenced function,
> if we have some relocatable constant needed in that function we
> emit those there.
> If linker picks up the comdat from current TU, it will be all fine,
> sure, but if it picks up the comdat from another TU, the
> .data.rel.ro.local._ZN4blah17_Function_handlerIFvvENS_5* section
> there might not be present or might contain some unrelated stuff.
> Given the handling of (const (plus (symbol_ref) (const_int)), we
> also don't know whether the section holds a reference to the start,
> or to some other offset of it, how many etc.
> And, we refenre a non-public symbol (.LC0) from non-comdat section
> to a comdat section.
TARGET_ASM_SELECT_RTX_SECTION is for constant in RTL.
It should have a non-public label reference which can't be used
by other TUs. The same section can contain other constants.
If there is a COMAT issue, linker will catch it.
> If I'm wrong on this, please try to explain.
>
> Jakub
>
--
H.J.
