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

[Jan Beulich]

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()?

> +  attr->tag = tag;
> +  attr->vals = vals;

Why "vals", not "val"?

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

> +	 : 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)?

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

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

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

... here.

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

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

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

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

Jan