[PATCH v2 8/9] gas: User readable warnings if SFrame FDE is not generated
Jens Remus
jremus@linux.ibm.com
Tue Apr 9 14:14:42 GMT 2024
Am 29.02.2024 um 08:39 schrieb Indu Bhagat:
> 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 ?
Sure. What about:
"skipping SFrame FDE due to .cfi_def_cfa defining non-SP/FP register
<regno> as CFA base register"
I think it would be helpful for the user to know that the issue is
caused by a non-SP/FP register. But maybe that is because I am new to
CFI and those that would actually debug the cause for these warnings
would not need this extra information?
If you prefer your suggestions I would go with yours. I understand that
the warning text is otherwise pretty long.
>> - 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.
Agreed.
>> - 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.
Agreed.
>> - 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 ?
I understand your reasoning. But I would prefer generation of SFrame FDE
not to be silently skipped. Even or especially if it is an obscure
special case.
With my changes sframe_do_cfi_insn() assumes any of its called
sframe_do_*() CFI instruction processing functions to have issued a
warning, if they return with an error such as
SFRAME_XLATE_ERR_NOTREPRESENTED. Thus skipping this warning would cause
generation of SFrame FDE to be silently skipped.
>> - 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.
Agreed.
> 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
>> #...
>
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/
More information about the Binutils
mailing list