On Mon, Jan 29, 2024 at 3:12 PM H.J. Lu <hjl.to...@gmail.com> wrote:
>
> On Mon, Jan 29, 2024 at 2:51 PM Jakub Jelinek <ja...@redhat.com> wrote:
> >
> > On Mon, Jan 29, 2024 at 11:29:22PM +0100, Jakub Jelinek wrote:
> > > On Mon, Jan 29, 2024 at 11:22:44PM +0100, Jakub Jelinek wrote:
> > > > On Mon, Jan 29, 2024 at 02:01:56PM -0800, H.J. Lu wrote:
> > > > > > A function accesses a function symbol defined in a comdat group.
> > > > > > If the function symbol is public, any comdat definition of the same
> > > > > > group
> > > > > > signature should provide the function definition. If the function
> > > > > > symbol
> > > > > > is private to the comdat group, only functions in the same comdat
> > > > > > group can access the private function symbol. If a function in a
> > > > > > different
> > > > > > comdat group accesses a private symbol, it is a compiler bug and
> > > > > > link may catch it like in this case.
> > > > > >
> > > > >
> > > > > My patch simply puts the constant pool of the function symbol
> > > > > reference
> > > > > in the same comdat group as the function definition. I believe it is
> > > > > the
> > > > > right thing to do.
> > > >
> > > > I disagree, I think we should use something like
> > > > if (current_function_decl)
> > >
> > > Or perhaps && DECL_COMDAT_GROUP (current_function_decl) added here as
> > > well,
> > > just to make it change things less often.
> > >
> > > > return targetm.asm_out.function_rodata_section
> > > > (current_function_decl,
> > > > true);
> > > >
> > > > Obviously, for non-reloc or non-pic, we don't want an unconditional
> > > > if (current_function_decl)
> > > > return targetm.asm_out.function_rodata_section
> > > > (current_function_decl,
> > > > false);
> > > > that would kill mergeable sections, so perhaps
> > > > if (current_function_decl
> > > > && reloc
> > > > && DECL_COMDAT_GROUP (current_function_decl))
> > > > return targetm.asm_out.function_rodata_section
> > > > (current_function_decl,
> > > > false);
> >
> > Now, that doesn't actually work, because current_function_decl is always
> > NULL when the constant pool entries are emitted.
> > But basing the output section on what it refers rather than what refers to
> > it seems wrong, plus there is the section anchors support, which treats them
> > yet differently.
> > So, I wonder if force_const_mem shouldn't punt if asked to emit from
LRA may call forcce_const_mem on this insn in the function
(gdb) call debug_tree (func_decl)
<function_decl 0x7ffff2770900 __ct_base
type <method_type 0x7ffff27511f8
type <void_type 0x7ffff7690f18 void type_6 VOID
align:8 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7ffff7690f18
pointer_to_this <pointer_type 0x7ffff7697000>>
QI
size <integer_cst 0x7ffff768a2b8 constant 8>
unit-size <integer_cst 0x7ffff768a2d0 constant 1>
align:8 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7ffff27512a0 method basetype <record_type
0x7ffff264b0a8 function_summary>
arg-types <tree_list 0x7ffff349acd0 value <pointer_type 0x7ffff26887e0>
chain <tree_list 0x7ffff349ac58 value <pointer_type 0x7ffff264b2a0>
chain <tree_list 0x7ffff48c0b68
purpose <integer_cst 0x7ffff768a510 constant 0>
value <boolean_type 0x7ffff7690b28 bool>
chain <tree_list 0x7ffff7685d98 value <void_type
0x7ffff7690f18 void>>>>>
pointer_to_this <pointer_type 0x7ffff258a888>>
addressable asm_written used nothrow public static weak decl_5 QI
/export/gnu/import/git/gitlab/x86-gcc/gcc/symbol-summary.h:268:1
align:16 warn_if_not_align:0 context <record_type 0x7ffff264b0a8
function_summary> initial <block 0x7ffff22c82a0> abstract_origin
<function_decl 0x7ffff2750700 __ct >
result <result_decl 0x7ffff22abd20 D.160175 type <void_type
0x7ffff7690f18 void>
ignored VOID
/export/gnu/import/git/gitlab/x86-gcc/gcc/symbol-summary.h:269:20
align:8 warn_if_not_align:0 context <function_decl
0x7ffff2770900 __ct_base >>
full-name "function_summary<T*>::function_summary(symbol_table*,
bool = false) [with T = clone_info]"
template-info <template_info 0x7ffff349ade8
template <template_decl 0x7ffff26fbf80 __ct type <method_type
0x7ffff2701540>
VOID
/export/gnu/import/git/gitlab/x86-gcc/gcc/symbol-summary.h:268:1
align:1 warn_if_not_align:0 context <record_type
0x7ffff27010a8 function_summary> result <function_decl 0x7ffff26fcc00
__ct >
parms <tree_list 0x7ffff37bc730 purpose <integer_cst
0x7ffff768a2d0 1>
value <tree_vec 0x7ffff304ed00 type <template_decl
0x7ffff2646e00 function_summary>
length:1
elt:0 <tree_list 0x7ffff37bc708 value <type_decl
0x7ffff26f5c78 T>>>>
full-name "template<class T>
function_summary<T*>::function_summary(symbol_table*, bool)">
args <tree_vec 0x7ffff30767a0 length:1 elt:0 <record_type
0x7ffff2987930 clone_info>>>
use_template=1
arguments <parm_decl 0x7ffff2773780 this
type <pointer_type 0x7ffff2751348 type <record_type
0x7ffff264b0a8 function_summary>
readonly sizes-gimplified public unsigned DI
size <integer_cst 0x7ffff768a1c8 constant 64>
unit-size <integer_cst 0x7ffff768a1e0 constant 8>
align:64 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7ffff2751348>
readonly used unsigned read DI
/export/gnu/import/git/gitlab/x86-gcc/gcc/symbol-summary.h:269:20 size
<integer_cst 0x7ffff768a1c8 64> unit-size <integer_cst 0x7ffff768a1e0
8>
align:64 warn_if_not_align:0 context <function_decl
0x7ffff2770900 __ct_base > abstract_origin <parm_decl 0x7ffff274fa00
this>
(reg/f:DI 117 [ this ]) arg-type <pointer_type 0x7ffff2751348>
incoming-rtl (reg:DI 5 di [ this ])
chain <parm_decl 0x7ffff2773800 symtab type <pointer_type
0x7ffff264b2a0>
used unsigned DI
/export/gnu/import/git/gitlab/x86-gcc/gcc/symbol-summary.h:268:56 size
<integer_cst 0x7ffff768a1c8 64> unit-size <integer_cst 0x7ffff768a1e0
8>
align:64 warn_if_not_align:0 context <function_decl
0x7ffff2770900 __ct_base > abstract_origin <parm_decl 0x7ffff22c1b80
symtab>
(reg/v/f:DI 118 [ symtab ]) arg-type <pointer_type 0x7ffff264b2a0>
incoming-rtl (reg:DI 4 si [ symtab ]) chain <parm_decl
0x7ffff2773880 ggc>>>
struct-function 0x7ffff22cb228 chain <function_decl 0x7ffff2770800
__ct_comp >>
(gdb)
in gcc master branch tree:
(gdb) call debug_rtx (curr_insn)
(insn 14 128 15 2 (set (reg:V2DI 121 [ _21 ])
(vec_concat:V2DI (symbol_ref/i:DI
("_ZN16function_summaryIP10clone_infoE16symtab_insertionEP11cgraph_nodePv")
[flags 0x3] <function_decl 0x7ffff2750e00 symtab_insertion>)
(symbol_ref/i:DI
("_ZN16function_summaryIP10clone_infoE14symtab_removalEP11cgraph_nodePv")
[flags 0x3] <function_decl 0x7ffff2750f00 symtab_removal>)))
"/export/gnu/import/git/gitlab/x86-gcc/gcc/symbol-summary.h":36:22
7521 {vec_concatv2di}
(expr_list:REG_DEAD (reg/f:DI 123)
(expr_list:REG_DEAD (reg/f:DI 122)
(expr_list:REG_EQUIV (vec_concat:V2DI (symbol_ref/i:DI
("_ZN16function_summaryIP10clone_infoE16symtab_insertionEP11cgraph_nodePv")
[flags 0x3] <function_decl 0x7ffff2750e00 symtab_insertion>)
(symbol_ref/i:DI
("_ZN16function_summaryIP10clone_infoE14symtab_removalEP11cgraph_nodePv")
[flags 0x3] <function_decl 0x7ffff2750f00 symtab_removal>))
(nil)))))
(gdb)
The referenced symbol,
function_summary<clone_info*>::symtab_removal(cgraph_node*, void*),
and the referencing function are in different COMDAT groups.
> > DECL_COMDAT_GROUP (current_function_decl) a SYMBOL_REF (or CONST PLUS
> > SYMBOL_REF ...) with the same DECL_COMDAT_GROUP with a private symbol,
> > or shouldn't punt unless using a per-function (i.e. non-shared) constant
> > pool, or force a per-function constant pool in that case somehow.
> >
>
> Here is the patch to only call function_rodata_section for COMDAT
> function symbol reference:
>
> https://patchwork.sourceware.org/project/gcc/list/?series=30329
> --
> H.J.
--
H.J.
