[v2,8/9] gas: User readable warnings if SFrame FDE is not generated

  The following generic warning message, which is printed whenever the
assembler skips generation of SFrame FDE, is not very helpful for the
user:

  skipping SFrame FDE due to DWARF CFI op <name> (0x<hexval>)

Whenever possible print meaningful warning messages, when the assembler
skips generation of SFrame FDE:

- skipping SFrame FDE due to .cfi_def_cfa specifying CFA base register
  other than SP or FP (<regno> instead of <SP-regno> or <FP-regno>)
- skipping SFrame FDE due to .cfi_def_cfa_register specifying CFA base
  register other than SP or FP (<regno> instead of <SP-regno> or
  <FP-regno>)
- skipping SFrame FDE due to .cfi_def_cfa_offset without CFA base
  register in effect
- skipping SFrame FDE due to .cfi_def_cfa_offset while CFA base register
  other than SP or FP in effect (<regno> instead of <SP-regno> or
  <FP-regno>)
- skipping SFrame FDE due to .cfi_val_offset specifying {FP|RA} register
  (<regno>)
- skipping SFrame FDE due to .cfi_remember_state without SFrame FRE
  state
- skipping SFrame FDE due to .cfi_register specifying {SP|FP|RA}
  register (<regno>)
- skipping SFrame FDE due to non-default DWARF return column (<regno>
  instead of <DWARF-def-ret-col-regno>)

gas/
	* gen-sframe.h (SFRAME_FRE_BASE_REG_INVAL): New macro for
	  invalid SFrame FRE CFA base register value of -1.
	* gen-sframe.c: User readable warnings if SFrame FDE is not
	  generated.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 gas/gen-sframe.c                              | 96 ++++++++++++++-----
 gas/gen-sframe.h                              |  2 +
 gas/testsuite/gas/cfi-sframe/common-empty-1.d |  2 +-
 gas/testsuite/gas/cfi-sframe/common-empty-2.d |  2 +-
 gas/testsuite/gas/cfi-sframe/common-empty-3.d |  2 +-
 5 files changed, 79 insertions(+), 25 deletions(-)
  

On 2/23/24 09:07, Jens Remus wrote:
> The following generic warning message, which is printed whenever the
> assembler skips generation of SFrame FDE, is not very helpful for the
> user:
> 
>    skipping SFrame FDE due to DWARF CFI op <name> (0x<hexval>)
> 
> Whenever possible print meaningful warning messages, when the assembler
> skips generation of SFrame FDE:
> 
> - skipping SFrame FDE due to .cfi_def_cfa specifying CFA base register
>    other than SP or FP (<regno> instead of <SP-regno> or <FP-regno>)

I find that stating "<SP-regno> or <FP-regno>" is not necessary in the 
warning.  This information is identified unambiguously, given an ABI.

How about:
"skipping SFrame FDE due to .cfi_def_cfa defining CFA base reg to 
<regno>" or similar ?

> - skipping SFrame FDE due to .cfi_def_cfa_register specifying CFA base
>    register other than SP or FP (<regno> instead of <SP-regno> or
>    <FP-regno>)

Same thought as above.

> - skipping SFrame FDE due to .cfi_def_cfa_offset without CFA base
>    register in effect
> - skipping SFrame FDE due to .cfi_def_cfa_offset while CFA base register
>    other than SP or FP in effect (<regno> instead of <SP-regno> or
>    <FP-regno>)

Ditto.

> - skipping SFrame FDE due to .cfi_val_offset specifying {FP|RA} register
>    (<regno>)
> - skipping SFrame FDE due to .cfi_remember_state without SFrame FRE
>    state

I dont expect this warning to occur at all out in the field 
(uhand-written asm with errors is the only case that comes to mind). 
The testcase which triggers it simply happens to be just a testcase for 
checking corner case handling.

WDYT about skipping this warning altogether ?

> - skipping SFrame FDE due to .cfi_register specifying {SP|FP|RA}
>    register (<regno>)
> - skipping SFrame FDE due to non-default DWARF return column (<regno>
>    instead of <DWARF-def-ret-col-regno>)

I think this can be simply "skipping SFrame FDE due to non-default 
return addr reg <regno>".  IMO, stating the default DWARF return address 
register information in each warning msg does not add value.

Thanks

