diff mbox series

[RFC,1/4] gas: ld: sframe: fix RELA r_offset handling

Message ID 20250308073853.78738-2-indu.bhagat@oracle.com
State New
Headers
Series Fix relocatable links with SFrame section |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 fail Patch failed to apply

Commit Message

Indu Bhagat March 8, 2025, 7:38 a.m. UTC 
  In SFrame V2, SFrame function descriptor entry is a 32-bit signed
integer (sfde_func_start_address) which is meant to indicate the start
PC of the function.  According to the specification, it intends to hold
the offset of the start PC of the function from the
_start_of_the_SFrame_section_.  This value can then be used by
stacktracers to simply do:

   sframe_find_fre ( pc - sframe_vaddr)

when looking up SFrame stack trace data corresponding to the program
counter (pc).

In the current implementation, however, GAS is actually emitting a
PC-relative RELA such that sfde_func_start_address is the offset of the
start PC of the function from the _start_of_the_SFrame_FDE_.  ld.bfd
was then fixing up the value by adjusting the final values by r_offset

Co-Authored-By: Jens Remus <jremus@linux.ibm.com>

bfd/
        * elf-sframe.c (_bfd_elf_merge_section_sframe): Do not fixup
	SFrame FDE function start address.
        * elfxx-x86.c (_bfd_x86_elf_finish_dynamic_sections): Likewise
	for PLT entries.
gas/
        * gen-sframe.c (output_sframe_funcdesc): New argument for SFrame
	section start.
        (output_sframe_internal): New symbol for SFrame section start.
include/
	* sframe.h: Fix incorrect comment.
---
 bfd/elf-sframe.c | 12 +++++-------
 bfd/elfxx-x86.c  |  9 +++------
 gas/gen-sframe.c | 10 ++++++----
 include/sframe.h |  2 +-
 4 files changed, 15 insertions(+), 18 deletions(-)

Comments

Jan Beulich March 13, 2025, 2:04 p.m. UTC | #1 
On 08.03.2025 08:38, Indu Bhagat wrote:
> In SFrame V2, SFrame function descriptor entry is a 32-bit signed
> integer (sfde_func_start_address) which is meant to indicate the start
> PC of the function.  According to the specification, it intends to hold
> the offset of the start PC of the function from the
> _start_of_the_SFrame_section_.

Which isn't something that can normally be expressed by relocations. Hence
is it maybe the spec which is flawed? As mentioned elsewhere, relocations
should retain their normal meaning. Custom adjustments based on it being
.sframe sections that are being processed feels wrong. Unaware tools ought
to be able to deal with this just fine.

There also looks to be a number of unrelated changes in this patch.
Without there being anything said about those they look as if they're
there by mistake.

Jan
Indu Bhagat March 20, 2025, 3:54 p.m. UTC | #2 
On 3/13/25 7:04 AM, Jan Beulich wrote:
> On 08.03.2025 08:38, Indu Bhagat wrote:
>> In SFrame V2, SFrame function descriptor entry is a 32-bit signed
>> integer (sfde_func_start_address) which is meant to indicate the start
>> PC of the function.  According to the specification, it intends to hold
>> the offset of the start PC of the function from the
>> _start_of_the_SFrame_section_.
> 
> Which isn't something that can normally be expressed by relocations. Hence
> is it maybe the spec which is flawed? As mentioned elsewhere, relocations
> should retain their normal meaning. Custom adjustments based on it being
> .sframe sections that are being processed feels wrong. Unaware tools ought
> to be able to deal with this just fine.
> 

Re: change the spec, this issue needs to be fixed for SFrame V2, and the 
fix backported.

Given the way SFrame sections are by design, unaware tools linking 
SFrame sections will not dump out valid output SFrame section.

> There also looks to be a number of unrelated changes in this patch.
> Without there being anything said about those they look as if they're
> there by mistake.
> 

