On Mon, Jan 29, 2024 at 2:01 PM H.J. Lu <hjl.to...@gmail.com> wrote: > > On Mon, Jan 29, 2024 at 1:42 PM H.J. Lu <hjl.to...@gmail.com> wrote: > > > > On Mon, Jan 29, 2024 at 1:22 PM H.J. Lu <hjl.to...@gmail.com> wrote: > > > > > > On Mon, Jan 29, 2024 at 1:00 PM H.J. Lu <hjl.to...@gmail.com> wrote: > > > > > > > > On Mon, Jan 29, 2024 at 9:34 AM H.J. Lu <hjl.to...@gmail.com> wrote: > > > > > > > > > > On Mon, Jan 29, 2024 at 9:00 AM Jakub Jelinek <ja...@redhat.com> > > > > > wrote: > > > > > > > > > > > > On Mon, Jan 29, 2024 at 08:45:45AM -0800, H.J. Lu wrote: > > > > > > > In this case, these are internal to the same comdat group: > > > > > > > > > > > > But that is only by accident, no? > > > > > > > > > > This may be by luck. I don't know if gcc checks it when > > > > > generating such references. > > > > > > > > > > > I mean, if you need to refer to such a symbol from > > > > > > non-comdat function or comdat function in a different comdat group > > > > > > and RA decides it wants the constant in memory rather than code? > > > > > > Your patch uses > > > > > > if (decl) > > > > > > return targetm.asm_out.function_rodata_section (decl, ???); > > > > > > and default_function_rodata_section only looks at comdat group of > > > > > > the > > > > > > passed in decl. But the decl here is what the constant refers to, > > > > > > not > > > > > > who is referring it. > > > > > > > > LRA puts a function symbol reference in a constant pool via > > > > > > > > #0 force_const_mem (in_mode=E_DImode, x=0x7fffe9e7e000) > > > > at /export/gnu/import/git/gitlab/x86-gcc-test/gcc/varasm.cc:3951 > > > > #1 0x0000000001833870 in curr_insn_transform (check_only_p=false) > > > > at > > > > /export/gnu/import/git/gitlab/x86-gcc-test/gcc/lra-constraints.cc:4473 > > > > #2 0x0000000001836eae in lra_constraints (first_p=true) > > > > at > > > > /export/gnu/import/git/gitlab/x86-gcc-test/gcc/lra-constraints.cc:5462 > > > > #3 0x000000000181fcf1 in lra (f=0x0, verbose=5) > > > > at /export/gnu/import/git/gitlab/x86-gcc-test/gcc/lra.cc:2442 > > > > #4 0x00000000017c8828 in do_reload () > > > > at /export/gnu/import/git/gitlab/x86-gcc-test/gcc/ira.cc:5973 > > > > #5 0x00000000017c8d25 in (anonymous namespace)::pass_reload::execute ( > > > > this=0x48d8730) > > > > at /export/gnu/import/git/gitlab/x86-gcc-test/gcc/ira.cc:6161 > > > > > > > > for > > > > > > > > (gdb) call debug_rtx (curr_insn) > > > > (insn 12 57 15 2 (set (reg:V2DI 101 [ _16 ]) > > > > (vec_concat:V2DI (symbol_ref:DI > > > > ("_ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE") > > > > [flags 0x3] <function_decl 0x7fffe9e2a600 _M_manager>) > > > > (reg/f:DI 109))) 7521 {vec_concatv2di} > > > > (expr_list:REG_DEAD (reg/f:DI 110) > > > > (expr_list:REG_DEAD (reg/f:DI 109) > > > > (expr_list:REG_EQUIV (vec_concat:V2DI (symbol_ref:DI > > > > ("_ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE") > > > > [flags 0x3] <function_decl 0x7fffe9e2a600 _M_manager>) > > > > (symbol_ref:DI > > > > ("_ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE9_M_invokeERKNS_9_Any_dataE") > > > > [flags 0x3] <function_decl 0x7fffe9e2a700 _M_invoke>)) > > > > (nil))))) > > > > (gdb) > > > > > > > > CONST_POOL_OK_P doesn't check if it is safe to do so for function > > > > symbols. Here is a patch to add the check. > > > > > > > > -- > > > > H.J. > > > > > > On the other hand, does C++ even allow access to non-public members > > > from different classes? So my patch should be safe and linker should > > > catch all invalid comdat usages like this bug. > > > > 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.
If we are concerned that not all comdat definitions provide such a constant pool, we can change LA to only allow such a constant pool when it is safe to do so. -- H.J.