* [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
@ 2022-08-24 8:17 Kewen.Lin
2022-09-28 5:41 ` PING^1 " Kewen.Lin
2022-09-29 20:31 ` Segher Boessenkool
0 siblings, 2 replies; 12+ messages in thread
From: Kewen.Lin @ 2022-08-24 8:17 UTC (permalink / raw)
To: GCC Patches
Cc: H.J. Lu, Jakub Jelinek, Richard Biener, Segher Boessenkool,
AlanM, Peter Bergner, Richard Sandiford, Jeff Law,
David Edelsohn
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" } } */
--
^ permalink raw reply [flat|nested] 12+ messages in thread
* PING^1 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2022-08-24 8:17 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889] Kewen.Lin
@ 2022-09-28 5:41 ` Kewen.Lin
2022-11-10 8:15 ` PING^2 " Kewen.Lin
2022-09-29 20:31 ` Segher Boessenkool
1 sibling, 1 reply; 12+ messages in thread
From: Kewen.Lin @ 2022-09-28 5:41 UTC (permalink / raw)
To: GCC Patches
Cc: Jakub Jelinek, Segher Boessenkool, AlanM, Richard Sandiford,
Peter Bergner, Jeff Law, David Edelsohn, jlaw, H.J. Lu,
Richard Biener
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" } } */
> --
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2022-08-24 8:17 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889] Kewen.Lin
2022-09-28 5:41 ` PING^1 " Kewen.Lin
@ 2022-09-29 20:31 ` Segher Boessenkool
2022-09-30 12:47 ` Kewen.Lin
1 sibling, 1 reply; 12+ messages in thread
From: Segher Boessenkool @ 2022-09-29 20:31 UTC (permalink / raw)
To: Kewen.Lin
Cc: GCC Patches, H.J. Lu, Jakub Jelinek, Richard Biener, AlanM,
Peter Bergner, Richard Sandiford, Jeff Law, David Edelsohn
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2022-09-29 20:31 ` Segher Boessenkool
@ 2022-09-30 12:47 ` Kewen.Lin
2022-09-30 17:47 ` Segher Boessenkool
0 siblings, 1 reply; 12+ messages in thread
From: Kewen.Lin @ 2022-09-30 12:47 UTC (permalink / raw)
To: Segher Boessenkool
Cc: GCC Patches, H.J. Lu, Jakub Jelinek, Richard Biener, AlanM,
Peter Bergner, Richard Sandiford, David Edelsohn, jlaw
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2022-09-30 12:47 ` Kewen.Lin
@ 2022-09-30 17:47 ` Segher Boessenkool
0 siblings, 0 replies; 12+ messages in thread
From: Segher Boessenkool @ 2022-09-30 17:47 UTC (permalink / raw)
To: Kewen.Lin
Cc: GCC Patches, H.J. Lu, Jakub Jelinek, Richard Biener, AlanM,
Peter Bergner, Richard Sandiford, David Edelsohn, jlaw
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2022-09-28 5:41 ` PING^1 " Kewen.Lin
@ 2022-11-10 8:15 ` Kewen.Lin
2022-11-21 14:20 ` Richard Sandiford
0 siblings, 1 reply; 12+ messages in thread
From: Kewen.Lin @ 2022-11-10 8:15 UTC (permalink / raw)
To: GCC Patches
Cc: Jakub Jelinek, Segher Boessenkool, AlanM, Richard Sandiford,
Peter Bergner, jlaw, David Edelsohn, Richard Biener, H.J. Lu
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" } } */
>> --
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2022-11-10 8:15 ` PING^2 " Kewen.Lin
@ 2022-11-21 14:20 ` Richard Sandiford
2022-11-22 2:58 ` Kewen.Lin
0 siblings, 1 reply; 12+ messages in thread
From: Richard Sandiford @ 2022-11-21 14:20 UTC (permalink / raw)
To: Kewen.Lin
Cc: GCC Patches, Jakub Jelinek, Segher Boessenkool, AlanM,
Peter Bergner, jlaw, David Edelsohn, Richard Biener, H.J. Lu
"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" } } */
>>> --
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2022-11-21 14:20 ` Richard Sandiford
@ 2022-11-22 2:58 ` Kewen.Lin
2022-11-22 16:08 ` Richard Sandiford
0 siblings, 1 reply; 12+ messages in thread
From: Kewen.Lin @ 2022-11-22 2:58 UTC (permalink / raw)
To: richard.sandiford
Cc: GCC Patches, Jakub Jelinek, Segher Boessenkool, Peter Bergner,
jlaw, AlanM, David Edelsohn, Richard Biener, H.J. Lu
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2022-11-22 2:58 ` Kewen.Lin
@ 2022-11-22 16:08 ` Richard Sandiford
2022-11-25 3:26 ` Kewen.Lin
0 siblings, 1 reply; 12+ messages in thread
From: Richard Sandiford @ 2022-11-22 16:08 UTC (permalink / raw)
To: Kewen.Lin
Cc: GCC Patches, Jakub Jelinek, Segher Boessenkool, Peter Bergner,
jlaw, AlanM, David Edelsohn, Richard Biener, H.J. Lu
"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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2022-11-22 16:08 ` Richard Sandiford
@ 2022-11-25 3:26 ` Kewen.Lin
2023-07-19 6:33 ` Fangrui Song
0 siblings, 1 reply; 12+ messages in thread
From: Kewen.Lin @ 2022-11-25 3:26 UTC (permalink / raw)
To: richard.sandiford
Cc: Jakub Jelinek, Segher Boessenkool, GCC Patches, Peter Bergner,
David Edelsohn, Richard Biener, H.J. Lu, AlanM, jlaw
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2022-11-25 3:26 ` Kewen.Lin
@ 2023-07-19 6:33 ` Fangrui Song
2023-07-19 8:49 ` Kewen.Lin
0 siblings, 1 reply; 12+ messages in thread
From: Fangrui Song @ 2023-07-19 6:33 UTC (permalink / raw)
To: Kewen.Lin
Cc: richard.sandiford, Jakub Jelinek, Segher Boessenkool,
GCC Patches, Peter Bergner, David Edelsohn, Richard Biener,
H.J. Lu, AlanM, jlaw
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.
--
宋方睿
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: PING^2 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889]
2023-07-19 6:33 ` Fangrui Song
@ 2023-07-19 8:49 ` Kewen.Lin
0 siblings, 0 replies; 12+ messages in thread
From: Kewen.Lin @ 2023-07-19 8:49 UTC (permalink / raw)
To: Fangrui Song
Cc: richard.sandiford, Jakub Jelinek, Segher Boessenkool,
GCC Patches, Peter Bergner, David Edelsohn, Richard Biener,
H.J. Lu, AlanM, jlaw
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-07-19 8:49 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-24 8:17 [PATCH] Adjust the symbol for SECTION_LINK_ORDER linked_to section [PR99889] Kewen.Lin
2022-09-28 5:41 ` PING^1 " Kewen.Lin
2022-11-10 8:15 ` PING^2 " Kewen.Lin
2022-11-21 14:20 ` Richard Sandiford
2022-11-22 2:58 ` Kewen.Lin
2022-11-22 16:08 ` Richard Sandiford
2022-11-25 3:26 ` Kewen.Lin
2023-07-19 6:33 ` Fangrui Song
2023-07-19 8:49 ` Kewen.Lin
2022-09-29 20:31 ` Segher Boessenkool
2022-09-30 12:47 ` Kewen.Lin
2022-09-30 17:47 ` Segher Boessenkool
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).