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

[Jakub Jelinek <jakub@redhat.com>]

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?
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).

	Jakub