[PATCH v1 02/14] Object Attributes v2: new abstractions for subsections and attributes
Jan Beulich
jbeulich@suse.com
Fri Mar 28 10:57:59 GMT 2025
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).
> --- 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 ...
> + }
> +
> + /* 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 ...
> + memset ((void *) attr, 0, sizeof (obj_attr_v2));
... here I question the need for casts. This is C, not C++.
> + 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).
> +{
> + 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 (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?
> +}
> +
> +#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?
> +{
> + 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.)
> + 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.
> +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.)
> --- /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).
I further fear the enumerators are too generic in their names. Much
like the types I think they want to gain disambiguating prefixes.
> +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.
> +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.
> +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?
> --- 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?
Jan
More information about the Binutils
mailing list