There are two set of changes in this patch, which need to go hand in 
hand (i.e., in the same patch I am afraid): gas emits a different RELA 
and ld does not do any fixup as it was doing earlier.  I cant seem to 
find which of the stub seems unrelated.  Can you point them to me ?

Thanks
Jan Beulich March 21, 2025, 2:13 p.m. UTC | #3 
On 20.03.2025 16:54, Indu Bhagat wrote:
> On 3/13/25 7:04 AM, Jan Beulich wrote:
>> On 08.03.2025 08:38, Indu Bhagat wrote:
>>> In SFrame V2, SFrame function descriptor entry is a 32-bit signed
>>> integer (sfde_func_start_address) which is meant to indicate the start
>>> PC of the function.  According to the specification, it intends to hold
>>> the offset of the start PC of the function from the
>>> _start_of_the_SFrame_section_.
>>
>> Which isn't something that can normally be expressed by relocations. Hence
>> is it maybe the spec which is flawed? As mentioned elsewhere, relocations
>> should retain their normal meaning. Custom adjustments based on it being
>> .sframe sections that are being processed feels wrong. Unaware tools ought
>> to be able to deal with this just fine.
>>
> 
> Re: change the spec, this issue needs to be fixed for SFrame V2, and the 
> fix backported.

I fear I'll need to leave it to Nick then to approve any of this series,
in line with him having approved the original sframe work.

> Given the way SFrame sections are by design, unaware tools linking 
> SFrame sections will not dump out valid output SFrame section.

Right - I consider this a deficiency of the spec.

>> There also looks to be a number of unrelated changes in this patch.
>> Without there being anything said about those they look as if they're
>> there by mistake.
> 
> There are two set of changes in this patch, which need to go hand in 
> hand (i.e., in the same patch I am afraid): gas emits a different RELA 
> and ld does not do any fixup as it was doing earlier.  I cant seem to 
> find which of the stub seems unrelated.  Can you point them to me ?

Starting from the top, there's renaming of a local var pltn_reloc_by_hand,
there's insertion of a (seemingly random) blank line, and there's removal
of a FIXME comment. (That latter change may indeed be related, and I may
merely not be appreciating the connection.) And then in output_sframe()
there's removal of a blank line, for no apparent reason.

Jan
diff mbox series

Patch

diff --git a/bfd/elf-sframe.c b/bfd/elf-sframe.c
index 67ed874e025..a5958464536 100644
--- a/bfd/elf-sframe.c
+++ b/bfd/elf-sframe.c
@@ -429,7 +429,7 @@  _bfd_elf_merge_section_sframe (bfd *abfd,
       uint32_t func_size = 0;
       unsigned char func_info = 0;
       unsigned int r_offset = 0;
-      bool pltn_reloc_by_hand = false;
+      bool pltn_reloc = false;
       unsigned int pltn_r_offset = 0;
       uint8_t rep_block_size = 0;
 
@@ -465,6 +465,7 @@  _bfd_elf_merge_section_sframe (bfd *abfd,
 		     of PLT_SFRAME_FDE_START_OFFSET is also set to the
 		     same.  */
 		  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.  */
@@ -472,19 +473,16 @@  _bfd_elf_merge_section_sframe (bfd *abfd,
 		    {
 		      pltn_r_offset
 			= r_offset + (i * sizeof (sframe_func_desc_entry));
-		      pltn_reloc_by_hand = true;
+		      pltn_reloc = true;
 		    }
 		}
 
 	      /* Get the SFrame FDE function start address after relocation.  */
 	      address = sframe_read_value (abfd, contents, r_offset, 4);
-	      if (pltn_reloc_by_hand)
+	      if (pltn_reloc)
 		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);
+	      address += sec->output_offset;
 
 	      func_start_addr = address;
 	    }
diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c
index 6ea41f29af1..465b5cb118c 100644
--- a/bfd/elfxx-x86.c
+++ b/bfd/elfxx-x86.c
@@ -2986,8 +2986,7 @@  _bfd_x86_elf_finish_dynamic_sections (bfd *output_bfd,
 	{
 	  bfd_vma plt_start = htab->elf.splt->output_section->vma;
 	  bfd_vma sframe_start = htab->plt_sframe->output_section->vma
-				   + htab->plt_sframe->output_offset
-				   + PLT_SFRAME_FDE_START_OFFSET;
+				   + htab->plt_sframe->output_offset;
 #if 0 /* FIXME Testing only. Remove before review.  */
 	  bfd_vma test_value = (plt_start - sframe_start)
 	    + htab->plt_sframe->output_section->vma
@@ -3020,8 +3019,7 @@  _bfd_x86_elf_finish_dynamic_sections (bfd *output_bfd,
 	  bfd_vma plt_start = htab->plt_second->output_section->vma;
 	  bfd_vma sframe_start
 	    = (htab->plt_second_sframe->output_section->vma
-	       + htab->plt_second_sframe->output_offset
-	       + PLT_SFRAME_FDE_START_OFFSET);
+	       + htab->plt_second_sframe->output_offset);
 #if 0 /* FIXME Testing only. Remove before review.  */
 	  bfd_vma test_value = (plt_start - sframe_start)
 	    + htab->plt_second_sframe->output_section->vma
@@ -3054,8 +3052,7 @@  _bfd_x86_elf_finish_dynamic_sections (bfd *output_bfd,
 	  bfd_vma plt_start = htab->plt_got->output_section->vma;
 	  bfd_vma sframe_start
 	    = (htab->plt_got_sframe->output_section->vma
-	       + htab->plt_got_sframe->output_offset
-	       + PLT_SFRAME_FDE_START_OFFSET);
+	       + htab->plt_got_sframe->output_offset);
 	  bfd_put_signed_32 (dynobj, plt_start - sframe_start,
 			     htab->plt_got_sframe->contents
 			     + PLT_SFRAME_FDE_START_OFFSET);
diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 947faf366d3..10afcf35448 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -608,7 +608,8 @@  output_sframe_row_entry (symbolS *fde_start_addr,
 }
 
 static void
-output_sframe_funcdesc (symbolS *start_of_fre_section,
+output_sframe_funcdesc (symbolS *sframe_start,
+			symbolS *start_of_fre_section,
 			symbolS *fre_symbol,
 			struct sframe_func_entry *sframe_fde)
 {
@@ -624,7 +625,7 @@  output_sframe_funcdesc (symbolS *start_of_fre_section,
   /* Start address of the function.  */
   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 = sframe_start; /* from location.  */
   exp.X_add_number = 0;
   emit_expr (&exp, addr_size);
 
@@ -683,6 +684,8 @@  output_sframe_internal (void)
   int fixed_ra_offset = SFRAME_CFA_FIXED_RA_INVALID;
   unsigned int addr_size;
 
+  symbolS *sframe_start = symbol_temp_new_now ();
+
   addr_size = SFRAME_RELOC_SIZE;
 
   /* The function descriptor entries as dumped by the assembler are not
@@ -766,7 +769,7 @@  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 (sframe_start, start_of_fre_section,
 			      fre_symbols[i], sframe_fde);
       i += sframe_fde->num_fres;
     }
@@ -1767,7 +1770,6 @@  void
 output_sframe (segT sframe_seg)
 {
   (void) sframe_seg;
-
   /* Setup the version specific access functions.  */
   sframe_set_version (SFRAME_VERSION_2);
 
diff --git a/include/sframe.h b/include/sframe.h
index a965e23bdd1..6c3152f6121 100644
--- a/include/sframe.h
+++ b/include/sframe.h
@@ -174,7 +174,7 @@  typedef struct sframe_header
 typedef struct sframe_func_desc_entry
 {
   /* Function start address.  Encoded as a signed offset, relative to the
-     beginning of the current FDE.  */
+     beginning of the SFrame section.  */
   int32_t sfde_func_start_address;
   /* Size of the function in bytes.  */
   uint32_t sfde_func_size;