| Message ID | 0558633c-b553-5ef1-aa6f-c76fcf297454@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889] | expand |
Hi, Gentle ping: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600190.html BR, Kewen on 2022/8/24 16:17, Kewen.Lin via Gcc-patches wrote: > Hi, > > As discussed in PR98125, -fpatchable-function-entry with > SECTION_LINK_ORDER support doesn't work well on powerpc64 > ELFv1 because the filled "Symbol" in > > .section name,"flags"o,@type,Symbol > > sits in .opd section instead of in the function_section > like .text or named .text*. > > Since we already generates one label LPFE* which sits in > function_section of current_function_decl, this patch is > to reuse it as the symbol for the linked_to section. It > avoids the above ABI specific issue when using the symbol > concluded from current_function_decl. > > Besides, with this support some previous workarounds for > powerpc64 ELFv1 can be reverted. > > btw, rs6000_print_patchable_function_entry can be dropped > but there is another rs6000 patch which needs this rs6000 > specific hook rs6000_print_patchable_function_entry, not > sure which one gets landed first, so just leave it here. > > Bootstrapped and regtested on below: > > 1) powerpc64-linux-gnu P8 with default binutils 2.27 > and latest binutils 2.39. > 2) powerpc64le-linux-gnu P9 (default binutils 2.30). > 3) powerpc64le-linux-gnu P10 (default binutils 2.30). > 4) x86_64-redhat-linux with default binutils 2.30 > and latest binutils 2.39. > 5) aarch64-linux-gnu with default binutils 2.30 > and latest binutils 2.39. > > Is it ok for trunk? > > BR, > Kewen > ----- > > PR target/99889 > > gcc/ChangeLog: > > * config/rs6000/rs6000.cc (rs6000_print_patchable_function_entry): > Adjust to call function default_print_patchable_function_entry. > * targhooks.cc (default_print_patchable_function_entry_1): Remove and > move the flags preparation ... > (default_print_patchable_function_entry): ... here, adjust to use > current_function_funcdef_no for label no. > * targhooks.h (default_print_patchable_function_entry_1): Remove. > * varasm.cc (default_elf_asm_named_section): Adjust code for > __patchable_function_entries section support with LPFE label. > > gcc/testsuite/ChangeLog: > > * g++.dg/pr93195a.C: Remove the skip on powerpc*-*-* 64-bit. > * gcc.target/aarch64/pr92424-2.c: Adjust LPFE1 with LPFE0. > * gcc.target/aarch64/pr92424-3.c: Likewise. > * gcc.target/i386/pr93492-2.c: Likewise. > * gcc.target/i386/pr93492-3.c: Likewise. > * gcc.target/i386/pr93492-4.c: Likewise. > * gcc.target/i386/pr93492-5.c: Likewise. > --- > gcc/config/rs6000/rs6000.cc | 13 +----- > gcc/varasm.cc | 15 ++++--- > gcc/targhooks.cc | 45 +++++++------------- > gcc/targhooks.h | 3 -- > gcc/testsuite/g++.dg/pr93195a.C | 1 - > gcc/testsuite/gcc.target/aarch64/pr92424-2.c | 4 +- > gcc/testsuite/gcc.target/aarch64/pr92424-3.c | 4 +- > gcc/testsuite/gcc.target/i386/pr93492-2.c | 4 +- > gcc/testsuite/gcc.target/i386/pr93492-3.c | 4 +- > gcc/testsuite/gcc.target/i386/pr93492-4.c | 4 +- > gcc/testsuite/gcc.target/i386/pr93492-5.c | 4 +- > 11 files changed, 40 insertions(+), 61 deletions(-) > > diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc > index df491bee2ea..dba28b8e647 100644 > --- a/gcc/config/rs6000/rs6000.cc > +++ b/gcc/config/rs6000/rs6000.cc > @@ -14771,18 +14771,9 @@ rs6000_print_patchable_function_entry (FILE *file, > unsigned HOST_WIDE_INT patch_area_size, > bool record_p) > { > - unsigned int flags = SECTION_WRITE | SECTION_RELRO; > - /* When .opd section is emitted, the function symbol > - default_print_patchable_function_entry_1 is emitted into the .opd section > - while the patchable area is emitted into the function section. > - Don't use SECTION_LINK_ORDER in that case. */ > - if (!(TARGET_64BIT && DEFAULT_ABI != ABI_ELFv2) > - && HAVE_GAS_SECTION_LINK_ORDER) > - flags |= SECTION_LINK_ORDER; > - default_print_patchable_function_entry_1 (file, patch_area_size, record_p, > - flags); > + default_print_patchable_function_entry (file, patch_area_size, record_p); > } > - > > + > enum rtx_code > rs6000_reverse_condition (machine_mode mode, enum rtx_code code) > { > diff --git a/gcc/varasm.cc b/gcc/varasm.cc > index 4db8506b106..d4de6e164ee 100644 > --- a/gcc/varasm.cc > +++ b/gcc/varasm.cc > @@ -6906,11 +6906,16 @@ default_elf_asm_named_section (const char *name, unsigned int flags, > fprintf (asm_out_file, ",%d", flags & SECTION_ENTSIZE); > if (flags & SECTION_LINK_ORDER) > { > - tree id = DECL_ASSEMBLER_NAME (decl); > - ultimate_transparent_alias_target (&id); > - const char *name = IDENTIFIER_POINTER (id); > - name = targetm.strip_name_encoding (name); > - fprintf (asm_out_file, ",%s", name); > + /* For now, only section "__patchable_function_entries" > + adopts flag SECTION_LINK_ORDER, internal label LPFE* > + was emitted in default_print_patchable_function_entry, > + just place it here for linked_to section. */ > + gcc_assert (!strcmp (name, "__patchable_function_entries")); > + fprintf (asm_out_file, ","); > + char buf[256]; > + ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", > + current_function_funcdef_no); > + assemble_name_raw (asm_out_file, buf); > } > if (HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE)) > { > diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc > index b15ae19bcb6..e80caecc418 100644 > --- a/gcc/targhooks.cc > +++ b/gcc/targhooks.cc > @@ -1979,15 +1979,17 @@ default_compare_by_pieces_branch_ratio (machine_mode) > return 1; > } > > -/* Helper for default_print_patchable_function_entry and other > - print_patchable_function_entry hook implementations. */ > +/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function > + entry. If RECORD_P is true and the target supports named sections, > + the location of the NOPs will be recorded in a special object section > + called "__patchable_function_entries". This routine may be called > + twice per function to put NOPs before and after the function > + entry. */ > > void > -default_print_patchable_function_entry_1 (FILE *file, > - unsigned HOST_WIDE_INT > - patch_area_size, > - bool record_p, > - unsigned int flags) > +default_print_patchable_function_entry (FILE *file, > + unsigned HOST_WIDE_INT patch_area_size, > + bool record_p) > { > const char *nop_templ = 0; > int code_num; > @@ -2001,13 +2003,17 @@ default_print_patchable_function_entry_1 (FILE *file, > if (record_p && targetm_common.have_named_sections) > { > char buf[256]; > - static int patch_area_number; > section *previous_section = in_section; > const char *asm_op = integer_asm_op (POINTER_SIZE_UNITS, false); > > gcc_assert (asm_op != NULL); > - patch_area_number++; > - ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number); > + /* If SECTION_LINK_ORDER is supported, this internal label will > + be filled as the symbol for linked_to section. */ > + ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", current_function_funcdef_no); > + > + unsigned int flags = SECTION_WRITE | SECTION_RELRO; > + if (HAVE_GAS_SECTION_LINK_ORDER) > + flags |= SECTION_LINK_ORDER; > > section *sect = get_section ("__patchable_function_entries", > flags, current_function_decl); > @@ -2029,25 +2035,6 @@ default_print_patchable_function_entry_1 (FILE *file, > output_asm_insn (nop_templ, NULL); > } > > -/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function > - entry. If RECORD_P is true and the target supports named sections, > - the location of the NOPs will be recorded in a special object section > - called "__patchable_function_entries". This routine may be called > - twice per function to put NOPs before and after the function > - entry. */ > - > -void > -default_print_patchable_function_entry (FILE *file, > - unsigned HOST_WIDE_INT patch_area_size, > - bool record_p) > -{ > - unsigned int flags = SECTION_WRITE | SECTION_RELRO; > - if (HAVE_GAS_SECTION_LINK_ORDER) > - flags |= SECTION_LINK_ORDER; > - default_print_patchable_function_entry_1 (file, patch_area_size, record_p, > - flags); > -} > - > bool > default_profile_before_prologue (void) > { > diff --git a/gcc/targhooks.h b/gcc/targhooks.h > index ecce55ebe79..5c1216fad0b 100644 > --- a/gcc/targhooks.h > +++ b/gcc/targhooks.h > @@ -229,9 +229,6 @@ extern bool default_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT, > bool); > extern int default_compare_by_pieces_branch_ratio (machine_mode); > > -extern void default_print_patchable_function_entry_1 (FILE *, > - unsigned HOST_WIDE_INT, > - bool, unsigned int); > extern void default_print_patchable_function_entry (FILE *, > unsigned HOST_WIDE_INT, > bool); > diff --git a/gcc/testsuite/g++.dg/pr93195a.C b/gcc/testsuite/g++.dg/pr93195a.C > index b14f1b3e341..26d265da74e 100644 > --- a/gcc/testsuite/g++.dg/pr93195a.C > +++ b/gcc/testsuite/g++.dg/pr93195a.C > @@ -1,5 +1,4 @@ > /* { dg-do link { target { ! { nvptx*-*-* visium-*-* } } } } */ > -/* { dg-skip-if "not supported" { { powerpc*-*-* } && lp64 } } */ > // { dg-require-effective-target o_flag_in_section } > /* { dg-options "-O0 -fpatchable-function-entry=1" } */ > /* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */ > diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c > index 0e75657a153..12465213aef 100644 > --- a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c > +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c > @@ -1,7 +1,7 @@ > /* { dg-do "compile" } */ > /* { dg-options "-O1" } */ > > -/* Test the placement of the .LPFE1 label. */ > +/* Test the placement of the .LPFE0 label. */ > > void > __attribute__ ((target("branch-protection=bti"), > @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti"), > f10_bti () > { > } > -/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti c\n.*\.LPFE1:\n\tnop\n.*\tret\n" } } */ > +/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\tret\n" } } */ > diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c > index 0a1f74d4096..2c6a73789f0 100644 > --- a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c > +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c > @@ -1,7 +1,7 @@ > /* { dg-do "compile" } */ > /* { dg-options "-O1" } */ > > -/* Test the placement of the .LPFE1 label. */ > +/* Test the placement of the .LPFE0 label. */ > > void > __attribute__ ((target("branch-protection=bti+pac-ret+leaf"), > @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti+pac-ret+leaf"), > f10_pac () > { > } > -/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti c\n.*\.LPFE1:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */ > +/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */ > diff --git a/gcc/testsuite/gcc.target/i386/pr93492-2.c b/gcc/testsuite/gcc.target/i386/pr93492-2.c > index 3d67095fd10..ede8c2077b7 100644 > --- a/gcc/testsuite/gcc.target/i386/pr93492-2.c > +++ b/gcc/testsuite/gcc.target/i386/pr93492-2.c > @@ -1,7 +1,7 @@ > /* { dg-do "compile" { target *-*-linux* } } */ > /* { dg-options "-O1 -fcf-protection -mmanual-endbr -fasynchronous-unwind-tables" } */ > > -/* Test the placement of the .LPFE1 label. */ > +/* Test the placement of the .LPFE0 label. */ > > void > __attribute__ ((cf_check,patchable_function_entry (1, 0))) > @@ -9,4 +9,4 @@ f10_endbr (void) > { > } > > -/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n\tret\n" } } */ > +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE0:\n\tnop\n\tret\n" } } */ > diff --git a/gcc/testsuite/gcc.target/i386/pr93492-3.c b/gcc/testsuite/gcc.target/i386/pr93492-3.c > index a625c927f4f..b68da30bd36 100644 > --- a/gcc/testsuite/gcc.target/i386/pr93492-3.c > +++ b/gcc/testsuite/gcc.target/i386/pr93492-3.c > @@ -2,7 +2,7 @@ > /* { dg-require-effective-target mfentry } */ > /* { dg-options "-O1 -fcf-protection -mmanual-endbr -mfentry -pg -fasynchronous-unwind-tables" } */ > > -/* Test the placement of the .LPFE1 label. */ > +/* Test the placement of the .LPFE0 label. */ > > void > __attribute__ ((cf_check,patchable_function_entry (1, 0))) > @@ -10,4 +10,4 @@ f10_endbr (void) > { > } > > -/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ > +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE0:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ > diff --git a/gcc/testsuite/gcc.target/i386/pr93492-4.c b/gcc/testsuite/gcc.target/i386/pr93492-4.c > index 8f205c345b8..c73034a4624 100644 > --- a/gcc/testsuite/gcc.target/i386/pr93492-4.c > +++ b/gcc/testsuite/gcc.target/i386/pr93492-4.c > @@ -1,11 +1,11 @@ > /* { dg-do "compile" { target *-*-linux* } } */ > /* { dg-options "-O1 -fpatchable-function-entry=1 -fasynchronous-unwind-tables" } */ > > -/* Test the placement of the .LPFE1 label. */ > +/* Test the placement of the .LPFE0 label. */ > > void > foo (void) > { > } > > -/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n\tret\n" } } */ > +/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n\tret\n" } } */ > diff --git a/gcc/testsuite/gcc.target/i386/pr93492-5.c b/gcc/testsuite/gcc.target/i386/pr93492-5.c > index 1ca5ba1fac1..ee9849ae852 100644 > --- a/gcc/testsuite/gcc.target/i386/pr93492-5.c > +++ b/gcc/testsuite/gcc.target/i386/pr93492-5.c > @@ -2,11 +2,11 @@ > /* { dg-options "-O1 -fpatchable-function-entry=1 -mfentry -pg -fasynchronous-unwind-tables" } */ > /* { dg-additional-options "-fno-PIE" { target ia32 } } */ > > -/* Test the placement of the .LPFE1 label. */ > +/* Test the placement of the .LPFE0 label. */ > > void > foo (void) > { > } > > -/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ > +/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ > --
Hi! On Wed, Aug 24, 2022 at 04:17:07PM +0800, Kewen.Lin wrote: > --- a/gcc/config/rs6000/rs6000.cc > +++ b/gcc/config/rs6000/rs6000.cc > @@ -14771,18 +14771,9 @@ rs6000_print_patchable_function_entry (FILE *file, > unsigned HOST_WIDE_INT patch_area_size, > bool record_p) > { > - unsigned int flags = SECTION_WRITE | SECTION_RELRO; > - /* When .opd section is emitted, the function symbol > - default_print_patchable_function_entry_1 is emitted into the .opd section > - while the patchable area is emitted into the function section. > - Don't use SECTION_LINK_ORDER in that case. */ > - if (!(TARGET_64BIT && DEFAULT_ABI != ABI_ELFv2) > - && HAVE_GAS_SECTION_LINK_ORDER) > - flags |= SECTION_LINK_ORDER; > - default_print_patchable_function_entry_1 (file, patch_area_size, record_p, > - flags); > + default_print_patchable_function_entry (file, patch_area_size, record_p); > } Please don't define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY at all, instead, and remove this whole function? The rs6000 changes are okay like that, thanks! Segher
Hi Segher, on 2022/9/30 04:31, Segher Boessenkool wrote: > Hi! > > On Wed, Aug 24, 2022 at 04:17:07PM +0800, Kewen.Lin wrote: >> --- a/gcc/config/rs6000/rs6000.cc >> +++ b/gcc/config/rs6000/rs6000.cc >> @@ -14771,18 +14771,9 @@ rs6000_print_patchable_function_entry (FILE *file, >> unsigned HOST_WIDE_INT patch_area_size, >> bool record_p) >> { >> - unsigned int flags = SECTION_WRITE | SECTION_RELRO; >> - /* When .opd section is emitted, the function symbol >> - default_print_patchable_function_entry_1 is emitted into the .opd section >> - while the patchable area is emitted into the function section. >> - Don't use SECTION_LINK_ORDER in that case. */ >> - if (!(TARGET_64BIT && DEFAULT_ABI != ABI_ELFv2) >> - && HAVE_GAS_SECTION_LINK_ORDER) >> - flags |= SECTION_LINK_ORDER; >> - default_print_patchable_function_entry_1 (file, patch_area_size, record_p, >> - flags); >> + default_print_patchable_function_entry (file, patch_area_size, record_p); >> } > > Please don't define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY at all, > instead, and remove this whole function? This hook is still needed for "ELFv2 support rework" which was just committed in r13-2984. There is also a note explaining this in the original mail: "btw, rs6000_print_patchable_function_entry can be dropped but there is another rs6000 patch which needs this rs6000 specific hook rs6000_print_patchable_function_entry, not sure which one gets landed first, so just leave it here." > > The rs6000 changes are okay like that, thanks! Thanks! BR, Kewen
On Fri, Sep 30, 2022 at 08:47:53PM +0800, Kewen.Lin wrote: > on 2022/9/30 04:31, Segher Boessenkool wrote: > > Please don't define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY at all, > > instead, and remove this whole function? > > This hook is still needed for "ELFv2 support rework" which > was just committed in r13-2984. There is also a note > explaining this in the original mail: > > "btw, rs6000_print_patchable_function_entry can be dropped > but there is another rs6000 patch which needs this rs6000 > specific hook rs6000_print_patchable_function_entry, not > sure which one gets landed first, so just leave it here." Ah, so this would be just churn? Okay then :-) Thanks, Segher
Hi, Gentle ping: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600190.html Any comments are highly appreciated. BR, Kewen on 2022/9/28 13:41, Kewen.Lin via Gcc-patches wrote: > Hi, > > Gentle ping: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600190.html > > BR, > Kewen > > on 2022/8/24 16:17, Kewen.Lin via Gcc-patches wrote: >> Hi, >> >> As discussed in PR98125, -fpatchable-function-entry with >> SECTION_LINK_ORDER support doesn't work well on powerpc64 >> ELFv1 because the filled "Symbol" in >> >> .section name,"flags"o,@type,Symbol >> >> sits in .opd section instead of in the function_section >> like .text or named .text*. >> >> Since we already generates one label LPFE* which sits in >> function_section of current_function_decl, this patch is >> to reuse it as the symbol for the linked_to section. It >> avoids the above ABI specific issue when using the symbol >> concluded from current_function_decl. >> >> Besides, with this support some previous workarounds for >> powerpc64 ELFv1 can be reverted. >> >> btw, rs6000_print_patchable_function_entry can be dropped >> but there is another rs6000 patch which needs this rs6000 >> specific hook rs6000_print_patchable_function_entry, not >> sure which one gets landed first, so just leave it here. >> >> Bootstrapped and regtested on below: >> >> 1) powerpc64-linux-gnu P8 with default binutils 2.27 >> and latest binutils 2.39. >> 2) powerpc64le-linux-gnu P9 (default binutils 2.30). >> 3) powerpc64le-linux-gnu P10 (default binutils 2.30). >> 4) x86_64-redhat-linux with default binutils 2.30 >> and latest binutils 2.39. >> 5) aarch64-linux-gnu with default binutils 2.30 >> and latest binutils 2.39. >> >> Is it ok for trunk? >> >> BR, >> Kewen >> ----- >> >> PR target/99889 >> >> gcc/ChangeLog: >> >> * config/rs6000/rs6000.cc (rs6000_print_patchable_function_entry): >> Adjust to call function default_print_patchable_function_entry. >> * targhooks.cc (default_print_patchable_function_entry_1): Remove and >> move the flags preparation ... >> (default_print_patchable_function_entry): ... here, adjust to use >> current_function_funcdef_no for label no. >> * targhooks.h (default_print_patchable_function_entry_1): Remove. >> * varasm.cc (default_elf_asm_named_section): Adjust code for >> __patchable_function_entries section support with LPFE label. >> >> gcc/testsuite/ChangeLog: >> >> * g++.dg/pr93195a.C: Remove the skip on powerpc*-*-* 64-bit. >> * gcc.target/aarch64/pr92424-2.c: Adjust LPFE1 with LPFE0. >> * gcc.target/aarch64/pr92424-3.c: Likewise. >> * gcc.target/i386/pr93492-2.c: Likewise. >> * gcc.target/i386/pr93492-3.c: Likewise. >> * gcc.target/i386/pr93492-4.c: Likewise. >> * gcc.target/i386/pr93492-5.c: Likewise. >> --- >> gcc/config/rs6000/rs6000.cc | 13 +----- >> gcc/varasm.cc | 15 ++++--- >> gcc/targhooks.cc | 45 +++++++------------- >> gcc/targhooks.h | 3 -- >> gcc/testsuite/g++.dg/pr93195a.C | 1 - >> gcc/testsuite/gcc.target/aarch64/pr92424-2.c | 4 +- >> gcc/testsuite/gcc.target/aarch64/pr92424-3.c | 4 +- >> gcc/testsuite/gcc.target/i386/pr93492-2.c | 4 +- >> gcc/testsuite/gcc.target/i386/pr93492-3.c | 4 +- >> gcc/testsuite/gcc.target/i386/pr93492-4.c | 4 +- >> gcc/testsuite/gcc.target/i386/pr93492-5.c | 4 +- >> 11 files changed, 40 insertions(+), 61 deletions(-) >> >> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc >> index df491bee2ea..dba28b8e647 100644 >> --- a/gcc/config/rs6000/rs6000.cc >> +++ b/gcc/config/rs6000/rs6000.cc >> @@ -14771,18 +14771,9 @@ rs6000_print_patchable_function_entry (FILE *file, >> unsigned HOST_WIDE_INT patch_area_size, >> bool record_p) >> { >> - unsigned int flags = SECTION_WRITE | SECTION_RELRO; >> - /* When .opd section is emitted, the function symbol >> - default_print_patchable_function_entry_1 is emitted into the .opd section >> - while the patchable area is emitted into the function section. >> - Don't use SECTION_LINK_ORDER in that case. */ >> - if (!(TARGET_64BIT && DEFAULT_ABI != ABI_ELFv2) >> - && HAVE_GAS_SECTION_LINK_ORDER) >> - flags |= SECTION_LINK_ORDER; >> - default_print_patchable_function_entry_1 (file, patch_area_size, record_p, >> - flags); >> + default_print_patchable_function_entry (file, patch_area_size, record_p); >> } >> - >> >> + >> enum rtx_code >> rs6000_reverse_condition (machine_mode mode, enum rtx_code code) >> { >> diff --git a/gcc/varasm.cc b/gcc/varasm.cc >> index 4db8506b106..d4de6e164ee 100644 >> --- a/gcc/varasm.cc >> +++ b/gcc/varasm.cc >> @@ -6906,11 +6906,16 @@ default_elf_asm_named_section (const char *name, unsigned int flags, >> fprintf (asm_out_file, ",%d", flags & SECTION_ENTSIZE); >> if (flags & SECTION_LINK_ORDER) >> { >> - tree id = DECL_ASSEMBLER_NAME (decl); >> - ultimate_transparent_alias_target (&id); >> - const char *name = IDENTIFIER_POINTER (id); >> - name = targetm.strip_name_encoding (name); >> - fprintf (asm_out_file, ",%s", name); >> + /* For now, only section "__patchable_function_entries" >> + adopts flag SECTION_LINK_ORDER, internal label LPFE* >> + was emitted in default_print_patchable_function_entry, >> + just place it here for linked_to section. */ >> + gcc_assert (!strcmp (name, "__patchable_function_entries")); >> + fprintf (asm_out_file, ","); >> + char buf[256]; >> + ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", >> + current_function_funcdef_no); >> + assemble_name_raw (asm_out_file, buf); >> } >> if (HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE)) >> { >> diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc >> index b15ae19bcb6..e80caecc418 100644 >> --- a/gcc/targhooks.cc >> +++ b/gcc/targhooks.cc >> @@ -1979,15 +1979,17 @@ default_compare_by_pieces_branch_ratio (machine_mode) >> return 1; >> } >> >> -/* Helper for default_print_patchable_function_entry and other >> - print_patchable_function_entry hook implementations. */ >> +/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function >> + entry. If RECORD_P is true and the target supports named sections, >> + the location of the NOPs will be recorded in a special object section >> + called "__patchable_function_entries". This routine may be called >> + twice per function to put NOPs before and after the function >> + entry. */ >> >> void >> -default_print_patchable_function_entry_1 (FILE *file, >> - unsigned HOST_WIDE_INT >> - patch_area_size, >> - bool record_p, >> - unsigned int flags) >> +default_print_patchable_function_entry (FILE *file, >> + unsigned HOST_WIDE_INT patch_area_size, >> + bool record_p) >> { >> const char *nop_templ = 0; >> int code_num; >> @@ -2001,13 +2003,17 @@ default_print_patchable_function_entry_1 (FILE *file, >> if (record_p && targetm_common.have_named_sections) >> { >> char buf[256]; >> - static int patch_area_number; >> section *previous_section = in_section; >> const char *asm_op = integer_asm_op (POINTER_SIZE_UNITS, false); >> >> gcc_assert (asm_op != NULL); >> - patch_area_number++; >> - ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number); >> + /* If SECTION_LINK_ORDER is supported, this internal label will >> + be filled as the symbol for linked_to section. */ >> + ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", current_function_funcdef_no); >> + >> + unsigned int flags = SECTION_WRITE | SECTION_RELRO; >> + if (HAVE_GAS_SECTION_LINK_ORDER) >> + flags |= SECTION_LINK_ORDER; >> >> section *sect = get_section ("__patchable_function_entries", >> flags, current_function_decl); >> @@ -2029,25 +2035,6 @@ default_print_patchable_function_entry_1 (FILE *file, >> output_asm_insn (nop_templ, NULL); >> } >> >> -/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function >> - entry. If RECORD_P is true and the target supports named sections, >> - the location of the NOPs will be recorded in a special object section >> - called "__patchable_function_entries". This routine may be called >> - twice per function to put NOPs before and after the function >> - entry. */ >> - >> -void >> -default_print_patchable_function_entry (FILE *file, >> - unsigned HOST_WIDE_INT patch_area_size, >> - bool record_p) >> -{ >> - unsigned int flags = SECTION_WRITE | SECTION_RELRO; >> - if (HAVE_GAS_SECTION_LINK_ORDER) >> - flags |= SECTION_LINK_ORDER; >> - default_print_patchable_function_entry_1 (file, patch_area_size, record_p, >> - flags); >> -} >> - >> bool >> default_profile_before_prologue (void) >> { >> diff --git a/gcc/targhooks.h b/gcc/targhooks.h >> index ecce55ebe79..5c1216fad0b 100644 >> --- a/gcc/targhooks.h >> +++ b/gcc/targhooks.h >> @@ -229,9 +229,6 @@ extern bool default_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT, >> bool); >> extern int default_compare_by_pieces_branch_ratio (machine_mode); >> >> -extern void default_print_patchable_function_entry_1 (FILE *, >> - unsigned HOST_WIDE_INT, >> - bool, unsigned int); >> extern void default_print_patchable_function_entry (FILE *, >> unsigned HOST_WIDE_INT, >> bool); >> diff --git a/gcc/testsuite/g++.dg/pr93195a.C b/gcc/testsuite/g++.dg/pr93195a.C >> index b14f1b3e341..26d265da74e 100644 >> --- a/gcc/testsuite/g++.dg/pr93195a.C >> +++ b/gcc/testsuite/g++.dg/pr93195a.C >> @@ -1,5 +1,4 @@ >> /* { dg-do link { target { ! { nvptx*-*-* visium-*-* } } } } */ >> -/* { dg-skip-if "not supported" { { powerpc*-*-* } && lp64 } } */ >> // { dg-require-effective-target o_flag_in_section } >> /* { dg-options "-O0 -fpatchable-function-entry=1" } */ >> /* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */ >> diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c >> index 0e75657a153..12465213aef 100644 >> --- a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c >> +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c >> @@ -1,7 +1,7 @@ >> /* { dg-do "compile" } */ >> /* { dg-options "-O1" } */ >> >> -/* Test the placement of the .LPFE1 label. */ >> +/* Test the placement of the .LPFE0 label. */ >> >> void >> __attribute__ ((target("branch-protection=bti"), >> @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti"), >> f10_bti () >> { >> } >> -/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti c\n.*\.LPFE1:\n\tnop\n.*\tret\n" } } */ >> +/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\tret\n" } } */ >> diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c >> index 0a1f74d4096..2c6a73789f0 100644 >> --- a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c >> +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c >> @@ -1,7 +1,7 @@ >> /* { dg-do "compile" } */ >> /* { dg-options "-O1" } */ >> >> -/* Test the placement of the .LPFE1 label. */ >> +/* Test the placement of the .LPFE0 label. */ >> >> void >> __attribute__ ((target("branch-protection=bti+pac-ret+leaf"), >> @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti+pac-ret+leaf"), >> f10_pac () >> { >> } >> -/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti c\n.*\.LPFE1:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */ >> +/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */ >> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-2.c b/gcc/testsuite/gcc.target/i386/pr93492-2.c >> index 3d67095fd10..ede8c2077b7 100644 >> --- a/gcc/testsuite/gcc.target/i386/pr93492-2.c >> +++ b/gcc/testsuite/gcc.target/i386/pr93492-2.c >> @@ -1,7 +1,7 @@ >> /* { dg-do "compile" { target *-*-linux* } } */ >> /* { dg-options "-O1 -fcf-protection -mmanual-endbr -fasynchronous-unwind-tables" } */ >> >> -/* Test the placement of the .LPFE1 label. */ >> +/* Test the placement of the .LPFE0 label. */ >> >> void >> __attribute__ ((cf_check,patchable_function_entry (1, 0))) >> @@ -9,4 +9,4 @@ f10_endbr (void) >> { >> } >> >> -/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n\tret\n" } } */ >> +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE0:\n\tnop\n\tret\n" } } */ >> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-3.c b/gcc/testsuite/gcc.target/i386/pr93492-3.c >> index a625c927f4f..b68da30bd36 100644 >> --- a/gcc/testsuite/gcc.target/i386/pr93492-3.c >> +++ b/gcc/testsuite/gcc.target/i386/pr93492-3.c >> @@ -2,7 +2,7 @@ >> /* { dg-require-effective-target mfentry } */ >> /* { dg-options "-O1 -fcf-protection -mmanual-endbr -mfentry -pg -fasynchronous-unwind-tables" } */ >> >> -/* Test the placement of the .LPFE1 label. */ >> +/* Test the placement of the .LPFE0 label. */ >> >> void >> __attribute__ ((cf_check,patchable_function_entry (1, 0))) >> @@ -10,4 +10,4 @@ f10_endbr (void) >> { >> } >> >> -/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ >> +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE0:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ >> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-4.c b/gcc/testsuite/gcc.target/i386/pr93492-4.c >> index 8f205c345b8..c73034a4624 100644 >> --- a/gcc/testsuite/gcc.target/i386/pr93492-4.c >> +++ b/gcc/testsuite/gcc.target/i386/pr93492-4.c >> @@ -1,11 +1,11 @@ >> /* { dg-do "compile" { target *-*-linux* } } */ >> /* { dg-options "-O1 -fpatchable-function-entry=1 -fasynchronous-unwind-tables" } */ >> >> -/* Test the placement of the .LPFE1 label. */ >> +/* Test the placement of the .LPFE0 label. */ >> >> void >> foo (void) >> { >> } >> >> -/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n\tret\n" } } */ >> +/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n\tret\n" } } */ >> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-5.c b/gcc/testsuite/gcc.target/i386/pr93492-5.c >> index 1ca5ba1fac1..ee9849ae852 100644 >> --- a/gcc/testsuite/gcc.target/i386/pr93492-5.c >> +++ b/gcc/testsuite/gcc.target/i386/pr93492-5.c >> @@ -2,11 +2,11 @@ >> /* { dg-options "-O1 -fpatchable-function-entry=1 -mfentry -pg -fasynchronous-unwind-tables" } */ >> /* { dg-additional-options "-fno-PIE" { target ia32 } } */ >> >> -/* Test the placement of the .LPFE1 label. */ >> +/* Test the placement of the .LPFE0 label. */ >> >> void >> foo (void) >> { >> } >> >> -/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ >> +/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ >> -- >
"Kewen.Lin" <linkw@linux.ibm.com> writes: > Hi, > > Gentle ping: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600190.html > > Any comments are highly appreciated. > > BR, > Kewen > > on 2022/9/28 13:41, Kewen.Lin via Gcc-patches wrote: >> Hi, >> >> Gentle ping: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600190.html >> >> BR, >> Kewen >> >> on 2022/8/24 16:17, Kewen.Lin via Gcc-patches wrote: >>> Hi, >>> >>> As discussed in PR98125, -fpatchable-function-entry with >>> SECTION_LINK_ORDER support doesn't work well on powerpc64 >>> ELFv1 because the filled "Symbol" in >>> >>> .section name,"flags"o,@type,Symbol >>> >>> sits in .opd section instead of in the function_section >>> like .text or named .text*. >>> >>> Since we already generates one label LPFE* which sits in >>> function_section of current_function_decl, this patch is >>> to reuse it as the symbol for the linked_to section. It >>> avoids the above ABI specific issue when using the symbol >>> concluded from current_function_decl. >>> >>> Besides, with this support some previous workarounds for >>> powerpc64 ELFv1 can be reverted. >>> >>> btw, rs6000_print_patchable_function_entry can be dropped >>> but there is another rs6000 patch which needs this rs6000 >>> specific hook rs6000_print_patchable_function_entry, not >>> sure which one gets landed first, so just leave it here. >>> >>> Bootstrapped and regtested on below: >>> >>> 1) powerpc64-linux-gnu P8 with default binutils 2.27 >>> and latest binutils 2.39. >>> 2) powerpc64le-linux-gnu P9 (default binutils 2.30). >>> 3) powerpc64le-linux-gnu P10 (default binutils 2.30). >>> 4) x86_64-redhat-linux with default binutils 2.30 >>> and latest binutils 2.39. >>> 5) aarch64-linux-gnu with default binutils 2.30 >>> and latest binutils 2.39. >>> >>> Is it ok for trunk? >>> >>> BR, >>> Kewen >>> ----- >>> >>> PR target/99889 >>> >>> gcc/ChangeLog: >>> >>> * config/rs6000/rs6000.cc (rs6000_print_patchable_function_entry): >>> Adjust to call function default_print_patchable_function_entry. >>> * targhooks.cc (default_print_patchable_function_entry_1): Remove and >>> move the flags preparation ... >>> (default_print_patchable_function_entry): ... here, adjust to use >>> current_function_funcdef_no for label no. >>> * targhooks.h (default_print_patchable_function_entry_1): Remove. >>> * varasm.cc (default_elf_asm_named_section): Adjust code for >>> __patchable_function_entries section support with LPFE label. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/pr93195a.C: Remove the skip on powerpc*-*-* 64-bit. >>> * gcc.target/aarch64/pr92424-2.c: Adjust LPFE1 with LPFE0. >>> * gcc.target/aarch64/pr92424-3.c: Likewise. >>> * gcc.target/i386/pr93492-2.c: Likewise. >>> * gcc.target/i386/pr93492-3.c: Likewise. >>> * gcc.target/i386/pr93492-4.c: Likewise. >>> * gcc.target/i386/pr93492-5.c: Likewise. >>> --- >>> gcc/config/rs6000/rs6000.cc | 13 +----- >>> gcc/varasm.cc | 15 ++++--- >>> gcc/targhooks.cc | 45 +++++++------------- >>> gcc/targhooks.h | 3 -- >>> gcc/testsuite/g++.dg/pr93195a.C | 1 - >>> gcc/testsuite/gcc.target/aarch64/pr92424-2.c | 4 +- >>> gcc/testsuite/gcc.target/aarch64/pr92424-3.c | 4 +- >>> gcc/testsuite/gcc.target/i386/pr93492-2.c | 4 +- >>> gcc/testsuite/gcc.target/i386/pr93492-3.c | 4 +- >>> gcc/testsuite/gcc.target/i386/pr93492-4.c | 4 +- >>> gcc/testsuite/gcc.target/i386/pr93492-5.c | 4 +- >>> 11 files changed, 40 insertions(+), 61 deletions(-) >>> >>> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc >>> index df491bee2ea..dba28b8e647 100644 >>> --- a/gcc/config/rs6000/rs6000.cc >>> +++ b/gcc/config/rs6000/rs6000.cc >>> @@ -14771,18 +14771,9 @@ rs6000_print_patchable_function_entry (FILE *file, >>> unsigned HOST_WIDE_INT patch_area_size, >>> bool record_p) >>> { >>> - unsigned int flags = SECTION_WRITE | SECTION_RELRO; >>> - /* When .opd section is emitted, the function symbol >>> - default_print_patchable_function_entry_1 is emitted into the .opd section >>> - while the patchable area is emitted into the function section. >>> - Don't use SECTION_LINK_ORDER in that case. */ >>> - if (!(TARGET_64BIT && DEFAULT_ABI != ABI_ELFv2) >>> - && HAVE_GAS_SECTION_LINK_ORDER) >>> - flags |= SECTION_LINK_ORDER; >>> - default_print_patchable_function_entry_1 (file, patch_area_size, record_p, >>> - flags); >>> + default_print_patchable_function_entry (file, patch_area_size, record_p); >>> } >>> - >>> >>> + >>> enum rtx_code >>> rs6000_reverse_condition (machine_mode mode, enum rtx_code code) >>> { >>> diff --git a/gcc/varasm.cc b/gcc/varasm.cc >>> index 4db8506b106..d4de6e164ee 100644 >>> --- a/gcc/varasm.cc >>> +++ b/gcc/varasm.cc >>> @@ -6906,11 +6906,16 @@ default_elf_asm_named_section (const char *name, unsigned int flags, >>> fprintf (asm_out_file, ",%d", flags & SECTION_ENTSIZE); >>> if (flags & SECTION_LINK_ORDER) >>> { >>> - tree id = DECL_ASSEMBLER_NAME (decl); >>> - ultimate_transparent_alias_target (&id); >>> - const char *name = IDENTIFIER_POINTER (id); >>> - name = targetm.strip_name_encoding (name); >>> - fprintf (asm_out_file, ",%s", name); >>> + /* For now, only section "__patchable_function_entries" >>> + adopts flag SECTION_LINK_ORDER, internal label LPFE* >>> + was emitted in default_print_patchable_function_entry, >>> + just place it here for linked_to section. */ >>> + gcc_assert (!strcmp (name, "__patchable_function_entries")); I like the idea of removing the rs600 workaround in favour of making the target-independent more robust. But this seems a bit hackish. What would we do if SECTION_LINK_ORDER was used for something else in future? Thanks, Richard >>> + fprintf (asm_out_file, ","); >>> + char buf[256]; >>> + ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", >>> + current_function_funcdef_no); >>> + assemble_name_raw (asm_out_file, buf); >>> } >>> if (HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE)) >>> { >>> diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc >>> index b15ae19bcb6..e80caecc418 100644 >>> --- a/gcc/targhooks.cc >>> +++ b/gcc/targhooks.cc >>> @@ -1979,15 +1979,17 @@ default_compare_by_pieces_branch_ratio (machine_mode) >>> return 1; >>> } >>> >>> -/* Helper for default_print_patchable_function_entry and other >>> - print_patchable_function_entry hook implementations. */ >>> +/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function >>> + entry. If RECORD_P is true and the target supports named sections, >>> + the location of the NOPs will be recorded in a special object section >>> + called "__patchable_function_entries". This routine may be called >>> + twice per function to put NOPs before and after the function >>> + entry. */ >>> >>> void >>> -default_print_patchable_function_entry_1 (FILE *file, >>> - unsigned HOST_WIDE_INT >>> - patch_area_size, >>> - bool record_p, >>> - unsigned int flags) >>> +default_print_patchable_function_entry (FILE *file, >>> + unsigned HOST_WIDE_INT patch_area_size, >>> + bool record_p) >>> { >>> const char *nop_templ = 0; >>> int code_num; >>> @@ -2001,13 +2003,17 @@ default_print_patchable_function_entry_1 (FILE *file, >>> if (record_p && targetm_common.have_named_sections) >>> { >>> char buf[256]; >>> - static int patch_area_number; >>> section *previous_section = in_section; >>> const char *asm_op = integer_asm_op (POINTER_SIZE_UNITS, false); >>> >>> gcc_assert (asm_op != NULL); >>> - patch_area_number++; >>> - ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number); >>> + /* If SECTION_LINK_ORDER is supported, this internal label will >>> + be filled as the symbol for linked_to section. */ >>> + ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", current_function_funcdef_no); >>> + >>> + unsigned int flags = SECTION_WRITE | SECTION_RELRO; >>> + if (HAVE_GAS_SECTION_LINK_ORDER) >>> + flags |= SECTION_LINK_ORDER; >>> >>> section *sect = get_section ("__patchable_function_entries", >>> flags, current_function_decl); >>> @@ -2029,25 +2035,6 @@ default_print_patchable_function_entry_1 (FILE *file, >>> output_asm_insn (nop_templ, NULL); >>> } >>> >>> -/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function >>> - entry. If RECORD_P is true and the target supports named sections, >>> - the location of the NOPs will be recorded in a special object section >>> - called "__patchable_function_entries". This routine may be called >>> - twice per function to put NOPs before and after the function >>> - entry. */ >>> - >>> -void >>> -default_print_patchable_function_entry (FILE *file, >>> - unsigned HOST_WIDE_INT patch_area_size, >>> - bool record_p) >>> -{ >>> - unsigned int flags = SECTION_WRITE | SECTION_RELRO; >>> - if (HAVE_GAS_SECTION_LINK_ORDER) >>> - flags |= SECTION_LINK_ORDER; >>> - default_print_patchable_function_entry_1 (file, patch_area_size, record_p, >>> - flags); >>> -} >>> - >>> bool >>> default_profile_before_prologue (void) >>> { >>> diff --git a/gcc/targhooks.h b/gcc/targhooks.h >>> index ecce55ebe79..5c1216fad0b 100644 >>> --- a/gcc/targhooks.h >>> +++ b/gcc/targhooks.h >>> @@ -229,9 +229,6 @@ extern bool default_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT, >>> bool); >>> extern int default_compare_by_pieces_branch_ratio (machine_mode); >>> >>> -extern void default_print_patchable_function_entry_1 (FILE *, >>> - unsigned HOST_WIDE_INT, >>> - bool, unsigned int); >>> extern void default_print_patchable_function_entry (FILE *, >>> unsigned HOST_WIDE_INT, >>> bool); >>> diff --git a/gcc/testsuite/g++.dg/pr93195a.C b/gcc/testsuite/g++.dg/pr93195a.C >>> index b14f1b3e341..26d265da74e 100644 >>> --- a/gcc/testsuite/g++.dg/pr93195a.C >>> +++ b/gcc/testsuite/g++.dg/pr93195a.C >>> @@ -1,5 +1,4 @@ >>> /* { dg-do link { target { ! { nvptx*-*-* visium-*-* } } } } */ >>> -/* { dg-skip-if "not supported" { { powerpc*-*-* } && lp64 } } */ >>> // { dg-require-effective-target o_flag_in_section } >>> /* { dg-options "-O0 -fpatchable-function-entry=1" } */ >>> /* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */ >>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c >>> index 0e75657a153..12465213aef 100644 >>> --- a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c >>> +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c >>> @@ -1,7 +1,7 @@ >>> /* { dg-do "compile" } */ >>> /* { dg-options "-O1" } */ >>> >>> -/* Test the placement of the .LPFE1 label. */ >>> +/* Test the placement of the .LPFE0 label. */ >>> >>> void >>> __attribute__ ((target("branch-protection=bti"), >>> @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti"), >>> f10_bti () >>> { >>> } >>> -/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti c\n.*\.LPFE1:\n\tnop\n.*\tret\n" } } */ >>> +/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\tret\n" } } */ >>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c >>> index 0a1f74d4096..2c6a73789f0 100644 >>> --- a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c >>> +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c >>> @@ -1,7 +1,7 @@ >>> /* { dg-do "compile" } */ >>> /* { dg-options "-O1" } */ >>> >>> -/* Test the placement of the .LPFE1 label. */ >>> +/* Test the placement of the .LPFE0 label. */ >>> >>> void >>> __attribute__ ((target("branch-protection=bti+pac-ret+leaf"), >>> @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti+pac-ret+leaf"), >>> f10_pac () >>> { >>> } >>> -/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti c\n.*\.LPFE1:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */ >>> +/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */ >>> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-2.c b/gcc/testsuite/gcc.target/i386/pr93492-2.c >>> index 3d67095fd10..ede8c2077b7 100644 >>> --- a/gcc/testsuite/gcc.target/i386/pr93492-2.c >>> +++ b/gcc/testsuite/gcc.target/i386/pr93492-2.c >>> @@ -1,7 +1,7 @@ >>> /* { dg-do "compile" { target *-*-linux* } } */ >>> /* { dg-options "-O1 -fcf-protection -mmanual-endbr -fasynchronous-unwind-tables" } */ >>> >>> -/* Test the placement of the .LPFE1 label. */ >>> +/* Test the placement of the .LPFE0 label. */ >>> >>> void >>> __attribute__ ((cf_check,patchable_function_entry (1, 0))) >>> @@ -9,4 +9,4 @@ f10_endbr (void) >>> { >>> } >>> >>> -/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n\tret\n" } } */ >>> +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE0:\n\tnop\n\tret\n" } } */ >>> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-3.c b/gcc/testsuite/gcc.target/i386/pr93492-3.c >>> index a625c927f4f..b68da30bd36 100644 >>> --- a/gcc/testsuite/gcc.target/i386/pr93492-3.c >>> +++ b/gcc/testsuite/gcc.target/i386/pr93492-3.c >>> @@ -2,7 +2,7 @@ >>> /* { dg-require-effective-target mfentry } */ >>> /* { dg-options "-O1 -fcf-protection -mmanual-endbr -mfentry -pg -fasynchronous-unwind-tables" } */ >>> >>> -/* Test the placement of the .LPFE1 label. */ >>> +/* Test the placement of the .LPFE0 label. */ >>> >>> void >>> __attribute__ ((cf_check,patchable_function_entry (1, 0))) >>> @@ -10,4 +10,4 @@ f10_endbr (void) >>> { >>> } >>> >>> -/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ >>> +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE0:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ >>> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-4.c b/gcc/testsuite/gcc.target/i386/pr93492-4.c >>> index 8f205c345b8..c73034a4624 100644 >>> --- a/gcc/testsuite/gcc.target/i386/pr93492-4.c >>> +++ b/gcc/testsuite/gcc.target/i386/pr93492-4.c >>> @@ -1,11 +1,11 @@ >>> /* { dg-do "compile" { target *-*-linux* } } */ >>> /* { dg-options "-O1 -fpatchable-function-entry=1 -fasynchronous-unwind-tables" } */ >>> >>> -/* Test the placement of the .LPFE1 label. */ >>> +/* Test the placement of the .LPFE0 label. */ >>> >>> void >>> foo (void) >>> { >>> } >>> >>> -/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n\tret\n" } } */ >>> +/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n\tret\n" } } */ >>> diff --git a/gcc/testsuite/gcc.target/i386/pr93492-5.c b/gcc/testsuite/gcc.target/i386/pr93492-5.c >>> index 1ca5ba1fac1..ee9849ae852 100644 >>> --- a/gcc/testsuite/gcc.target/i386/pr93492-5.c >>> +++ b/gcc/testsuite/gcc.target/i386/pr93492-5.c >>> @@ -2,11 +2,11 @@ >>> /* { dg-options "-O1 -fpatchable-function-entry=1 -mfentry -pg -fasynchronous-unwind-tables" } */ >>> /* { dg-additional-options "-fno-PIE" { target ia32 } } */ >>> >>> -/* Test the placement of the .LPFE1 label. */ >>> +/* Test the placement of the .LPFE0 label. */ >>> >>> void >>> foo (void) >>> { >>> } >>> >>> -/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ >>> +/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ >>> -- >>
Hi Richard, Many thanks for your review comments! >>> on 2022/8/24 16:17, Kewen.Lin via Gcc-patches wrote: >>>> Hi, >>>> >>>> As discussed in PR98125, -fpatchable-function-entry with >>>> SECTION_LINK_ORDER support doesn't work well on powerpc64 >>>> ELFv1 because the filled "Symbol" in >>>> >>>> .section name,"flags"o,@type,Symbol >>>> >>>> sits in .opd section instead of in the function_section >>>> like .text or named .text*. >>>> >>>> Since we already generates one label LPFE* which sits in >>>> function_section of current_function_decl, this patch is >>>> to reuse it as the symbol for the linked_to section. It >>>> avoids the above ABI specific issue when using the symbol >>>> concluded from current_function_decl. >>>> >>>> Besides, with this support some previous workarounds for >>>> powerpc64 ELFv1 can be reverted. >>>> >>>> btw, rs6000_print_patchable_function_entry can be dropped >>>> but there is another rs6000 patch which needs this rs6000 >>>> specific hook rs6000_print_patchable_function_entry, not >>>> sure which one gets landed first, so just leave it here. >>>> >>>> Bootstrapped and regtested on below: >>>> >>>> 1) powerpc64-linux-gnu P8 with default binutils 2.27 >>>> and latest binutils 2.39. >>>> 2) powerpc64le-linux-gnu P9 (default binutils 2.30). >>>> 3) powerpc64le-linux-gnu P10 (default binutils 2.30). >>>> 4) x86_64-redhat-linux with default binutils 2.30 >>>> and latest binutils 2.39. >>>> 5) aarch64-linux-gnu with default binutils 2.30 >>>> and latest binutils 2.39. >>>> [snip...] >>>> diff --git a/gcc/varasm.cc b/gcc/varasm.cc >>>> index 4db8506b106..d4de6e164ee 100644 >>>> --- a/gcc/varasm.cc >>>> +++ b/gcc/varasm.cc >>>> @@ -6906,11 +6906,16 @@ default_elf_asm_named_section (const char *name, unsigned int flags, >>>> fprintf (asm_out_file, ",%d", flags & SECTION_ENTSIZE); >>>> if (flags & SECTION_LINK_ORDER) >>>> { >>>> - tree id = DECL_ASSEMBLER_NAME (decl); >>>> - ultimate_transparent_alias_target (&id); >>>> - const char *name = IDENTIFIER_POINTER (id); >>>> - name = targetm.strip_name_encoding (name); >>>> - fprintf (asm_out_file, ",%s", name); >>>> + /* For now, only section "__patchable_function_entries" >>>> + adopts flag SECTION_LINK_ORDER, internal label LPFE* >>>> + was emitted in default_print_patchable_function_entry, >>>> + just place it here for linked_to section. */ >>>> + gcc_assert (!strcmp (name, "__patchable_function_entries")); > > I like the idea of removing the rs600 workaround in favour of making the > target-independent more robust. But this seems a bit hackish. What > would we do if SECTION_LINK_ORDER was used for something else in future? > Good question! I think it depends on how we can get the symbol for the linked_to section, if adopting the name of the decl will suffer the similar issue which this patch wants to fix, we have to reuse the label LPFE* or some kind of new artificial label in the related section; or we can just go with the name of the given decl, or something related to that decl. Since we can't predict any future uses, I just placed an assertion here to ensure that we would revisit and adjust this part at that time. Does it sound reasonable to you? BR, Kewen
"Kewen.Lin" <linkw@linux.ibm.com> writes: > Hi Richard, > > Many thanks for your review comments! > >>>> on 2022/8/24 16:17, Kewen.Lin via Gcc-patches wrote: >>>>> Hi, >>>>> >>>>> As discussed in PR98125, -fpatchable-function-entry with >>>>> SECTION_LINK_ORDER support doesn't work well on powerpc64 >>>>> ELFv1 because the filled "Symbol" in >>>>> >>>>> .section name,"flags"o,@type,Symbol >>>>> >>>>> sits in .opd section instead of in the function_section >>>>> like .text or named .text*. >>>>> >>>>> Since we already generates one label LPFE* which sits in >>>>> function_section of current_function_decl, this patch is >>>>> to reuse it as the symbol for the linked_to section. It >>>>> avoids the above ABI specific issue when using the symbol >>>>> concluded from current_function_decl. >>>>> >>>>> Besides, with this support some previous workarounds for >>>>> powerpc64 ELFv1 can be reverted. >>>>> >>>>> btw, rs6000_print_patchable_function_entry can be dropped >>>>> but there is another rs6000 patch which needs this rs6000 >>>>> specific hook rs6000_print_patchable_function_entry, not >>>>> sure which one gets landed first, so just leave it here. >>>>> >>>>> Bootstrapped and regtested on below: >>>>> >>>>> 1) powerpc64-linux-gnu P8 with default binutils 2.27 >>>>> and latest binutils 2.39. >>>>> 2) powerpc64le-linux-gnu P9 (default binutils 2.30). >>>>> 3) powerpc64le-linux-gnu P10 (default binutils 2.30). >>>>> 4) x86_64-redhat-linux with default binutils 2.30 >>>>> and latest binutils 2.39. >>>>> 5) aarch64-linux-gnu with default binutils 2.30 >>>>> and latest binutils 2.39. >>>>> > > [snip...] > >>>>> diff --git a/gcc/varasm.cc b/gcc/varasm.cc >>>>> index 4db8506b106..d4de6e164ee 100644 >>>>> --- a/gcc/varasm.cc >>>>> +++ b/gcc/varasm.cc >>>>> @@ -6906,11 +6906,16 @@ default_elf_asm_named_section (const char *name, unsigned int flags, >>>>> fprintf (asm_out_file, ",%d", flags & SECTION_ENTSIZE); >>>>> if (flags & SECTION_LINK_ORDER) >>>>> { >>>>> - tree id = DECL_ASSEMBLER_NAME (decl); >>>>> - ultimate_transparent_alias_target (&id); >>>>> - const char *name = IDENTIFIER_POINTER (id); >>>>> - name = targetm.strip_name_encoding (name); >>>>> - fprintf (asm_out_file, ",%s", name); >>>>> + /* For now, only section "__patchable_function_entries" >>>>> + adopts flag SECTION_LINK_ORDER, internal label LPFE* >>>>> + was emitted in default_print_patchable_function_entry, >>>>> + just place it here for linked_to section. */ >>>>> + gcc_assert (!strcmp (name, "__patchable_function_entries")); >> >> I like the idea of removing the rs600 workaround in favour of making the >> target-independent more robust. But this seems a bit hackish. What >> would we do if SECTION_LINK_ORDER was used for something else in future? >> > > Good question! I think it depends on how we can get the symbol for the > linked_to section, if adopting the name of the decl will suffer the > similar issue which this patch wants to fix, we have to reuse the label > LPFE* or some kind of new artificial label in the related section; or > we can just go with the name of the given decl, or something related to > that decl. Since we can't predict any future uses, I just placed an > assertion here to ensure that we would revisit and adjust this part at > that time. Does it sound reasonable to you? Yeah, I guess that's good enough. If the old scheme ends up being correct for some future use, we can make the new behaviour conditional on __patchable_function_entries. So yeah, the patch LGTM to me, thanks. Richard
Hi Richard, on 2022/11/23 00:08, Richard Sandiford wrote: > "Kewen.Lin" <linkw@linux.ibm.com> writes: >> Hi Richard, >> >> Many thanks for your review comments! >> >>>>> on 2022/8/24 16:17, Kewen.Lin via Gcc-patches wrote: >>>>>> Hi, >>>>>> >>>>>> As discussed in PR98125, -fpatchable-function-entry with >>>>>> SECTION_LINK_ORDER support doesn't work well on powerpc64 >>>>>> ELFv1 because the filled "Symbol" in >>>>>> >>>>>> .section name,"flags"o,@type,Symbol >>>>>> >>>>>> sits in .opd section instead of in the function_section >>>>>> like .text or named .text*. >>>>>> >>>>>> Since we already generates one label LPFE* which sits in >>>>>> function_section of current_function_decl, this patch is >>>>>> to reuse it as the symbol for the linked_to section. It >>>>>> avoids the above ABI specific issue when using the symbol >>>>>> concluded from current_function_decl. >>>>>> >>>>>> Besides, with this support some previous workarounds for >>>>>> powerpc64 ELFv1 can be reverted. >>>>>> >>>>>> btw, rs6000_print_patchable_function_entry can be dropped >>>>>> but there is another rs6000 patch which needs this rs6000 >>>>>> specific hook rs6000_print_patchable_function_entry, not >>>>>> sure which one gets landed first, so just leave it here. >>>>>> >>>>>> Bootstrapped and regtested on below: >>>>>> >>>>>> 1) powerpc64-linux-gnu P8 with default binutils 2.27 >>>>>> and latest binutils 2.39. >>>>>> 2) powerpc64le-linux-gnu P9 (default binutils 2.30). >>>>>> 3) powerpc64le-linux-gnu P10 (default binutils 2.30). >>>>>> 4) x86_64-redhat-linux with default binutils 2.30 >>>>>> and latest binutils 2.39. >>>>>> 5) aarch64-linux-gnu with default binutils 2.30 >>>>>> and latest binutils 2.39. >>>>>> >> >> [snip...] >> >>>>>> diff --git a/gcc/varasm.cc b/gcc/varasm.cc >>>>>> index 4db8506b106..d4de6e164ee 100644 >>>>>> --- a/gcc/varasm.cc >>>>>> +++ b/gcc/varasm.cc >>>>>> @@ -6906,11 +6906,16 @@ default_elf_asm_named_section (const char *name, unsigned int flags, >>>>>> fprintf (asm_out_file, ",%d", flags & SECTION_ENTSIZE); >>>>>> if (flags & SECTION_LINK_ORDER) >>>>>> { >>>>>> - tree id = DECL_ASSEMBLER_NAME (decl); >>>>>> - ultimate_transparent_alias_target (&id); >>>>>> - const char *name = IDENTIFIER_POINTER (id); >>>>>> - name = targetm.strip_name_encoding (name); >>>>>> - fprintf (asm_out_file, ",%s", name); >>>>>> + /* For now, only section "__patchable_function_entries" >>>>>> + adopts flag SECTION_LINK_ORDER, internal label LPFE* >>>>>> + was emitted in default_print_patchable_function_entry, >>>>>> + just place it here for linked_to section. */ >>>>>> + gcc_assert (!strcmp (name, "__patchable_function_entries")); >>> >>> I like the idea of removing the rs600 workaround in favour of making the >>> target-independent more robust. But this seems a bit hackish. What >>> would we do if SECTION_LINK_ORDER was used for something else in future? >>> >> >> Good question! I think it depends on how we can get the symbol for the >> linked_to section, if adopting the name of the decl will suffer the >> similar issue which this patch wants to fix, we have to reuse the label >> LPFE* or some kind of new artificial label in the related section; or >> we can just go with the name of the given decl, or something related to >> that decl. Since we can't predict any future uses, I just placed an >> assertion here to ensure that we would revisit and adjust this part at >> that time. Does it sound reasonable to you? > > Yeah, I guess that's good enough. If the old scheme ends up being > correct for some future use, we can make the new behaviour conditional > on __patchable_function_entries. Yes, we can check if the given section name is "__patchable_function_entries". > > So yeah, the patch LGTM to me, thanks. Thanks again! I rebased and re-tested it on x86/aarch64/powerpc64{,le}, just committed in r13-4294-gf120196382ac5a. BR, Kewen
On Thu, Nov 24, 2022 at 7:26 PM Kewen.Lin via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi Richard, > > on 2022/11/23 00:08, Richard Sandiford wrote: > > "Kewen.Lin" <linkw@linux.ibm.com> writes: > >> Hi Richard, > >> > >> Many thanks for your review comments! > >> > >>>>> on 2022/8/24 16:17, Kewen.Lin via Gcc-patches wrote: > >>>>>> Hi, > >>>>>> > >>>>>> As discussed in PR98125, -fpatchable-function-entry with > >>>>>> SECTION_LINK_ORDER support doesn't work well on powerpc64 > >>>>>> ELFv1 because the filled "Symbol" in > >>>>>> > >>>>>> .section name,"flags"o,@type,Symbol > >>>>>> > >>>>>> sits in .opd section instead of in the function_section > >>>>>> like .text or named .text*. > >>>>>> > >>>>>> Since we already generates one label LPFE* which sits in > >>>>>> function_section of current_function_decl, this patch is > >>>>>> to reuse it as the symbol for the linked_to section. It > >>>>>> avoids the above ABI specific issue when using the symbol > >>>>>> concluded from current_function_decl. > >>>>>> > >>>>>> Besides, with this support some previous workarounds for > >>>>>> powerpc64 ELFv1 can be reverted. > >>>>>> > >>>>>> btw, rs6000_print_patchable_function_entry can be dropped > >>>>>> but there is another rs6000 patch which needs this rs6000 > >>>>>> specific hook rs6000_print_patchable_function_entry, not > >>>>>> sure which one gets landed first, so just leave it here. > >>>>>> > >>>>>> Bootstrapped and regtested on below: > >>>>>> > >>>>>> 1) powerpc64-linux-gnu P8 with default binutils 2.27 > >>>>>> and latest binutils 2.39. > >>>>>> 2) powerpc64le-linux-gnu P9 (default binutils 2.30). > >>>>>> 3) powerpc64le-linux-gnu P10 (default binutils 2.30). > >>>>>> 4) x86_64-redhat-linux with default binutils 2.30 > >>>>>> and latest binutils 2.39. > >>>>>> 5) aarch64-linux-gnu with default binutils 2.30 > >>>>>> and latest binutils 2.39. > >>>>>> > >> > >> [snip...] > >> > >>>>>> diff --git a/gcc/varasm.cc b/gcc/varasm.cc > >>>>>> index 4db8506b106..d4de6e164ee 100644 > >>>>>> --- a/gcc/varasm.cc > >>>>>> +++ b/gcc/varasm.cc > >>>>>> @@ -6906,11 +6906,16 @@ default_elf_asm_named_section (const char *name, unsigned int flags, > >>>>>> fprintf (asm_out_file, ",%d", flags & SECTION_ENTSIZE); > >>>>>> if (flags & SECTION_LINK_ORDER) > >>>>>> { > >>>>>> - tree id = DECL_ASSEMBLER_NAME (decl); > >>>>>> - ultimate_transparent_alias_target (&id); > >>>>>> - const char *name = IDENTIFIER_POINTER (id); > >>>>>> - name = targetm.strip_name_encoding (name); > >>>>>> - fprintf (asm_out_file, ",%s", name); > >>>>>> + /* For now, only section "__patchable_function_entries" > >>>>>> + adopts flag SECTION_LINK_ORDER, internal label LPFE* > >>>>>> + was emitted in default_print_patchable_function_entry, > >>>>>> + just place it here for linked_to section. */ > >>>>>> + gcc_assert (!strcmp (name, "__patchable_function_entries")); > >>> > >>> I like the idea of removing the rs600 workaround in favour of making the > >>> target-independent more robust. But this seems a bit hackish. What > >>> would we do if SECTION_LINK_ORDER was used for something else in future? > >>> > >> > >> Good question! I think it depends on how we can get the symbol for the > >> linked_to section, if adopting the name of the decl will suffer the > >> similar issue which this patch wants to fix, we have to reuse the label > >> LPFE* or some kind of new artificial label in the related section; or > >> we can just go with the name of the given decl, or something related to > >> that decl. Since we can't predict any future uses, I just placed an > >> assertion here to ensure that we would revisit and adjust this part at > >> that time. Does it sound reasonable to you? > > > > Yeah, I guess that's good enough. If the old scheme ends up being > > correct for some future use, we can make the new behaviour conditional > > on __patchable_function_entries. > > Yes, we can check if the given section name is > "__patchable_function_entries". > > > > > So yeah, the patch LGTM to me, thanks. > > Thanks again! I rebased and re-tested it on x86/aarch64/powerpc64{,le}, > just committed in r13-4294-gf120196382ac5a. > > BR, > Kewen Hi, Kewen, do you think whether your patch fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110729 (__patchable_function_entries has wrong sh_link) ? If yes, it may be useful to include some assembly tests... Right now rg '\.section.*__patchable' gcc/testsuite/ returns nothing.
Hi Fangrui, on 2023/7/19 14:33, Fangrui Song wrote: > On Thu, Nov 24, 2022 at 7:26 PM Kewen.Lin via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> Hi Richard, >> >> on 2022/11/23 00:08, Richard Sandiford wrote: >>> "Kewen.Lin" <linkw@linux.ibm.com> writes: >>>> Hi Richard, >>>> >>>> Many thanks for your review comments! >>>> >>>>>>> on 2022/8/24 16:17, Kewen.Lin via Gcc-patches wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> As discussed in PR98125, -fpatchable-function-entry with >>>>>>>> SECTION_LINK_ORDER support doesn't work well on powerpc64 >>>>>>>> ELFv1 because the filled "Symbol" in >>>>>>>> >>>>>>>> .section name,"flags"o,@type,Symbol >>>>>>>> >>>>>>>> sits in .opd section instead of in the function_section >>>>>>>> like .text or named .text*. >>>>>>>> >>>>>>>> Since we already generates one label LPFE* which sits in >>>>>>>> function_section of current_function_decl, this patch is >>>>>>>> to reuse it as the symbol for the linked_to section. It >>>>>>>> avoids the above ABI specific issue when using the symbol >>>>>>>> concluded from current_function_decl. >>>>>>>> >>>>>>>> Besides, with this support some previous workarounds for >>>>>>>> powerpc64 ELFv1 can be reverted. >>>>>>>> >>>>>>>> btw, rs6000_print_patchable_function_entry can be dropped >>>>>>>> but there is another rs6000 patch which needs this rs6000 >>>>>>>> specific hook rs6000_print_patchable_function_entry, not >>>>>>>> sure which one gets landed first, so just leave it here. >>>>>>>> >>>>>>>> Bootstrapped and regtested on below: >>>>>>>> >>>>>>>> 1) powerpc64-linux-gnu P8 with default binutils 2.27 >>>>>>>> and latest binutils 2.39. >>>>>>>> 2) powerpc64le-linux-gnu P9 (default binutils 2.30). >>>>>>>> 3) powerpc64le-linux-gnu P10 (default binutils 2.30). >>>>>>>> 4) x86_64-redhat-linux with default binutils 2.30 >>>>>>>> and latest binutils 2.39. >>>>>>>> 5) aarch64-linux-gnu with default binutils 2.30 >>>>>>>> and latest binutils 2.39. >>>>>>>> >>>> >>>> [snip...] >>>> >>>>>>>> diff --git a/gcc/varasm.cc b/gcc/varasm.cc >>>>>>>> index 4db8506b106..d4de6e164ee 100644 >>>>>>>> --- a/gcc/varasm.cc >>>>>>>> +++ b/gcc/varasm.cc >>>>>>>> @@ -6906,11 +6906,16 @@ default_elf_asm_named_section (const char *name, unsigned int flags, >>>>>>>> fprintf (asm_out_file, ",%d", flags & SECTION_ENTSIZE); >>>>>>>> if (flags & SECTION_LINK_ORDER) >>>>>>>> { >>>>>>>> - tree id = DECL_ASSEMBLER_NAME (decl); >>>>>>>> - ultimate_transparent_alias_target (&id); >>>>>>>> - const char *name = IDENTIFIER_POINTER (id); >>>>>>>> - name = targetm.strip_name_encoding (name); >>>>>>>> - fprintf (asm_out_file, ",%s", name); >>>>>>>> + /* For now, only section "__patchable_function_entries" >>>>>>>> + adopts flag SECTION_LINK_ORDER, internal label LPFE* >>>>>>>> + was emitted in default_print_patchable_function_entry, >>>>>>>> + just place it here for linked_to section. */ >>>>>>>> + gcc_assert (!strcmp (name, "__patchable_function_entries")); >>>>> >>>>> I like the idea of removing the rs600 workaround in favour of making the >>>>> target-independent more robust. But this seems a bit hackish. What >>>>> would we do if SECTION_LINK_ORDER was used for something else in future? >>>>> >>>> >>>> Good question! I think it depends on how we can get the symbol for the >>>> linked_to section, if adopting the name of the decl will suffer the >>>> similar issue which this patch wants to fix, we have to reuse the label >>>> LPFE* or some kind of new artificial label in the related section; or >>>> we can just go with the name of the given decl, or something related to >>>> that decl. Since we can't predict any future uses, I just placed an >>>> assertion here to ensure that we would revisit and adjust this part at >>>> that time. Does it sound reasonable to you? >>> >>> Yeah, I guess that's good enough. If the old scheme ends up being >>> correct for some future use, we can make the new behaviour conditional >>> on __patchable_function_entries. >> >> Yes, we can check if the given section name is >> "__patchable_function_entries". >> >>> >>> So yeah, the patch LGTM to me, thanks. >> >> Thanks again! I rebased and re-tested it on x86/aarch64/powerpc64{,le}, >> just committed in r13-4294-gf120196382ac5a. >> >> BR, >> Kewen > > Hi, Kewen, do you think whether your patch fixed > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110729 > (__patchable_function_entries has wrong sh_link) ? I just had a check and confirmed that it did fix the wrong sh_link, in the past it always uses the decl saved in named.decl (always f for the test case in PR110729), with this patch, it switches to use the label in its corresponding .text* (function section). > If yes, it may be useful to include some assembly tests... Right now > > rg '\.section.*__patchable' gcc/testsuite/ > > returns nothing. It's a good idea to add some testing coverage, I'm going to make a test case by checking the given ".section.*__patchable_function_entries.*,\.LPFE[012]". Thanks for the suggestion! BR, Kewen
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc index df491bee2ea..dba28b8e647 100644 --- a/gcc/config/rs6000/rs6000.cc +++ b/gcc/config/rs6000/rs6000.cc @@ -14771,18 +14771,9 @@ rs6000_print_patchable_function_entry (FILE *file, unsigned HOST_WIDE_INT patch_area_size, bool record_p) { - unsigned int flags = SECTION_WRITE | SECTION_RELRO; - /* When .opd section is emitted, the function symbol - default_print_patchable_function_entry_1 is emitted into the .opd section - while the patchable area is emitted into the function section. - Don't use SECTION_LINK_ORDER in that case. */ - if (!(TARGET_64BIT && DEFAULT_ABI != ABI_ELFv2) - && HAVE_GAS_SECTION_LINK_ORDER) - flags |= SECTION_LINK_ORDER; - default_print_patchable_function_entry_1 (file, patch_area_size, record_p, - flags); + default_print_patchable_function_entry (file, patch_area_size, record_p); } - + enum rtx_code rs6000_reverse_condition (machine_mode mode, enum rtx_code code) { diff --git a/gcc/varasm.cc b/gcc/varasm.cc index 4db8506b106..d4de6e164ee 100644 --- a/gcc/varasm.cc +++ b/gcc/varasm.cc @@ -6906,11 +6906,16 @@ default_elf_asm_named_section (const char *name, unsigned int flags, fprintf (asm_out_file, ",%d", flags & SECTION_ENTSIZE); if (flags & SECTION_LINK_ORDER) { - tree id = DECL_ASSEMBLER_NAME (decl); - ultimate_transparent_alias_target (&id); - const char *name = IDENTIFIER_POINTER (id); - name = targetm.strip_name_encoding (name); - fprintf (asm_out_file, ",%s", name); + /* For now, only section "__patchable_function_entries" + adopts flag SECTION_LINK_ORDER, internal label LPFE* + was emitted in default_print_patchable_function_entry, + just place it here for linked_to section. */ + gcc_assert (!strcmp (name, "__patchable_function_entries")); + fprintf (asm_out_file, ","); + char buf[256]; + ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", + current_function_funcdef_no); + assemble_name_raw (asm_out_file, buf); } if (HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE)) { diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc index b15ae19bcb6..e80caecc418 100644 --- a/gcc/targhooks.cc +++ b/gcc/targhooks.cc @@ -1979,15 +1979,17 @@ default_compare_by_pieces_branch_ratio (machine_mode) return 1; } -/* Helper for default_print_patchable_function_entry and other - print_patchable_function_entry hook implementations. */ +/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function + entry. If RECORD_P is true and the target supports named sections, + the location of the NOPs will be recorded in a special object section + called "__patchable_function_entries". This routine may be called + twice per function to put NOPs before and after the function + entry. */ void -default_print_patchable_function_entry_1 (FILE *file, - unsigned HOST_WIDE_INT - patch_area_size, - bool record_p, - unsigned int flags) +default_print_patchable_function_entry (FILE *file, + unsigned HOST_WIDE_INT patch_area_size, + bool record_p) { const char *nop_templ = 0; int code_num; @@ -2001,13 +2003,17 @@ default_print_patchable_function_entry_1 (FILE *file, if (record_p && targetm_common.have_named_sections) { char buf[256]; - static int patch_area_number; section *previous_section = in_section; const char *asm_op = integer_asm_op (POINTER_SIZE_UNITS, false); gcc_assert (asm_op != NULL); - patch_area_number++; - ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", patch_area_number); + /* If SECTION_LINK_ORDER is supported, this internal label will + be filled as the symbol for linked_to section. */ + ASM_GENERATE_INTERNAL_LABEL (buf, "LPFE", current_function_funcdef_no); + + unsigned int flags = SECTION_WRITE | SECTION_RELRO; + if (HAVE_GAS_SECTION_LINK_ORDER) + flags |= SECTION_LINK_ORDER; section *sect = get_section ("__patchable_function_entries", flags, current_function_decl); @@ -2029,25 +2035,6 @@ default_print_patchable_function_entry_1 (FILE *file, output_asm_insn (nop_templ, NULL); } -/* Write PATCH_AREA_SIZE NOPs into the asm outfile FILE around a function - entry. If RECORD_P is true and the target supports named sections, - the location of the NOPs will be recorded in a special object section - called "__patchable_function_entries". This routine may be called - twice per function to put NOPs before and after the function - entry. */ - -void -default_print_patchable_function_entry (FILE *file, - unsigned HOST_WIDE_INT patch_area_size, - bool record_p) -{ - unsigned int flags = SECTION_WRITE | SECTION_RELRO; - if (HAVE_GAS_SECTION_LINK_ORDER) - flags |= SECTION_LINK_ORDER; - default_print_patchable_function_entry_1 (file, patch_area_size, record_p, - flags); -} - bool default_profile_before_prologue (void) { diff --git a/gcc/targhooks.h b/gcc/targhooks.h index ecce55ebe79..5c1216fad0b 100644 --- a/gcc/targhooks.h +++ b/gcc/targhooks.h @@ -229,9 +229,6 @@ extern bool default_use_by_pieces_infrastructure_p (unsigned HOST_WIDE_INT, bool); extern int default_compare_by_pieces_branch_ratio (machine_mode); -extern void default_print_patchable_function_entry_1 (FILE *, - unsigned HOST_WIDE_INT, - bool, unsigned int); extern void default_print_patchable_function_entry (FILE *, unsigned HOST_WIDE_INT, bool); diff --git a/gcc/testsuite/g++.dg/pr93195a.C b/gcc/testsuite/g++.dg/pr93195a.C index b14f1b3e341..26d265da74e 100644 --- a/gcc/testsuite/g++.dg/pr93195a.C +++ b/gcc/testsuite/g++.dg/pr93195a.C @@ -1,5 +1,4 @@ /* { dg-do link { target { ! { nvptx*-*-* visium-*-* } } } } */ -/* { dg-skip-if "not supported" { { powerpc*-*-* } && lp64 } } */ // { dg-require-effective-target o_flag_in_section } /* { dg-options "-O0 -fpatchable-function-entry=1" } */ /* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */ diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c index 0e75657a153..12465213aef 100644 --- a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c @@ -1,7 +1,7 @@ /* { dg-do "compile" } */ /* { dg-options "-O1" } */ -/* Test the placement of the .LPFE1 label. */ +/* Test the placement of the .LPFE0 label. */ void __attribute__ ((target("branch-protection=bti"), @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti"), f10_bti () { } -/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti c\n.*\.LPFE1:\n\tnop\n.*\tret\n" } } */ +/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\tret\n" } } */ diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c index 0a1f74d4096..2c6a73789f0 100644 --- a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c +++ b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c @@ -1,7 +1,7 @@ /* { dg-do "compile" } */ /* { dg-options "-O1" } */ -/* Test the placement of the .LPFE1 label. */ +/* Test the placement of the .LPFE0 label. */ void __attribute__ ((target("branch-protection=bti+pac-ret+leaf"), @@ -9,4 +9,4 @@ __attribute__ ((target("branch-protection=bti+pac-ret+leaf"), f10_pac () { } -/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti c\n.*\.LPFE1:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */ +/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti c\n.*\.LPFE0:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */ diff --git a/gcc/testsuite/gcc.target/i386/pr93492-2.c b/gcc/testsuite/gcc.target/i386/pr93492-2.c index 3d67095fd10..ede8c2077b7 100644 --- a/gcc/testsuite/gcc.target/i386/pr93492-2.c +++ b/gcc/testsuite/gcc.target/i386/pr93492-2.c @@ -1,7 +1,7 @@ /* { dg-do "compile" { target *-*-linux* } } */ /* { dg-options "-O1 -fcf-protection -mmanual-endbr -fasynchronous-unwind-tables" } */ -/* Test the placement of the .LPFE1 label. */ +/* Test the placement of the .LPFE0 label. */ void __attribute__ ((cf_check,patchable_function_entry (1, 0))) @@ -9,4 +9,4 @@ f10_endbr (void) { } -/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n\tret\n" } } */ +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE0:\n\tnop\n\tret\n" } } */ diff --git a/gcc/testsuite/gcc.target/i386/pr93492-3.c b/gcc/testsuite/gcc.target/i386/pr93492-3.c index a625c927f4f..b68da30bd36 100644 --- a/gcc/testsuite/gcc.target/i386/pr93492-3.c +++ b/gcc/testsuite/gcc.target/i386/pr93492-3.c @@ -2,7 +2,7 @@ /* { dg-require-effective-target mfentry } */ /* { dg-options "-O1 -fcf-protection -mmanual-endbr -mfentry -pg -fasynchronous-unwind-tables" } */ -/* Test the placement of the .LPFE1 label. */ +/* Test the placement of the .LPFE0 label. */ void __attribute__ ((cf_check,patchable_function_entry (1, 0))) @@ -10,4 +10,4 @@ f10_endbr (void) { } -/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE1:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ +/* { dg-final { scan-assembler "\t\.cfi_startproc\n\tendbr(32|64)\n.*\.LPFE0:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ diff --git a/gcc/testsuite/gcc.target/i386/pr93492-4.c b/gcc/testsuite/gcc.target/i386/pr93492-4.c index 8f205c345b8..c73034a4624 100644 --- a/gcc/testsuite/gcc.target/i386/pr93492-4.c +++ b/gcc/testsuite/gcc.target/i386/pr93492-4.c @@ -1,11 +1,11 @@ /* { dg-do "compile" { target *-*-linux* } } */ /* { dg-options "-O1 -fpatchable-function-entry=1 -fasynchronous-unwind-tables" } */ -/* Test the placement of the .LPFE1 label. */ +/* Test the placement of the .LPFE0 label. */ void foo (void) { } -/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n\tret\n" } } */ +/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n\tret\n" } } */ diff --git a/gcc/testsuite/gcc.target/i386/pr93492-5.c b/gcc/testsuite/gcc.target/i386/pr93492-5.c index 1ca5ba1fac1..ee9849ae852 100644 --- a/gcc/testsuite/gcc.target/i386/pr93492-5.c +++ b/gcc/testsuite/gcc.target/i386/pr93492-5.c @@ -2,11 +2,11 @@ /* { dg-options "-O1 -fpatchable-function-entry=1 -mfentry -pg -fasynchronous-unwind-tables" } */ /* { dg-additional-options "-fno-PIE" { target ia32 } } */ -/* Test the placement of the .LPFE1 label. */ +/* Test the placement of the .LPFE0 label. */ void foo (void) { } -/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE1:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */ +/* { dg-final { scan-assembler "\t\.cfi_startproc\n.*\.LPFE0:\n\tnop\n1:\tcall\t\[^\n\]*__fentry__\[^\n\]*\n\tret\n" } } */
Hi, As discussed in PR98125, -fpatchable-function-entry with SECTION_LINK_ORDER support doesn't work well on powerpc64 ELFv1 because the filled "Symbol" in .section name,"flags"o,@type,Symbol sits in .opd section instead of in the function_section like .text or named .text*. Since we already generates one label LPFE* which sits in function_section of current_function_decl, this patch is to reuse it as the symbol for the linked_to section. It avoids the above ABI specific issue when using the symbol concluded from current_function_decl. Besides, with this support some previous workarounds for powerpc64 ELFv1 can be reverted. btw, rs6000_print_patchable_function_entry can be dropped but there is another rs6000 patch which needs this rs6000 specific hook rs6000_print_patchable_function_entry, not sure which one gets landed first, so just leave it here. Bootstrapped and regtested on below: 1) powerpc64-linux-gnu P8 with default binutils 2.27 and latest binutils 2.39. 2) powerpc64le-linux-gnu P9 (default binutils 2.30). 3) powerpc64le-linux-gnu P10 (default binutils 2.30). 4) x86_64-redhat-linux with default binutils 2.30 and latest binutils 2.39. 5) aarch64-linux-gnu with default binutils 2.30 and latest binutils 2.39. Is it ok for trunk? BR, Kewen ----- PR target/99889 gcc/ChangeLog: * config/rs6000/rs6000.cc (rs6000_print_patchable_function_entry): Adjust to call function default_print_patchable_function_entry. * targhooks.cc (default_print_patchable_function_entry_1): Remove and move the flags preparation ... (default_print_patchable_function_entry): ... here, adjust to use current_function_funcdef_no for label no. * targhooks.h (default_print_patchable_function_entry_1): Remove. * varasm.cc (default_elf_asm_named_section): Adjust code for __patchable_function_entries section support with LPFE label. gcc/testsuite/ChangeLog: * g++.dg/pr93195a.C: Remove the skip on powerpc*-*-* 64-bit. * gcc.target/aarch64/pr92424-2.c: Adjust LPFE1 with LPFE0. * gcc.target/aarch64/pr92424-3.c: Likewise. * gcc.target/i386/pr93492-2.c: Likewise. * gcc.target/i386/pr93492-3.c: Likewise. * gcc.target/i386/pr93492-4.c: Likewise. * gcc.target/i386/pr93492-5.c: Likewise. --- gcc/config/rs6000/rs6000.cc | 13 +----- gcc/varasm.cc | 15 ++++--- gcc/targhooks.cc | 45 +++++++------------- gcc/targhooks.h | 3 -- gcc/testsuite/g++.dg/pr93195a.C | 1 - gcc/testsuite/gcc.target/aarch64/pr92424-2.c | 4 +- gcc/testsuite/gcc.target/aarch64/pr92424-3.c | 4 +- gcc/testsuite/gcc.target/i386/pr93492-2.c | 4 +- gcc/testsuite/gcc.target/i386/pr93492-3.c | 4 +- gcc/testsuite/gcc.target/i386/pr93492-4.c | 4 +- gcc/testsuite/gcc.target/i386/pr93492-5.c | 4 +- 11 files changed, 40 insertions(+), 61 deletions(-) --