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

[Jan Beulich]

On 23.04.2025 11:13, Matthieu Longo wrote:
> On 2025-04-22 13:39, Jan Beulich wrote:
>> On 22.04.2025 14:00, Matthieu Longo wrote:
>>> On 2025-04-10 08:34, Jan Beulich wrote:
>>>> On 09.04.2025 12:36, Matthieu Longo wrote:
>>>>> --- a/include/dwarf2.def
>>>>> +++ b/include/dwarf2.def
>>>>> @@ -780,20 +780,46 @@ DW_CFA (DW_CFA_val_offset, 0x14)
>>>>>    DW_CFA (DW_CFA_val_offset_sf, 0x15)
>>>>>    DW_CFA (DW_CFA_val_expression, 0x16)
>>>>>    
>>>>> +/* Users extensions.
>>>>> +   Note: DW_CFA_lo_user and DW_CFA_hi_user can be aliased to used as extension.
>>>>> +   This is a closed interval.  */
>>>>>    DW_CFA (DW_CFA_lo_user, 0x1c)
>>>>> -DW_CFA (DW_CFA_hi_user, 0x3f)
>>>>>    
>>>>> -/* SGI/MIPS specific.  */
>>>>> +/* For gas, we disable the extensions according to the target architecture.  */
>>>>> +#ifdef MODULE_GAS
>>>>> +/* SGI/MIPS specific extensions.  */
>>>>> +#ifdef TC_MIPS
>>>>>    DW_CFA (DW_CFA_MIPS_advance_loc8, 0x1d)
>>>>> -/* AArch64 extensions.  */
>>>>> +#endif /* TC_MIPS */
>>>>> +
>>>>> +/* AArch64 specific extensions.  */
>>>>> +#ifdef TC_AARCH64
>>>>>    DW_CFA (DW_CFA_AARCH64_negate_ra_state_with_pc, 0x2c)
>>>>> -/* GNU extensions.
>>>>> -   NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.  */
>>>>> +DW_CFA (DW_CFA_AARCH64_negate_ra_state, 0x2d)
>>>>> +/* NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.  */
>>>>> +DW_CFA_DUP (DW_CFA_GNU_window_save, 0x2d)
>>>>> +#endif /* TC_AARCH64 */
>>>>> +
>>>>> +/* Sparc specific extensions.  */
>>>>> +#ifdef TC_SPARC
>>>>>    DW_CFA (DW_CFA_GNU_window_save, 0x2d)
>>>>> -DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
>>>>> +#endif /* TC_SPARC */
>>>>> +#else
>>>>> +/* SGI/MIPS specific extensions.  */
>>>>> +DW_CFA (DW_CFA_MIPS_advance_loc8, 0x1d)
>>>>> +/* AArch64 specific extensions.  */
>>>>> +DW_CFA (DW_CFA_AARCH64_negate_ra_state_with_pc, 0x2c)
>>>>> +DW_CFA (DW_CFA_AARCH64_negate_ra_state, 0x2d)
>>>>> +/* NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.  */
>>>>> +DW_CFA_DUP (DW_CFA_GNU_window_save, 0x2d)
>>>>> +#endif
>>>>
>>>> Does this really need doing with nested #if/#else? I think it would be easier
>>>> to follow if each DW_CFA_* appeared just once here. And whether ...
>>>>
>>>
>>> I am not sure I understand what you would like to see here.
>>> The idea is to make the file easy to parse for the eyes.
>>
>> Right, and to my eyes the above is pretty hard to follow, i.e. ...
>>
>>> The first level is whether it is included from gas, or not.
>>> The second level (for gas only) is to select the directives depending on
>>> the backend with TC_<arch>.
>>> The occurrence of the directives twice depending on the include context
>>> looks acceptable to me as it make the code easier to read from my
>>> perspective.
>>
>> ... we apparently disagree here. Can you remind me why this file needs
>> touching at all here? The definitions are all fine to have, it's how
>> they're used which needs adjustment? Leaving this file alone would, aiui,
>> also eliminate the question about the TC_* constants (further up).
>>
> 
> Indeed the definitions are all fine to have, but it means that this also 
> allows values to be used whereas they should not even be defined for the 
> target architecture (this comment is gas-specific).
> 
> My second argument in favor of this change is rather weak, but is more 
> regarding the intent of DW_CFA_DUP.
> When I see this line with DW_CFA_DUP in the code,
>    DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)
> I think that DW_CFA_GNU_window_save is actually the legitimate operation 
> to use, whereas:
>    DW_CFA (DW_CFA_AARCH64_negate_ra_state, 0x2d)
>    /* NOTE: DW_CFA_GNU_window_save is multiplexed on Sparc and AArch64.*/
>    DW_CFA_DUP (DW_CFA_GNU_window_save, 0x2d)
> expresses that the second definition is only an alias to 
> DW_CFA_AARCH64_negate_ra_state.

Looks like you have an Arm64 bias here. DW_CFA_GNU_window_save was there
for much longer, aiui, and hence if either of he two deserves the
attribute "legitimate", it's that one. Really both are "legitimate" only
within their respective arch. In that sense we probably better had

DW_CFA_DUP (DW_CFA_GNU_window_save, 0x2d)
DW_CFA_DUP (DW_CFA_AARCH64_negate_ra_state, 0x2d)

The one place where the difference between DW_CFA() and DW_CFA_DUP()
matters is libiberty afaics, yet that's not target-specific code.

> This mistake of using DW_CFA_GNU_window_save on AArch64 whereas it is 
> intended to be Sparc-only, might not have taken place if we had first 
> the TC_* in this file to disable the arch extensions not intended for 
> AArch64. At least, it would have been explicit, and I think that it 
> would have encouraged to redefine an AArch64 extension properly.

Or if the identifier properly named SPARC rather than GNU.

Jan