From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id ED6F83853D60 for ; Mon, 21 Nov 2022 14:20:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org ED6F83853D60 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id EBACE1FB; Mon, 21 Nov 2022 06:20:11 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.62]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 07FA23F73B; Mon, 21 Nov 2022 06:20:03 -0800 (PST) From: Richard Sandiford To: "Kewen.Lin" Mail-Followup-To: "Kewen.Lin" ,GCC Patches , Jakub Jelinek , Segher Boessenkool , AlanM , Peter Bergner , jlaw@ventanamicro.com, David Edelsohn , Richard Biener , "H.J. Lu" , richard.sandiford@arm.com Cc: GCC Patches , Jakub Jelinek , Segher Boessenkool , AlanM , Peter Bergner , jlaw@ventanamicro.com, David Edelsohn , Richard Biener , "H.J. Lu" Subject: Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889] References: <0558633c-b553-5ef1-aa6f-c76fcf297454@linux.ibm.com> <52ca56ad-af0f-598f-4ccf-aed61fce67b4@linux.ibm.com> <15b488a5-1f5e-c24e-be12-f402b0dcdb5e@linux.ibm.com> Date: Mon, 21 Nov 2022 14:20:02 +0000 In-Reply-To: <15b488a5-1f5e-c24e-be12-f402b0dcdb5e@linux.ibm.com> (Kewen Lin's message of "Thu, 10 Nov 2022 16:15:43 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-40.2 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: "Kewen.Lin" 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" } } */ >>> -- >>