Re: [PATCH v3 05/15] gas: Print DWARF call frame insn name in SFrame warning message

[Indu Bhagat <indu.bhagat@oracle.com>]

On 4/12/24 07:47, Jens Remus wrote:
> SFrame generation prints the DWARF call frame instruction opcode in
> hexadecimal. Leverage get_DW_CFA_name to additionally print the
> DWARF call frame instruction name in human readable form, while also
> respecting fake CFI types. Use "(unknown)", if the DWARF call frame
> instruction name is not known.
> 
> This changes the following assembler SFrame generation warning message
> as follows:
> 
> Old:
> Warning: skipping SFrame FDE due to DWARF CFI op 0x<hexnum>
> 
> New:
> Warning: skipping SFrame FDE due to DWARF CFI op <name> (0x<hexnum>)
> 
> gas/
> 	* gen-sframe.c (sframe_get_cfi_name): New function to get the
> 	DWARF call frame instruction name for a DWARF call frame
> 	instruction opcode.
> 	(sframe_do_cfi_insn): Use sframe_get_cfi_name to print the
> 	DWARF call frame instruction name for the DWARF call frame
> 	instruction opcode in the warning message.
> 
> gas/testsuite/
> 	* gas/cfi-sframe/common-empty-1.d: Update expected SFrame
> 	warning message text for DWARF call frame insn name.
> 	* gas/cfi-sframe/common-empty-2.d: Likewise.
> 
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Reviewed-by: Indu Bhagat <indu.bhagat@oracle.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - Removed stale ChangeLog entries from commit message.
>      - Corrected formatting of ChangeLog in commit message.
> 
>   gas/gen-sframe.c                              | 49 ++++++++++++++++++-
>   gas/testsuite/gas/cfi-sframe/common-empty-1.d |  2 +-
>   gas/testsuite/gas/cfi-sframe/common-empty-2.d |  2 +-
>   3 files changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index 75781fc8ccbd..d35baaac54b2 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1197,6 +1197,46 @@ sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>     return SFRAME_XLATE_OK;
>   }
>   
> +/* Returns the DWARF call frame instruction name or fake CFI name for the
> +   specified CFI opcode, or NULL if the value is not recognized.  */
> +
> +static const char *
> +sframe_get_cfi_name (int cfi_opc)
> +{
> +  const char *cfi_name;
> +
> +  switch (cfi_opc)
> +    {
> +      /* Fake CFI type; outside the byte range of any real CFI insn.  */
> +      /* See gas/dw2gencfi.h.  */
> +      case CFI_adjust_cfa_offset:
> +	cfi_name = "CFI_adjust_cfa_offset";
> +	break;
> +      case CFI_return_column:
> +	cfi_name = "CFI_return_column";
> +	break;
> +      case CFI_rel_offset:
> +	cfi_name = "CFI_rel_offset";
> +	break;
> +      case CFI_escape:
> +	cfi_name = "CFI_escape";
> +	break;
> +      case CFI_signal_frame:
> +	cfi_name = "CFI_signal_frame";
> +	break;
> +      case CFI_val_encoded_addr:
> +	cfi_name = "CFI_val_encoded_addr";
> +	break;
> +      case CFI_label:
> +	cfi_name = "CFI_label";
> +	break;
> +      default:
> +	cfi_name = get_DW_CFA_name (cfi_opc);
> +    }
> +
> +  return cfi_name;
> +}
> +
>   /* Process CFI_INSN and update the translation context with the FRE
>      information.
>   
> @@ -1272,7 +1312,14 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx *xlate_ctx,
>     /* An error here will cause no SFrame FDE later.  Warn the user because this
>        will affect the overall coverage and hence, asynchronicity.  */
>     if (err)
> -    as_warn (_("skipping SFrame FDE due to DWARF CFI op %#x"), 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);

Nit again: I realized when looking at this patch again that DWARF 
standard uses the terminology of "instruction" for these DW_CFA_*.  I 
looked in the existing codebase and there is precedent to using "CFI 
instruction" in error messages.  How about we change this to "due to CFI 
insn" ?

OK with that change.

Thanks for the patch.

> +    }
>   
>     return err;
>   }
> diff --git a/gas/testsuite/gas/cfi-sframe/common-empty-1.d b/gas/testsuite/gas/cfi-sframe/common-empty-1.d
> index 125612ff841f..d7756302b559 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 0xa
> +#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_remember_state \(0xa\)
>   #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 59328fc1033f..20282c7854e8 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 0xe
> +#warning: skipping SFrame FDE due to DWARF CFI op DW_CFA_def_cfa_offset \(0xe\)
>   #objdump: --sframe=.sframe
>   #name: SFrame supports only FP/SP based CFA
>   #...