Home Original page

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

Jan Beulich jbeulich@suse.com
Wed Oct 29 07:37:14 GMT 2025 
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
'*'.)

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

>>> +/* 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?

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

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

>>> +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
More information about the Binutils mailing list