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

[Matthieu Longo]

On 2025-10-24 14:05, Jan Beulich wrote:
> On 01.09.2025 18:56, Matthieu Longo wrote:
>> @@ -492,6 +519,8 @@ _bfd_elf_obj_attrs_arg_type (bfd *abfd,
>>   			     obj_attr_vendor_t vendor,
>>   			     obj_attr_tag_t tag)
>>   {
>> +  BFD_ASSERT (elf_obj_attr_version (abfd) == OBJ_ATTR_V1);
>> +  /* This function should only be called for object attributes version 1.  */
> 
> Imo such a comment wants to come ahead of what is being commented (i.e. the
> BFD_ASSERT()).
> 

Fixed.

>> @@ -834,8 +867,215 @@ _bfd_elf_merge_unknown_attribute_list (bfd *ibfd, bfd *obfd)
>>     return result;
>>   }
>>   
>> -bool _bfd_elf_write_section_build_attributes (bfd *abfd,
>> -					      struct bfd_link_info *info ATTRIBUTE_UNUSED)
>> +/* Create a new object attribute with key TAG and value VALS.
>> +   Return a pointer to it.  */
>> +
>> +obj_attr_v2 *
>> +_bfd_elf_obj_attr_v2_init (obj_attr_tag_t tag,
>> +			   union obj_attr_value_v2 val)
>> +{
>> +  obj_attr_v2 *attr = XCNEW (obj_attr_v2);
>> +  attr->tag = tag;
>> +  attr->val = val;
>> +  return attr;
>> +}
> 
> At the example of this: Is this a BFD-internal function, or one to be called by
> e.g. gas or ld? In the latter case the leading underscore would want dropping
> from the name. I'm actually about to submit some cleanup patches towards some
> of the inconsistencies we have there. (All internal functions are then going
> to gain the hidden visibility attribute.)
> 

There is only one reference to this function outside BFD in gas to 
create an OAv2 and record it.
I dropped the underscore in the next revision.

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

...

void
_bfd_elf_obj_attr_v2_free (obj_attr_v2 *attr, obj_attr_encoding_v2 encoding)
{
   if (encoding == OA_ENC_NTBS)
     /* Note: this field never holds a string literal.  */
     free ((char *) attr->val.string_val);
   free (attr);
}

