Re: [PATCH v2 7/9] gas: Warn if SFrame FDE is skipped due to non-default return column

[Jens Remus <jremus@linux.ibm.com>]

Am 24.02.2024 um 09:43 schrieb Indu Bhagat:
> On 2/23/24 09:07, Jens Remus wrote:
>> Print a warning message if SFrame FDE is skipped due to a non-default
>> DWARF return column (i.e. return address (RA) register number). This
>> may be caused by the use of CFI directive .cfi_return_column with a
>> non-default return address (RA) register number in the processed
>> assembler source code.
>>
>>    Warning: skipping SFrame FDE due to non-default DWARF return column
>>
>> gas/
>>     * gen-sframe.c: Warn if SFrame FDE is skipped due to non-default
>>       DWARF return column.
>>
>> gas/testsuite/
>>     * gas/cfi-sframe/common-empty-3.d: Update test case to expect
>>       for new warning message when SFrame FDE is skipped due to
>>       a non-default DWARF return column.
>>
> 
> One request in my comments below.
> 
> LGTM, Thanks!

Thank you for the review!

> PS: I will take a look at the next 2 patches (Patch 8/9 and Patch 9/9) 
> in the series soon.
> 
>> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>
>> Notes (jremus):
>>      Without this patch the assembler would erroneously generate bad 
>> SFrame
>>      information for the s390-specific SFrame error test case 4, that 
>> gets
>>      introduced by patch "s390: Initial support to generate .sframe 
>> from CFI
>>      directives in assembler".
>>
> 
> This patch is only adding a warning.  The absence of it shouldn't have 
> caused any bad SFrame info.

You are correct! It is not bad SFrame info, but unexpected absent SFrame 
info. Without the warning the user does not know that the SFrame info is 
incomplete, which may be an issue during unwinding.

>>   gas/gen-sframe.c                              | 8 ++++++--
>>   gas/testsuite/gas/cfi-sframe/common-empty-3.d | 1 +
>>   2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> index 1269b2b77c54..28b49a2a8425 100644
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1345,7 +1345,10 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>>     /* If the return column is not RIP, SFrame format cannot represent 
>> it.  */
> 
> May I ask you to also include an update to this incorrect comment in 
> your patch to something like:
> 
> /* SFrame format cannot represent a non-default DWARF return column 
> reg.  */

Sure.

>>     if (xlate_ctx->dw_fde->return_column != DWARF2_DEFAULT_RETURN_COLUMN)
>> -    return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +    {
>> +      as_warn (_("skipping SFrame FDE due to non-default DWARF return 
>> column"));
>> +      return SFRAME_XLATE_ERR_NOTREPRESENTED;
>> +    }

Actually I wonder whether the test should better use SFRAME_CFA_RA_REG
instead of DWARF2_DEFAULT_RETURN_COLUMN? Of course that would require 
SFRAME_CFA_RA_REG and x86_sframe_cfa_ra_reg to be defined and assigned 
for x86 AMD64, instead of removing the currently unused variable in 
patch 1 of this series.

Thinking more about this I wonder why doesn't SFrame by default #define 
SFRAME_CFA_RA_REG to be DWARF2_DEFAULT_RETURN_COLUMN?

>>     /* Iterate over the CFIs and create SFrame FREs.  */
>>     for (cfi_insn = dw_fde->data; cfi_insn; cfi_insn = cfi_insn->next)
>> @@ -1355,7 +1358,8 @@ sframe_do_fde (struct sframe_xlate_ctx *xlate_ctx,
>>         if (err != SFRAME_XLATE_OK)
>>       {
>>         /* Skip generating SFrame stack trace info for the function if 
>> any
>> -         offending CFI is encountered by sframe_do_cfi_insn ().  */
>> +         offending CFI is encountered by sframe_do_cfi_insn ().  Warning
>> +         message already printed by sframe_do_cfi_insn ().  */
>>         return err; /* Return the error code.  */
>>       }
>>       }
>> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d 
>> b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
>> index 5914c620760d..d17521dd88ea 100644
>> --- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
>> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
>> @@ -1,4 +1,5 @@
>>   #as: --gsframe
>> +#warning: skipping SFrame FDE due to non-default DWARF return column
>>   #objdump: --sframe=.sframe
>>   #name: SFrame supports only default return column
>>   #...

Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+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/