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

[Jan Beulich]

On 04.07.2025 15:42, Matthieu Longo wrote:
> On 2025-07-04 10:02, Jan Beulich wrote:
>> On 03.07.2025 18:27, Matthieu Longo wrote:
>>> @@ -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 ?

Because one can expect people writing C code to know how C is defined?
At least as far as not overly complex aspects go?

> Why not making it explicit ?

One certainly can, but as you say - it's unnecessarily verbose.

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

Hmm, so you added this purely for cosmetic reasons. That's not what I had
expected; in such an event this could (and imo should) indeed be omitted.
But: Fuzzers may take shortcuts when running successive tests against the
same binary. Alan has been adding such seeming unnecessary initialization
in other places. Therefore I think you want to keep this, but perhaps add
a brief comment.

Jan