[PATCH v6 02/19] gas: move code for object attribute parsing into obj-elf-attr.c
Matthieu Longo
matthieu.longo@arm.com
Mon Jul 14 16:41:42 GMT 2025
On 2025-07-14 09:25, Jan Beulich wrote:
> On 11.07.2025 13:28, Matthieu Longo wrote:
>> Gas, contrarilly to others binutils tools, is compiled for a specific
>> target. Some targets don't support Object Attributes (OAs). For those
>> cases, today the OA directive ".gnu_attribute" is still enabled but the
>> processing would probably fail in most of cases because the named tag
>> would be unknown. Most of the parsing code on such a target can be
>> considered as dead code.
>>
>> This patch aims at removing this dead code from Gas when the target does
>> not support the OAs by:
>> - moving the code of OA parsing into a separate file under gas/config
>> which is only included for the relevant targets supporting OAs.
>> - disabling the code related to OAs on non-OA target via a TC_OBJ_ATTR
>> macro.
>>
>> Adding/removing the OA feature from Gas for a specific target can easilly
>> be done from tc-<arch>.h by changing the values of TC_OBJ_ATTR: 1 enabled,
>> 0 disabled. You might also want to guard the enablement of OAs only for
>> ELF targets with OBJ_ELF (see example below).
>>
>> \#ifdef OBJ_ELF
>> /* The target supports Object Attributes. */
>> \#define TC_OBJ_ATTR 1
>> \#endif
>> ---
>> gas/Makefile.am | 2 +
>> gas/Makefile.in | 5 +
>> gas/config/obj-elf-attr.c | 239 ++++++++++++++++++++++++++++++++++++++
>> gas/config/obj-elf-attr.h | 36 ++++++
>> gas/config/obj-elf.c | 224 ++---------------------------------
>> gas/config/obj-elf.h | 4 +-
>> gas/config/tc-arc.h | 5 +
>> gas/config/tc-arm.h | 8 ++
>> gas/config/tc-csky.h | 5 +
>> gas/config/tc-m68k.h | 5 +
>> gas/config/tc-mips.h | 5 +
>> gas/config/tc-msp430.h | 8 ++
>> gas/config/tc-ppc.h | 8 ++
>> gas/config/tc-riscv.h | 5 +
>> gas/config/tc-s390.h | 8 ++
>> gas/config/tc-sparc.h | 5 +
>> gas/config/tc-tic6x.h | 9 ++
>> gas/configure | 13 +++
>> gas/configure.ac | 13 +++
>> gas/doc/as.texi | 3 +-
>> 20 files changed, 394 insertions(+), 216 deletions(-)
>> create mode 100644 gas/config/obj-elf-attr.c
>> create mode 100644 gas/config/obj-elf-attr.h
>
> I think gas/po/POTFILES.in also needs adding to. It's a generated file, but
> it shouldn't go stale.
>
I added it in the next revision.
>> --- /dev/null
>> +++ b/gas/config/obj-elf-attr.c
>> @@ -0,0 +1,239 @@
>> +/* Object attributes parsing.
>> + Copyright (C) 2025 Free Software Foundation, Inc.
>> +
>> + This file is part of GAS, the GNU Assembler.
>> +
>> + GAS is free software; you can redistribute it and/or modify
>> + it under the terms of the GNU General Public License as published by
>> + the Free Software Foundation; either version 3, or (at your option)
>> + any later version.
>> +
>> + GAS is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + GNU General Public License for more details.
>> +
>> + You should have received a copy of the GNU General Public License
>> + along with GAS; see the file COPYING. If not, write to the Free
>> + Software Foundation, 51 Franklin Street - Fifth Floor, Boston, MA
>> + 02110-1301, USA. */
>> +
>> +#include "obj-elf-attr.h"
>
> Why's this ahead of ...
>
>> +#ifdef TC_OBJ_ATTR
>
> ... this? All you need is e.g. as.h, aiui.
>
In the next revision, obj-elf-attr.h includes as.h so I should be enough
to include only obj-elf-attr.h in obj-elf-attr.c.
> Also, considering this #ifdef covers the entire file, I think a blank line would
> be pretty nice to have after it.
>
Fixed in the next revision.
>> +#include "safe-ctype.h"
>> +
>> +#define skip_whitespace(str) do { if (is_whitespace (*(str))) ++(str); } while (0)
>> +
>> +static inline bool
>> +skip_past_char (char ** str, char c)
>
> I think Richard already mentioned that star(s) like these ones aren't supposed
> to be followed by a blank.
>
This was code that I copy-pasted.
I fixed it in the next revision.
>> +{
>> + if (**str == c)
>> + {
>> + (*str)++;
>> + return true;
>> + }
>> + else
>> + return false;
>
> Personally I'd also prefer if "else" was omitted in situations like this one.
>
Fixed in the next revision.
>> +}
>> +#define skip_past_comma(str) skip_past_char (str, ',')
>> +
>> +/* A list of attributes that have been explicitly set by the assembly code.
>> + VENDOR is the vendor id, BASE is the tag shifted right by the number
>> + of bits in MASK, and bit N of MASK is set if tag BASE+N has been set. */
>> +typedef struct recorded_attribute_info_t {
>
> No _t here please.
>
Fixed in the next revision.
>> + struct recorded_attribute_info_t *next;
>> + obj_attr_vendor_t vendor;
>> + unsigned int base;
>> + unsigned long mask;
>> +} recorded_attribute_info_t;
>
> This one is where it indeed belongs.
>
>> +static recorded_attribute_info_t *recorded_attributes;
>> +
>> +static void
>> +oav1_attr_info_free (recorded_attribute_info_t *node)
>> +{
>> + recorded_attribute_info_t *next;
>> + while (node != NULL)
>> + {
>> + next = node->next;
>> + free (node);
>> + node = next;
>> + }
>> +}
>
> Not something you absolutely have to change here (I understand you only
> move code), but in general please limit the scope of helper variables
> like "next" here.
>
Yes, I agree. It is better to limit the scope to what is required.
Fixed in the next revision.
>> +void
>> +oav1_attr_info_init ()
>> +{
>> + /* Note: this "constructor" was added for symetry with oav1_attr_info_exit.
>> + recorded_attributes is a static variable which is automatically initialized
>> + to NULL. There is no need to initialize it another time except for a
>> + cosmetic reason. */
>> + recorded_attributes = NULL;
>> +}
>
> Hmm, the comment doesn't reflect what I gave as an explanation, making
> it more than just "cosmetic". Please either update the comment or at
> least drop the 2nd sentence.
>
I dropped the second sentence in the next revision.
>> --- /dev/null
>> +++ b/gas/config/obj-elf-attr.h
>> @@ -0,0 +1,36 @@
>> +/* Object attributes parsing.
>> + Copyright (C) 2025 Free Software Foundation, Inc.
>> +
>> + This file is part of GAS, the GNU Assembler.
>> +
>> + GAS is free software; you can redistribute it and/or modify
>> + it under the terms of the GNU General Public License as published by
>> + the Free Software Foundation; either version 3, or (at your option)
>> + any later version.
>> +
>> + GAS is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + GNU General Public License for more details.
>> +
>> + You should have received a copy of the GNU General Public License
>> + along with GAS; see the file COPYING. If not, write to the Free
>> + Software Foundation, 51 Franklin Street - Fifth Floor, Boston, MA
>> + 02110-1301, USA. */
>> +
>> +#ifndef _OBJ_ELF_ATTR_H
>> +#define _OBJ_ELF_ATTR_H
>> +
>> +#include "as.h"
>> +#include "bfd/elf-bfd.h"
>
> Same question here - while I see why you need as.h, why is the other one not
> ...
I removed bfd/elf-bfd.h as it is included by obj-elf, which is included
by as.h.
>
>> +#ifdef TC_OBJ_ATTR
>
> ... living here?
>
>> --- a/gas/config/tc-arm.h
>> +++ b/gas/config/tc-arm.h
>> @@ -20,6 +20,7 @@
>> Software Foundation, 51 Franklin Street - Fifth Floor, Boston, MA
>> 02110-1301, USA. */
>>
>> +#ifndef TC_ARM
>> #define TC_ARM 1
>
> Changes like this one (you have multiple) look unrelated. I'd prefer if such
> were omitted, especially from an already large patch; the only alternative
> is to actually mention in the description why such changes are done.
>
Moved to a separate patch series:
gas: add missing header guard in tc-<arch>.h files
>> @@ -377,3 +378,10 @@ extern bool arm_tc_equal_in_insn (int, char *);
>> #define TC_LARGEST_EXPONENT_IS_NORMAL(PRECISION) \
>> arm_is_largest_exponent_ok ((PRECISION))
>> int arm_is_largest_exponent_ok (int precision);
>> +
>> +#ifdef OBJ_ELF
>> +/* The target supports Object Attributes. */
>> +#define TC_OBJ_ATTR 1
>> +#endif
>
> As you're about to add v2, aren't these comments then going to need touching
> another time, to add the version indicator? Better do so here right away?
>
Yes, they are updated in gas: implement parsing of object attributes v2
Yes, I could directly write "The target supports Object Attributes v1."
but the define will also change to TC_OBJ_ATTR_V1, so I thought that it
was preferable to keep this comment neutral regarding the version of
object attributes.
>> --- a/gas/config/tc-mips.h
>> +++ b/gas/config/tc-mips.h
>> @@ -217,4 +217,9 @@ extern bfd_reloc_code_real_type mips_cfi_reloc_for_encoding (int encoding);
>> #define CONVERT_SYMBOLIC_ATTRIBUTE(name) mips_convert_symbolic_attribute (name)
>> extern int mips_convert_symbolic_attribute (const char *);
>>
>> +#ifdef OBJ_ELF
>> +/* The target supports Object Attributes. */
>> +#define TC_OBJ_ATTR 1
>> +#endif
>
> What about OBJ_MAYBE_ELF? The header uses it elsewhere. Did you firmly
> determine that's unnecessary? (If so, please mention such in the description.
> Also please be sure to Cc arch maintainers of arch files you touch.)
>
In my understanding, OBJ_MAYBE_ELF is not necessary because only ELF
files support object attributes on MIPS (the only merge I could find is
inside mips_elf_merge_obj_attributes).
I will Cc the MIPS maintainer to make sure that this is correct.
>> --- a/gas/configure.ac
>> +++ b/gas/configure.ac
>> @@ -454,6 +454,19 @@ changequote([,])dnl
>> ;;
>> esac
>>
>> + # Does the target support Object Attributes ?
>> + case ${cpu_type} in
>> + aarch64* | arc* | arm* | csky | m68k | mips* | msp430 | powerpc* \
>> + | riscv* | s390* | sparc* | tic6x)
>
> Why is aarch64 included here? You don't edit tc-aarch64.h. Aren't you
> transiently breaking that target anyway, by removing v1 support from it
> before adding v2 support?
>
Sorry, that's a miss when I moved the patch down into history. I missed
aarch64. Fixed in the next revision.
> Jan
More information about the Binutils
mailing list