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

Wed Apr 23 09:13:22 GMT 2025

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

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.

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.

>>>> +/* 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