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

  DWARF 5 standard provides a space for vendor-specific DWARF directives.
Binutils currently defines GNU-specific DWARF directives and vendor-specific
ones for AArch64, SGI/MIPS, and Sparc architectures.

The architecture specific code is not guarded. This has caused confusion in
the past, and .cfi_window_save (a Sparc-specific directive) was used on
AArch64 in place of .cfi_negate_ra_state, the defined directive by the "DWARF
extensions on AArch64" document (see [1]).

Gas only supports one target architecture (the one selected with --target),
whereas others tools like readelf, objdump, etc. deduce the target
architecture from the input object at runtime. Hence, using TC_<arch>
definitions to enable/disable DWARF directives at compilation time outside
of gas is excluded.

Since Gas is target-specific, the solution consists in detecting whether
dwarf2.def is included from Gas or somewhere else. To do so, a new define
MODULE_GAS was added in configure.ac to allow to detect the inclusion
provenance, and the two following behaviors can be observed in dwarf2.def:
- if MODULE_GAS is defined (included from gas folder), the architecture-
  specific DWARF instructions are enabled/disabled via TC_<arch> defines.
- if MODULE_GAS is undefined (included from bfd and binutils folders), all
  the DWARF instructions are defined (as it is today).

In bfd, the architecture of the input object is detected at runtime using
bfd_get_arch, and only the architecture-specific instructions matching the
target of the object are considered during the processing.

This patch deliberately excluded from the scope binutils/dwarf.c that also
requires a check of the architecture at runtime. This part will be addressed
in a follow-up patch.

[1]: https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst#id2
---
 bfd/elf-eh-frame.c | 90 +++++++++++++++++++++++++++++++++++++++-------
 gas/config.in      |  3 ++
 gas/configure      |  4 +++
 gas/configure.ac   |  2 ++
 gas/dw2gencfi.c    | 33 ++++++++++++++---
 gas/gen-sframe.c   | 35 ++++--------------
 include/dwarf2.def | 38 ++++++++++++++++----
 7 files changed, 154 insertions(+), 51 deletions(-)
  

On 09.04.2025 12:36, Matthieu Longo wrote:
> --- a/bfd/elf-eh-frame.c
> +++ b/bfd/elf-eh-frame.c
> @@ -338,12 +338,79 @@ next_cie_fde_offset (const struct eh_cie_fde *ent,
>    return sec->size;
>  }
>  
> +static bool
> +skip_cfa_op_aarch64 (bfd_byte op,
> +		     bfd_byte **iter ATTRIBUTE_UNUSED,
> +		     bfd_byte *end ATTRIBUTE_UNUSED)
> +{
> +  switch (op)
> +    {
> +      case DW_CFA_AARCH64_negate_ra_state:
> +      case DW_CFA_AARCH64_negate_ra_state_with_pc:

Much like it is in existing code you modify (further down), case labels want
to align with the opening brace. I.e. here and below you will want to un-
indent what's inside the figure braces of switch() by one level.

> +static bool
> +skip_cfa_op_arch_specific (enum bfd_architecture arch,
> +			   bfd_byte **iter, bfd_byte *end,
> +			   bfd_byte op)
> +{
> +  switch (arch)
> +    {
> +      case bfd_arch_aarch64:
> +	return skip_cfa_op_aarch64 (op, iter, end);
> +      case bfd_arch_mips:
> +	return skip_cfa_op_mips (op, iter, end);
> +      case bfd_arch_sparc:
> +	return skip_cfa_op_sparc (op, iter, end);

Hmm, I'm uncertain here - from the name DW_CFA_GNU_window_save isn't Sparc-
specific. Yet then I can see that gcc uses it only there.

In any event, much like you do further up, having blank lines between non-
fall-through case blocks would be nice.

> --- 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
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?

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

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

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

Jan
  

On 2025-04-10 08:34, Jan Beulich wrote:
> On 09.04.2025 12:36, Matthieu Longo wrote:
>> --- a/bfd/elf-eh-frame.c
>> +++ b/bfd/elf-eh-frame.c
>> @@ -338,12 +338,79 @@ next_cie_fde_offset (const struct eh_cie_fde *ent,
>>     return sec->size;
>>   }
>>   
>> +static bool
>> +skip_cfa_op_aarch64 (bfd_byte op,
>> +		     bfd_byte **iter ATTRIBUTE_UNUSED,
>> +		     bfd_byte *end ATTRIBUTE_UNUSED)
>> +{
>> +  switch (op)
>> +    {
>> +      case DW_CFA_AARCH64_negate_ra_state:
>> +      case DW_CFA_AARCH64_negate_ra_state_with_pc:
> 
> Much like it is in existing code you modify (further down), case labels want
> to align with the opening brace. I.e. here and below you will want to un-
> indent what's inside the figure braces of switch() by one level.
> 

Will be fixed in the next revision.

>> +static bool
>> +skip_cfa_op_arch_specific (enum bfd_architecture arch,
>> +			   bfd_byte **iter, bfd_byte *end,
>> +			   bfd_byte op)
>> +{
>> +  switch (arch)
>> +    {
>> +      case bfd_arch_aarch64:
>> +	return skip_cfa_op_aarch64 (op, iter, end);
>> +      case bfd_arch_mips:
>> +	return skip_cfa_op_mips (op, iter, end);
>> +      case bfd_arch_sparc:
>> +	return skip_cfa_op_sparc (op, iter, end);
> 
> Hmm, I'm uncertain here - from the name DW_CFA_GNU_window_save isn't Sparc-
> specific. Yet then I can see that gcc uses it only there.
> 

The name of DW_CFA_GNU_window_save is misleading. I confirm that this is 
only used for Sparc in binutils, and GCC.
AArch64 backend was wrongly using cfi_window_save instead of 
cfi_negate_ra_state, which added to the confusion, but those two 
directives point to the same code 0x2d.

> In any event, much like you do further up, having blank lines between non-
> fall-through case blocks would be nice.
> 

Will be fixed in the next revision.

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

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 ?
Maybe the name of the define should be changed to something less 
dependent of gas, like DWARF_EXTENSIONS_ALL ? What do you think ?

>> --- 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 ?

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

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

> 
> Jan

Matthieu.
  

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
  

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
  

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
  

On 2025-04-23 14:19, Jan Beulich wrote:
> 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.
> dwar
> Or if the identifier properly named SPARC rather than GNU.
> 
> Jan

To summarize, you don't want to touch dwarf2.def, and we accept that 
someone could misuse those definitions if they use them with the wrong 
target architecture.
This means that we don't need a #ifdef GAS_MODULE, and no #ifdef TC_* in 
dwarf2.def.
I will remove this from the patch in the next revision.