[PATCH,V3 6/9] bfd: gas: ld: libsframe: adopt new encoding for FDE func start addr field
Indu Bhagat
indu.bhagat@oracle.com
Mon Jun 16 06:43:50 GMT 2025
On 6/13/25 5:39 AM, Jan Beulich wrote:
> On 13.06.2025 09:32, Indu Bhagat wrote:
>> This patch convenes a set of changes in bfd, gas, ld, libsframe towards
>> moving to the new encoding for the 'sfde_func_start_address' field in
>> SFrame FDE.
>>
>> First, gas must now mark all SFrame sections with the new flag
>> SFRAME_F_FDE_FUNC_START_ADDR_PCREL. gas was already emitting the field
>> in the said encoding.
>
> Looks like I'm lost again: If gas did things the "new way" already, why would
> a change be needed?
Short answer:
Because SFrame relocatable links were broken. To fix them using
scheme#B approach, we needed to fix the sfde_func_start_address in the
ET_REL binary emitted by the linker. To convey a bugfix, we now emit
the flag SFRAME_F_FDE_FUNC_START_ADDR_PCREL.
As gas also emits ET_REL binary (with sfde_func_start_address as offset
from the field itself), so we mark with
SFRAME_F_FDE_FUNC_START_ADDR_PCREL. Its basically "the right thing to
do" for consistency (all ET_REL binaries have the same encoding and
hence, the flag set) moving forward.
Longer version:
On trunk, there are two sets of encodings (and buggy SFrame relocatable
links):
- (#1) ET_REL generated by gas had sfde_func_start_address as offset
from the field itself.
- (#2) ET_DYN, ET_EXEC, and ET_REL (relocatable links) generated by
ld had sfde_func_start_address as offset from start of section.
In the first attempt
(https://sourceware.org/pipermail/binutils/2025-March/139853.html), I
tried to bring (#1) to also emit sfde_func_start_address as offset from
start of section. I informally refer to this one as Scheme#A patchset,
but the review feedback was to avoid abusing the available relocations
(https://sourceware.org/pipermail/binutils/2025-March/139980.html).
So now with this current patchset (informally called Scheme#B), we:
- (#1) ET_REL generated by gas continues to have
sfde_func_start_address as offset from the field itself.
- (#2) ET_DYN, ET_EXEC, and ET_REL (relocatable links) generated by
ld have sfde_func_start_address as offset from field itself.
> And how would one distinguish "new with flag", "new without
> flag", and "old without flag"?
>
For those generated by gas, there is no way to distinguish between the
"new with flag" or "old without flag". The emitted data has always been
the same, thats all that can be said and relied on.
>> * gas/gen-sframe.c (output_sframe_internal): Emit the flag
>> SFRAME_F_FDE_FUNC_START_ADDR_PCREL.
>>
>> Similarly for ld, adopt the new semantics of sfde_func_start_address
>> consistently. This means:
>> - When merging SFrame sections, check that all input SFrame sections
>> have the SFRAME_F_FDE_FUNC_START_ADDR_PCREL flag set. If the check
>> fails, ld errors out.
>> - When merging SFrame sections, keep even the in-memory contents of
>> the FDE function start address (buffer passed to libsframe
>> sframe_encoder_write () for writing out) are encoded in the new
>> semantics. While it is, in theory, possible that instead of doing this
>> change here, we adjust the value of sfde_func_start_address at the final
>> write (sframe_encoder_write) time. But latter is not favorable for
>> maintenanance and may be generally confusing for developers.
>> - When creating SFrame for PLT entries, emit flag
>> SFRAME_F_FDE_FUNC_START_ADDR_PCREL.
>>
>> bfd/
>> * elf-sframe.c (_bfd_elf_merge_section_sframe): Check for flag
>> SFRAME_F_FDE_FUNC_START_ADDR_PCREL set for all input bfds. If
>> not, error out. Also, adopt the new semantics of function start
>> address encoding.
>> * bfd/elfxx-x86.c (_bfd_x86_elf_create_sframe_plt): Emit flag
>> SFRAME_F_FDE_FUNC_START_ADDR_PCREL.
>>
>> Next, for dumping SFrame sections, now that we are emitting the same
>> encoding in GAS, non-relocatable and relocatable SFrame links, it is the
>> time to set relocate to TRUE in debug_displays[].
>>
>> binutils/
>> * dwarf.c (struct dwarf_section_display): Allow sframe sections
>> to now be relocated.
>> gas/testsuite/
>> * gas/cfi-sframe/cfi-sframe-aarch64-pac-ab-key-1.d: Update the
>> test. Relocatable SFrame sections now display non-zero value
>> (appropriate function start address).
>>
>> Now, as the SFrame sections on-disk and in-memory use the new semantics of
>> sfde_func_start_address encoding (i.e., function start address is the
>> offset from the sfde_func_start_address field to the start PC), the
>> calculation to make it human readable (i.e., relatable to the addresses
>> in .text sections) needs adjustment.
>>
>> libsframe/
>> * sframe-dump.c (dump_sframe_func_with_fres): Adjust the
>> function start address for dumping.
>>
>> Now that both the emission of the new encoding, and the relocation of
>> sections before dumping them is in place, it is time to adjust the
>> testcases.
>>
>> gas/testsuite/
>> * gas/cfi-sframe/cfi-sframe-aarch64-1.d: Update expected output
>> to include SFRAME_F_FDE_FUNC_START_ADDR_PCREL instead of NONE.
>> * gas/cfi-sframe/cfi-sframe-aarch64-2.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-aarch64-3.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-aarch64-4.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-1.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-10.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-11.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-2.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-3.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-4.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-5.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-6.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-7.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-8.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-common-9.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-x86_64-1.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-x86_64-2.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-x86_64-empty-1.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-x86_64-empty-2.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-x86_64-empty-3.d: Likewise.
>> * gas/cfi-sframe/cfi-sframe-x86_64-empty-4.d: Likewise.
>> * gas/cfi-sframe/common-empty-1.d: Likewise.
>> * gas/cfi-sframe/common-empty-2.d: Likewise.
>> * gas/cfi-sframe/common-empty-3.d: Likewise.
>> * gas/scfi/x86_64/scfi-cfi-sections-1.d: Likewise.
>> * gas/scfi/x86_64/scfi-dyn-stack-1.d: Likewise.
>> ld/testsuite/
>> * ld-aarch64/sframe-simple-1.d: Update expected output to
>> include SFRAME_F_FDE_FUNC_START_ADDR_PCREL.
>> * ld-x86-64/sframe-ibt-plt-1.d: Likewise.
>> * ld-x86-64/sframe-plt-1.d: Likewise.
>> * ld-x86-64/sframe-pltgot-1.d: Likewise.
>> * ld-x86-64/sframe-pltgot-2.d: Likewise.
>> * ld-x86-64/sframe-simple-1.d: Likewise.
>>
>> Naturally, the change of semantics for 'SFrame FDE function start address'
>> has consequences on the implementation in libsframe. As per the new
>> semantics:
>> - Function start address in the SFrame FDE (sfde_func_start_address)
>> is an offset from the FDE function start address field to the start
>> PC of the associated function.
>>
>> Note that, the libsframe library brings the SFrame section contents into
>> its own memory to create a sframe_decoder_ctx object via sframe_decode
>> (). Many internal and user-interfacing APIs then use sframe_decoder_ctx
>> object to interact and fulfill the work.
>>
>> In context of changing semantics for sfde_func_start_address, following
>> relevant examples may help understand the impact:
>> - sframe_find_fre () finds a the SFrame stack trace data (SFrame FRE)
>> given a lookup offset (offset of lookup_pc from the start of SFrame
>> section). Now that the sfde_func_start_address includes the
>> distance from the sfde_func_start_address field to the start of
>> SFrame section itself, the comparison checks of
>> sfde_func_start_address with the incoming lookup offset need
>> adjustment.
>> - Some internal functions (sframe_get_funcdesc_with_addr_internal ()
>> finds SFrame FDE by using binary seach comparing
>> sfde_func_start_address fields, etc.) need adjustments.
>> - sframe_encoder_write () sorts the SFrame FDEs before writing out
>> the SFrame data. Sorting of SFrame FDE via the internal function
>> sframe_sort_funcdesc() needs adjustments: the new encoding of
>> sfde_func_start_address means the distances are not from the same
>> anchor, so cannot be sorted directly.
>>
>> This patch takes the approach of adding a new internal function:
>> - sframe_decoder_get_secrel_func_start_addr (): This function returns
>> the offset of the start PC of the function from the start of SFrame
>> section, i.e., it gives a section-relative offset.
>>
>> As the sframe_decoder_get_secrel_func_start_addr () API needs the value
>> of the function index in the FDE list, another internal API needs
>> sframe_fre_check_range_p () adjustments too.
>>
>> Sorting the FDEs (via sframe_sort_funcdesc ()) is done by first bringing
>> all offsets in sfde_func_start_address relative to start of SFrame
>> section, followed by sorting, and then readjusting the offsets accroding
>> to the new position in the FDE list.
>>
>> TBD:
>> - Version bump libsframe. The change in encoding of
>> sfde_func_start_address means the APIs sframe_encoder_add_funcdesc ()
>> and sframe_find_fre () etc. are now backwards incompatible with previous
>> releases.
>
> It's not just a version bump of the library. You also need to move the
> affected public functions to a new version (2.0?) in the .ver file, I
> think.
>
IIUC, bumping the SONAME is enough to indicate the backward incompatible
changes for consumers. Unless the binary incompatible APIs are added
with new names, a construct like following:
LIBSFRAME_2.0 {
global:
sframe_decoder_get_flags;
sframe_decoder_get_offsetof_fde_start_addr;
sframe_encoder_get_flags;
sframe_encoder_get_offsetof_fde_start_addr;
sframe_decoder_get_funcdesc_v2;
sframe_encoder_add_funcdesc_v2;
sframe_encoder_write;
} LIBSFRAME_1.0;
will still emit (readelf -s --wide libsframe.so.2 output):
35: 0000000000003f55 221 FUNC GLOBAL DEFAULT 13
sframe_decoder_get_funcdesc_v2@@LIBSFRAME_1.0
...
41: 0000000000005303 432 FUNC GLOBAL DEFAULT 13
sframe_encoder_write@@LIBSFRAME_1.0
In any case, I need to remove an API in 2.45 release
(sframe_get_funcdesc_with_addr was marked for deprecation a few releases
earlier). So I will need to create a new (non-inheritance version below)
2_0 entry anyway:
LIBSFRAME_2.0 {
global:
sframe_decoder_get_flags;
sframe_decoder_get_offsetof_fde_start_addr;
sframe_encoder_get_flags;
sframe_encoder_get_offsetof_fde_start_addr;
... // list all the APIs.
};
Which will achieve what you are suggesting.
>> If this change is backported, we need to reserve a version
>> bump for a backport too I think.
>
> That can simply be 1.1? I'd be rather hesitant with backporting, though.
>
Yes, I am hesitant too ATM.
>> --- a/bfd/elf-sframe.c
>> +++ b/bfd/elf-sframe.c
>> @@ -328,6 +328,8 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>> int8_t sfd_ctx_fixed_ra_offset;
>> uint8_t dctx_version;
>> uint8_t ectx_version;
>> + uint8_t dctx_flags;
>> + uint8_t ectx_flags;
>> int encerr = 0;
>>
>> struct elf_link_hash_table *htab;
>> @@ -351,6 +353,8 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>> if (sfd_ctx == NULL || sfe_info == NULL)
>> return false;
>>
>> + dctx_flags = sframe_decoder_get_flags (sfd_ctx);
>> +
>> if (htab->sfe_info.sfe_ctx == NULL)
>> {
>> sfd_ctx_abi_arch = sframe_decoder_get_abi_arch (sfd_ctx);
>> @@ -361,8 +365,12 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>> if (!sfd_ctx_abi_arch)
>> return false;
>>
>> + /* Reset SFRAME_F_FDE_SORTED for the encoder context. This will be set
>> + later when FDEs are finally sorted before emission in the output
>> + section. */
>> + uint8_t tflags = dctx_flags & ~SFRAME_F_FDE_SORTED;
>> htab->sfe_info.sfe_ctx = sframe_encode (SFRAME_VERSION_2,
>> - 0, /* SFrame flags. */
>> + tflags, /* SFrame flags. */
>> sfd_ctx_abi_arch,
>> sfd_ctx_fixed_fp_offset,
>> sfd_ctx_fixed_ra_offset,
>
> I'm likely confused here, too: Why would the one flag need special casing?
> What about flags which are set, but which aren't defined right now?
>
Flags which arent defined are not set. The rest of the bits (bits at
position [4:8] of the flags field should be zero. Perhaps I should add
a line in the spec document that undefined bits in the flags field are
expected to be zero.
This code will need to evolve as more flags are added. Currently the
three flags defined are such that only SFRAME_F_FDE_SORTED needs reset
at the time of linking, because this property in broken while the
sections are being linked in memory (as the SFrame FDEs are being placed
in the output section) until the eventual sort at write time.
The code does not deal with SFRAME_F_FRAME_POINTER currently as this
flag is not set by any producer ATM. This flag indicates that the
functions in the section also preserve frame pointer.
That said, to be more precise, the creation of the encoder object for
the output SFrame section above should explicitly set
SFRAME_F_FDE_FUNC_START_ADDR_PCREL now (not being done above). This is
because the data created by the linker is in that encoding now. I will
add this flag here.
>> @@ -410,7 +418,16 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>> " .sframe generation"));
>> return false;
>> }
>> -
>> + /* Check that all SFrame sections being linked have harmonious flags. */
>> + ectx_flags = sframe_encoder_get_flags (sfe_ctx);
>> + if ((dctx_flags & SFRAME_F_FDE_FUNC_START_ADDR_PCREL)
>> + != (ectx_flags & SFRAME_F_FDE_FUNC_START_ADDR_PCREL))
>
> And why is the one flag special?
>
Of the other flags, SFRAME_F_FRAME_POINTER should have similar handling,
but no producer sets it currently. So it is skipped from this check ATM...
If more flags are added, this may need adjustments later.
> Such a check can, btw, be written in a less verbose / redundant manner:
>
> if ((dctx_flags ^ ectx_flags) & ~SFRAME_F_FDE_FUNC_START_ADDR_PCREL)
>
OK.
>> --- a/bfd/elfxx-x86.c
>> +++ b/bfd/elfxx-x86.c
>> @@ -1891,7 +1891,7 @@ _bfd_x86_elf_create_sframe_plt (bfd *output_bfd,
>> }
>>
>> *ectx = sframe_encode (SFRAME_VERSION_2,
>> - 0,
>> + SFRAME_F_FDE_FUNC_START_ADDR_PCREL,
>
> If the PC-relative encoding is now the only valid way of encoding things,
> why would every caller need to specify the flag?
>
Perhaps its a symptom of (my) biased perspective from the starting point
of this implementation, but now that you question, I still do find the
flag specified like so in the caller to be clearer and less error prone.
(Somewhat unrelated to your question, but) It now strikes me that
perhaps its better to also check that flag
SFRAME_F_FDE_FUNC_START_ADDR_PCREL is set around the time when we invoke
sframe_sort_funcdesc (), as the implementation now adjusts the
sfde_func_start_address before and after sorting. IOW, the
implemenation of sframe_encoder_write_sframe is meant for the case when
SFRAME_F_FDE_FUNC_START_ADDR_PCREL is set.
Thanks for reviewing.
More information about the Binutils
mailing list