[v3,3/3] sframe: FDE function start address relative to SFrame section

  The SFrame FDE sfde_func_start_address field is specified to contain
the function offset from .sframe section.  This is true in executables
and shared libraries after final link.  In assembler generated objects
it contained the function offset from SFrame FDE (using a PC-relative
relocation) and the linker performed a respective fixup when merging
the .sframe sections.

Change the assembler to emit sfde_func_start_address relative from the
.sframe section.  Change the linker to only apply the output .sframe
section offset as fixup when merging the sections.

While at it enhance the comments in the code, fix a typo in SFD_INFO,
and remove commented out debugging code.

gas/
	* gen-sframe.c (output_sframe_internal): Create temporary symbol
	for start of SFrame section.  Pass it to output_sframe_funcdesc.
	(output_sframe_funcdesc): Emit SFrame FDE function start address
	as offset from SFrame section instead of FDE.

bfd/
	* elf-sframe.c (_bfd_elf_merge_section_sframe): Fixup SFrame FDE
	function start address by adding output .sframe offset.  Enhance
	comments.
	(sframe_decoder_set_func_reloc_index): Correct typo in comment.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes in V3:
    - New patch.  Replaces V2 patch ("sframe: Enhance comments and
      documentation on FDE function start address").

 bfd/elf-sframe.c | 44 ++++++++++++++++++++++++++------------------
 gas/gen-sframe.c | 11 +++++++----
 2 files changed, 33 insertions(+), 22 deletions(-)
  

On 2/28/25 5:32 AM, Jens Remus wrote:
> The SFrame FDE sfde_func_start_address field is specified to contain
> the function offset from .sframe section.  This is true in executables
> and shared libraries after final link.  In assembler generated objects
> it contained the function offset from SFrame FDE (using a PC-relative
> relocation) and the linker performed a respective fixup when merging
> the .sframe sections.
> 
> Change the assembler to emit sfde_func_start_address relative from the
> .sframe section.  Change the linker to only apply the output .sframe
> section offset as fixup when merging the sections.
> 
> While at it enhance the comments in the code, fix a typo in SFD_INFO,
> and remove commented out debugging code.
> 

Thanks for the patch.

I would like to consider this patch along with some other patches that I 
have for fixing the outstanding SFrame issues (PR 32589) and its 
repercussions on relocatable links (PR 32666) and --gc-sections.

I think it is better to review this change along with the fixes for the 
above issues.  I will include this patch in a separate series I plan to 
post soon.

Thanks
Indu

> gas/
> 	* gen-sframe.c (output_sframe_internal): Create temporary symbol
> 	for start of SFrame section.  Pass it to output_sframe_funcdesc.
> 	(output_sframe_funcdesc): Emit SFrame FDE function start address
> 	as offset from SFrame section instead of FDE.
> 
> bfd/
> 	* elf-sframe.c (_bfd_elf_merge_section_sframe): Fixup SFrame FDE
> 	function start address by adding output .sframe offset.  Enhance
> 	comments.
> 	(sframe_decoder_set_func_reloc_index): Correct typo in comment.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes in V3:
>      - New patch.  Replaces V2 patch ("sframe: Enhance comments and
>        documentation on FDE function start address").
> 
>   bfd/elf-sframe.c | 44 ++++++++++++++++++++++++++------------------
>   gas/gen-sframe.c | 11 +++++++----
>   2 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/bfd/elf-sframe.c b/bfd/elf-sframe.c
> index 97e007345152..83611921e015 100644
> --- a/bfd/elf-sframe.c
> +++ b/bfd/elf-sframe.c
> @@ -93,7 +93,7 @@ sframe_decoder_set_func_reloc_index (struct sframe_dec_info *sfd_info,
>       sfd_info->sfd_func_bfdinfo[func_idx].func_reloc_index = reloc_index;
>   }
>   
> -/* Initialize the set of additional information in CFD_INFO,
> +/* Initialize the set of additional information in SFD_INFO,
>      needed for linking SEC.  Returns TRUE if setup is done successfully.  */
>   
>   static bool
> @@ -446,27 +446,28 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>   	    {
>   	      if (!(sec->flags & SEC_LINKER_CREATED))
>   		{
> -		  /* Get relocated contents by reading the value of the
> -		     relocated function start address at the beginning of the
> -		     function descriptor entry.  */
> +		  /* Offset to SFrame FDE sfde_func_start_address from input
> +		     .sframe section.  It contains the relocated function
> +		     offset from input .sframe section.  */
>   		  r_offset = sframe_decoder_get_func_r_offset (sfd_info, i);
>   		}
>   	      else
>   		{
>   		  /* Expected to land here when SFrame stack trace info is
> -		     created dynamically for the .plt* sections.  These
> -		     sections are expected to have upto two SFrame FDE entries.
> +		     created dynamically for the .plt* sections.  These .sframe
> +		     sections are expected to have upto two SFrame FDEs.
>   		     Although the code should work for > 2,  leaving this
>   		     assert here for safety.  */
>   		  BFD_ASSERT (num_fidx <= 2);
> -		  /* For the first entry, we know the offset of the SFrame FDE's
> -		     sfde_func_start_address.  Side note: see how the value
> -		     of PLT_SFRAME_FDE_START_OFFSET is also set to the
> -		     same.  */
> +		  /* Offset to the first linker-generated SFrame FDE (for PLT0)
> +		     sfde_func_start_address from input .sframe section.  It
> +		     contains the PLT0 offset from FDE.  NB: See how it is set
> +		     using PLT_SFRAME_FDE_START_OFFSET.  */
>   		  r_offset = sframe_decoder_get_hdr_size (sfd_ctx);
> -		  /* For any further SFrame FDEs, the generator has already put
> -		     in an offset in place of sfde_func_start_address of the
> -		     corresponding FDE.  We will use it by hand to relocate.  */
> +		  /* Offset to any subsequent linker-generated SFrame FDE (for
> +		     PLTn) sfde_func_start_address from input .sframe section.
> +		     It contains the PLTn offset from PLT0.  We will use it by
> +		     hand to relocate.  */
>   		  if (i > 0)
>   		    {
>   		      pltn_r_offset
> @@ -475,15 +476,22 @@ _bfd_elf_merge_section_sframe (bfd *abfd,
>   		    }
>   		}
>   
> -	      /* Get the SFrame FDE function start address after relocation.  */
> +	      /* Get the SFrame FDE sfde_func_start_address value after
> +		 relocation.  In object files it is the function offset
> +		 from input .sframe section.  For linker-generated FDE
> +		 it is the PLT0 offset from FDE.  */
>   	      address = sframe_read_value (abfd, contents, r_offset, 4);
> +	      /* For linker-generated PLTn add the PLTn offset from PLT0.  */
>   	      if (pltn_reloc_by_hand)
>   		address += sframe_read_value (abfd, contents,
>   					      pltn_r_offset, 4);
> -	      address += (sec->output_offset + r_offset);
> -
> -	      /* FIXME For testing only. Cleanup later.  */
> -	      // address += (sec->output_section->vma);
> +	      /* Fixup by adding the output .sframe section offset.  For
> +		 linker-generated FDE additionally add the first FDE
> +		 offset from input .sframe section.  */
> +	      if (!(sec->flags & SEC_LINKER_CREATED))
> +		address += sec->output_offset;
> +	      else
> +		address += (sec->output_offset + r_offset);
>   
>   	      func_start_addr = address;
>   	    }
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index dd5fb25e367f..b50c6613c5a6 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -607,7 +607,8 @@ output_sframe_row_entry (symbolS *fde_start_addr,
>   }
>   
>   static void
> -output_sframe_funcdesc (symbolS *start_of_fre_section,
> +output_sframe_funcdesc (symbolS *start_of_sframe_section,
> +			symbolS *start_of_fre_section,
>   			symbolS *fre_symbol,
>   			struct sframe_func_entry *sframe_fde)
>   {
> @@ -618,10 +619,10 @@ output_sframe_funcdesc (symbolS *start_of_fre_section,
>     dw_fde_start_addrS = get_dw_fde_start_addrS (sframe_fde->dw_fde);
>     dw_fde_end_addrS = get_dw_fde_end_addrS (sframe_fde->dw_fde);
>   
> -  /* Start address of the function.  */
> +  /* Start address of the function (function offset from .sframe section).  */
>     exp.X_op = O_subtract;
>     exp.X_add_symbol = dw_fde_start_addrS; /* to location.  */
> -  exp.X_op_symbol = symbol_temp_new_now (); /* from location.  */
> +  exp.X_op_symbol = start_of_sframe_section; /* from location.  */
>     exp.X_add_number = 0;
>     emit_expr (&exp, sizeof_member (sframe_func_desc_entry,
>   				  sfde_func_start_address));
> @@ -672,6 +673,7 @@ output_sframe_internal (void)
>     expressionS exp;
>     unsigned int i = 0;
>   
> +  symbolS *start_of_sframe_section = symbol_temp_new_now ();
>     symbolS *end_of_frame_hdr;
>     symbolS *end_of_frame_section;
>     symbolS *start_of_func_desc_section;
> @@ -763,7 +765,8 @@ output_sframe_internal (void)
>     i = 0;
>     for (sframe_fde = all_sframe_fdes; sframe_fde; sframe_fde = sframe_fde->next)
>       {
> -      output_sframe_funcdesc (start_of_fre_section,
> +      output_sframe_funcdesc (start_of_sframe_section,
> +			      start_of_fre_section,
>   			      fre_symbols[i], sframe_fde);
>         i += sframe_fde->num_fres;
>       }
  

On 05.03.2025 15:24, Indu Bhagat wrote:
> On 2/28/25 5:32 AM, Jens Remus wrote:
>> The SFrame FDE sfde_func_start_address field is specified to contain
>> the function offset from .sframe section.  This is true in executables
>> and shared libraries after final link.  In assembler generated objects
>> it contained the function offset from SFrame FDE (using a PC-relative
>> relocation) and the linker performed a respective fixup when merging
>> the .sframe sections.
>>
>> Change the assembler to emit sfde_func_start_address relative from the
>> .sframe section.  Change the linker to only apply the output .sframe
>> section offset as fixup when merging the sections.
>>
>> While at it enhance the comments in the code, fix a typo in SFD_INFO,
>> and remove commented out debugging code.
>>
> 
> Thanks for the patch.
> 
> I would like to consider this patch along with some other patches
> that I have for fixing the outstanding SFrame issues (PR 32589) and
> its repercussions on relocatable links (PR 32666) and --gc-sections.
> 
> I think it is better to review this change along with the fixes for
> the above issues.  I will include this patch in a separate series I
> plan to post soon.

Makes sense. Thank you!

What about the rest of the series?  Ok to commit to mainline?  I think
any further refinement of patch 1 can be done with SFrame V3, once the
requirements are defined.

Thanks and regards,
Jens
  

On 3/7/25 1:42 AM, Jens Remus wrote:
> On 05.03.2025 15:24, Indu Bhagat wrote:
>> On 2/28/25 5:32 AM, Jens Remus wrote:
>>> The SFrame FDE sfde_func_start_address field is specified to contain
>>> the function offset from .sframe section.  This is true in executables
>>> and shared libraries after final link.  In assembler generated objects
>>> it contained the function offset from SFrame FDE (using a PC-relative
>>> relocation) and the linker performed a respective fixup when merging
>>> the .sframe sections.
>>>
>>> Change the assembler to emit sfde_func_start_address relative from the
>>> .sframe section.  Change the linker to only apply the output .sframe
>>> section offset as fixup when merging the sections.
>>>
>>> While at it enhance the comments in the code, fix a typo in SFD_INFO,
>>> and remove commented out debugging code.
>>>
>>
>> Thanks for the patch.
>>
>> I would like to consider this patch along with some other patches
>> that I have for fixing the outstanding SFrame issues (PR 32589) and
>> its repercussions on relocatable links (PR 32666) and --gc-sections.
>>
>> I think it is better to review this change along with the fixes for
>> the above issues.  I will include this patch in a separate series I
>> plan to post soon.
> 
> Makes sense. Thank you!
> 
> What about the rest of the series?  Ok to commit to mainline?  I think
> any further refinement of patch 1 can be done with SFrame V3, once the
> requirements are defined.
> 

OK.

Thanks