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

[Matthieu Longo]

On 2025-07-04 10:02, Jan Beulich wrote:
> 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.
> 

This function disappears in the next commit titled: "gas: use common 
code for object attribute v1 & v2 parsing".

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

Fixed in the next revision.

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

Fixed in the next revision.

However, you can notice that a few lines below, Tag_* values are 
explicitly assigned a value. This is not consistent through the code.

Also explicitly setting values is a bit verbose, I agree, but does not 
hurt. After all, why would someone need to know that the C standard 
guarantees says that the first value in an enum is zero by default ? Why 
not making it explicit ?

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

Fixed in the next revision.

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

Indeed they don't need any clearing. However, it is not clear to me what 
your expectations are. Do you want me to delete this ?

Deleting this means that I can delete oav1_attr_info_init () completely, 
but unfortunately, that I also lose the symmetry with 
oav1_attr_info_free() in elf_end(). This bothers me a bit, because while 
reading the code, if I see a free() in elf_end(), I would also naturally 
look for a constructor somewhere in elf_begin(), even if it is only a 
setting to NULL of a pointer, and here it would not be here.

Please let me know what you want to do here.

Matthieu