>> +  free (attr);
>> +}
>> +
>> +/* Copy an object attribute OTHER, and return a pointer to the copy.  */
>> +
>> +obj_attr_v2 *
>> +_bfd_elf_obj_attr_v2_copy (obj_attr_v2 *other,
>> +			   obj_attr_encoding_v2 encoding)
>> +{
>> +  union obj_attr_value_v2 val;
>> +  if (encoding == OA_ENC_NTBS)
>> +    val.string_val
>> +      = (other->val.string_val != NULL
>> +	 ? xstrdup (other->val.string_val)
>> +	 : NULL);
>> +  else
>> +    val.uint_val = other->val.uint_val;
> 
> Shouldn't this be "else if (...)" and then an else following which aborts or
> asserts?
> 

Yes, I agree. It makes the code easier to adapt in the future if we add 
a new type.

Fixed in the next revision.

>> +  return _bfd_elf_obj_attr_v2_init (other->tag, val);
>> +}
>> +
>> +/* 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.

>> +  return 0;
>> +}
>> +
>> +/* Return an object attribute in SUBSEC matching TAG or NULL if one is not
>> +   found.  SORTED specifies whether the given list is ordered by tag number.
>> +   This allows an early return if we find a higher numbered tag.  */
>> +
>> +obj_attr_v2 *
>> +obj_attr_v2_find_by_tag (const obj_attr_subsection_v2 *subsec,
>> +			 obj_attr_tag_t tag,
>> +			 bool sorted)
>> +{
>> +  for (obj_attr_v2 *attr = subsec->first;
>> +       attr != NULL;
>> +       attr = attr->next)
>> +    if (attr->tag == tag)
>> +      return attr;
>> +    else if (sorted && attr->tag > tag)
>> +      break;
> 
> Similarly here the "else" would imo better be omitted, and (irrespectively)
> braces added around the body of the loop.
> 

Fixed in the next revision.

>> +  return NULL;
>> +}
>> +
>> +/* 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.

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

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

>> +  for (obj_attr_v2 *attr = other->first;
>> +       attr != NULL;
>> +       attr = attr->next)
>> +    {
>> +      obj_attr_v2 *new_attr = _bfd_elf_obj_attr_v2_copy (attr, other->encoding);
>> +      LINKED_LIST_APPEND(obj_attr_v2) (new_subsec, new_attr);
>> +    }
>> +  return new_subsec;
>> +}
>> +
>> +/* Compare two object attributes based on all the attributes (scope is computed
>> +   from the name, so is not used for the comparison) of a subsection (total
>> +   ordering) , and return an integer indicating the result of the comparison,
> 
> Nit: Stray blank before comma.
> 

Fixed in the next revision.

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

>> +/* Return a subsection in the list FIRST matching NAME or NULL if one is not
>> +   found.
> 
> I think you want to have a comma after NAME, for it to not read "matching NAME
> or NULL".
> 

Fixed.

>> --- /dev/null
>> +++ b/bfd/elf-attrs.h
>> @@ -0,0 +1,116 @@
>> +/* ELF attributes support (based on ARM EABI attributes).
>> +   Copyright (C) 2025 Free Software Foundation, Inc.
>> +
>> +   This file is part of BFD, the Binary File Descriptor library.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program; if not, write to the Free Software
>> +   Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
>> +   MA 02110-1301, USA.  */
>> +
>> +#pragma once
>> +
>> +#include <stdint.h>
>> +
>> +typedef enum obj_attr_version_t {
>> +  OBJ_ATTR_VERSION_NONE = 0,
>> +  OBJ_ATTR_VERSION_UNSUPPORTED,
>> +  OBJ_ATTR_V1,
>> +  OBJ_ATTR_V2,
>> +  OBJ_ATTR_MAX_V = OBJ_ATTR_V2,
> 
> V is imo ambiguous when seen outside of the context here. And considering
> the first two odentifiers here, how about OBJ_ATTR_VERSION_MAX?
> 

Fixed in the next revision.

>> +} obj_attr_version_t;
>> +
>> +/* --------------------
>> +   Object attributes v2
>> +   -------------------- */
>> +
>> +typedef enum obj_attr_encoding_v2
>> +{
>> +  OA_ENC_UNSET   = 0,
>> +  OA_ENC_ULEB128,
>> +  OA_ENC_NTBS,
>> +  OA_ENC_MAX     = OA_ENC_NTBS,
>> +} obj_attr_encoding_v2;
>> +
>> +#define obj_attr_encoding_v2_from_u8(value) \
>> +  ((enum obj_attr_encoding_v2) (value + 1))
>> +#define obj_attr_encoding_v2_to_u8(value) \
>> +  ((uint8_t) (value - 1))
> 
> In both please also properly parenthesize "value".
> 

Fixed in the next revision.

>> +typedef union obj_attr_value_v2 {
>> +  uint32_t uint_val;
>> +  const char* string_val;
> 
> Didn't you indicate you would have correct all such misplaced *-s? (I would
> expect such to be done using e.g. grep or sed on the entire patch set.)
> 

I fixed the issue and ran this command.

$ git diff master..HEAD | grep -E "[a-zA-Z_][a-zA-Z0-9_]*\s+\*\s+[a-zA-Z_]+"

  _bfd_elf_parse_attributes (bfd *abfd, Elf_Internal_Shdr * hdr)
@@ -635,19 +2787,31 @@ _bfd_elf_parse_attributes (bfd *abfd, 
Elf_Internal_Shdr * hdr)
+extern obj_attr_subsection_v2 * obj_attr_subsection_v2_find_by_name
  extern elf_property_list * _bfd_elf_find_property
-  struct elf_aarch64_obj_tdata * tdata = elf_aarch64_tdata 
(info->output_bfd);
@@ -19857,12 +19858,334 @@ process_attributes (Filedata * filedata,
@@ -24126,6 +24449,10 @@ process_arch_specific (Filedata * filedata)
  extern struct fix * obj_elf_get_vtable_inherit (void);
  extern struct fix * obj_elf_get_vtable_entry (void);
  const char * elf32_csky_target_format (void);

None of those are from me, except:
+extern obj_attr_subsection_v2 * obj_attr_subsection_v2_find_by_name
All others functions have a space between the pointer and the function 
name, so I preserved it.

Sorry if I missed something last time.

> Also, do the _val suffixes serve any purpose in a struct having "value" in
> its name?
> 
> Jan

Yes, I can remove the suffix.
Fixed in the next revision.

Mattthieu