diff mbox series

[v2,6/9] gas: Skip SFrame FDE if CFI specifies non-FP/SP base register

Message ID 20240223170800.3993092-7-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 
  Do not generate SFrame FDE if DWARF CFI directives .cfi_def_cfa or
.cfi_def_cfa_register specify a CFA base register number other than
the architecture-specific stack-pointer (SP) or frame-pointer (FP)
register numbers.

This also causes the assembler to print a warning message, so that
skipping of the SFrame FDE does not occur silently.

Update the generic ld SFrame test case to be architecture independent.
Do not use CFI directive .cfi_def_cfa, as the specified CFA base
register number is not a valid SP/FP register number on all
architectures. An invalid SP/FP register number will now cause the
assembler to print a warning message and skip SFrame FDE generation.
Remove the offending CFI directive, that cannot be coded architecture-
independent, as the test case requires SFrame information to be
generated. This was reported by the Linaro-TCWG-CI for AArch64.

gas/
	* gen-sframe.c: Skip SFrame generation if CFI specifies
	  non-FP/SP base register.

ld/testsuite/
	* ld-sframe/discard.s: Update generic SFrame test case to be
	  architecture independent.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    v1 -> v2:
    - Update generic SFrame test case to be architecture independent to
      resolve generic ld SFrame test case failure reported by
      Linaro-TCWG-CI for AArch64. It would fail similar on s390x.
    
    Without this patch the assembler would erroneously generate bad SFrame
    information for the s390-specific SFrame error test cases 1 and 2, that
    get introduced by patch "s390: Initial support to generate .sframe from
    CFI directives in assembler".

 gas/gen-sframe.c                 | 13 +++++++++++--
 ld/testsuite/ld-sframe/discard.s |  1 -
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

Indu Bhagat Feb. 24, 2024, 7:57 a.m. UTC | #1 
On 2/23/24 09:07, Jens Remus wrote:
> Do not generate SFrame FDE if DWARF CFI directives .cfi_def_cfa or
> .cfi_def_cfa_register specify a CFA base register number other than
> the architecture-specific stack-pointer (SP) or frame-pointer (FP)
> register numbers.
> 
> This also causes the assembler to print a warning message, so that
> skipping of the SFrame FDE does not occur silently.
> 
> Update the generic ld SFrame test case to be architecture independent.
> Do not use CFI directive .cfi_def_cfa, as the specified CFA base
> register number is not a valid SP/FP register number on all
> architectures. An invalid SP/FP register number will now cause the
> assembler to print a warning message and skip SFrame FDE generation.
> Remove the offending CFI directive, that cannot be coded architecture-
> independent, as the test case requires SFrame information to be
> generated. This was reported by the Linaro-TCWG-CI for AArch64.
> 
> gas/
> 	* gen-sframe.c: Skip SFrame generation if CFI specifies
> 	  non-FP/SP base register.
> 
> ld/testsuite/
> 	* ld-sframe/discard.s: Update generic SFrame test case to be
> 	  architecture independent.
> 

LGTM.

Thanks

> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      v1 -> v2:
>      - Update generic SFrame test case to be architecture independent to
>        resolve generic ld SFrame test case failure reported by
>        Linaro-TCWG-CI for AArch64. It would fail similar on s390x.
>      
>      Without this patch the assembler would erroneously generate bad SFrame
>      information for the s390-specific SFrame error test cases 1 and 2, that
>      get introduced by patch "s390: Initial support to generate .sframe from
>      CFI directives in assembler".
> 
>   gas/gen-sframe.c                 | 13 +++++++++++--
>   ld/testsuite/ld-sframe/discard.s |  1 -
>   2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index d35baaac54b2..1269b2b77c54 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -986,7 +986,11 @@ sframe_xlate_do_def_cfa (struct sframe_xlate_ctx *xlate_ctx,
>   			       get_dw_fde_start_addrS (xlate_ctx->dw_fde));
>     }
>     /* Define the current CFA rule to use the provided register and
> -     offset.  */
> +     offset.  However, if the register is not FP/SP, skip creating
> +     SFrame stack trace info for the function.  */
> +  if (cfi_insn->u.r != SFRAME_CFA_SP_REG
> +      && cfi_insn->u.r != SFRAME_CFA_FP_REG)
> +    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>     sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
>     sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.ri.offset);
>     cur_fre->merge_candidate = false;
> @@ -1004,9 +1008,14 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
>     struct sframe_row_entry *last_fre = xlate_ctx->last_fre;
>     /* Get the scratchpad FRE.  This FRE will eventually get linked in.  */
>     struct sframe_row_entry *cur_fre = xlate_ctx->cur_fre;
> +
>     gas_assert (cur_fre);
>     /* Define the current CFA rule to use the provided register (but to
> -     keep the old offset).  */
> +     keep the old offset).  However, if the register is not FP/SP,
> +     skip creating SFrame stack trace info for the function.  */
> +  if (cfi_insn->u.r != SFRAME_CFA_SP_REG
> +      && cfi_insn->u.r != SFRAME_CFA_FP_REG)
> +    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
>     sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
>     sframe_fre_set_cfa_offset (cur_fre, last_fre->cfa_offset);
>     cur_fre->merge_candidate = false;
> diff --git a/ld/testsuite/ld-sframe/discard.s b/ld/testsuite/ld-sframe/discard.s
> index a438b42bffa1..5591a50d486a 100644
> --- a/ld/testsuite/ld-sframe/discard.s
> +++ b/ld/testsuite/ld-sframe/discard.s
> @@ -5,7 +5,6 @@
>   foo:
>   	.cfi_startproc
>   	.cfi_def_cfa_offset 16
> -	.cfi_def_cfa 7, 8
>   	.cfi_endproc
>   
>   	.globl _start
diff mbox series

Patch

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index d35baaac54b2..1269b2b77c54 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -986,7 +986,11 @@  sframe_xlate_do_def_cfa (struct sframe_xlate_ctx *xlate_ctx,
 			       get_dw_fde_start_addrS (xlate_ctx->dw_fde));
   }
   /* Define the current CFA rule to use the provided register and
-     offset.  */
+     offset.  However, if the register is not FP/SP, skip creating
+     SFrame stack trace info for the function.  */
+  if (cfi_insn->u.r != SFRAME_CFA_SP_REG
+      && cfi_insn->u.r != SFRAME_CFA_FP_REG)
+    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
   sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
   sframe_fre_set_cfa_offset (cur_fre, cfi_insn->u.ri.offset);
   cur_fre->merge_candidate = false;
@@ -1004,9 +1008,14 @@  sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
   struct sframe_row_entry *last_fre = xlate_ctx->last_fre;
   /* Get the scratchpad FRE.  This FRE will eventually get linked in.  */
   struct sframe_row_entry *cur_fre = xlate_ctx->cur_fre;
+
   gas_assert (cur_fre);
   /* Define the current CFA rule to use the provided register (but to
-     keep the old offset).  */
+     keep the old offset).  However, if the register is not FP/SP,
+     skip creating SFrame stack trace info for the function.  */
+  if (cfi_insn->u.r != SFRAME_CFA_SP_REG
+      && cfi_insn->u.r != SFRAME_CFA_FP_REG)
+    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
   sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.ri.reg);
   sframe_fre_set_cfa_offset (cur_fre, last_fre->cfa_offset);
   cur_fre->merge_candidate = false;
diff --git a/ld/testsuite/ld-sframe/discard.s b/ld/testsuite/ld-sframe/discard.s
index a438b42bffa1..5591a50d486a 100644
--- a/ld/testsuite/ld-sframe/discard.s
+++ b/ld/testsuite/ld-sframe/discard.s
@@ -5,7 +5,6 @@ 
 foo:
 	.cfi_startproc
 	.cfi_def_cfa_offset 16
-	.cfi_def_cfa 7, 8
 	.cfi_endproc
 
 	.globl _start