[PATCH v2 2/2] Enable arch-specific CFI directives and DWARF instructions only when required by the target (part 1)

[Matthieu Longo]

On 2025-04-25 23:00, Indu Bhagat wrote:
> On 4/25/25 7:02 AM, Matthieu Longo wrote:
>> On 2025-04-25 14:10, Jan Beulich wrote:
>>> On 25.04.2025 13:04, Matthieu Longo wrote:
>>>> --- a/gas/dw2gencfi.c
>>>> +++ b/gas/dw2gencfi.c
>>>> @@ -720,9 +720,16 @@ const pseudo_typeS cfi_pseudo_table[] =
>>>>       { "cfi_same_value", dot_cfi, DW_CFA_same_value },
>>>>       { "cfi_remember_state", dot_cfi, DW_CFA_remember_state },
>>>>       { "cfi_restore_state", dot_cfi, DW_CFA_restore_state },
>>>> -    { "cfi_window_save", dot_cfi, DW_CFA_GNU_window_save },
>>>> +#if TC_AARCH64
>>>> +    /* cfi_window_save is an alias of cfi_negate_ra_state which is 
>>>> kept for
>>>> +       backward-compatibility concerns.  */
>>>> +    { "cfi_window_save", dot_cfi, DW_CFA_AARCH64_negate_ra_state },
>>>
>>> Why's this of concern only for Arm64?
>>>
>>
>> I am not sure that I understand what your question is here.
>> cfi_window_save is only used by AArch64 and Sparc.
>>
>>>> --- a/gas/gen-sframe.c
>>>> +++ b/gas/gen-sframe.c
>>>> @@ -1257,6 +1257,8 @@ sframe_xlate_do_restore (struct 
>>>> sframe_xlate_ctx *xlate_ctx,
>>>>     return SFRAME_XLATE_OK;
>>>>   }
>>>> +#if TC_AARCH64
>>>> +
>>>>   /* Translate DW_CFA_AARCH64_negate_ra_state into SFrame context.
>>>>      Return SFRAME_XLATE_OK if success.  */
>>>> @@ -1287,30 +1289,7 @@ 
>>>> sframe_xlate_do_aarch64_negate_ra_state_with_pc (struct 
>>>> sframe_xlate_ctx *xlate_
>>>>     return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>>>>   }
>>>> -/* Translate DW_CFA_GNU_window_save into SFrame context.
>>>> -   DW_CFA_GNU_window_save is a DWARF Sparc extension, but is 
>>>> multiplexed with a
>>>> -   directive of DWARF AArch64 extension: 
>>>> DW_CFA_AARCH64_negate_ra_state.
>>>> -   The AArch64 backend of GCC 14 and older versions was emitting 
>>>> mistakenly the
>>>> -   Sparc CFI directive (.cfi_window_save).  From GCC 15, the 
>>>> AArch64 backend
>>>> -   only emits .cfi_negate_ra_state.  For backward compatibility, 
>>>> the handler for
>>>> -   .cfi_window_save needs to check whether the directive was used 
>>>> in a AArch64
>>>> -   ABI context or not.
>>>> -   Return SFRAME_XLATE_OK if success.  */
>>>> -
>>>> -static int
>>>> -sframe_xlate_do_gnu_window_save (struct sframe_xlate_ctx *xlate_ctx,
>>>> -                 struct cfi_insn_data *cfi_insn)
>>>> -{
>>>> -  unsigned char abi_arch = sframe_get_abi_arch ();
>>>> -
>>>> -  /* Translate DW_CFA_AARCH64_negate_ra_state into SFrame context.  */
>>>> -  if (abi_arch == SFRAME_ABI_AARCH64_ENDIAN_BIG
>>>> -      || abi_arch == SFRAME_ABI_AARCH64_ENDIAN_LITTLE)
>>>> -    return sframe_xlate_do_aarch64_negate_ra_state (xlate_ctx, 
>>>> cfi_insn);
>>>> -
>>>> -  as_warn (_("skipping SFrame FDE; .cfi_window_save"));
>>>> -  return SFRAME_XLATE_ERR_NOTREPRESENTED;  /* Not represented.  */
>>>> -}
>>>> +#endif /* TC_AARCH64 */
>>>>   /* Handle DW_CFA_expression in .cfi_escape.
>>>> @@ -1622,14 +1601,14 @@ sframe_do_cfi_insn (struct sframe_xlate_ctx 
>>>> *xlate_ctx,
>>>>       case DW_CFA_restore:
>>>>         err = sframe_xlate_do_restore (xlate_ctx, cfi_insn);
>>>>         break;
>>>> -    /* DW_CFA_AARCH64_negate_ra_state is multiplexed with
>>>> -       DW_CFA_GNU_window_save.  */
>>>> -    case DW_CFA_GNU_window_save:
>>>> -      err = sframe_xlate_do_gnu_window_save (xlate_ctx, cfi_insn);
>>>> +#if TC_AARCH64
>>>> +    case DW_CFA_AARCH64_negate_ra_state:
>>>> +      err = sframe_xlate_do_aarch64_negate_ra_state (xlate_ctx, 
>>>> cfi_insn);
>>>>         break;
>>>>       case DW_CFA_AARCH64_negate_ra_state_with_pc:
>>>>         err = sframe_xlate_do_aarch64_negate_ra_state_with_pc 
>>>> (xlate_ctx, cfi_insn);
>>>>         break;
>>>> +#endif /* TC_AARCH64 */
>>>>       case DW_CFA_register:
>>>>         err = sframe_xlate_do_register (xlate_ctx, cfi_insn);
>>>>         break;
>>>
>>> This wants looking over by Indu (now Cc-ed).
>>
>> FYI I removed DW_CFA_GNU_window_save following the suggestion of Jens 
>> Remus:
>> https://inbox.sourceware.org/binutils/08aa86e2-f1ba-49cf-a755- 
>> ecb7114308e5@linux.ibm.com/
> 
> Hi Matthieu,
> 
> I think the diffs for gen-sframe.c can be dropped altogether.
> 
> The code in gen-sframe.c is already checking for the relevant ABI.  And 
> SFrame
> generation here feeds off the DWARF cfi_insn created by dw2gencfi* process.
> With your changes in gas/dw2gencfi.c, user will anyway see a 'Error: 
> unknown
> pseudo-op: ' for say .cfi_window_save on AMD64.
> 
> Having said that, IMO, inspite of whether we error out / silently ignore 
> these
> usages for non-supported arches in dw2gencfi, its best that the code in
> gen-sframe.c continues to warn and skip FDE like it was doing earlier.
> 
> Thanks
> Indu

Hi Indu,

 From what I can read in gas/gen-sframe.c, this patch is not silently 
ignoring the non-supported directives.
The default case will handle them and will print out:
   skipping SFrame FDE; CFI insn <name of the DWARF instruction> (0xXX)

get_DW_CFA_name defined in libiberty/dwarfnames.c will look up at the 
definitions and return the right name. So instead of returning "skipping 
SFrame FDE; .cfi_window_save", it would return "skipping SFrame FDE; 
DW_CFA_GNU_window_save (0x2d)".
This message seems acceptable to me even if it does not refer to the CFI 
directive but the associated DWARF instruction.

 From this perspective, sframe_xlate_do_gnu_window_save seems redundant, 
and does not bring any value.
In the light of this explanation, do you still want me to revert the 
patch on this file ?

Matthieu