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

[Matthieu Longo]

On 2025-07-31 13:18, Jan Beulich wrote:
> On 15.07.2025 13:39, Matthieu Longo wrote:
>> @@ -834,8 +867,218 @@ _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 vals)
>> +{
>> +  obj_attr_v2 *attr = (obj_attr_v2 *) malloc (sizeof (*attr));
> 
> No need for a cast? Also, if you don't ...
> 
>> +  memset (attr, 0, sizeof (*attr));
> 
> ... check the return value before use, likely you mean xmalloc()? Or, as you
> want the array zeroed, e.g. XCNEW()?
> 

Replaced with XCNEW().

>> +  attr->tag = tag;
>> +  attr->vals = vals;
> 
> Why "vals", not "val"?
> 

It contains only one value even if it is an union, so "val" is better.
Changed in the next revision.

>> +  return attr;
>> +}
>> +
>> +/* 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->vals.string_val);
>> +  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 vals;
>> +  if (encoding == OA_ENC_NTBS)
>> +    vals.string_val
>> +      = (other->vals.string_val != NULL
>> +	 ? strdup (other->vals.string_val)
> 
> xstrdup()
> 

Fixed.

>> +	 : NULL);
>> +  else
>> +    vals.uint_val = other->vals.uint_val;
>> +
>> +  return _bfd_elf_obj_attr_v2_init (other->tag, vals);
>> +}
>> +
>> +/* 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;
>> +  else
>> +    return 0;
> 
> Please may I ask that unnecessary "else" be omitted (also elsewhere)?
> 

Fixed.

>> +}
>> +
>> +/* 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;
>> +  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 */)
>> +
>> +/* Create a new object attribute subsection with the following properties:
>> +   - NAME: the name of the subsection.
>> +   - 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).
>> +   Return a pointer to it.  */
>> +
>> +obj_attr_subsection_v2 *
>> +_bfd_elf_obj_attr_subsection_v2_init (const char *name,
>> +				      obj_attr_subsection_scope_v2 scope,
>> +				      bool optional,
>> +				      obj_attr_encoding_v2 encoding)
>> +{
>> +  obj_attr_subsection_v2 *subsection = (obj_attr_subsection_v2 *)
>> +    malloc (sizeof (*subsection));
>> +  memset (subsection, 0, sizeof (*subsection));
> 
> Same comments as for _bfd_elf_obj_attr_v2_init().
> 

Replaced by XCNEW().

>> +  subsection->name = name;
>> +  subsection->scope = scope;
>> +  subsection->optional = optional;
>> +  subsection->encoding = encoding;
>> +  return subsection;
>> +}
>> +
>> +/* 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 (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 (other->name, other->scope,
>> +					    other->optional, other->encoding);
>> +  for (obj_attr_v2* attr = other->first;
> 
> Nit: Misplaced *, also ...
> 

Fixed.
I used check_GNU_style.py inside gcc/contrib, and didn't find others 
issues regarding the pointers in the patch series.

>> +       attr != NULL;
>> +       attr = attr->next)
>> +    {
>> +      obj_attr_v2* new_attr = _bfd_elf_obj_attr_v2_copy (attr, other->encoding);
> 
> ... here.
> 

Fixed.

>> --- /dev/null
>> +++ b/bfd/elf-attrs.h
>> @@ -0,0 +1,118 @@
>> +/* 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,
>> +} obj_attr_version_t;
>> +
>> +/* --------------------
>> +   Object attributes v2
>> +   -------------------- */
>> +
>> +typedef enum obj_attr_encoding_v2
>> +{
>> +  OA_ENC_UNSET   = 0,
>> +  OA_ENC_ULEB128 = 1,
>> +  OA_ENC_NTBS    = 2,
>> +  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))
>> +
>> +typedef union obj_attr_value_v2 {
>> +  uint32_t uint_val;
>> +  const char* string_val;
> 
> Another misplaced *. Please check the entire series.
> 

Fixed.

>> +} obj_attr_value_v2;
>> +
>> +typedef uint32_t obj_attr_tag_t;
>> +
>> +typedef struct obj_attr_v2 {
>> +  /* The name/tag of an attribute.  */
>> +  obj_attr_tag_t tag;
>> +
>> +  /* The value assigned to an attribute, can be ULEB128 or NTBS.  */
>> +  union obj_attr_value_v2 vals;
>> +
>> +  /* The next attribute in the list or NULL.  */
>> +  struct obj_attr_v2 *next;
>> +
>> +  /* The previous attribute in the list or NULL.  */
>> +  struct obj_attr_v2 *prev;
>> +
> 
> Nit: Unnecessary / unwanted blank line. (Again below.)
> 

Fixed.

>> +} obj_attr_v2;
>> +
>> +typedef enum obj_attr_subsection_scope_v2
>> +{
>> +  OA_SUBSEC_PUBLIC = 0,
>> +  OA_SUBSEC_PRIVATE = 1,
> 
> Is the explicit specifying of values actually needed for something?
> Or is this reflecting some part of the specification, rather being an
> internal-only representation?
> 
>> +} obj_attr_subsection_scope_v2;
>> +
>> +typedef struct obj_attr_subsection_v2 {
>> +  /* The name of the subsection.  */
>> +  const char *name;
>> +
>> +  /* The scope of the subsection.  */
>> +  obj_attr_subsection_scope_v2 scope;
>> +
>> +  /* Is this subsection optional ? Can it be skipped ?  */
> 
> Like full stops, question marks don't really want/need separating by a
> blank.
> 

Fixed.

>> +  bool optional;
>> +
>> +  /* The value encoding of attributes in this subsection.  */
>> +  obj_attr_encoding_v2 encoding;
>> +
>> +  /* The size of the list.  */
>> +  uint32_t size;
> 
> Here and elsewhere I think it would be better to use "unsigned int".
> Fixed-width types are helpful to describe ABIs or file formats.

What about using size_t instead ?
The only issue I see with size_t is that it is platform dependent so the 
size can change. But is it an issue here ?

> 
> Jan