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

[Jan Beulich]

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

> @@ -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.)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Jan