| Message ID | 20240127151055.358066-1-hjl.tools@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Handle function symbol reference in readonly data section | expand |
On Sat, Jan 27, 2024 at 07:10:55AM -0800, H.J. Lu wrote: > For function symbol reference in readonly data section, instead of putting > it in .data.rel.ro or .rodata.cst section, call function_rodata_section to > get the read-only or relocated read-only data section associated with the > function DECL so that the COMDAT section will be used for a COMDAT function > symbol. I have to admit I still don't understand what the linker doesn't like on what GCC emits and why references to the public symbols at the start of comdat sections are ok in .text but not in .data.rel.ro but are in .data or .rodata sections (or what the exact rules are, see also what we emit on __attribute__((noinline, noipa)) inline void foo () {} void bar () { foo (); } void (*p) () = foo; void (*const q) () = foo; void (*const *r) () = &q; ). I've always thought that the problematic references are when something references non-public symbols in comdat sections, especially not at their start, because if linker selects some comdat section(s) from some other TU, there is no guarantee e.g. the code is identical (just in valid program should behave the same) and if such reference comes from other comdat that is kept or from non-comdat sections, the question is what should be referenced. But in this case, I believe we are referencing the function at the start of a code comdat section. Now, in my limited understanding what the patch does is totally wrong for multiple reasons. On the first testcase it changes - .section .data.rel.ro.local,"aw" + .section .data.rel.ro.local._ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE,"awG",@progbits,_ZN26vtkStaticCellLinksTemplateIxE18ThreadedBuildLinksExxP12vtkCellArray,comdat .align 8 .LC0: .quad _ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE Now, I believe such a .data.rel.ro.local.* section is normally used for .data.rel.ro.local constants from the referenced function, if we have some relocatable constant needed in that function we emit those there. If linker picks up the comdat from current TU, it will be all fine, sure, but if it picks up the comdat from another TU, the .data.rel.ro.local._ZN4blah17_Function_handlerIFvvENS_5* section there might not be present or might contain some unrelated stuff. Given the handling of (const (plus (symbol_ref) (const_int)), we also don't know whether the section holds a reference to the start, or to some other offset of it, how many etc. And, we refenre a non-public symbol (.LC0) from non-comdat section to a comdat section. If I'm wrong on this, please try to explain. Jakub
On Mon, Jan 29, 2024 at 3:03 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Sat, Jan 27, 2024 at 07:10:55AM -0800, H.J. Lu wrote: > > For function symbol reference in readonly data section, instead of putting > > it in .data.rel.ro or .rodata.cst section, call function_rodata_section to > > get the read-only or relocated read-only data section associated with the > > function DECL so that the COMDAT section will be used for a COMDAT function > > symbol. > > I have to admit I still don't understand what the linker doesn't like on > what GCC emits and why references to the public symbols at the start of > comdat sections are ok in .text but not in .data.rel.ro but are in .data > or .rodata sections (or what the exact rules are, see also what we emit on > __attribute__((noinline, noipa)) inline void foo () {} > void bar () { foo (); } void (*p) () = foo; void (*const q) () = foo; void (*const *r) () = &q; > ). > I've always thought that the problematic references are when something > references non-public symbols in comdat sections, especially not at their > start, because if linker selects some comdat section(s) from some other > TU, there is no guarantee e.g. the code is identical (just in valid program > should behave the same) and if such reference comes from other comdat that > is kept or from non-comdat sections, the question is what should be > referenced. > > But in this case, I believe we are referencing the function at the start of > a code comdat section. > > Now, in my limited understanding what the patch does is totally wrong > for multiple reasons. On the first testcase it changes > - .section .data.rel.ro.local,"aw" > + .section .data.rel.ro.local._ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE,"awG",@progbits,_ZN26vtkStaticCellLinksTemplateIxE18ThreadedBuildLinksExxP12vtkCellArray,comdat > .align 8 > .LC0: > .quad _ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE > Now, I believe such a .data.rel.ro.local.* section is normally > used for .data.rel.ro.local constants from the referenced function, > if we have some relocatable constant needed in that function we > emit those there. > If linker picks up the comdat from current TU, it will be all fine, > sure, but if it picks up the comdat from another TU, the > .data.rel.ro.local._ZN4blah17_Function_handlerIFvvENS_5* section > there might not be present or might contain some unrelated stuff. > Given the handling of (const (plus (symbol_ref) (const_int)), we > also don't know whether the section holds a reference to the start, > or to some other offset of it, how many etc. > And, we refenre a non-public symbol (.LC0) from non-comdat section > to a comdat section. TARGET_ASM_SELECT_RTX_SECTION is for constant in RTL. It should have a non-public label reference which can't be used by other TUs. The same section can contain other constants. If there is a COMAT issue, linker will catch it. > If I'm wrong on this, please try to explain. > > Jakub >
On Mon, Jan 29, 2024 at 06:36:47AM -0800, H.J. Lu wrote: > TARGET_ASM_SELECT_RTX_SECTION is for constant in RTL. > It should have a non-public label reference which can't be used > by other TUs. The same section can contain other constants. > If there is a COMAT issue, linker will catch it. Let me try to explain on short assembly snippet what I believe your patch is doing and what I'm afraid of. I believe your patch when we need to emit a RTL constant foo or foo+1 or foo+2 (where foo is defined in a comdat section) instead of emitting using say foo in assembly puts those constants into .data.rel.ro.local section determined by the decl that is referenced. Now, when first_tu.o wins and emits the qux comdat, it will contain the .data.rel.ro.local.foo which bar function refers to, but in second_tu.o it wants to refer to different offsets from the same function and loses. I simply believe the constants need to be in section based on what refers to those symbols, not the value of those constants, and that is what we used to do before your patch (and I'd like to understand what's wrong with what GCC emits and why). first_tu.s: ============ .section .text.foo,"axG",@progbits,qux,comdat .p2align 4 .type foo, @function foo: xorl %eax, %eax ret .size foo, .-foo .text .p2align 4 .type bar, @function bar: movq .LC0(%rip), %xmm0 ret .size bar, .-bar .section .data.rel.ro.local.foo,"awG",@progbits,qux,comdat .align 8 .LC0: .quad foo second_tu.s: ============ .section .text.foo,"axG",@progbits,qux,comdat .p2align 4 .type foo, @function foo: xorl %eax, %eax ret .size foo, .-foo .text .p2align 4 .type baz, @function baz: movq .LC0(%rip), %xmm0 ret .size baz, .-baz .section .data.rel.ro.local.foo,"awG",@progbits,qux,comdat .align 8 .LC0: .quad foo+1 .text .p2align 4 .type corge, @function corge: movq .LC1(%rip), %xmm0 ret .size corge, .-corge .section .data.rel.ro.local.foo,"awG",@progbits,qux,comdat .align 8 .LC1: .quad foo+2 gcc -shared -o test.so first_tu.s second_tu.s `.data.rel.ro.local.foo' referenced in section `.text' of /tmp/cceeUWyH.o: defined in discarded section `.data.rel.ro.local.foo[qux]' of /tmp/cceeUWyH.o `.data.rel.ro.local.foo' referenced in section `.text' of /tmp/cceeUWyH.o: defined in discarded section `.data.rel.ro.local.foo[qux]' of /tmp/cceeUWyH.o collect2: error: ld returned 1 exit status Jakub
On Mon, Jan 29, 2024 at 8:03 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Jan 29, 2024 at 06:36:47AM -0800, H.J. Lu wrote: > > TARGET_ASM_SELECT_RTX_SECTION is for constant in RTL. > > It should have a non-public label reference which can't be used > > by other TUs. The same section can contain other constants. > > If there is a COMAT issue, linker will catch it. > > Let me try to explain on short assembly snippet what I believe your patch is > doing and what I'm afraid of. I believe your patch when we need to emit > a RTL constant foo or foo+1 or foo+2 (where foo is defined in a comdat > section) instead of emitting using say foo in assembly puts those > constants into .data.rel.ro.local section determined by the decl that is > referenced. > Now, when first_tu.o wins and emits the qux comdat, it will contain > the .data.rel.ro.local.foo which bar function refers to, but in second_tu.o > it wants to refer to different offsets from the same function and loses. > > I simply believe the constants need to be in section based on what refers > to those symbols, not the value of those constants, and that is what we used > to do before your patch (and I'd like to understand what's wrong with what > GCC emits and why). > > first_tu.s: > ============ > .section .text.foo,"axG",@progbits,qux,comdat > .p2align 4 > .type foo, @function > foo: > xorl %eax, %eax > ret > .size foo, .-foo > .text > .p2align 4 > .type bar, @function > bar: > movq .LC0(%rip), %xmm0 > ret > .size bar, .-bar > .section .data.rel.ro.local.foo,"awG",@progbits,qux,comdat > .align 8 > .LC0: > .quad foo > > second_tu.s: > ============ > .section .text.foo,"axG",@progbits,qux,comdat > .p2align 4 > .type foo, @function > foo: > xorl %eax, %eax > ret > .size foo, .-foo > .text > .p2align 4 > .type baz, @function > baz: > movq .LC0(%rip), %xmm0 > ret I don't think this is valid. We can't reference a non-public symbol outside of a COMDAT group. It is OK to reference foo or foo + 1, but not .LC0. > .size baz, .-baz > .section .data.rel.ro.local.foo,"awG",@progbits,qux,comdat > .align 8 > .LC0: > .quad foo+1 > .text > .p2align 4 > .type corge, @function > corge: > movq .LC1(%rip), %xmm0 > ret > .size corge, .-corge > .section .data.rel.ro.local.foo,"awG",@progbits,qux,comdat > .align 8 > .LC1: > .quad foo+2 > gcc -shared -o test.so first_tu.s second_tu.s > `.data.rel.ro.local.foo' referenced in section `.text' of /tmp/cceeUWyH.o: defined in discarded section `.data.rel.ro.local.foo[qux]' of /tmp/cceeUWyH.o > `.data.rel.ro.local.foo' referenced in section `.text' of /tmp/cceeUWyH.o: defined in discarded section `.data.rel.ro.local.foo[qux]' of /tmp/cceeUWyH.o > collect2: error: ld returned 1 exit status > > Jakub >
On Mon, Jan 29, 2024 at 08:23:21AM -0800, H.J. Lu wrote: > > baz: > > movq .LC0(%rip), %xmm0 > > ret > > I don't think this is valid. We can't reference a non-public > symbol outside of a COMDAT group. It is OK to reference > foo or foo + 1, but not .LC0. But that is exactly what your patch does, e.g. on the first testcase: --- pr113617-1a.s 2024-01-29 11:29:55.831512974 +0100 +++ pr113617-1a.s 2024-01-29 11:30:04.335394116 +0100 @@ -51,28 +-51,28 @@ .section .text._ZN3vtk6detail3smp15vtkSMPToolsImplILi1EE3ForINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvxxxRT_,"axG",@progbits,_ZN26vtkStaticCellLinksTemplateIxE18ThreadedBuildLinksExxP12vtkCellArray,comdat .align 2 .p2align 4 .type _ZN3vtk6detail3smp15vtkSMPToolsImplILi1EE3ForINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvxxxRT_, @function _ZN3vtk6detail3smp15vtkSMPToolsImplILi1EE3ForINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvxxxRT_: pushq %r15 leaq _ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE9_M_invokeERKNS_9_Any_dataE(%rip), %rax leaq _ZN3vtk6detail3smp23ExecuteFunctorSTDThreadINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvPvxxx(%rip), %r15 pushq %r14 movq %rax, %xmm1 pushq %r13 pushq %r12 movq %rdx, %r12 pushq %rbp movq %r8, %rbp pushq %rbx movq %rcx, %rbx subq $40, %rsp movl For_threadNumber(%rip), %esi movq .LC0(%rip), %xmm0 leaq 31(%rsp), %r13 punpcklqdq %xmm1, %xmm0 movq %r13, %rdi movaps %xmm0, (%rsp) call _ZN3vtk6detail3smp16vtkSMPThreadPoolC1Ei@PLT movq (%rsp), %r14 .p2align 4,,10 .p2align 3 @@ -191,9 +191,9 @@ vtkConstrainedSmoothingFilterRequestData .size For_threadNumber, 4 For_threadNumber: .zero 4 - .section .data.rel.ro.local,"aw" + .section .data.rel.ro.local._ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE,"awG",@progbits,_ZN26vtkStaticCellLinksTemplateIxE18ThreadedBuildLinksExxP12vtkCellArray,comdat .align 8 .LC0: .quad _ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE - .ident "GCC: (GNU) 14.0.1 20240127 (experimental)" + .ident "GCC: (GNU) 14.0.1 20240129 (experimental)" .section .note.GNU-stack,"",@progbits Jakub
On Mon, Jan 29, 2024 at 8:34 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Jan 29, 2024 at 08:23:21AM -0800, H.J. Lu wrote: > > > baz: > > > movq .LC0(%rip), %xmm0 > > > ret > > > > I don't think this is valid. We can't reference a non-public > > symbol outside of a COMDAT group. It is OK to reference > > foo or foo + 1, but not .LC0. > > But that is exactly what your patch does, e.g. on the first testcase: > --- pr113617-1a.s 2024-01-29 11:29:55.831512974 +0100 > +++ pr113617-1a.s 2024-01-29 11:30:04.335394116 +0100 > @@ -51,28 +-51,28 @@ > .section .text._ZN3vtk6detail3smp15vtkSMPToolsImplILi1EE3ForINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvxxxRT_,"axG",@progbits,_ZN26vtkStaticCellLinksTemplateIxE18ThreadedBuildLinksExxP12vtkCellArray,comdat > .align 2 > .p2align 4 > .type _ZN3vtk6detail3smp15vtkSMPToolsImplILi1EE3ForINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvxxxRT_, @function > _ZN3vtk6detail3smp15vtkSMPToolsImplILi1EE3ForINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvxxxRT_: > pushq %r15 > leaq _ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE9_M_invokeERKNS_9_Any_dataE(%rip), %rax > leaq _ZN3vtk6detail3smp23ExecuteFunctorSTDThreadINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvPvxxx(%rip), %r15 > pushq %r14 > movq %rax, %xmm1 > pushq %r13 > pushq %r12 > movq %rdx, %r12 > pushq %rbp > movq %r8, %rbp > pushq %rbx > movq %rcx, %rbx > subq $40, %rsp > movl For_threadNumber(%rip), %esi > movq .LC0(%rip), %xmm0 > leaq 31(%rsp), %r13 > punpcklqdq %xmm1, %xmm0 > movq %r13, %rdi > movaps %xmm0, (%rsp) > call _ZN3vtk6detail3smp16vtkSMPThreadPoolC1Ei@PLT > movq (%rsp), %r14 > .p2align 4,,10 > .p2align 3 > @@ -191,9 +191,9 @@ vtkConstrainedSmoothingFilterRequestData > .size For_threadNumber, 4 > For_threadNumber: > .zero 4 > - .section .data.rel.ro.local,"aw" > + .section .data.rel.ro.local._ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE,"awG",@progbits,_ZN26vtkStaticCellLinksTemplateIxE18ThreadedBuildLinksExxP12vtkCellArray,comdat > .align 8 > .LC0: > .quad _ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE > - .ident "GCC: (GNU) 14.0.1 20240127 (experimental)" > + .ident "GCC: (GNU) 14.0.1 20240129 (experimental)" > .section .note.GNU-stack,"",@progbits > > Jakub > In this case, these are internal to the same comdat group: .section .text._ZN3vtk6detail3smp15vtkSMPToolsImplILi1EE3ForINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvxxxRT_,"axG",@progbits,_ZN26vtkStaticCellLinksTemplateIxE18ThreadedBuildLinksExxP12vtkCellArray,comdat .align 2 .p2align 4 .type _ZN3vtk6detail3smp15vtkSMPToolsImplILi1EE3ForINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvxxxRT_, @function _ZN3vtk6detail3smp15vtkSMPToolsImplILi1EE3ForINS1_27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEEEEvxxxRT_: .LFB27: .cfi_startproc ... movq .LC0(%rip), %xmm0 ... .section .text._ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE,"axG",@progbits,_ZN26vtkStaticCellLinksTemplateIxE18ThreadedBuildLinksExxP12vtkCellArray,comdat .p2align 4 .type _ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE, @function _ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE: .LFB34: .cfi_startproc xorl %eax, %eax ret .cfi_endproc ... .section .data.rel.ro.local._ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE,"awG",@progbits,_ZN26vtkStaticCellLinksTemplateIxE18ThreadedBuildLinksExxP12vtkCellArray,comdat .align 8 .LC0: .quad _ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE
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?
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.
Jakub
On Mon, Jan 29, 2024 at 9:00 AM Jakub Jelinek <jakub@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. > > Jakub >
On Mon, Jan 29, 2024 at 9:34 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Mon, Jan 29, 2024 at 9:00 AM Jakub Jelinek <jakub@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.
On Mon, Jan 29, 2024 at 1:00 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Mon, Jan 29, 2024 at 9:34 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > On Mon, Jan 29, 2024 at 9:00 AM Jakub Jelinek <jakub@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.
On Mon, Jan 29, 2024 at 1:22 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Mon, Jan 29, 2024 at 1:00 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > On Mon, Jan 29, 2024 at 9:34 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > On Mon, Jan 29, 2024 at 9:00 AM Jakub Jelinek <jakub@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.
On Mon, Jan 29, 2024 at 1:42 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Mon, Jan 29, 2024 at 1:22 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > On Mon, Jan 29, 2024 at 1:00 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > On Mon, Jan 29, 2024 at 9:34 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > > > On Mon, Jan 29, 2024 at 9:00 AM Jakub Jelinek <jakub@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.
On Mon, Jan 29, 2024 at 2:01 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Mon, Jan 29, 2024 at 1:42 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > On Mon, Jan 29, 2024 at 1:22 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > On Mon, Jan 29, 2024 at 1:00 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > > > On Mon, Jan 29, 2024 at 9:34 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > > > > > On Mon, Jan 29, 2024 at 9:00 AM Jakub Jelinek <jakub@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.
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) 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); Jakub
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); Jakub
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 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. Jakub
On Mon, Jan 29, 2024 at 2:51 PM Jakub Jelinek <jakub@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 > 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
On Mon, Jan 29, 2024 at 3:12 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Mon, Jan 29, 2024 at 2:51 PM Jakub Jelinek <jakub@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.
On Mon, Jan 29, 2024 at 06:05:25PM -0800, H.J. Lu wrote: > 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. And is the referenced symbol non-public? If so, how does that work? Jakub
On Tue, Jan 30, 2024 at 4:51 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Jan 29, 2024 at 06:05:25PM -0800, H.J. Lu wrote: > > 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. > > And is the referenced symbol non-public? If so, how does that work? > I didn't check if it is public or private. It is OK for public, but not OK for private if they are in different comdat groups.
On Tue, Jan 30, 2024 at 4:58 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Tue, Jan 30, 2024 at 4:51 AM Jakub Jelinek <jakub@redhat.com> wrote: > > > > On Mon, Jan 29, 2024 at 06:05:25PM -0800, H.J. Lu wrote: > > > 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. > > > > And is the referenced symbol non-public? If so, how does that work? > > > > I didn't check if it is public or private. It is OK for public, but not OK > for private if they are in different comdat groups. > In GCC master source, when we force a comdat function symbol, which may be in the same comdat group or a different comdat group, into a constant pool, the symbol is always public. GCC never references a private comdat function symbol in a different comdat group. We just need to handle private COMDAT function symbol reference in the same comdat group by calling function_rodata_section.
diff --git a/gcc/testsuite/g++.dg/pr113617-1a.C b/gcc/testsuite/g++.dg/pr113617-1a.C new file mode 100644 index 00000000000..effd50841c0 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr113617-1a.C @@ -0,0 +1,170 @@ +// { dg-do compile { target fpic } } +// { dg-require-visibility "" } +// { dg-options "-O2 -std=c++11 -fPIC -fvisibility=hidden -fvisibility-inlines-hidden" } + +namespace { +template <int __v> struct integral_constant { + static constexpr int value = __v; +}; +template <bool __v> using __bool_constant = integral_constant<__v>; +using true_type = __bool_constant<true>; +template <int> struct __conditional { + template <typename _Tp, typename> using type = _Tp; +}; +template <bool _Cond, typename _If, typename _Else> +using __conditional_t = typename __conditional<_Cond>::type<_If, _Else>; +true_type __trans_tmp_1; +template <typename _Tp> struct remove_cv { using type = _Tp; }; +template <typename _Up> +struct __decay_selector + : __conditional_t<true_type ::value, remove_cv<_Up>, _Up> {}; +template <typename _Tp> struct decay { + using type = typename __decay_selector<_Tp>::type; +}; +} +struct vtkCellArray {}; +namespace blah { +struct _Any_data; +enum _Manager_operation {}; +template <typename> class function; +struct _Function_base { + using _Manager_type = bool (*)(_Any_data &, const _Any_data &, + _Manager_operation); + _Manager_type _M_manager; +}; +template <typename, typename> class _Function_handler; +template <typename _Res, typename _Functor, typename... _ArgTypes> +struct _Function_handler<_Res(_ArgTypes...), _Functor> { + static bool _M_manager(_Any_data &, const _Any_data &, _Manager_operation) { + return false; + } + __attribute__((noipa)) static _Res _M_invoke(const _Any_data &) {} +}; +template <typename _Res, typename... _ArgTypes> +struct function<_Res(_ArgTypes...)> : _Function_base { + template <typename _Functor> + using _Handler = _Function_handler<_Res(), _Functor>; + template <typename _Functor> function(_Functor) { + using _My_handler = _Handler<_Functor>; + _M_invoker = _My_handler::_M_invoke; + _M_manager = _My_handler::_M_manager; + } + using _Invoker_type = _Res (*)(const _Any_data &); + _Invoker_type _M_invoker; +}; +template <typename> class _Bind; +template <typename _Functor, typename... _Bound_args> +struct _Bind<_Functor(_Bound_args...)> {}; +template <typename> using __is_socketlike = decltype(__trans_tmp_1); +template <int, typename _Func, typename... _BoundArgs> struct _Bind_helper { + typedef _Bind<typename decay<_Func>::type( + typename decay<_BoundArgs>::type...)> + type; +}; +template <typename _Func, typename... _BoundArgs> +__attribute__((noipa)) typename _Bind_helper<__is_socketlike<_Func>::value, _Func, _BoundArgs...>::type +bind(_Func, _BoundArgs...) { return typename _Bind_helper<__is_socketlike<_Func>::value, _Func, _BoundArgs...>::type (); } +template <typename _Tp> struct __uniq_ptr_impl { + template <typename _Up> struct _Ptr { using type = _Up *; }; + using pointer = typename _Ptr<_Tp>::type; +}; +template <typename _Tp> struct unique_ptr { + using pointer = typename __uniq_ptr_impl<_Tp>::pointer; + pointer operator->(); +}; +} +extern int For_threadNumber; +namespace vtk { +namespace detail { +namespace smp { +enum BackendType { Sequential, STDThread }; +template <int> struct vtkSMPToolsImpl { + template <typename FunctorInternal> + __attribute__((noipa)) void For(long long, long long, long long, FunctorInternal &) {} +}; +struct vtkSMPThreadPool { + vtkSMPThreadPool(int); + void DoJob(blah::function<void()>); +}; +template <typename> +__attribute__((noipa)) void ExecuteFunctorSTDThread(void *, long long, long long, long long) {} +template <> +template <typename FunctorInternal> +void vtkSMPToolsImpl<STDThread>::For(long long, long long last, long long grain, + FunctorInternal &fi) { + vtkSMPThreadPool pool(For_threadNumber); + for (;;) { + auto job = blah::bind(ExecuteFunctorSTDThread<FunctorInternal>, &fi, grain, + grain, last); + pool.DoJob(job); + } +} +struct vtkSMPToolsAPI { + static vtkSMPToolsAPI &GetInstance(); + template <typename FunctorInternal> + void For(long first, long last, long grain, FunctorInternal fi) { + switch (ActivatedBackend) { + case Sequential: + SequentialBackend->For(first, last, grain, fi); + case STDThread: + STDThreadBackend->For(first, last, grain, fi); + } + } + BackendType ActivatedBackend; + blah::unique_ptr<vtkSMPToolsImpl<Sequential>> SequentialBackend; + blah::unique_ptr<vtkSMPToolsImpl<STDThread>> STDThreadBackend; +}; +template <typename, bool> struct vtkSMPTools_FunctorInternal; +template <typename Functor> struct vtkSMPTools_FunctorInternal<Functor, false> { + __attribute__((noipa)) vtkSMPTools_FunctorInternal(Functor) {} + void For(long first, long last, long grain) { + auto SMPToolsAPI = vtkSMPToolsAPI::GetInstance(); + SMPToolsAPI.For(first, last, grain, *this); + } +}; +template <typename Functor> struct vtkSMPTools_Lookup_For { + typedef vtkSMPTools_FunctorInternal<Functor, 0> type; +}; +} +} +} +struct vtkSMPTools { + template <typename Functor> + static void For(long first, long last, long grain, Functor f) { + typename vtk::detail::smp::vtkSMPTools_Lookup_For<Functor>::type fi(f); + fi.For(first, last, grain); + } + template <typename Functor> + static void For(long first, long last, Functor f) { + For(first, last, 0, f); + } +}; +template <typename> struct vtkStaticCellLinksTemplate { + void ThreadedBuildLinks(long long, long long, vtkCellArray *); + int *Offsets; +}; +namespace { +template <typename> struct CountUses { __attribute__((noipa)) CountUses(vtkCellArray *, int *) {} }; +} +template <typename TIds> +void vtkStaticCellLinksTemplate<TIds>::ThreadedBuildLinks( + long long numPts, long long numCells, vtkCellArray *cellArray) { + int counts; + CountUses<TIds> count(cellArray, &counts); + vtkSMPTools::For(0, numCells, count); + long ptId, npts; + for (ptId = 0; ptId < numPts; ptId++) + Offsets[ptId] = Offsets[ptId - 1] + npts; +} + +int For_threadNumber; +long vtkConstrainedSmoothingFilterRequestData_numPts; +void vtkConstrainedSmoothingFilterRequestData() { + vtkCellArray lines; + vtkStaticCellLinksTemplate<long long> links; + links.ThreadedBuildLinks(vtkConstrainedSmoothingFilterRequestData_numPts, 0, + &lines); +} + +// { dg-final { scan-assembler-not ".section\t\.data\.rel\.ro\.local,\"aw\"" { target { { i?86-*-linux* x86_64-*-linux* } && { ! ia32 } } } } } +// { dg-final { scan-assembler ".section\t.data\.rel\.ro\.local\..*,\"awG\"" { target { { i?86-*-linux* x86_64-*-linux* } && { ! ia32 } } } } } diff --git a/gcc/testsuite/g++.dg/pr113617-1b.C b/gcc/testsuite/g++.dg/pr113617-1b.C new file mode 100644 index 00000000000..4a02bf44e72 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr113617-1b.C @@ -0,0 +1,8 @@ +// { dg-do compile { target fpic } } +// { dg-require-visibility "" } +// { dg-options "-O2 -std=c++11 -fno-pic -fvisibility=hidden -fvisibility-inlines-hidden" } + +#include "pr113617-1a.C" + +// { dg-final { scan-assembler-not ".section\t\.rodata\.cst(4|8),\"aM\"" { target { { i?86-*-linux* x86_64-*-linux* } && { ! ia32 } } } } } +// { dg-final { scan-assembler ".section\t\.rodata\..*,\"aG\"" { target { { i?86-*-linux* x86_64-*-linux* } && { ! ia32 } } } } } diff --git a/gcc/varasm.cc b/gcc/varasm.cc index fa17eff551e..31d57edbe8a 100644 --- a/gcc/varasm.cc +++ b/gcc/varasm.cc @@ -7459,16 +7459,34 @@ default_elf_select_rtx_section (machine_mode mode, rtx x, { int reloc = compute_reloc_for_rtx (x); + /* If it is a 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 a + COMDAT function symbol. */ + tree decl = nullptr; + if (GET_CODE (x) == SYMBOL_REF) + { + decl = SYMBOL_REF_DECL (x); + if (TREE_CODE (decl) != FUNCTION_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); + if (reloc == 1) return get_named_section (NULL, ".data.rel.ro.local", 1); else return get_named_section (NULL, ".data.rel.ro", 3); } + if (decl) + return targetm.asm_out.function_rodata_section (decl, false); + return mergeable_constant_section (mode, align, 0); }