[PATCH v9 03/19] Object Attributes v2: new abstractions for subsections and attributes

[Matthieu Longo]

On 2025-10-29 07:37, Jan Beulich wrote:
> On 28.10.2025 18:33, Matthieu Longo wrote:
>> On 2025-10-24 14:05, Jan Beulich wrote:
>>> On 01.09.2025 18:56, Matthieu Longo wrote:
>>>> +/* Free memory allocated by the object attribute ATTR.  */ + 
>>>> +void +_bfd_elf_obj_attr_v2_free (obj_attr_v2 *attr, 
>>>> obj_attr_encoding_v2 encoding) +{ +  if (encoding == 
>>>> OA_ENC_NTBS) +    free ((char *) attr->val.string_val);
>>> 
>>> Such imo strictly needs a comment (and likely one here and one 
>>> in the struct decl, next to the field). This, for example, makes 
>>> it impossible to put a string literal into the field. And of 
>>> course casting away const is generally bad practice (albeit the 
>>> price to pay for making such fields pointer-to-const).
>>> 
>> 
>> I added the following comments. Is it clear enough ?
>> 
>> 
>> typedef union obj_attr_value_v2 { uint32_t uint_val;
>> 
>> /* Note: this field cannot hold a string literal as the value 
>> needs to be freeable.  */ const char* string_val; } 
>> obj_attr_value_v2;
> 
> Would you mind adding "e.g." after "hold"? (Also once again note the 
> misplaced '*'.)
> 

Done.
Regarding the *, it was before I fix all of them in the patch. Sorry for
the confusion. Be assured that it is fixed in the next revision.

>>>> +/* Compare two object attributes based on their TAG value 
>>>> only (partial +   ordering), and return an integer indicating 
>>>> the result of the comparison, +   as follows: +   - 0, if A1 
>>>> and A2 are equal. +   - a negative value if A1 is less than 
>>>> A2. +   - a positive value if A1 is greater than A2.  */ + 
>>>> +int +_bfd_elf_obj_attr_v2_cmp (const obj_attr_v2 *a1, const 
>>>> obj_attr_v2 *a2) +{ +  if (a1->tag < a2->tag) +    return -1; 
>>>> +  else if (a1->tag > a2->tag) +    return 1;
>>> 
>>> You got rid of on "else" here, but not the other. (Same issue 
>>> apparently elsewhere.)
>>> 
>> 
>> Usually, I preserve all the if/else if/ else. This might be the 
>> result of a change you requested in a previous revision.
>> 
>> I usually don't adopt this style as it makes the code more 
>> difficult to read in my opinion, unless the nestedness reach a 
>> level of 2 or more, and the flattening make things more readable.
>> 
>> I will fix it, and the next ones in this patch for the next 
>> revision. However, please, could we not flatten the if/else if/ 
>> else blocks that I might have added elsewhere in the next patches 
>> if everything is correctly balanced.
> 
> I'm surprised people (you're not the only one) pretty much insist on 
> such "else", for (supposed) readability. In the main project I'm 
> working on, we are in the process of making the code Misra 
> compliant. Misra, among its many rules, has one which requires dead 
> code to be eliminated. I'm pretty sure at the first glance you agree 
> that dead code can be distracting / misleading. However, here's the 
> wording of the rule: "An operation that is executed but whose 
> removal would not affect program behaviour constitutes dead code." 
> Of course it very much depends on the definition of "operation" 
> (which isn't given), but the removal of such "else" definitely won't 
> affect program behavior.
> 
> In any event, my perspective is that such "else" actually hamper 
> readability.
> 

 From my personal perspective, when I see 'if ... else if ...', I can see
directly that the second 'if' is an alternative, whereas when I see a
flattened structure, it requires me to see that there is a return that
will short-cut all others subsequent 'if's.
In the second case, at a first glance without analyzing the condition of
the 'if's, I would assume by default that potentially, there is an
existing path by which all the conditions might be true, and so their
content would be executed.
In the first case, I can eliminate right away this possibility. The
semantic is different. The program behavior is not different, but the 
code semantic changed thus I would not consider 'else if' or 'else' as 
dead code.

I guess that, at the end, it is more a matter of how people parse the
code in their head, and not everyone does it in the same way.

>>>> +/* Sort the object attributes inside a subsection. +   Note: 
>>>> since a subsection is a list of attributes, the sorting 
>>>> algorithm is +   implemented with a merge sort. +   See more 
>>>> details in libiberty/doubly-linked-list.h  */ + 
>>>> +LINKED_LIST_MUTATIVE_OPS_DECL(obj_attr_subsection_v2, 
>>>> obj_attr_v2, /* public */) 
>>>> +LINKED_LIST_MERGE_SORT_DECL(obj_attr_subsection_v2, 
>>>> obj_attr_v2, /* public */)
>>> 
>>> What are the /* public */ comments about?
>> 
>> The third parameter is used to restrict the scope of the function. 
>> You can pass 'static' in a .c file and the symbols will be 
>> restricted to the compilation unit.
>> 
>> If the scope is not restricted, then it is public. I passed a 
>> comment /* public */ to make things explicit instead of nothing.
> 
> I see. To me "public" has a notion of C++. I'm not going to insist, 
> but if already you deem a comment desirable, might the more C-ish 
> "extern" be more suitable then?
> 

That's a good idea. the header declaration takes extern as parameter, so
it makes sense to pass /* extern */ in the .c file.

Replaced in the next revision.

>>>> +/* Free memory allocated by the object attribute subsection 
>>>> SUBSEC.  */ + +void +_bfd_elf_obj_attr_subsection_v2_free 
>>>> (obj_attr_subsection_v2 *subsec) +{ +  obj_attr_v2 *attr = 
>>>> subsec->first; +  while (attr != NULL) +    { + obj_attr_v2 *a
>>>> = attr; +      attr = attr->next; + _bfd_elf_obj_attr_v2_free
>>>> (a, subsec->encoding); +    } + free ((void *) subsec->name);
>>> 
>>> See the related comment further up. (What you cast to may also 
>>> want to be consistent.)
>>> 
>>> If you free unconditionally, then ...
>>> 
>> 
>> I am not sure what you mean here. free() does not accept 'const' 
>> pointer, so I need to cast away the 'const'.
>> 
>> ../../bfd/elf-attrs.c: In function 
>> ‘_bfd_elf_obj_attr_subsection_v2_free’: ../../bfd/elf- 
>> attrs.c:993:15: error: passing argument 1 of ‘free’ discards 
>> ‘const’ qualifier from pointer target type [-Werror=discarded- 
>> qualifiers] 993 |   free (subsec->name); |         ~~~~~~^~~~~~ In 
>> file included from ../../bfd/sysdep.h:36, from ../../bfd/elf- 
>> attrs.c:21: /usr/include/stdlib.h:687:25: note: expected ‘void *’ 
>> but argument is of type ‘const char *’ 687 | extern void free 
>> (void *__ptr) __THROW; |                   ~~~~~~^~~~~
>> 
>> Am I misunderstanding something ?
> 
> Yes. The comment isn't about "const" at all, but about the free() 
> call being unconditional. You imply that an allocation has happened 
> (or the pointer is still NULL), yet the counterpart function didn't 
> allocate anything. Like with the other comment further up, someone 
> could again have put a string literal there, or the address of 
> another variable.
> 

Ok, I added the following comments.

diff --git a/bfd/elf-attrs.c b/bfd/elf-attrs.c
index cb04ac8ed9e..e09d2c5c1da 100644
--- a/bfd/elf-attrs.c
+++ b/bfd/elf-attrs.c
@@ -958,7 +958,8 @@ 
LINKED_LIST_MUTATIVE_OPS_DECL(obj_attr_subsection_v2, obj_attr_v2, /* 
extern */)
  LINKED_LIST_MERGE_SORT_DECL(obj_attr_subsection_v2, obj_attr_v2, /* 
extern */)

  /* Create a new object attribute subsection with the following properties:
-   - NAME: the name of the subsection.
+   - NAME: the name of the subsection.  Note: this parameter never holds a
+     string literal, so the value has to be freeable.
     - SCOPE: the scope of the subsection (public or private).
     - OPTIONAL: whether this subsection is optional (true) or required 
(false).
     - ENCODING: the expected encoding for the attributes values 
(ULEB128 or NTBS).
@@ -990,6 +991,7 @@ _bfd_elf_obj_attr_subsection_v2_free 
(obj_attr_subsection_v2 *subsec)
        attr = attr->next;
        _bfd_elf_obj_attr_v2_free (a, subsec->encoding);
      }
+  /* Note: this field never holds a string literal.  */
    free ((char *) subsec->name);
    free (subsec);
  }
diff --git a/bfd/elf-attrs.h b/bfd/elf-attrs.h
index 17a3233f069..eab7b69db02 100644
--- a/bfd/elf-attrs.h
+++ b/bfd/elf-attrs.h
@@ -78,7 +78,9 @@ typedef enum obj_attr_subsection_scope_v2
  } obj_attr_subsection_scope_v2;

  typedef struct obj_attr_subsection_v2 {
-  /* The name of the subsection.  */
+  /* The name of the subsection.
+     Note: this field cannot hold e.g. a string literal as the value 
has to be
+     freeable.  */
    const char *name;

    /* The scope of the subsection.  */

>>>> +  free (subsec); +} + +/* Deep copy an object attribute 
>>>> subsection OTHER, and return a pointer to the +   copy.  */ + 
>>>> +obj_attr_subsection_v2 * 
>>>> +_bfd_elf_obj_attr_subsection_v2_copy (obj_attr_subsection_v2 
>>>> const *other) +{ +  obj_attr_subsection_v2 *new_subsec +    = 
>>>> _bfd_elf_obj_attr_subsection_v2_init (xstrdup (other->name), 
>>>> other->scope, +					    other->optional, other->encoding);
>>> 
>>> ... imo the allocation wants to happen in 
>>> _bfd_elf_obj_attr_subsection_v2_init(), not at the call sites.
>>> 
>> 
>> The issue of moving xstrdup() inside 
>> _bfd_elf_obj_attr_subsection_v2_init() is that I cannot "move" the 
>> value anymore, and a copy will occur every time whereas it 
>> sometimes can be avoided.
>> 
>> There is an example of it in obj_attr_v2_subsection_record() where 
>> the ownership of 'name' can just be transferred to 
>> _bfd_elf_obj_attr_subsection_v2_init without requiring any copy.
>> 
>> obj_attr_subsection_v2 *new_subsection = 
>> _bfd_elf_obj_attr_subsection_v2_init (name, scope, 
>> comprehension_optional, encoding);
> 
> Such "transfer of ownership" imo needs to be made explicit by way of 
> commentary then, I think.
> 

I also added those comments to the call sites in the corresponding 
subsequent patches.

diff --git a/bfd/elf-attrs.c b/bfd/elf-attrs.c
index d4a8fe94538..4f713979bdb 100644
--- a/bfd/elf-attrs.c
+++ b/bfd/elf-attrs.c
@@ -2634,6 +2634,9 @@ oav2_parse_subsection (bfd *abfd,
         at the end of the string, so no risk of buffer overrun.
       2. the data for comprehension and encoding can also safely be 
read.  */
    {
+    /* Note: read_ntbs() assigns a dynamically allocated string to
+       subsection_name.  Either the string has to be freed in case of 
errors,
+       or its ownership must be transferred.  */
      int read = read_ntbs (abfd, cursor, cursor + subsection_name_len + 1,
                           &subsection_name);
      total_read += read;
@@ -2680,6 +2683,8 @@ oav2_parse_subsection (bfd *abfd,
           && !gnu_testing_namespace (subsection_name)))
      scope = OA_SUBSEC_PUBLIC;

+  /* Note: ownership of 'subsection_name' is transfered to the callee when
+     initializing the subsection.  That is why we skip free() at the 
end.  */
    *subsec = _bfd_elf_obj_attr_subsection_v2_init
      (subsection_name, scope, comprehension_raw, value_encoding);

diff --git a/gas/config/obj-elf-attr.c b/gas/config/obj-elf-attr.c
index 37cc4838d49..e386ee4ac33 100644
--- a/gas/config/obj-elf-attr.c
+++ b/gas/config/obj-elf-attr.c
@@ -1084,6 +1084,8 @@ obj_attr_v2_subsection_record (const char *name,
               && strncmp (name + 3, "-testing", 8) != 0))
         scope = OA_SUBSEC_PUBLIC;

+      /* Note: ownership of 'name' is transfered to the callee when 
initializing
+        the subsection.  That is why we skip free() at the end.  */
        obj_attr_subsection_v2 *new_subsection
         = _bfd_elf_obj_attr_subsection_v2_init (name, scope,
                                                 comprehension_optional,
@@ -1166,10 +1168,11 @@ obj_attr_process_subsection ()
    if (args == NULL)
      return;

-  /* move the value to avoid double free.  */
+  /* Note: move the value to avoid double free.  */
    const char *name = args[0].val.string;
    args[0].val.string = NULL;

+  /* Note: ownership of 'name' is transferred to the callee.  */
    obj_attr_v2_subsection_record (name, &args[1], &args[2]);
    args_list_free (args, N_ARGS);
  }

>>>> +int +_bfd_elf_obj_attr_subsection_v2_cmp (const 
>>>> obj_attr_subsection_v2 *s1, +				     const 
>>>> obj_attr_subsection_v2 *s2) +{ +  int res = strcmp (s1->name, 
>>>> s2->name); +  if (res != 0) +    return res; + +  /* Giving to 
>>>> the optionality a higher priority than the encoding is + 
>>>> artificial.  Its only purpose is to give a total ordering to
>>>> a +     collection of subsections.  */ +  if (!s1->optional && 
>>>> s2->optional) +    return -1; +  else if (s1->optional && !s2-
>>>>> optional) +    return 1; + +  if (s1->encoding < s2- 
>>>>> encoding) +    return -1; +  else if (s1->encoding > s2- 
>>>>> encoding) +    return 1; + +  return 0; +}
>>> 
>>> I can't bring comment (ahead of the function) and code in line 
>>> with one another: You're - not comparing attributes, but 
>>> attribute sub-sections,
>> 
>> It seems to me that I copy-pasted the description for somewhere 
>> else, but messed up the rewriting. This is a mistake of mine. 
>> Sorry for the confusion.
>> 
>> The function compares subsections based on their properties, not 
>> their content (i.e. the list of attributes), the goal being to 
>> obtain a total ordering in a collection of subsections. Another 
>> comparison operator is used to sort the attributes inside a 
>> subsection: _bfd_elf_obj_attr_v2_cmp.
>> 
>>> - not comparing all attributes of the sub-section, - how 
>>> "encoding" and "optional" sort seems entirely arbitrary, i.e. I 
>>> cannot make sense of "less" or "greater" there (numeric values 
>>> could easily be flipped around as long as these are only 
>>> internal representations).
>> 
>> Indeed the values themselves don't really matter. The core idea is 
>> to provide a comparison operator with the required properties so 
>> that there is a total order after the sorting.
>> 
>> When the linker loads the OAs of two different object files, the 
>> easiest way to merge them is to have the collection sorted, 
>> otherwise the merge algorithm would be more complicated and should 
>> be based on a hashing mechanism.
>> 
>>> Thinking about it, the first two points make me wonder whether 
>>> "attribute" here isn't the same as what the entire series is 
>>> about. In which case it may help to disambiguate things.
>>> 
>> 
>> Here is the fixed description. Please let me know if it is clear 
>> enough.
>> 
>> /* Compare two object attribute subsections based on all their 
>> properties. This operator can be used to obtain a total order in a 
>> collection. Return an integer indicating the result of the 
>> comparison, as follows: - 0, if S1 and S2 are equal. - a negative 
>> value if S1 is less than S2. - a positive value if S1 is greater 
>> than S2.
>> 
>> NB: the scope is computed from the name, so is not used for the 
>> comparison.  */
>> 
>> int _bfd_elf_obj_attr_subsection_v2_cmp (const 
>> obj_attr_subsection_v2 *s1, const obj_attr_subsection_v2 *s2) { 
>> int res = strcmp (s1->name, s2->name); if (res != 0) return res;
>> 
>> /* Note: the comparison of the encoding and optionality of 
>> subsections is completely arbitrary.  Numeric values could 
>> completely being flipped around, it would not matter.  Also, 
>> giving to the optionality a higher priority than the encoding is 
>> artificial.  The searched properties for this comparison operator 
>> are reflexivity, transitivity, antisymmetry, and totality in order 
>> to achieve a total ordering after the sorting of a collection of 
>> subsections.  */
>> 
>> if (!s1->optional && s2->optional) return -1; else if (s1-
>>> optional && !s2->optional) return 1;
>> 
>> if (s1->encoding < s2->encoding) return -1; else if (s1->encoding
>>> s2->encoding) return 1;
>> 
>> return 0; }
> 
> This is much better, yes. However, the arbitrary nature of the 
> ordering (i.e. what "less" and "greater" really mean) still concerns 
> me. If this is arbitrary, it could be altered going forward. If it 
> was altered, would cross operation (old gas + new ld or vice versa) 
> still function correctly, seeing that you say "the easiest way to 
> merge them is to have the collection sorted"? My implication from 
> this is that linker and assembler will need to agree on the sorting 
> criteria used. But perhaps there's some misunderstanding on my part.
> 
> Jan

The merge always sorts the subsections and attributes, and never assumes 
that the object already contains sorted OAs.

See in the description of patch 12/19:

* Phase 1, map: successive per-file operations applied on the list of
   compatible input objects.
   ...
   3. Sorting of the subsections and object attributes. Further
      operations rely on the ordering to perform some optimization in
      the processing of the data.

This is the function containing the sorting:

/* Merge duplicated subsections and object attributes inside a same object
    file.  After a call to this function, the subsections and object 
attributes
    are sorted.  */
static bool
oav2_file_scope_merge_subsections (bfd *abfd)

If the ordering nature were to be altered going forward, it won't be an 
issue as it is internal to ld, and ld only requires a sorted collection 
with a total order. The ordering nature does not really matter.

Matthieu