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