[PATCH] bfd: include: sframe: fix PR ld/32789
Jan Beulich
jbeulich@suse.com
Wed Dec 17 07:12:37 GMT 2025
More information about the Binutils mailing list
Wed Dec 17 07:12:37 GMT 2025
- Previous message (by thread): [PATCH] bfd: include: sframe: fix PR ld/32789
- Next message (by thread): [PATCH] bfd: include: sframe: fix PR ld/32789
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
On 16.12.2025 23:52, Indu Bhagat via Binutils wrote:
> Currently, when SFrame sections are emitted after linking the input
> SFrame sections, the SFrame FDEs are sorted on start PC. Doing so for
> relocatable links has no effect (SFrame FDEs remain in place), because
> the start PC is unrelocated data. For relocatable links, then, the
> emitted SFrame FDEs in the output section remain in the same order as
> that in the respective input BFD.
>
> The assembler does not guarantee the emission of SFrame FDEs in the same
> order as the placement of the associated .text* sections,
> (SFRAME_F_FDE_SORTED is not set by ET_REL generated by GAS). This means
> setting SFRAME_F_FDE_SORTED by the linker was wrong when:
> - doing relocatable link, and
> - the input bfds contain multiple .text sections, say .text.hot,
> .text.init, .text.unlikely etc.
>
> For relocatable links, skip sorting the SFrame FDEs. Do not set
> SFRAME_F_FDE_SORTED for relocatable links.
>
> This is achieved by adding an explicit argument (bool sort_fde_p) to the
> sframe_encoder_write API. Move the API from 2.0 to the 2.1 node as this
> is an ABI-incompatible change. Skip bumping the "current" in
> libsframe/libtool-version ATM, we will do so closer to release.
I keep being concerned of such movements, but it also remains unclear to
me whether the library is actually intended for any out-of-tree
consumption. If it is, shouldn't the existing API stay in 2.0, with its
implementation simply being a thin wrapper around the new (2.1) API?
And then I would wish the patch subject was more descriptive. Mentioning
a PR alone means you force people to either read through enough of the
description, or go look up the PR, to know what the patch is about.
Otherwise looks okay to me, for whatever that's worth.
Jan
> When writing of SFrame data for PLT entries, indicate sort_fde_p to
> false: these sections are like the other SFrame sections for any other
> ET_REL binary.
>
> Add a test in ld/testsuite/ld-sframe/sframe.exp, these tests are run for
> all ABIs supported for SFrame. In this test, for object file generated
> for pr32789-1a.c:
> - the emitted SFrame FDEs by GAS are in the order of the .text* in the
> input assembly (i.e., .text.init, .text, .text.exit)
> - the emitted .text* sections by GAS are placed in the following order
> .text, .text.init, .text.exit.
> - GAS does not set SFRAME_F_FDE_SORTED, as expected.
>
> TBD:
> - The testcase is more for documentation purpose than ascertaining a
> specific order of the emitted .text.* sections/relas.
> Compiler/assembler are free to emit the .text* in any order. So in
> that sense, the testcase may not be appropriate ?
>
> bfd/
> PR ld/32789
> * elf-sframe.c (_bfd_elf_write_section_sframe): Skip sorting the
> SFrame FDEs for relocatable links.
> * elf64-s390.c (_bfd_s390_elf_write_sframe_plt): Additional
> argument to sframe_encoder_write.
> * elfxx-x86.c (_bfd_x86_elf_write_sframe_plt): Likewise.
> libsframe/
> * libsframe.ver: Move from 2.0 node to 2.1.
> * sframe.c (sframe_encoder_write_sframe): Conditionalize based
> on argument sort_fde_p.
> (sframe_encoder_write): New argument to indicate whether SFrame
> FDEs are to be sorted in output.
> include/
> * sframe-api.h (sframe_encoder_write): New argument.
> ld/testsuite/
> PR ld/32789
> * ld/testsuite/ld-sframe/sframe.exp: New test.
> * ld/testsuite/ld-sframe/pr32789-1.rd: New test.
> * ld/testsuite/ld-sframe/pr32789-1.sd: New test.
> * ld/testsuite/ld-sframe/pr32789-1a.c: New test.
> * ld/testsuite/ld-sframe/pr32789-1b.c: New test.
> * ld/testsuite/ld-x86-64/sframe-reloc-1.d: Remove
> SFRAME_F_FDE_SORTED.
> ---
> bfd/elf-sframe.c | 3 ++-
> bfd/elf64-s390.c | 2 +-
> bfd/elfxx-x86.c | 2 +-
> include/sframe-api.h | 8 ++++----
> ld/testsuite/ld-sframe/pr32789-1.rd | 9 +++++++++
> ld/testsuite/ld-sframe/pr32789-1.sd | 4 ++++
> ld/testsuite/ld-sframe/pr32789-1a.c | 22 ++++++++++++++++++++++
> ld/testsuite/ld-sframe/pr32789-1b.c | 12 ++++++++++++
> ld/testsuite/ld-sframe/sframe.exp | 16 ++++++++++++++++
> ld/testsuite/ld-x86-64/sframe-reloc-1.d | 3 +--
> libsframe/libsframe.ver | 2 +-
> libsframe/sframe.c | 23 ++++++++++++-----------
> 12 files changed, 85 insertions(+), 21 deletions(-)
> create mode 100644 ld/testsuite/ld-sframe/pr32789-1.rd
> create mode 100644 ld/testsuite/ld-sframe/pr32789-1.sd
> create mode 100644 ld/testsuite/ld-sframe/pr32789-1a.c
> create mode 100644 ld/testsuite/ld-sframe/pr32789-1b.c
>
> diff --git a/bfd/elf-sframe.c b/bfd/elf-sframe.c
> index 80043550777..f34d58f3f5a 100644
> --- a/bfd/elf-sframe.c
> +++ b/bfd/elf-sframe.c
> @@ -692,7 +692,8 @@ _bfd_elf_write_section_sframe (bfd *abfd, struct bfd_link_info *info)
> if (sec == NULL)
> return true;
>
> - contents = sframe_encoder_write (sfe_ctx, &sec_size, &err);
> + bool sort_p = !bfd_link_relocatable (info);
> + contents = sframe_encoder_write (sfe_ctx, &sec_size, sort_p, &err);
> sec->size = (bfd_size_type) sec_size;
>
> if (!bfd_set_section_contents (abfd, sec->output_section, contents,
> diff --git a/bfd/elf64-s390.c b/bfd/elf64-s390.c
> index 182a1119cdd..fe07e486795 100644
> --- a/bfd/elf64-s390.c
> +++ b/bfd/elf64-s390.c
> @@ -1682,7 +1682,7 @@ _bfd_s390_elf_write_sframe_plt (struct bfd_link_info *info)
>
> BFD_ASSERT (ectx);
>
> - void *contents = sframe_encoder_write (ectx, &sec_size, &err);
> + void *contents = sframe_encoder_write (ectx, &sec_size, false, &err);
>
> sec->size = (bfd_size_type) sec_size;
> sec->contents = (unsigned char *) bfd_zalloc (dynobj, sec->size);
> diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c
> index 6400d3e6473..14bbcc26448 100644
> --- a/bfd/elfxx-x86.c
> +++ b/bfd/elfxx-x86.c
> @@ -2016,7 +2016,7 @@ _bfd_x86_elf_write_sframe_plt (bfd *output_bfd,
>
> BFD_ASSERT (ectx);
>
> - void *contents = sframe_encoder_write (ectx, &sec_size, &err);
> + void *contents = sframe_encoder_write (ectx, &sec_size, false, &err);
>
> sec->size = (bfd_size_type) sec_size;
> sec->contents = (unsigned char *) bfd_zalloc (dynobj, sec->size);
> diff --git a/include/sframe-api.h b/include/sframe-api.h
> index 79a748f9e3b..7e37e6af84d 100644
> --- a/include/sframe-api.h
> +++ b/include/sframe-api.h
> @@ -309,11 +309,11 @@ sframe_encoder_add_funcdesc_v2 (sframe_encoder_ctx *ectx,
> uint32_t num_fres);
>
> /* Serialize the contents of the encoder context ECTX and return the buffer.
> - ENCODED_SIZE is updated to the size of the buffer.
> - Sets ERRP if failure. */
> + Sort the SFrame FDEs on start PC if SORT_FDE_P is true. ENCODED_SIZE is
> + updated to the size of the buffer. Sets ERRP if failure. */
> extern char *
> -sframe_encoder_write (sframe_encoder_ctx *ectx,
> - size_t *encoded_size, int *errp);
> +sframe_encoder_write (sframe_encoder_ctx *ectx, size_t *encoded_size,
> + bool sort_fde_p, int *errp);
>
> #ifdef __cplusplus
> }
> diff --git a/ld/testsuite/ld-sframe/pr32789-1.rd b/ld/testsuite/ld-sframe/pr32789-1.rd
> new file mode 100644
> index 00000000000..6fe8f7dff6d
> --- /dev/null
> +++ b/ld/testsuite/ld-sframe/pr32789-1.rd
> @@ -0,0 +1,9 @@
> +#...
> +Relocation section '.rela.sframe' at offset 0x[0-9a-f]+ contains 5 entries:
> + +Offset +Info +Type +Sym.* Value +Sym.* Name \+ Addend
> +[0-9a-f ]+ R_.* +0+ .text.init \+ 0
> +[0-9a-f ]+ R_.* +0+ .text \+ 0
> +[0-9a-f ]+ R_.* +0+ .text.exit \+ 0
> +[0-9a-f ]+ R_.* +0+ .text \+ [0-9a-f]+
> +[0-9a-f ]+ R_.* +0+ .text \+ [0-9a-f]+
> +#...
> diff --git a/ld/testsuite/ld-sframe/pr32789-1.sd b/ld/testsuite/ld-sframe/pr32789-1.sd
> new file mode 100644
> index 00000000000..5219a8c27bb
> --- /dev/null
> +++ b/ld/testsuite/ld-sframe/pr32789-1.sd
> @@ -0,0 +1,4 @@
> +#failif
> +#...
> +.*\(SFRAME_F_FDE_SORTED\).*
> +#...
> diff --git a/ld/testsuite/ld-sframe/pr32789-1a.c b/ld/testsuite/ld-sframe/pr32789-1a.c
> new file mode 100644
> index 00000000000..b7d1e88a778
> --- /dev/null
> +++ b/ld/testsuite/ld-sframe/pr32789-1a.c
> @@ -0,0 +1,22 @@
> +static int a = 0;
> +
> +extern int bar(void);
> +
> +__attribute__((section((".text.init"))))
> +int foo_init(void)
> +{
> + return 10;
> +}
> +
> +void foo (void)
> +{
> + foo_init ();
> + a++;
> + bar ();
> +}
> +
> +__attribute__((section((".text.exit"))))
> +void foo_exit(void)
> +{
> + foo ();
> +}
> diff --git a/ld/testsuite/ld-sframe/pr32789-1b.c b/ld/testsuite/ld-sframe/pr32789-1b.c
> new file mode 100644
> index 00000000000..e3e64eb5201
> --- /dev/null
> +++ b/ld/testsuite/ld-sframe/pr32789-1b.c
> @@ -0,0 +1,12 @@
> +static int bar_var = 2;
> +
> +int bar(void)
> +{
> + return 20;
> +}
> +
> +void bar2(void)
> +{
> + bar_var++;
> + bar ();
> +}
> diff --git a/ld/testsuite/ld-sframe/sframe.exp b/ld/testsuite/ld-sframe/sframe.exp
> index 5358cfd8184..c9c3bed9df3 100644
> --- a/ld/testsuite/ld-sframe/sframe.exp
> +++ b/ld/testsuite/ld-sframe/sframe.exp
> @@ -90,6 +90,22 @@ foreach sframe_test $sframe_test_list {
>
> check_pr33401
>
> +# Run other compile and link tests.
> +if { [check_compiler_available] } {
> + run_cc_link_tests [list \
> + [list \
> + "Build pr32789-1a.o pr32789-1b.o" \
> + "-r" \
> + "-Wa,--gsframe" \
> + { pr32789-1a.c pr32789-1b.c } \
> + {{readelf --sframe pr32789-1.sd}
> + {readelf -r pr32789-1.rd}} \
> + "pr32789-1.o" \
> + "c" \
> + ] \
> + ]
> +}
> +
> if {[info exists old_lc_all]} {
> set env(LC_ALL) $old_lc_all
> } else {
> diff --git a/ld/testsuite/ld-x86-64/sframe-reloc-1.d b/ld/testsuite/ld-x86-64/sframe-reloc-1.d
> index 19725e8bf09..d4915bd28f3 100644
> --- a/ld/testsuite/ld-x86-64/sframe-reloc-1.d
> +++ b/ld/testsuite/ld-x86-64/sframe-reloc-1.d
> @@ -11,8 +11,7 @@ Contents of the SFrame section .sframe:
> Header :
>
> Version: SFRAME_VERSION_2
> - Flags: SFRAME_F_FDE_SORTED,
> - SFRAME_F_FDE_FUNC_START_PCREL
> + Flags: SFRAME_F_FDE_FUNC_START_PCREL
> CFA fixed RA offset: \-8
> Num FDEs: 2
> Num FREs: 8
> diff --git a/libsframe/libsframe.ver b/libsframe/libsframe.ver
> index 4286a72b93b..fdd08a11ad9 100644
> --- a/libsframe/libsframe.ver
> +++ b/libsframe/libsframe.ver
> @@ -34,7 +34,6 @@ LIBSFRAME_2.0 {
> sframe_encoder_add_fre;
> sframe_encoder_add_funcdesc;
> sframe_encoder_add_funcdesc_v2;
> - sframe_encoder_write;
> dump_sframe;
> sframe_errmsg;
>
> @@ -45,4 +44,5 @@ LIBSFRAME_2.0 {
> LIBSFRAME_2.1 {
> global:
> sframe_fre_get_ra_undefined_p;
> + sframe_encoder_write;
> } LIBSFRAME_2.0;
> diff --git a/libsframe/sframe.c b/libsframe/sframe.c
> index 0dcc782b1f6..8a485929fcc 100644
> --- a/libsframe/sframe.c
> +++ b/libsframe/sframe.c
> @@ -1983,11 +1983,11 @@ sframe_encoder_write_fre (char *contents, sframe_frame_row_entry *frep,
> }
>
> /* Serialize the core contents of the SFrame section and write out to the
> - output buffer held in the encoder context ECTX. Return SFRAME_ERR if
> - failure. */
> + output buffer held in the encoder context ECTX. Sort the SFrame FDEs on
> + start PC if SORT_FDE_P is true. Return SFRAME_ERR if failure. */
>
> static int
> -sframe_encoder_write_sframe (sframe_encoder_ctx *ectx)
> +sframe_encoder_write_sframe (sframe_encoder_ctx *ectx, bool sort_fde_p)
> {
> char *contents;
> size_t buf_size;
> @@ -2067,12 +2067,13 @@ sframe_encoder_write_sframe (sframe_encoder_ctx *ectx)
> sframe_assert ((size_t)(contents - ectx->sfe_data) == buf_size);
>
> /* Sort the FDE table */
> - sframe_sort_funcdesc (ectx);
> + if (sort_fde_p)
> + sframe_sort_funcdesc (ectx);
>
> /* Sanity checks:
> - the FDE section must have been sorted by now on the start address
> - of each function. */
> - if (!(sframe_encoder_get_flags (ectx) & SFRAME_F_FDE_SORTED)
> + of each function, if sorting was needed. */
> + if ((sort_fde_p != (sframe_encoder_get_flags (ectx) & SFRAME_F_FDE_SORTED))
> || (fd_info == NULL))
> return sframe_set_errno (&err, SFRAME_ERR_FDE_INVAL);
>
> @@ -2090,12 +2091,12 @@ sframe_encoder_write_sframe (sframe_encoder_ctx *ectx)
> }
>
> /* Serialize the contents of the encoder context ECTX and return the buffer.
> - ENCODED_SIZE is updated to the size of the buffer.
> - Sets ERRP if failure. */
> + Sort the SFrame FDEs on start PC if SORT_FDE_P is true. ENCODED_SIZE is
> + updated to the size of the buffer. Sets ERRP if failure. */
>
> char *
> -sframe_encoder_write (sframe_encoder_ctx *ectx,
> - size_t *encoded_size, int *errp)
> +sframe_encoder_write (sframe_encoder_ctx *ectx, size_t *encoded_size,
> + bool sort_fde_p, int *errp)
> {
> sframe_header *ehp;
> size_t hdrsize, fsz, fresz, bufsize;
> @@ -2136,7 +2137,7 @@ sframe_encoder_write (sframe_encoder_ctx *ectx,
> foreign_endian = need_swapping (ehp->sfh_abi_arch);
>
> /* Write out the FDE Index and the FRE table in the sfe_data. */
> - if (sframe_encoder_write_sframe (ectx))
> + if (sframe_encoder_write_sframe (ectx, sort_fde_p))
> return sframe_ret_set_errno (errp, SFRAME_ERR_BUF_INVAL);
>
> /* Endian flip the contents if necessary. */
- Previous message (by thread): [PATCH] bfd: include: sframe: fix PR ld/32789
- Next message (by thread): [PATCH] bfd: include: sframe: fix PR ld/32789
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the Binutils mailing list