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

Tue Apr 22 12:39:13 GMT 2025

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/gas/configure.ac
>>> +++ b/gas/configure.ac
>>> @@ -937,6 +937,8 @@ AC_DEFINE_UNQUOTED(TARGET_CPU,		"${target_cpu}",   [Target CPU.])
>>>   AC_DEFINE_UNQUOTED(TARGET_VENDOR,	"${target_vendor}", [Target vendor.])
>>>   AC_DEFINE_UNQUOTED(TARGET_OS,		"${target_os}",    [Target OS.])
>>>   
>>> +AC_DEFINE(MODULE_GAS, 1, [Building gas module])
>>
>> Hmm, here I'm again not sure: As this is unconditional, I'd have expected
>> the definition to come from Makefile.am instead. Question is whether we need
> 
> It makes sense. See my next comment.
> 
>> the extra #define in the first place: TC_* shouldn't be defined in subdirs
>> other than gas/. Then again dwarf2.def is shared with at least gcc, so
>> perhaps better to be on the safe side. However, as.h already defines GAS -
>> can't we key to that?
>>
> 
> ## About TC_*
> 
> Well... You don't want that those values be defined in gas in the wrong 
> backend, so I don't see how we can achieve that without using TC_<arch>.
> 
> ## About using the guard GAS
> 
> Your proposal of using the guard GAS defined in as.h will work because:
> 1. dwarf2.def is only included by dwarf2.h
> 2. dwarf2.h is included by:
>    - gas/dw2gencfi.h does not include as.h
>      but is included by:
>      - gas/as.c
>      - gas/dw2gencfi.c
>      - gas/gen-sframe.c
>      which all include as.h before dw2gencfi.h
> 
>    - gas/dwarf2dbg.c
>    - gas/ehopt.c
>    - gas/scfidw2gen.h
>      which all three include as.h before dwarf2.h
> 
> However, this approach can allow a silent breakage of the logic if the 
> headers are reordered.

I have been growing the understanding that as.h is expect to be included
first. Otherwise it defining GAS and it including config.h may come too
late.

> Using makefile.am to force a define for all the files in gas directory 
> seems the safest and correct option. Unless you disagree, I will adopt 
> this approach in the next revision.
> I added -DMODULE_GAS to AM_CFLAGS line 45 in gas/Makefile.am. Is this 
> the correct way ?

I'm far from being an automake expert, so I'd rather not say "yes" or "no"
here.

> Maybe the name of the define should be changed to something less 
> dependent of gas, like DWARF_EXTENSIONS_ALL ? What do you think ?

Possibly. Yet its bounded to binutils as a project, so unless some other
user appears internal to the project, it may not make much of a difference.

>>> --- 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 },
>>>       { "cfi_negate_ra_state", dot_cfi, DW_CFA_AARCH64_negate_ra_state },
>>>       { "cfi_negate_ra_state_with_pc", dot_cfi, DW_CFA_AARCH64_negate_ra_state_with_pc },
>>> +#endif /* TC_AARCH64 */
>>> +#if TC_SPARC
>>> +    { "cfi_window_save", dot_cfi, DW_CFA_GNU_window_save },
>>> +#endif /* TC_SPARC */
>>
>> See comment further up (applicable also further down).
> 
> What is this comment you are referring to ?

The one discussing Sparc-only vs generic to all GNU.

>>> --- 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).

>>> +/* GNU extensions.  */
>>>   DW_CFA (DW_CFA_GNU_args_size, 0x2e)
>>>   DW_CFA (DW_CFA_GNU_negative_offset_extended, 0x2f)
>>>   
>>> +DW_CFA (DW_CFA_hi_user, 0x3f)
>>
>> ... this really needs to move I'm also unsure.
> 
> The code of the directives is currently ordered by their code. Following 
> the same logic, DW_CFA_hi_user should be at the bottom, not below 
> DW_CFA_lo_user.

Yet you disconnect it from the comment (that you add) mentioning it. My
general request is: Don't move things around when it's not part of the
purpose of a change (which imo pretty clearly it isn't here).

Jan