[PATCH v4 3/4] gas: sframe: Represent .cfi_undefined RA as FRE without offsets

[Jens Remus]

On 7/29/2025 9:12 AM, Indu Bhagat wrote:
> On 7/25/25 9:23 AM, Jens Remus wrote:

>> Notes (jremus):
>>      Changes in V4:
>>      - Represent RA undefined as SFrame FRE without any offsets. (Indu)
>>        This aligns to DWARF and saves from adding a SFrame FDE flag.
>>           Changes in V3:
>>      - Introduce a FDE info word flag ra_undefined.  Represent RA undefined
>>        as FDE without any FREs and the ra_undefined flag set. (Indu)
>>      - Set sfde_func_start_fre_off to zero, if FDE without any FREs.
>>      - In x86-64 tests use spaces to separate mnemonic from operands. (Indu)
>>      - Reword commit subject, commit message, and GNU ChangeLog.
>>           An alternative to introducing a FDE info word flag ra_undefined would
>>      be to repurpose the FRE offset in the FDE to carry indications, if
>>      the FRE count is zero.  While that would be easy in the assembler to
>>      generate and easy for a stack tracer to process it would require
>>      a few modifications in libsframe to preserve those special FRE offset
>>      values.
>>           Wouldn't it make sense to remove the half baked versioned operations
>>      (see struct sframe_version_ops) with a preparatory cleanup patch?  I
>>      don't see how the current implementation would work in the future.
>>      The operations would otherwise probably need to be split up into
>>      smaller ones: fde_info_init(fde_type, fre_type), fde_info_set_pauth_key,
>>      fde_info_set_ra_undefined, ... .
>>      
> 
> Yeah, sure. Makes sense.

This was a leftover from V3, where I had to touch the sframe_version_ops.
Given this is no longer the case with V4 I would leave this cleanup
for a future general cleanup of the static function names.


>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c

>> @@ -164,6 +164,7 @@ sframe_fre_set_ra_track (struct sframe_row_entry *fre, offsetT ra_offset)
>>   {
>>     fre->ra_loc = SFRAME_FRE_ELEM_LOC_STACK;
>>     fre->ra_offset = ra_offset;
>> +  fre->ra_undefined = false;
>>     fre->merge_candidate = false;
>>   }
>>   @@ -335,6 +336,9 @@ get_fre_base_reg_id (struct sframe_row_entry *sframe_fre)
>>     unsigned int cfi_insn_cfa_base_reg = sframe_fre->cfa_base_reg;
>>     unsigned fre_base_reg = SFRAME_BASE_REG_SP;
>>   +  if (sframe_fre->ra_undefined)
>> +    return 0;
>> +
> 
> This effectively means SFRAME_BASE_REG_FP; return 0 is generally
> confusing to read (at least for me).
> 
> How about we return the SFRAME_BASE_REG_SP instead with some code
> comments if you see fit.

My intend was to set as much of the FRE info word zero as possible,
to make it easier for humans to parse SFrame data for debugging
purposes.  I did not touch mangled_ra_p, as I was unsure whether
that can also be unconditionally set to zero.  But I guess that
would be fine?

I used zero, to denote that this is about setting the field to zero
instead of a specific value with an assigned meaning, like
SFRAME_BASE_REG_SP.

What if I reverted all the changes to the SFrame helpers and did the
following change to output_sframe_row_entry instead, to make that
more explicit?

   /* Create the fre_info using the CFA base register, number of offsets and max
-     size of offset in this frame row entry.  */
-  fre_base_reg = get_fre_base_reg_id (sframe_fre);
-  fre_num_offsets = get_fre_num_offsets (sframe_fre);
-  fre_offset_size = sframe_get_fre_offset_size (sframe_fre);
+     size of offset in this frame row entry.  RA undefined is represented
+     as FRE without any offsets and all FRE info word fields zeroed.  */
+  if (sframe_fre->ra_undefined)
+    {
+      fre_base_reg = 0;
+      fre_num_offsets = 0;
+      fre_offset_size = 0;
+      fre_mangled_ra_p = 0;
+    }
+  else
+    {
+      fre_base_reg = get_fre_base_reg_id (sframe_fre);
+      fre_num_offsets = get_fre_num_offsets (sframe_fre);
+      fre_offset_size = sframe_get_fre_offset_size (sframe_fre);
+      fre_mangled_ra_p = sframe_fre->mangled_ra_p;
+    }
   fre_info = sframe_set_fre_info (fre_base_reg, fre_num_offsets,
-                                 fre_offset_size, sframe_fre->mangled_ra_p);
+                                 fre_offset_size, fre_mangled_ra_p);
   out_one (fre_info);

   /* Represent RA undefined as FRE without any offsets.  */

Alternatively:

  fre_info = 0;


>> diff --git a/gas/gen-sframe.h b/gas/gen-sframe.h

>> @@ -55,6 +55,9 @@ struct sframe_row_entry
>>     /* Whether the return address is mangled with pauth code.  */
>>     bool mangled_ra_p;
>>   +  /* Whether RA is undefined.  */
>> +  bool ra_undefined;
>> +
> 
> Can we rename this to ra_undefined_p as well ?

Sure.

>> diff --git a/libsframe/doc/sframe-spec.texi b/libsframe/doc/sframe-spec.texi

>> @@ -165,6 +165,11 @@ CFA offset adjustment and then scale down by CFA offset alignment factor).
>>     @item SFRAME_V2_S390X_CFA_OFFSET_DECODE: Decode CFA offset (i.e., scale up
>>   by CFA offset alignment factor and then revert CFA offset adjustment).
>>    @end itemize
>> +@item
>> +An SFrame FRE info word offset count of zero indicates that the return
>> +address (RA) is undefined for the range of PCs covered by the SFrame FRE.
>> +A stack tracer may use this as indication that an outermost frame has been
>> +reached and the stack trace is complete.
> 
> Please go ahead and use "errata 2" in the spec.  I will later add a
> way to say that this spec is the draft version.

Will do.

>>   @end itemize
>>     SFrame version 1 is now obsolete and should not be used.
Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/