> 
> gas/
> 	* gen-sframe.h (SFRAME_FRE_BASE_REG_INVAL): New macro for
> 	  invalid SFrame FRE CFA base register value of -1.
> 	* gen-sframe.c: User readable warnings if SFrame FDE is not
> 	  generated.
> 
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>   gas/gen-sframe.c                              | 96 ++++++++++++++-----
>   gas/gen-sframe.h                              |  2 +
>   gas/testsuite/gas/cfi-sframe/common-empty-1.d |  2 +-
>   gas/testsuite/gas/cfi-sframe/common-empty-2.d |  2 +-
>   gas/testsuite/gas/cfi-sframe/common-empty-3.d |  2 +-
>   5 files changed, 79 insertions(+), 25 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 28b49a2a8425..339f4412ca05 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -867,7 +867,7 @@ sframe_row_entry_new (void)
>     struct sframe_row_entry *fre = XCNEW (struct sframe_row_entry);
>     /* Reset cfa_base_reg to -1.  A value of 0 will imply some valid register
>        for the supported arches.  */
> -  fre->cfa_base_reg = -1;
> +  fre->cfa_base_reg = SFRAME_FRE_BASE_REG_INVAL;
>     fre->merge_candidate = true;
>     /* Reset the mangled RA status bit to zero by default.  We will initialize it in
>        sframe_row_entry_initialize () with the sticky bit if set.  */
> @@ -922,6 +922,24 @@ sframe_row_entry_initialize (struct sframe_row_entry *cur_fre,
>     cur_fre->mangled_ra_p = prev_fre->mangled_ra_p;
>   }
>   
> +/* Return SFrame register name for SP, FP, and RA, or NULL if other.  */
> +
> +static const char *
> +sframe_register_name (unsigned int reg)
> +{
> +  if (reg == SFRAME_CFA_SP_REG)
> +    return "SP";
> +  else if (reg == SFRAME_CFA_FP_REG)
> +    return "FP";
> +#ifdef SFRAME_FRE_RA_TRACKING
> +  else if (sframe_ra_tracking_p ()
> +	   && (reg == SFRAME_CFA_RA_REG))
> +    return "RA";
> +#endif
> +  else
> +    return NULL;
> +}
> +
>   /* Translate DW_CFA_advance_loc into SFrame context.
>      Return SFRAME_XLATE_OK if success.  */
>   
> @@ -990,7 +1008,12 @@ sframe_xlate_do_def_cfa (struct sframe_xlate_ctx *xlate_ctx,
>        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.  */
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_def_cfa specifying CFA base "
> +		 "register other than SP or FP (%u instead of %u or %u)"),
> +	       cfi_insn->u.r, SFRAME_CFA_SP_REG, 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;
> @@ -1015,7 +1038,12 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
>        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.  */
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_register specifying "
> +		 "CFA base register other than SP or FP (%u instead of %u or %u)"),
> +	       cfi_insn->u.r, SFRAME_CFA_SP_REG, 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;
> @@ -1046,7 +1074,16 @@ sframe_xlate_do_def_cfa_offset (struct sframe_xlate_ctx *xlate_ctx,
>         cur_fre->merge_candidate = false;
>       }
>     else
> -    return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +    {
> +      if (cur_fre->cfa_base_reg == SFRAME_FRE_BASE_REG_INVAL)
> +	as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_offset without CFA "
> +		   "base register in effect"));
> +      else
> +	as_warn (_("skipping SFrame FDE due to .cfi_def_cfa_offset while CFA "
> +		   "base register other than SP or FP in effect (%u instead of %u or %u)"),
> +		 cur_fre->cfa_base_reg, SFRAME_CFA_SP_REG, SFRAME_CFA_FP_REG);
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED;
> +    }
>   
>     return SFRAME_XLATE_OK;
>   }
> @@ -1097,11 +1134,19 @@ sframe_xlate_do_val_offset (struct sframe_xlate_ctx *xlate_ctx ATTRIBUTE_UNUSED,
>        instruction can be safely skipped without sacrificing the asynchronicity of
>        stack trace information.  */
>     if (cfi_insn->u.r == SFRAME_CFA_FP_REG)
> -    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_val_offset specifying FP register (%u)"),
> +	       SFRAME_CFA_FP_REG);
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    }
>   #ifdef SFRAME_FRE_RA_TRACKING
>     else if (sframe_ra_tracking_p ()
>   	   && cfi_insn->u.r == SFRAME_CFA_RA_REG)
> -    return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_val_offset specifying RA register (%u)"),
> +	       SFRAME_CFA_RA_REG);
> +      return SFRAME_XLATE_ERR_NOTREPRESENTED; /* Not represented.  */
> +    }
>   #endif
>   
>     /* Safe to skip.  */
> @@ -1120,7 +1165,10 @@ sframe_xlate_do_remember_state (struct sframe_xlate_ctx *xlate_ctx)
>        early with non-zero error code, this will cause no SFrame stack trace
>        info for the function involved.  */
>     if (!last_fre)
> -    return SFRAME_XLATE_ERR_INVAL;
> +    {
> +      as_warn (_("skipping SFrame FDE due to .cfi_remember_state without SFrame FRE state"));
> +      return SFRAME_XLATE_ERR_INVAL;
> +    }
>   
>     if (!xlate_ctx->remember_fre)
>       xlate_ctx->remember_fre = sframe_row_entry_new ();
> @@ -1307,7 +1355,12 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
>   	  || cfi_insn->u.rr.reg1 == SFRAME_CFA_RA_REG
>   #endif
>   	  || cfi_insn->u.rr.reg1 == SFRAME_CFA_FP_REG)
> -	err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +	{
> +	  as_warn (_("skipping SFrame FDE due to .cfi_register specifying %s register (%u)"),
> +		   sframe_register_name (cfi_insn->u.rr.reg1),
> +		   cfi_insn->u.rr.reg1);
> +	  err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +	}
>         break;
>       case DW_CFA_undefined:
>       case DW_CFA_same_value:
> @@ -1315,21 +1368,19 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
>       default:
>         /* Following skipped operations do, however, impact the asynchronicity:
>   	  - CFI_escape.  */
> -      err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> -    }
> -
> -  /* An error here will cause no SFrame FDE later.  Warn the user because this
> -     will affect the overall coverage and hence, asynchronicity.  */
> -  if (err)
> -    {
> -      const char *cfi_name = sframe_get_cfi_name (op);
> -
> -      if (!cfi_name)
> -	cfi_name = _("(unknown)");
> -      as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
> -	       cfi_name, op);
> +      {
> +	const char *cfi_name = sframe_get_cfi_name (op);
> +
> +	if (!cfi_name)
> +	  cfi_name = _("(unknown)");
> +	as_warn (_("skipping SFrame FDE due to DWARF CFI op %s (%#x)"),
> +		 cfi_name, op);
> +        err = SFRAME_XLATE_ERR_NOTREPRESENTED;
> +      }
>       }
>   
> +  /* Any error will cause no SFrame FDE later.  The user has already been
> +     warned.  */
>     return err;
>   }
>   
> @@ -1346,7 +1397,8 @@ 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)
>       {
> -      as_warn (_("skipping SFrame FDE due to non-default DWARF return column"));
> +      as_warn (_("skipping SFrame FDE due to non-default DWARF return column (%u instead of %u)"),
> +	       xlate_ctx->dw_fde->return_column, DWARF2_DEFAULT_RETURN_COLUMN);
>         return SFRAME_XLATE_ERR_NOTREPRESENTED;
>       }
>   
> diff --git a/gas/gen-sframe.h b/gas/gen-sframe.h
> index fbe2fd5d9368..8ed46dbb087b 100644
> --- a/gas/gen-sframe.h
> +++ b/gas/gen-sframe.h
> @@ -24,6 +24,8 @@
>   #define SFRAME_FRE_ELEM_LOC_REG		0
>   #define SFRAME_FRE_ELEM_LOC_STACK	1
>   
> +#define SFRAME_FRE_BASE_REG_INVAL	((unsigned int)-1)
> +
>   /* SFrame Frame Row Entry (FRE).
>   
>      A frame row entry is a slice of the frame and can be valid for a set of
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> index d7756302b559..08731b069229 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> @@ -1,5 +1,5 @@
>   #as: --gsframe
> -#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_remember_state \(0xa\)
> +#warning: skipping SFrame FDE due to \.cfi_remember_state without SFrame FRE state
>   #objdump: --sframe=.sframe
>   #name: Uninteresting cfi directives generate an empty SFrame section
>   #...
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-2.d b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> index 20282c7854e8..e759cddfcc20 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-2.d
> @@ -1,5 +1,5 @@
>   #as: --gsframe
> -#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_def_cfa_offset \(0xe\)
> +#warning: skipping SFrame FDE due to \.cfi_def_cfa_offset without CFA base register in effect
>   #objdump: --sframe=.sframe
>   #name: SFrame supports only FP/SP based CFA
>   #...
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-3.d b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> index d17521dd88ea..9a46b016a9ba 100644
> --- a/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> +++ b/gas/testsuite/gas/cfi-sframe/common-empty-3.d
> @@ -1,5 +1,5 @@
>   #as: --gsframe
> -#warning: skipping SFrame FDE due to non-default DWARF return column
> +#warning: skipping SFrame FDE due to non-default DWARF return column \(0 instead of \d+\)
>   #objdump: --sframe=.sframe
>   #name: SFrame supports only default return column
>   #...