diff mbox series

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

Message ID 20240223170800.3993092-8-jremus@linux.ibm.com
State Unresolved
Headers
Series s390: Initial support to generate SFrame info from CFI directives in assembler |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Jens Remus Feb. 23, 2024, 5:07 p.m. UTC 
  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.

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".

 gas/gen-sframe.c                              | 8 ++++++--
 gas/testsuite/gas/cfi-sframe/common-empty-3.d | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Indu Bhagat Feb. 24, 2024, 8:43 a.m. UTC | #1 
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!

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.

>   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.  */
>     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;
> +    }
>   
>     /* 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
>   #...
Jens Remus Feb. 26, 2024, 4:19 p.m. UTC | #2 
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
diff mbox series

Patch

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.  */
   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;
+    }
 
   /* 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
 #...