[PATCH v1 02/14] Object Attributes v2: new abstractions for subsections and attributes
Matthieu Longo
matthieu.longo@arm.com
Thu Apr 24 16:34:22 GMT 2025
On 2025-03-28 10:57, Jan Beulich wrote:
> On 21.03.2025 18:14, Matthieu Longo wrote:
>> From: Richard Ball <richard.ball@arm.com>
>>
>> This patch lays the groundwork for the support of Object Attributes v2 (OAv2).
>> OAv2 is an enhancement of OAv1. They retain successful aspects of OAv1, define
>> the relationship between object attributes and existing GNU properties, separate
>> architectural requirements from software ABI requirements, and simplify the
>> format to make it easier for OAv2 consumers to parse, and skip subsections and
>> attributes. Interestingly, OAv2 have only one scope: the whole relocatable file
>> where they were specified. For the reason behind this choice, see [1], "Build
>> attributes at file scope only". This document also provides more insights into
>> the design rationale for OAv2.
>>
>> Even if OAv2 was designed primarily for AArch64, this implementation splits the
>> generic core logic from the backend-specific one, and aims at facilitating OAv2
>> adoption by others backend. This logic will apply for any subsequent OAv2 patch.
>
> While at the same time for a majority of targets this will all be dead code.
>
> While being unconvinced of the approach for now, I'll at least give some
> general comments (many / most applicable throughout the series then).
>
The idea is to make available OAv2 for others targets at a "low cost".
Ideally, the architecture should look like a "sandwich" composed of:
- a deserialization layer toward a generic abstraction covering OAv1,
OAv2(, and even GNU properties because it is basically the same thing).
- a generic middle end merging the generic attributes.
- a serialization layer toward the latest version of OA + GNU properties
as we cannot really switch to all-OA because of performance concerns in
the runtime linker. I am not 100% convinced by this performance concern
argument, but the long history with GNU properties tells us that they
are not going to go away soon.
This requires a middle-end abstraction agnostic of V1 or V2. It also
requires managing dependencies between tags (there are some craziness in
OAv1 for AArch32).
Unfortunately building this middle end is a lot of work, and I don't
have the allocated time for it.
I am interested in your thoughts about the above, and I can provide more
details if you wish.
>> --- a/bfd/elf-attrs.c
>> +++ b/bfd/elf-attrs.c
>> @@ -20,6 +20,7 @@
>>
>> #include "sysdep.h"
>> #include "bfd.h"
>> +#include "double-linked-list.h"
>> #include "libiberty.h"
>> #include "libbfd.h"
>> #include "elf-bfd.h"
>> @@ -476,6 +477,19 @@ gnu_obj_attrs_arg_type (unsigned int tag)
>> int
>> _bfd_elf_obj_attrs_arg_type (bfd *abfd, int vendor, unsigned int tag)
>> {
>> + if (get_elf_backend_data (abfd)->obj_attrs_version == 2)
>> + {
>> + BFD_ASSERT (elf_obj_attr_subsections (abfd).last_ != NULL);
>> + switch (elf_obj_attr_subsections (abfd).last_->encoding)
>> + {
>> + case ULEB128:
>> + return ATTR_TYPE_FLAG_INT_VAL;
>> + case NTBS:
>> + return ATTR_TYPE_FLAG_STR_VAL;
>> + }
>
> And everything else falls through to ...
>
I changed the implementation in the next revision.
_bfd_elf_obj_attrs_arg_type is never called by OAv2, so I kept the code
the same as it was for OAv1.
>> + }
>> +
>> + /* Version 1. */
>> switch (vendor)
>> {
>> case OBJ_ATTR_PROC:
>
> ... version 1 handling? Not very robust for a library.
>
>> @@ -818,6 +832,200 @@ _bfd_elf_merge_unknown_attribute_list (bfd *ibfd, bfd *obfd)
>> return result;
>> }
>>
>> +obj_attr_v2 *
>> +_bfd_elf_obj_attr_v2_init (unsigned int tag,
>> + union obj_attr_value_v2 vals)
>> +{
>> + obj_attr_v2 *attr = (obj_attr_v2*) malloc (sizeof (obj_attr_v2));
>
> Imo it's preferable to use sizeof(*attr) here (and similarly elsewhere), or
> else there's a small chance for things to be/go out of sync.
>
> Nit: *(s) in pointer types want to be separated by a blank from the base type.
> Yet then, here and ...
>
Fixed in the next revision. Sorry for the * with pointers, I reviewed
the code several times, and ran clang-format on my diffs. clang-format
is creating a lot of noise that I needed to revert so there might still
be some I missed.
>> + memset ((void *) attr, 0, sizeof (obj_attr_v2));
>
> ... here I question the need for casts. This is C, not C++.
>
Fixed in the next revision.
>> + attr->tag = tag;
>> + attr->vals = vals;
>> + return attr;
>> +}
>> +
>> +void
>> +_bfd_elf_obj_attr_v2_free (obj_attr_v2 * attr, obj_attr_encoding_v2 encoding)
>
> Nit (continued from above): The *(s) then don't want to be followed by blanks,
> though (as you have it most everywhere else).
>
Sorry for that. Fixed in the next revision.
>> +{
>> + if (encoding == NTBS && attr->vals.string_val != NULL)
>> + free ((void *) attr->vals.string_val);
>
> The cast here is questionable too - casting away const-ness is bad practice.
> Otoh the NULL check is unnecessary; free() deals with that just fine.
>
free() complains about attr->vals.string_val being const.
>> + free (attr);
>> +}
>> +
>> +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 == NTBS)
>> + vals.string_val =
>> + (other->vals.string_val != NULL)
>> + ? strdup (other->vals.string_val)
>> + : NULL;
>> + else
>> + vals.uint_val = other->vals.uint_val;
>> +
>> + obj_attr_v2 *copy =_bfd_elf_obj_attr_v2_init (other->tag, vals);
>> + return copy;
>
> What purpose does the local variable serve here?
>
There is a new line that will appear later in a following patch.
I will remove the variable from this patch.
>> +}
>> +
>> +#define SWAP(a, b) do { typeof(a) temp = a; a = b; b = temp; } while (0)
>> +
>> +void
>> +_bfd_elf_obj_attr_v2_swap (obj_attr_subsection_v2 *s,
>> + obj_attr_v2 *a1, obj_attr_v2 *a2)
>> +{
>> + SWAP (a1->next, a2->next);
>> + SWAP (a1->prev, a2->prev);
>> +
>> + if (s->first_ == a1)
>> + s->first_ = a2;
>> + else if (s->first_ == a2)
>> + s->first_ = a1;
>> +
>> + if (s->last_ == a1)
>> + s->last_ = a2;
>> + else if (s->last_ == a2)
>> + s->last_ = a1;
>> +}
>> +
>> +int
>> +_bfd_elf_obj_attr_v2_cmp (obj_attr_v2 *a1, obj_attr_v2 *a2)
>
> Both parameter can be pointer-to-const here?
>
Fixed in the next revision.
>> +{
>> + if (a1->tag < a2->tag)
>> + return -1;
>> + else if (a1->tag > a2->tag)
>> + return 1;
>> + else
>> + return 0;
>> +}
>
> Personally I dislike such unnecessary uses of "else", but I guess it's used
> this way in many other places.
>
>> +/* Find the object attribute matching the given tag (object attributes v2 only).
>> + Note: The parameter 'sorted' specifies whether the given list is sorted or
>> + not. If the list of attributes is sorted, the linear search stops as soon as
>> + it finds an attribute with a greater tag. */
>> +obj_attr_v2 *
>> +obj_attr_v2_find_by_tag (obj_attr_subsection_v2 *subsec,
>
> Pointer-to-const again? (I guess I won't further repeat this. Please make
> it a goal to use pointer-to-const wherever possible. This serves a
> documentation purpose and at the same time can help avoiding accidents.)
>
Fixed in the next revision.
>> + uint32_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;
>> +}
>> +
>> +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 */)
>> +
>> +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 (obj_attr_subsection_v2));
>> + memset ((void *) subsection, 0, sizeof (obj_attr_subsection_v2));
>> + subsection->name = name;
>> + subsection->scope = scope;
>> + subsection->optional = optional;
>> + subsection->encoding = encoding;
>> + return subsection;
>> +}
>> +
>> +void
>> +_bfd_elf_obj_attr_subsection_v2_free (obj_attr_subsection_v2 *subsec)
>> +{
>> + for (obj_attr_v2 *attr = subsec->first_; attr != NULL; attr = attr->next)
>> + _bfd_elf_obj_attr_v2_free (attr, subsec->encoding);
>> + free (subsec);
>> +}
>> +
>> +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_;
>> + 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;
>> +}
>> +
>> +void
>> +_bfd_elf_obj_attr_subsection_v2_swap (obj_attr_subsection_list *plist,
>> + obj_attr_subsection_v2 *s1,
>> + obj_attr_subsection_v2 *s2)
>> +{
>> + SWAP (s1->next, s2->next);
>> + SWAP (s1->prev, s2->prev);
>> +
>> + if (plist->first_ == s1)
>> + plist->first_ = s2;
>> + else if (plist->first_ == s2)
>> + plist->first_ = s1;
>> +
>> + if (plist->last_ == s1)
>> + plist->last_ = s2;
>> + else if (plist->last_ == s2)
>> + plist->last_ = s1;
>> +}
>
> Wouldn't this better be a generic list operation? The way you have it it's
> open-coding list internals, imo.
>
You are right.
Moved this to the patch that introduces the double linked list.
>> +int
>> +_bfd_elf_obj_attr_subsection_v2_cmp (obj_attr_subsection_v2 *s1,
>> + obj_attr_subsection_v2 *s2)
>> +{
>> + int res = strcmp (s1->name, s2->name);
>> + if (res != 0)
>> + return res;
>> +
>> + if (s1->optional < s2->optional)
>> + return -1;
>> + else if (s1->optional > s2->optional)
>> + return 1;
>> +
>> + if (s1->encoding < s2->encoding)
>> + return -1;
>> + if (s1->encoding > s2->encoding)
>> + return 1;
>> + else
>> + return 0;
>> +}
>
> Without any comment it feels unexpected that optionality would take higher
> precedence than encoding. (Comparing booleans like this is also a little
> unusual.)
>
Fixed in the next revision.
I added a comment.
>> --- /dev/null
>> +++ b/bfd/elf-attrs.h
>> @@ -0,0 +1,102 @@
>> +/* ELF attributes support (based on ARM EABI attributes).
>> + Copyright (C) 2025 Free Software Foundation, Inc.
>> + Written by Cygnus Support.
>> +
>> + 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>
>> +
>> +/* --------------------
>> + Object attributes v2
>> + -------------------- */
>> +
>> +typedef enum obj_attr_encoding_v2
>> +{
>> + ULEB128 = 0,
>> + NTBS = 1,
>> +} obj_attr_encoding_v2;
>
> Might be worth leaving 0 unused, such that explicit initialization is
> always needed (helping to spot where the encodings are used).
>
Fixed in the next revision.
> I further fear the enumerators are too generic in their names. Much
> like the types I think they want to gain disambiguating prefixes.
>
Fixed in the next revision.
>> +typedef union obj_attr_value_v2 {
>> + uint32_t uint_val;
>> + const char* string_val;
>> +} obj_attr_value_v2;
>> +
>> +typedef struct obj_attr_v2 {
>> + /* The name/tag of an attribute. */
>> + uint32_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;
>> +
>> +} obj_attr_v2;
>> +
>> +typedef enum obj_attr_subsection_scope_v2
>> +{
>> + SUBSEC_PUBLIC = 0,
>> + SUBSEC_PRIVATE = 1,
>> +} obj_attr_subsection_scope_v2;
>
> Same here then, even if maybe to a lesser degree.
>
Fixed in the next revision.
>> +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 ? */
>> + bool optional;
>> +
>> + /* The value encoding of attributes in this subsection. */
>> + obj_attr_encoding_v2 encoding;
>> +
>> + /* The next subsection in the list, or NULL. */
>> + struct obj_attr_subsection_v2 *next;
>> +
>> + /* The previous subsection in the list, or NULL. */
>> + struct obj_attr_subsection_v2 *prev;
>> +
>> + /* A pointer to the first node of the list. */
>> + struct obj_attr_v2* first_;
>> +
>> + /* A pointer to the last node of the list. */
>> + struct obj_attr_v2* last_;
>> +
>> + /* The size of the list. */
>> + uint32_t size;
>> +
>> +} obj_attr_subsection_v2;
>
> For LP64 hosts the layout could be improved by moving "size" up into the
> 32-bit hole that's there. That'll have no negative impact on ILP32.
>
Done.
>> +typedef struct obj_attr_subsection_list
>> +{
>> + /* A pointer to the first node of the list. */
>> + obj_attr_subsection_v2 *first_;
>> +
>> + /* A pointer to the last node of the list. */
>> + obj_attr_subsection_v2 *last_;
>> +
>> + /* The size of the list. */
>> + uint64_t size;
>
> Why a 64-bit size here when obj_attr_subsection_v2 has a 32-bit one?
>
That's not very consistent indeed. Changed to uint32_t.
>> --- a/bfd/elfnn-aarch64.c
>> +++ b/bfd/elfnn-aarch64.c
>> @@ -10760,8 +10760,21 @@ const struct elf_size_info elfNN_aarch64_size_info =
>> #define elf_backend_extern_protected_data 0
>> #define elf_backend_hash_symbol elf_aarch64_hash_symbol
>>
>> +/* elf_backend_obj_attrs_vendor is unused for AArch64. It is replaced by the
>> + concept of subsections, each one having its own name defined by the
>> + assembly directive "aeabi_subsection". */
>> +#undef elf_backend_obj_attrs_vendor
>> +#define elf_backend_obj_attrs_vendor "aeabi"
>> #undef elf_backend_obj_attrs_section
>> #define elf_backend_obj_attrs_section SEC_AARCH64_ATTRIBUTES
>> +/* elf_backend_obj_attrs_arg_type is unused as the type of an attribute is
>> + determined by looking up at the current subsection, not the value of the
>> + tag. */
>> +#define elf_backend_obj_attrs_arg_type NULL
>> +#undef elf_backend_obj_attrs_section_type
>> +#define elf_backend_obj_attrs_section_type SHT_AARCH64_ATTRIBUTES
>> +#undef elf_backend_obj_attrs_version
>> +#define elf_backend_obj_attrs_version 2
>
> The version is a per-object-file property. Together with using a pre-
> existing section type, how is linking together heterogeneous objects
> going to work?
Inputs objects with OAv1 or OAv2 (or a future OAvX) could in theory be
mixed, but the current specs of OAv1 and OAv2 use the same version
number 'A'. It means that mixing OAv1 and OAv2 is impossible because we
cannot differentiate the versions.
The current approach imposes the usage of the same version for all
objects of a backend. This was the simplest approach to mutualize the
serialization/deserialization code for OAv1 and OAv2, but with this
major drawback that you pointed out.
This is definitely not great. It makes impossible the deployment of a
future version in the same backend, or a smooth migration from OAv1 to
OAv2 for others backends (I mean here a transition across maybe 10+
years the time that the toolchains out there all support OAv2, and OAv1
can be retired).
I am in discussion with the author of the specs to see whether this
could be changed. I will come back to you to see whether this can be
changed.
>
> Jan
More information about the Binutils
mailing list