[PATCH v3 06/26] bfd: rename parsing methods of object attribute v1 API
Richard Earnshaw (lists)
Richard.Earnshaw@arm.com
Tue Jun 17 12:44:59 GMT 2025
On 09/05/2025 16:12, Matthieu Longo wrote:
> This patch is a preparation for the introduction of object attributes
> v2. It aims at:
> - making clear what methods are used to parse OAv1
> - adding more constaints on parameters type by using enums instead of
> defines.
> - preparing the move of object attributes's parsing code to another
> file.
> ---
> bfd/elf-attrs.c | 4 +--
> bfd/elf-bfd.h | 10 +++++---
> bfd/elf32-arc.c | 2 +-
> bfd/elf32-arm.c | 2 +-
> bfd/elf32-csky.c | 2 +-
> bfd/elf32-msp430.c | 2 +-
> bfd/elf32-tic6x.c | 2 +-
> bfd/elfnn-riscv.c | 2 +-
> gas/config/obj-elf.c | 59 +++++++++++++++++++++++++++++--------------
> gas/config/obj-elf.h | 7 ++---
> gas/config/tc-arc.c | 2 +-
> gas/config/tc-arm.c | 4 +--
> gas/config/tc-m68k.c | 2 +-
> gas/config/tc-mips.c | 2 +-
> gas/config/tc-ppc.c | 2 +-
> gas/config/tc-riscv.c | 2 +-
> gas/config/tc-tic6x.c | 2 +-
> 17 files changed, 66 insertions(+), 42 deletions(-)
I can't approve any of this, except for the tc-arm.c changes; but those are so trivial that once the rest of this is approved those should be pretty automatic.
So these are just my general comments on this change...
>
> diff --git a/bfd/elf-attrs.c b/bfd/elf-attrs.c
> index b4a57f4803c..0f9b2c4d1b5 100644
> --- a/bfd/elf-attrs.c
> +++ b/bfd/elf-attrs.c
> @@ -236,7 +236,7 @@ bfd_elf_set_obj_attr_contents (bfd *abfd, bfd_byte *buffer, bfd_vma size)
>
> /* Allocate/find an object attribute. */
> static obj_attribute *
> -elf_new_obj_attr (bfd *abfd, int vendor, unsigned int tag)
> +elf_new_obj_attr (bfd *abfd, obj_attr_vendor vendor, unsigned int tag)
Here you use "unsigned int" for a tag, but...
> {
> obj_attribute *attr;
> obj_attribute_list *list;
> @@ -474,7 +474,7 @@ gnu_obj_attrs_arg_type (unsigned int tag)
>
> /* Determine what arguments an attribute tag takes. */
> int
> -_bfd_elf_obj_attrs_arg_type (bfd *abfd, int vendor, unsigned int tag)
> +_bfd_elf_obj_attrs_arg_type (bfd *abfd, obj_attr_vendor vendor, uint32_t tag)
... here you use uint32_t. There's no guarantee these are the same type.
In fact, I think I'd suggest a new type entirely for this, perhaps something like obj_attr_tag_t.
> {
> switch (vendor)
> {
> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> index 5903d857faa..d528db07a0c 100644
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -1632,7 +1632,7 @@ struct elf_backend_data
>
> /* Return 1, 2 or 3 to indicate what type of arguments a
> processor-specific tag takes. */
> - int (*obj_attrs_arg_type) (int);
> + int (*obj_attrs_arg_type) (uint32_t);
>
> /* The section type to use for an attributes section. */
> unsigned int obj_attrs_section_type;
> @@ -1951,8 +1951,10 @@ typedef struct obj_attribute_list
> /* Object attributes may either be defined by the processor ABI, index
> OBJ_ATTR_PROC in the *_obj_attributes arrays, or be GNU-specific
> (and possibly also processor-specific), index OBJ_ATTR_GNU. */
> -#define OBJ_ATTR_PROC 0
> -#define OBJ_ATTR_GNU 1
> +typedef enum {
> + OBJ_ATTR_PROC = 0,
> + OBJ_ATTR_GNU = 1,
> +} obj_attr_vendor;
> #define OBJ_ATTR_FIRST OBJ_ATTR_PROC
> #define OBJ_ATTR_LAST OBJ_ATTR_GNU
>
> @@ -3074,7 +3076,7 @@ extern bool _bfd_elf_write_section_build_attributes
> (bfd *, struct bfd_link_info *);
> extern char *_bfd_elf_attr_strdup (bfd *, const char *);
> extern void _bfd_elf_copy_obj_attributes (bfd *, bfd *);
> -extern int _bfd_elf_obj_attrs_arg_type (bfd *, int, unsigned int);
> +extern int _bfd_elf_obj_attrs_arg_type (bfd *, obj_attr_vendor, uint32_t);
> extern void _bfd_elf_parse_attributes (bfd *, Elf_Internal_Shdr *);
> extern bool _bfd_elf_merge_object_attributes
> (bfd *, struct bfd_link_info *);
> diff --git a/bfd/elf32-arc.c b/bfd/elf32-arc.c
> index 0a6e66569ba..882f1ea2147 100644
> --- a/bfd/elf32-arc.c
> +++ b/bfd/elf32-arc.c
> @@ -2899,7 +2899,7 @@ elf32_arc_grok_prstatus (bfd *abfd, Elf_Internal_Note *note)
> string or both. */
>
> static int
> -elf32_arc_obj_attrs_arg_type (int tag)
> +elf32_arc_obj_attrs_arg_type (uint32_t tag)
> {
> if (tag == Tag_ARC_CPU_name
> || tag == Tag_ARC_ISA_config
> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> index 5a87462cb26..12b0748a45a 100644
> --- a/bfd/elf32-arm.c
> +++ b/bfd/elf32-arm.c
> @@ -14089,7 +14089,7 @@ enum
> string or both. */
>
> static int
> -elf32_arm_obj_attrs_arg_type (int tag)
> +elf32_arm_obj_attrs_arg_type (uint32_t tag)
> {
> if (tag == Tag_compatibility)
> return ATTR_TYPE_FLAG_INT_VAL | ATTR_TYPE_FLAG_STR_VAL;
> diff --git a/bfd/elf32-csky.c b/bfd/elf32-csky.c
> index 47a63895b55..6946c07a577 100644
> --- a/bfd/elf32-csky.c
> +++ b/bfd/elf32-csky.c
> @@ -5255,7 +5255,7 @@ csky_elf_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
> string or both. */
>
> static int
> -elf32_csky_obj_attrs_arg_type (int tag)
> +elf32_csky_obj_attrs_arg_type (uint32_t tag)
> {
> switch (tag)
> {
> diff --git a/bfd/elf32-msp430.c b/bfd/elf32-msp430.c
> index 77099655aba..1b7204304df 100644
> --- a/bfd/elf32-msp430.c
> +++ b/bfd/elf32-msp430.c
> @@ -2679,7 +2679,7 @@ elf32_msp430_obj_attrs_handle_unknown (bfd *abfd, int tag)
> string or both. */
>
> static int
> -elf32_msp430_obj_attrs_arg_type (int tag)
> +elf32_msp430_obj_attrs_arg_type (uint32_t tag)
> {
> if (tag == Tag_compatibility)
> return ATTR_TYPE_FLAG_INT_VAL | ATTR_TYPE_FLAG_STR_VAL;
> diff --git a/bfd/elf32-tic6x.c b/bfd/elf32-tic6x.c
> index 0adab1acc07..5ee05fdd8cd 100644
> --- a/bfd/elf32-tic6x.c
> +++ b/bfd/elf32-tic6x.c
> @@ -3462,7 +3462,7 @@ elf32_tic6x_plt_sym_val (bfd_vma i, const asection *plt,
> }
>
> static int
> -elf32_tic6x_obj_attrs_arg_type (int tag)
> +elf32_tic6x_obj_attrs_arg_type (uint32_t tag)
> {
> if (tag == Tag_ABI_compatibility)
> return ATTR_TYPE_FLAG_INT_VAL | ATTR_TYPE_FLAG_STR_VAL;
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 1c494f5f986..1975bfc56a3 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -5640,7 +5640,7 @@ riscv_elf_object_p (bfd *abfd)
> string or both. */
>
> static int
> -riscv_elf_obj_attrs_arg_type (int tag)
> +riscv_elf_obj_attrs_arg_type (uint32_t tag)
> {
> return (tag & 1) != 0 ? ATTR_TYPE_FLAG_STR_VAL : ATTR_TYPE_FLAG_INT_VAL;
> }
> diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
> index bc981f05dd5..b738c2dbb62 100644
> --- a/gas/config/obj-elf.c
> +++ b/gas/config/obj-elf.c
> @@ -2074,23 +2074,47 @@ skip_past_char (char ** str, char c)
> /* 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. */
> -struct recorded_attribute_info {
> +typedef struct recorded_attribute_info {
> struct recorded_attribute_info *next;
> - int vendor;
> + obj_attr_vendor vendor;
> unsigned int base;
> unsigned long mask;
> -};
> -static struct recorded_attribute_info *recorded_attributes;
> +} recorded_attribute_info;
> +static recorded_attribute_info *recorded_attributes;
> +
> +static void
> +obj_attr_v1_rai_free (recorded_attribute_info *node)
Since this is static function, I think
oav1_attr_info_free()
might be a better name. But why do we need both this and ...rai_exit below? Are there instances where we need to free up something other than 'recorded_attributes'? I certainly can't see any at this point.
> +{
> + recorded_attribute_info *next;
> + while (node != NULL)
> + {
> + next = node->next;
> + free (node);
> + node = next;
> + }
> +}
> +
> +static void
> +obj_attr_v1_rai_enter (void)
oav1_attr_info_init()?
> +{
> + recorded_attributes = NULL;
> +}
> +
> +static void
> +obj_attr_v1_rai_exit (void)
> +{
> + obj_attr_v1_rai_free (recorded_attributes);
> +}
>
> /* Record that we have seen an explicit specification of attribute TAG
> for vendor VENDOR. */
>
> static void
> -record_attribute (int vendor, unsigned int tag)
> +obj_attr_v1_record_seen (obj_attr_vendor vendor, uint32_t tag)
> {
> unsigned int base;
> unsigned long mask;
> - struct recorded_attribute_info *rai;
> + recorded_attribute_info *rai;
>
> base = tag / (8 * sizeof (rai->mask));
> mask = 1UL << (tag % (8 * sizeof (rai->mask)));
> @@ -2101,7 +2125,7 @@ record_attribute (int vendor, unsigned int tag)
> return;
> }
>
> - rai = XNEW (struct recorded_attribute_info);
> + rai = XNEW (recorded_attribute_info);
> rai->next = recorded_attributes;
> rai->vendor = vendor;
> rai->base = base;
> @@ -2113,11 +2137,11 @@ record_attribute (int vendor, unsigned int tag)
> for vendor VENDOR. */
>
> bool
> -obj_elf_seen_attribute (int vendor, unsigned int tag)
> +obj_attr_v1_rai_seen (obj_attr_vendor vendor, uint32_t tag)
> {
> unsigned int base;
> unsigned long mask;
> - struct recorded_attribute_info *rai;
> + recorded_attribute_info *rai;
>
> base = tag / (8 * sizeof (rai->mask));
> mask = 1UL << (tag % (8 * sizeof (rai->mask)));
> @@ -2130,8 +2154,8 @@ obj_elf_seen_attribute (int vendor, unsigned int tag)
> /* Parse an attribute directive for VENDOR.
> Returns the attribute number read, or zero on error. */
>
> -int
> -obj_elf_vendor_attribute (int vendor)
> +uint32_t
> +obj_attr_v1_process_attribute (obj_attr_vendor vendor)
> {
> expressionS exp;
> int type;
> @@ -2209,7 +2233,7 @@ obj_elf_vendor_attribute (int vendor)
> s = demand_copy_C_string (&len);
> }
>
> - record_attribute (vendor, tag);
> + obj_attr_v1_record_seen (vendor, tag);
> bool ok = false;
> switch (type & 3)
> {
> @@ -2246,7 +2270,7 @@ obj_elf_vendor_attribute (int vendor)
> static void
> obj_elf_gnu_attribute (int ignored ATTRIBUTE_UNUSED)
> {
> - obj_elf_vendor_attribute (OBJ_ATTR_GNU);
> + obj_attr_v1_process_attribute (OBJ_ATTR_GNU);
> }
>
> void
> @@ -3195,6 +3219,8 @@ elf_begin (void)
> previous_subsection = 0;
> comment_section = NULL;
> memset (&groups, 0, sizeof (groups));
> +
> + obj_attr_v1_rai_enter ();
> }
>
> void
> @@ -3206,17 +3232,12 @@ elf_end (void)
> section_stack = top->next;
> free (top);
> }
> - while (recorded_attributes)
> - {
> - struct recorded_attribute_info *rai = recorded_attributes;
> - recorded_attributes = rai->next;
> - free (rai);
> - }
> if (groups.indexes)
> {
> htab_delete (groups.indexes);
> free (groups.head);
> }
> + obj_attr_v1_rai_exit ();
> }
>
> #ifdef USE_EMULATIONS
> diff --git a/gas/config/obj-elf.h b/gas/config/obj-elf.h
> index c8b57406685..87f4dc6d3cf 100644
> --- a/gas/config/obj-elf.h
> +++ b/gas/config/obj-elf.h
> @@ -204,9 +204,10 @@ extern void obj_elf_vtable_inherit (int);
> extern void obj_elf_vtable_entry (int);
> extern struct fix * obj_elf_get_vtable_inherit (void);
> extern struct fix * obj_elf_get_vtable_entry (void);
> -extern bool obj_elf_seen_attribute
> - (int, unsigned int);
> -extern int obj_elf_vendor_attribute (int);
> +
> +/* Object attributes v1. */
> +extern bool obj_attr_v1_rai_seen (obj_attr_vendor, uint32_t);
> +extern uint32_t obj_attr_v1_process_attribute (obj_attr_vendor);
The convention seems to be, though I'm not sure if it's universally held, that symbols exported from obj-elf.c begin with obj_elf_. I'd suggest better names for these functions would be
obj_elf_oav1_seen_attribute
and
obj_elf_oav1_vendor_attribute
To match more closely the existing names.
But see also the comment below. I think we want to try to keep the callers of these functions mostly unaware of any changes in the implementation; but that could possibly be handled, as elsewhere in gas, by just defining one function in terms of another:
#define obj_elf_seen_attribute obj_elf_oav1_seen_attribute
Note that "rai" in an exported API seems particularly confusing. A reader might legitimately ask what's an rai and why do I need to call that?
>
> /* BFD wants to write the udata field, which is a no-no for the
> predefined section symbols in bfd/section.c. They are read-only. */
> diff --git a/gas/config/tc-arc.c b/gas/config/tc-arc.c
> index 30d6cb9c50d..d12924818ea 100644
> --- a/gas/config/tc-arc.c
> +++ b/gas/config/tc-arc.c
> @@ -4928,7 +4928,7 @@ arc_extcorereg (int opertype)
> static void
> arc_attribute (int ignored ATTRIBUTE_UNUSED)
> {
> - int tag = obj_elf_vendor_attribute (OBJ_ATTR_PROC);
> + uint32_t tag = obj_attr_v1_process_attribute (OBJ_ATTR_PROC);
>From the context of this call here, the function name seems wrong. We shouldn't need to be calling some specific version, just processing some attribute. If you want to change the symbol name internally, perhaps you should arrange for the existing name to be mapped to something newer via some bindings in obj-elf.h.
>
> if (tag < NUM_KNOWN_OBJ_ATTRIBUTES)
> attributes_set_explicitly[tag] = true;
> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
> index 1d401d17b6b..f881e538a67 100644
> --- a/gas/config/tc-arm.c
> +++ b/gas/config/tc-arm.c
> @@ -4974,9 +4974,9 @@ s_arm_unwind_raw (int ignored ATTRIBUTE_UNUSED)
> static void
> s_arm_eabi_attribute (int ignored ATTRIBUTE_UNUSED)
> {
> - int tag = obj_elf_vendor_attribute (OBJ_ATTR_PROC);
> + uint32_t tag = obj_attr_v1_process_attribute (OBJ_ATTR_PROC);
Similarly here.
>
> - if (tag >= 0 && tag < NUM_KNOWN_OBJ_ATTRIBUTES)
> + if (tag < NUM_KNOWN_OBJ_ATTRIBUTES)
> attributes_set_explicitly[tag] = 1;
> }
>
> diff --git a/gas/config/tc-m68k.c b/gas/config/tc-m68k.c
> index 46b26d4f38a..8914bbc6456 100644
> --- a/gas/config/tc-m68k.c
> +++ b/gas/config/tc-m68k.c
> @@ -7917,7 +7917,7 @@ m68k_elf_cons (int nbytes /* 4=.long */)
> static void
> m68k_elf_gnu_attribute (int ignored ATTRIBUTE_UNUSED)
> {
> - int tag = obj_elf_vendor_attribute (OBJ_ATTR_GNU);
> + uint32_t tag = obj_attr_v1_process_attribute (OBJ_ATTR_GNU);
and here.
>
> /* Check validity of defined m68k tags. */
> if (tag == Tag_GNU_M68K_ABI_FP)
> diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c
> index eeb34ef9971..b8d1b3a4534 100644
> --- a/gas/config/tc-mips.c
> +++ b/gas/config/tc-mips.c
> @@ -20596,7 +20596,7 @@ mips_md_finish (void)
> file_mips_check_options ();
>
> /* Set a floating-point ABI if the user did not. */
> - if (obj_elf_seen_attribute (OBJ_ATTR_GNU, Tag_GNU_MIPS_ABI_FP))
> + if (obj_attr_v1_rai_seen (OBJ_ATTR_GNU, Tag_GNU_MIPS_ABI_FP))
and here.
> {
> /* Perform consistency checks on the floating-point ABI. */
> fpabi = bfd_elf_get_obj_attr_int (stdoutput, OBJ_ATTR_GNU,
> diff --git a/gas/config/tc-ppc.c b/gas/config/tc-ppc.c
> index 0de27140d6a..9dc00b81264 100644
> --- a/gas/config/tc-ppc.c
> +++ b/gas/config/tc-ppc.c
> @@ -2577,7 +2577,7 @@ ppc_elf_abiversion (int ignore ATTRIBUTE_UNUSED)
> static void
> ppc_elf_gnu_attribute (int ignored ATTRIBUTE_UNUSED)
> {
> - int tag = obj_elf_vendor_attribute (OBJ_ATTR_GNU);
> + uint32_t tag = obj_attr_v1_process_attribute (OBJ_ATTR_GNU);
>
and here.
> /* Check validity of defined powerpc tags. */
> if (tag == Tag_GNU_Power_ABI_FP
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index ca4030d6a93..1eb03878216 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -5789,7 +5789,7 @@ riscv_convert_symbolic_attribute (const char *name)
> static void
> s_riscv_attribute (int ignored ATTRIBUTE_UNUSED)
> {
> - int tag = obj_elf_vendor_attribute (OBJ_ATTR_PROC);
> + uint32_t tag = obj_attr_v1_process_attribute (OBJ_ATTR_PROC);
> unsigned old_xlen;
> obj_attribute *attr;
>
> diff --git a/gas/config/tc-tic6x.c b/gas/config/tc-tic6x.c
> index aeb22395b05..719e7d6ff96 100644
> --- a/gas/config/tc-tic6x.c
> +++ b/gas/config/tc-tic6x.c
> @@ -689,7 +689,7 @@ static bool tic6x_attributes_set_explicitly[NUM_KNOWN_OBJ_ATTRIBUTES];
> static void
> s_tic6x_c6xabi_attribute (int ignored ATTRIBUTE_UNUSED)
> {
> - int tag = obj_elf_vendor_attribute (OBJ_ATTR_PROC);
> + uint32_t tag = obj_attr_v1_process_attribute (OBJ_ATTR_PROC);
and here.
>
> if (tag < NUM_KNOWN_OBJ_ATTRIBUTES)
> tic6x_attributes_set_explicitly[tag] = true;
More information about the Binutils
mailing list