[PATCH v4 01/22] bfd: rename parsing methods of object attribute v1 API

[Jan Beulich]

On 03.07.2025 18:27, 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.
> - hiding the attribute tag type behind a typedef.
> - preparing the move of object attributes's parsing code to another
>   file.

Looks largely okay (a few remarks below), and ...

> Note: the name obj_attr_v1_process_attribute is exposed in the API.
> Ideally, the version should not be part of the name, and be hidden
> behind a macro. However, a later patch will unify the parsing of
> OAv1 and OAv2, and will make the use of such a macro obsolete.

... if this is indeed transient, then I expect this is fine.

> --- 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, obj_attr_tag_t tag)

Seeing these side by side I wonder why one has a _t suffix while the other
doesn't.

> @@ -1944,15 +1946,17 @@ typedef struct obj_attribute
>  typedef struct obj_attribute_list
>  {
>    struct obj_attribute_list *next;
> -  unsigned int tag;
> +  obj_attr_tag_t tag;
>    obj_attribute attr;
>  } 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,

I never understood why in cases like this one the enumerator values would
need specifying explicitly. The C standard is clear about how values are
assigned.

> --- a/gas/config/obj-elf.c
> +++ b/gas/config/obj-elf.c
> @@ -2061,23 +2061,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;

If already a typedef needs adding here (for whatever reason, and causing
extra churn), then I'd raise the same question about the absence of an _t
(or T, as also used elsewhere in gas) suffix here. Yes, there is a mess
(just see symbolS vs struct expressionS vs segT / subsegT), but I think
it would be nice if we didn't grow the mix, and instead for new typedef-s
made clear they're typedef-s just by their names.

> +static void
> +oav1_attr_info_free (recorded_attribute_info *node)
> +{
> +  recorded_attribute_info *next;
> +  while (node != NULL)
> +    {
> +      next = node->next;
> +      free (node);
> +      node = next;
> +    }
> +}
> +
> +static void
> +oav1_attr_info_init (void)
> +{
> +  recorded_attributes = NULL;

This wasn't there before. I have a vague guess what it's wanted for, but
technically static variables don't need any (re)clearing.

Jan