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.
--
H.J.
