[PATCH v8 04/19] gas: implement parsing of object attributes v2
Jan Beulich
jbeulich@suse.com
Thu Jul 31 14:40:19 GMT 2025
On 15.07.2025 13:39, Matthieu Longo wrote:
> From: Richard Ball <richard.ball@arm.com>
>
> This patch adds the parsing logic for Object Attributes v2 (OAv2), enabling
> Gas to interpret and process these attributes correctly. It also updates the
> AArch64 backend to utilize the new parsing capabilities, and handle the new
> AArch64-specific directives.
>
> This patch relies on the abstractions introduced in the previous patch to
> store the data. Its scope is limited to parsing the new assembly directives,
> checking the inputs, and storing the data into the relevant OAv2 abstractions.
> Note that, for now, the new parsing capabilities are only available for AArch64.
> Even if the implementation was splitted into a generic part available in
> gas/config/obj-elf.c, and an AArch64-specific one in gas/config/tc-aarch64.c,
> the lack of GNU generic directives to handle OAv2 prevented the capability
> from being exposed to others backends.
>
> ** GNU assembler interface for aeabi subsections
>
> OAv2 introduced two new directives for AArch64:
> - .aeabi_subsection name, comprehension, encoding
> Create or switch the current subsection to 'name'.
> Comprehension values can be 'required' or 'optional'.
> Encoding values are limited for now to 'ULEB128', and 'NTBS'
So within a subsection there can be only attributes of same "comprehension"
and same encoding?
> - .aeabi_attribute tag, value
> Set 'tag' to 'value' in the current subsection.
> Tag can either be an integer, or one of the defined symbols in the backend.
>
> The usage of those directives will error if the following requirements
> are breached:
> - If the subsection X has been previously declared, the comprehension and
> encoding parameters of the current .aeabi_subsection that redeclares X
> have to match with the previous declaration.
If I'm reading things right, such a re-declaration still needs to provide
not only the name, but also "comprehension" and encoding. Just for it to
be checked against the earlier decl. As with ordinary sections, I think
it would be nice if an "incomplete" re-decl would be accepted.
> ---
> bfd/elf-attrs.c | 146 +++++-
> bfd/elf-attrs.h | 77 ++++
> bfd/elf-bfd.h | 7 +
> bfd/elfnn-aarch64.c | 6 +
> bfd/elfxx-aarch64.c | 73 +++
> bfd/elfxx-aarch64.h | 2 +
> bfd/elfxx-target.h | 8 +
> gas/config/obj-elf-attr.c | 949 ++++++++++++++++++++++++++++++++++++++
> gas/config/obj-elf-attr.h | 24 +
> gas/config/obj-elf.c | 17 +-
> gas/config/tc-aarch64.c | 16 +
> gas/config/tc-aarch64.h | 5 +
> gas/config/tc-arc.h | 4 +-
> gas/config/tc-arm.h | 6 +-
> gas/config/tc-csky.h | 4 +-
> gas/config/tc-m68k.h | 4 +-
> gas/config/tc-mips.h | 4 +-
> gas/config/tc-msp430.h | 4 +-
> gas/config/tc-ppc.h | 6 +-
> gas/config/tc-riscv.h | 4 +-
> gas/config/tc-s390.h | 4 +-
> gas/config/tc-sparc.h | 4 +-
> gas/config/tc-tic6x.h | 4 +-
> gas/configure | 2 +-
> gas/configure.ac | 2 +-
> gas/doc/c-aarch64.texi | 27 ++
> include/elf/aarch64.h | 13 +
> 27 files changed, 1393 insertions(+), 29 deletions(-)
Overall I have to say that I find it extremely hard to review patches of this
size. Which is also why it has taken my quite long to actually take a look
again. I can't promise I'll ever convince myself enough of the (apparent)
correctness that I would eventually give an "okay".
> --- a/bfd/elf-attrs.c
> +++ b/bfd/elf-attrs.c
> @@ -255,8 +255,152 @@ bfd_elf_set_obj_attr_contents (bfd *abfd, bfd_byte *buffer, bfd_vma size)
> write_obj_attr_section_v1 (abfd, buffer, size);
> }
>
> +/* The first two tags in gnu-testing namespace are known, and so have a name and
> + can be initialized to the default value ('0' or NULL) depending on the
> + encoding specified on the subsection. Any tags above 1 will be considered
> + unknown, so will be default initialized in the same way but its status will
> + be set to obj_attr_subsection_v2_unknown. */
> +static const obj_attr_info_t known_tags_gnu_testing[] =
> +{
> + {
> + .tag = {"GNUTestTag_0", .value = {
> + .val.u32 = 0,
> + .vtype = VALUE_U32
> + }},
> + .default_value = {.val.u64 = 0, .vtype = VALUE_UNSIGNED_INTEGER},
> + .encoding = OA_ENC_ULEB128,
> + },
> + {
> + .tag = {"GNUTestTag_1", .value = {
> + .val.u32 = 1,
> + .vtype = VALUE_U32
> + }},
> + .default_value = {.val.u64 = 0, .vtype = VALUE_UNSIGNED_INTEGER},
> + .encoding = OA_ENC_ULEB128,
> + },
> +};
I went to look at the doc referenced by [2] in the cover letter, but I couldn't
find anything there about the known-ness of these two tags.
> +/* List of known GNU subsections.
> + Note: this list needs to be sorted. */
> +static known_subsection_v2 obj_attr_v2_known_gnu_subsections[] =
const?
> +{
> + {
> + /* Note: the currently set values for the subsection name, its optionality,
> + and encoding are irrelevant for a testing subsection. These values are
> + unused. This entry is only a placeholder for list of known GNU testing
> + tags. */
> + .subsec_name = NULL,
> + .known_tags = known_tags_gnu_testing,
> + .optional = true,
> + .encoding = OA_ENC_ULEB128,
> + .len = sizeof (known_tags_gnu_testing) / sizeof (obj_attr_info_t),
Please use ARRAY_SIZE(), and even if you didn't please avoid sizeof() on a
type when really you mean the type of some specific variable.
> + },
> + /* Note for the future: GNU subsections can be added here below. */
> +};
> +
> +/* Return True if the given subsection name is part of the reserved "gnu-testing"
> + namespace. */
> +static bool
> +gnu_testing_namespace (const char *subsec_name)
> +{
> + return strncmp ("gnu-testing", subsec_name, 11) == 0;
> +}
Coming back to Richard's comment (on v5) here: Is it really the case that
any arbitrary continuation (i.e. possibly not something that is a separator
following immediately) is acceptable?
> +/* Search for the attribute information associated to TAG in the list of known
> + tags registered in the known subsection SUBSEC. Return the tag information
> + if it is found, NULL otherwise. */
> +static const obj_attr_info_t *
> +identify_tag (const known_subsection_v2 *subsec, obj_attr_tag_t tag)
> +{
> + for (unsigned i = 0; i < subsec->len; ++i)
> + {
> + const obj_attr_info_t *known_tag = &subsec->known_tags[i];
> + if (known_tag->tag.value.val.u32 == tag)
> + return known_tag;
> + else if (known_tag->tag.value.val.u32 > tag)
> + break;
> + }
> + return NULL;
> +}
> +
> +/* Return the attribute information associated to the pair SUBSEC, TAG if it
> + exists, NULL otherwise. */
> +const obj_attr_info_t *
> +known_obj_attr_v2_find_by_tag (const struct elf_backend_data *be,
I think all non-static functions would better consistently start with
obj_attr_v2_. Hence here maybe obj_attr_v2_find_known_by_tag()? (I only
later noticed that identify_subsection() also is non-static, so the
comment applies there, too.)
> + const char *subsec_name,
> + obj_attr_tag_t tag)
> +{
> + const known_subsection_v2 *subsec_info
> + = identify_subsection (be, subsec_name);
> + if (subsec_info != NULL)
> + {
> + const obj_attr_info_t *tag_info = identify_tag (subsec_info, tag);
> + return tag_info;
No need for such a local variable?
> + }
> + return NULL;
> +}
> +
> +/* To-string function for the pair <SUBSEC, TAG>. Returns the identifier
> + associated to TAG if it is found, NULL otherwise. */
> +const char *
> +obj_attr_v2_tag_to_string (const struct elf_backend_data *be,
Is "string" a good name when an identifier is meant, and not an NTBS value?
I'm also confused by "identifier associated to TAG". I can't find (in this
patch) where such an association would be established, and I hence can't
close to appearant hole in my understanding of what this is about.
> + const char *subsec_name,
> + obj_attr_tag_t tag)
> +{
> + const obj_attr_info_t *tag_info
> + = known_obj_attr_v2_find_by_tag (be, subsec_name, tag);
> + if (tag_info != NULL)
> + return tag_info->tag.identifier;
> + return NULL;
> +}
> +
> +/* To-string function for the subsection parameter "comprehension". */
> +const char *
> +oav2_comprehension_to_string (bool comprehension)
> +{
> + return comprehension ? "optional" : "required";
> +}
Considering the sole use of this function, I wonder whether these strings
should be translatable. After all, comprehension= and encoding= there are
translatable, too.
> --- a/bfd/elf-attrs.h
> +++ b/bfd/elf-attrs.h
> @@ -47,6 +47,9 @@ typedef enum obj_attr_encoding_v2
> #define obj_attr_encoding_v2_to_u8(value) \
> ((uint8_t) (value - 1))
>
> +extern const char *
> +oav2_encoding_to_string (obj_attr_encoding_v2);
> +
> typedef union obj_attr_value_v2 {
> uint32_t uint_val;
> const char* string_val;
> @@ -105,6 +108,9 @@ typedef struct obj_attr_subsection_v2 {
>
> } obj_attr_subsection_v2;
>
> +extern const char *
> +oav2_comprehension_to_string (bool);
> +
> typedef struct obj_attr_subsection_list
> {
> /* A pointer to the first node of the list. */
> @@ -116,3 +122,74 @@ typedef struct obj_attr_subsection_list
> /* The size of the list. */
> uint32_t size;
> } obj_attr_subsection_list;
> +
> +/* Basic implementation of a variant for the possible types associated to an
> + object attribute. */
> +struct gas_variant_t;
This isn't really needed, is it? Also, is gas_ a good prefix to use here?
We're in bfd source, which is intended to be tool-neutral (to a certain
degree). Unless of course "gas" here stands for something else than "GNU
assembler".
> +typedef struct {
> + size_t len;
> + struct gas_variant_t *elts;
> +} gas_variant_list;
> +
> +typedef union {
> + const char *string;
> + uint8_t u8;
> + uint32_t u32;
> + uint64_t u64;
> + int64_t i64;
> + bool b;
> + gas_variant_list list;
> +} gas_variant_value;
I'm not convinced having such an isolated union is a good idea, when with
just it in hand one can't determine which field is valid to use.
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -1657,6 +1657,12 @@ struct elf_backend_data
> /* Encode the object attributes version into the output object. */
> uint8_t (*obj_attrs_version_enc) (obj_attr_version_t);
>
> + /* The known subsections and attributes (v2 only). */
> + const known_subsection_v2 *obj_attr_v2_known_subsections;
> +
> + /* The size of the array of known subsections. */
> + const size_t obj_attr_v2_known_subsections_size;
> +
> /* This function determines the order in which any attributes are
> written. It must be defined for input in the range
> LEAST_KNOWN_OBJ_ATTRIBUTE..NUM_KNOWN_OBJ_ATTRIBUTES-1 (this range
> @@ -3091,6 +3097,7 @@ extern obj_attr_version_t _bfd_obj_attrs_version_dec (uint8_t);
> extern uint8_t _bfd_obj_attrs_version_enc (obj_attr_version_t);
> extern bfd_vma bfd_elf_obj_attr_size (bfd *);
> extern void bfd_elf_set_obj_attr_contents (bfd *, bfd_byte *, bfd_vma);
> +extern obj_attribute * elf_new_obj_attr (bfd *, obj_attr_vendor_t, obj_attr_tag_t);
Nit: Excess blank after *.
> --- a/bfd/elfnn-aarch64.c
> +++ b/bfd/elfnn-aarch64.c
> @@ -10800,6 +10800,12 @@ const struct elf_size_info elfNN_aarch64_size_info =
> #undef elf_backend_obj_attrs_version_enc
> #define elf_backend_obj_attrs_version_enc \
> _bfd_aarch64_obj_attrs_version_enc
> +/* Object attributes v2 specific values. */
> +#undef elf_backend_obj_attr_v2_known_subsections
> +#define elf_backend_obj_attr_v2_known_subsections \
> + aarch64_obj_attr_v2_known_subsections
> +#undef elf_backend_obj_attr_v2_known_subsections_size
> +#define elf_backend_obj_attr_v2_known_subsections_size 2
Something will want adding to make sure the build fails if someone brings
this hard coded number and ARRAY_SIZE(aarch64_obj_attr_v2_known_subsections)
out of sync.
> --- a/bfd/elfxx-aarch64.c
> +++ b/bfd/elfxx-aarch64.c
> @@ -21,6 +21,7 @@
> #include "sysdep.h"
> #include "bfd.h"
> #include "elf-bfd.h"
> +#include "elf/aarch64.h"
> #include "elfxx-aarch64.h"
> #include "libbfd.h"
> #include <stdarg.h>
> @@ -887,6 +888,78 @@ _bfd_aarch64_obj_attrs_version_enc (obj_attr_version_t version)
> abort ();
> }
>
> +/* Note: this array has to be sorted. */
> +static const obj_attr_info_t known_tags_aeabi_feature_and_bits[] =
> +{
> + {
> + .tag = {"Tag_Feature_BTI", .value = {
> + .val.u32 = Tag_Feature_BTI,
> + .vtype = VALUE_U32
> + }},
> + .default_value = {.val.u32 = 0, .vtype = VALUE_UNSIGNED_INTEGER},
> + .encoding = OA_ENC_ULEB128,
> + },
> + {
> + .tag = {"Tag_Feature_PAC", .value = {
> + .val.u32 = Tag_Feature_PAC,
> + .vtype = VALUE_U32
> + }},
> + .default_value = {.val.u32 = 0, .vtype = VALUE_UNSIGNED_INTEGER},
> + .encoding = OA_ENC_ULEB128,
> + },
> + {
> + .tag = {"Tag_Feature_GCS", .value = {
> + .val.u32 = Tag_Feature_GCS,
> + .vtype = VALUE_U32
> + }},
> + .default_value = {.val.u32 = 0, .vtype = VALUE_UNSIGNED_INTEGER},
> + .encoding = OA_ENC_ULEB128,
> + },
> +};
> +
> +/* This is a required subsection to use PAuthABI (which is currently
> + unsupported by GCC).
How does gcc matter here?
> A value of 0 for any the tags below means that
> + the user did not permit this entity to use the PAuthABI.
> + Note: this array has to be sorted. */
> +static const obj_attr_info_t known_tags_aeabi_pauthabi[] =
> +{
> + {
> + .tag = {"Tag_PAuth_Platform", .value = {
> + .val.u32 = Tag_PAuth_Platform,
> + .vtype = VALUE_U32
> + }},
> + .default_value = {.val.u32 = 0, .vtype = VALUE_UNSIGNED_INTEGER},
> + .encoding = OA_ENC_ULEB128,
> + },
> + {
> + .tag = {"Tag_PAuth_Schema", .value = {
> + .val.u32 = Tag_PAuth_Schema,
> + .vtype = VALUE_U32
> + }},
> + .default_value = {.val.u32 = 0, .vtype = VALUE_UNSIGNED_INTEGER},
> + .encoding = OA_ENC_ULEB128,
> + },
> +};
> +
> +/* Note: this array is exported by the backend, and needs to be sorted. */
> +const known_subsection_v2 aarch64_obj_attr_v2_known_subsections[] =
> +{
> + {
> + .subsec_name = "aeabi_feature_and_bits",
> + .known_tags = known_tags_aeabi_feature_and_bits,
> + .optional = true,
> + .encoding = OA_ENC_ULEB128,
> + .len = sizeof (known_tags_aeabi_feature_and_bits) / sizeof (obj_attr_info_t),
ARRAY_SIZE() again please here and ...
> + },
> + {
> + .subsec_name = "aeabi_pauthabi",
> + .known_tags = known_tags_aeabi_pauthabi,
> + .optional = false,
> + .encoding = OA_ENC_ULEB128,
> + .len = sizeof (known_tags_aeabi_pauthabi) / sizeof (obj_attr_info_t),
... here.
> +/* Extract an integer literal from the input.
> + Anything matched by O_constant is considered an integer literal (see the
> + usage of O_constant in expr.c to see all the matches. */
But an expression the value of is known only at the end of assembly is
deliberately not permitted?
> +static bool
> +extract_integer_literal (arg_t *arg_out,
> + bool signedness_unsigned)
Maybe shorter as "want_unsigned"?
> +{
> + expressionS exp;
> + expression (&exp);
As you want to use the value right away, don't you mean expression_and_evaluate()?
> + if (exp.X_op != O_constant)
> + {
> + as_bad (_("invalid value, expected an integer literal"));
> + goto bad;
> + }
> +
> + int64_t val = exp.X_add_number;
Any reason not to use offsetT here (and wherever else applicable)?
> + if (val < 0 && signedness_unsigned)
> + {
> + as_bad (_("invalid negative value %ld, expected an unsigned integer"),
You can't use %ld to print int64_t.
> + val);
> + goto bad;
> + }
Why not have this check ...
> + if (signedness_unsigned)
> + {
... in here? And is the check actually appropriate? The use specifying
0xffffffffffffffff will appear negative here, but it actually is a
valid unsigned value. Please consider looking at X_unsigned, even if
there's a warning about it in the struct decl. You care only about
O_constant here, after all.
> + arg_out->val.u64 = val;
> + arg_out->vtype = VALUE_UNSIGNED_INTEGER;
> + }
> + else
> + {
> + arg_out->val.i64 = val;
> + arg_out->vtype = VALUE_SIGNED_INTEGER;
> + }
> + return true;
> +
> +bad:
> + ignore_rest_of_line ();
> + return false;
> +}
> +
> +/* Extract an identifier based on the provided character matcher. */
> +static bool
> +extract_identifier (bool (*char_predicate) (char), arg_t *arg_out)
> +{
> + const char *s = input_line_pointer;
> + unsigned int i = 0;
> + for (; char_predicate (*input_line_pointer); ++input_line_pointer)
> + i++;
As indicated before, I'm not convinced of this open-coded parsing.
> + if (i == 0)
> + {
> + as_bad (_("invalid value '%c', expected an identifier"),
> + *input_line_pointer);
> + ignore_rest_of_line ();
> + return false;
> + }
> +
> + char *val = xmemdup0 (s, i);
> + arg_out->vtype = VALUE_STRING;
> + arg_out->val.string = val;
Again a variable that isn't really needed, while making reading (even if only
very slightly) harder.
> + return true;
> +}
> +
> +#if (TC_OBJ_ATTR_v2)
> +/* Resolve the identifier if it matches the given symbol. */
> +static bool
> +resolve_if_matching (const char *identifier,
> + const gas_symbol_t *symbol,
> + arg_t *val_out)
> +{
> + if (strcmp (symbol->identifier, identifier) != 0)
> + return false;
> +
> + /* Free the identifier since we found the value. */
> + free ((void *) val_out->val.string);
> +
> + switch (symbol->value.vtype)
> + {
> + case VALUE_BOOL:
> + val_out->val.u64 = symbol->value.val.b;
> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
> + break;
> + case VALUE_U8:
> + val_out->val.u64 = symbol->value.val.u8;
> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
> + break;
> + case VALUE_U32:
> + val_out->val.u64 = symbol->value.val.u32;
> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
> + break;
> + case VALUE_U64:
> + val_out->val.u64 = symbol->value.val.u64;
> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
> + break;
> + case VALUE_I64:
> + val_out->val.i64 = symbol->value.val.i64;
> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
> + break;
> + case VALUE_STRING:
> + val_out->val.string = strdup (symbol->value.val.string);
xstrdup()
> +/* Look up the symbol table of this compilation unit, and try to resolve the
> + given identifier. */
> +static bool
> +lookup_symbol_table (const char *identifier,
> + const arg_token_t expected_ttype,
> + arg_t *val_out)
> +{
> + if (identifier == NULL)
> + return false;
> +
> + /* Note: signed integer are unsupported for now. */
> + gas_assert (expected_ttype & UNSIGNED_INTEGER);
> + /* The identifier should match the value in val_out. */
> + gas_assert (val_out->val.string == identifier);
> +
> + symbolS *symbolP = symbol_find (identifier);
> + if (symbolP == NULL)
> + return false;
> +
> + if (! S_IS_DEFINED (symbolP))
> + return false;
> +
> + valueT val = S_GET_VALUE (symbolP);
> +
> + /* Free the identifier since we found the value. */
> + free ((void *) val_out->val.string);
> +
> + val_out->val.u64 = val;
> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
Unlike further up here you silently convert a signed value to an unsigned one.
> +/* Return true if the next characters are suspected to represent an integer
> + literal. */
> +static bool
> +look_like_integer_literal (char const* const cursor)
> +{
> + return ISDIGIT (*cursor)
> + || ((*cursor == '+' || *cursor == '-') && ISDIGIT (*(cursor + 1)));
> +}
Please allow for whitespace between unary operator and number. Also, what
about unary ~?
> +/* Parse an argument, and set its type accordingly depending on the input
> + value, and the constraints on the expected argument. */
> +static bool
> +obj_attr_parse_arg (arg_token_t expected_ttype,
> + bool (*match_identifier) (char c),
> + bool resolve_identifier,
> + arg_t *arg_out)
> +{
> + const arg_token_t low_ttype = (expected_ttype & LT_MASK);
> +
> + /* Note: symbol look-up for string literals is not available. */
> + if (((low_ttype & STRING) && *input_line_pointer == '"')
> + || !(low_ttype & ~STRING))
> + return extract_string_literal (arg_out);
> +
> + if (((low_ttype & (UNSIGNED_INTEGER | SIGNED_INTEGER))
> + && look_like_integer_literal (input_line_pointer))
> + || !(low_ttype & ~(UNSIGNED_INTEGER | SIGNED_INTEGER)))
> + return extract_integer_literal (arg_out, (low_ttype & UNSIGNED_INTEGER));
What if someone wants to use an equate?
> +/* Trim white spaces before a parameter.
> + Error if it meets a parameter separator before a parameter. */
> +static bool
> +trim_whitespaces_before_param (int n)
> +{
> + bool error = false;
> + do
> + {
> + skip_whitespace (input_line_pointer);
> + if (*input_line_pointer == ',')
> + {
> + error = true;
> + if (n < 0)
> + as_bad (_("unexpected comma before value"));
> + else
> + as_bad (_("unexpected comma before parameter %d"), n + 1);
If you started counting from 1, you could use 0 as "before value", and
n could be of an unsigned type, as you actually pass in from
vendor_attribute_parse_args(). (To me, argument number -1 is non-sensical.)
> +/* Parse the arguments of [vendor]_attribute directive. */
> +static arg_t *
> +vendor_attribute_parse_args (
> +#if (TC_OBJ_ATTR_v1 && TC_OBJ_ATTR_v2)
> + obj_attr_vendor_t vendor,
> + const obj_attr_subsection_v2 *subsec,
> +#elif (TC_OBJ_ATTR_v1)
> + obj_attr_vendor_t vendor,
> + const obj_attr_subsection_v2 *subsec ATTRIBUTE_UNUSED,
> +#else /* TC_OBJ_ATTR_v2 */
> + obj_attr_vendor_t vendor ATTRIBUTE_UNUSED,
> + const obj_attr_subsection_v2 *subsec,
> +#endif
Too much #ifdef-ary for my taste. Simply
obj_attr_vendor_t vendor ATTRIBUTE_UNUSED,
const obj_attr_subsection_v2 *subsec ATTRIBUTE_UNUSED,
will be much easier to follow, and be in line with what is done elsewhere.
> + unsigned int nargs, ...)
> +{
> + va_list args;
> + va_start (args, nargs);
> +
> + arg_t *args_out = xcalloc (nargs, sizeof (arg_t));
> +
> + for (unsigned int n = 0; n < nargs; ++n)
> + {
> + if (! trim_whitespaces_before_param (n))
> + goto bad;
> +
> + arg_t *arg_out = &args_out[n];
> +
> + arg_token_t expected_ttype = va_arg (args, arg_token_t);
> + arg_token_t high_ttype = (expected_ttype & HT_MASK);
> + /* Make sure that we called the right parse_args(). */
> + gas_assert (high_ttype == ATTRIBUTE_KEY
> + || high_ttype == ATTRIBUTE_VALUE);
> +
> + if (high_ttype == ATTRIBUTE_VALUE)
> + {
> + arg_token_t type_attr_value
> +#if (TC_OBJ_ATTR_v1 && TC_OBJ_ATTR_v2)
> + = (subsec != NULL)
> + ? obj_attr_v2_get_arg_type (subsec->encoding)
> + : obj_attr_v1_get_arg_type (stdoutput, vendor,
> + args_out[n-1].val.u32);
> +#elif (TC_OBJ_ATTR_v1)
> + = obj_attr_v1_get_arg_type (stdoutput, vendor,
> + args_out[n-1].val.u32);
> +#else /* TC_OBJ_ATTR_v2 */
> + = obj_attr_v2_get_arg_type (subsec->encoding);
> +#endif
> + expected_ttype |= type_attr_value;
> + }
> +
> + if (! obj_attr_parse_args (expected_ttype,
> + match_tag_identifier, true,
> + arg_out))
> + {
> + if (high_ttype == ATTRIBUTE_KEY)
> + as_bad (_("could not parse attribute tag"));
> + else
> + as_bad (_("could not parse attribute value"));
> + goto bad;
> + }
> +
> + if (n + 1 < nargs && !skip_whitespaces_past_comma (n))
> + goto bad;
> + }
> +
> + va_end (args);
> + demand_empty_rest_of_line ();
> +
> + return args_out;
> +
> +bad:
Please can you indent labels by at least one blank, for "diff -p" to
not screw up?
> --- a/gas/doc/c-aarch64.texi
> +++ b/gas/doc/c-aarch64.texi
> @@ -479,6 +479,33 @@ The AArch64 architecture uses @sc{ieee} floating-point numbers.
>
> @c AAAAAAAAAAAAAAAAAAAAAAAAA
>
> +@cindex @code{.aeabi_subsection} directive, AArch64
> +@item .aeabi_subsection @var{name}, @var{comprehension}, @var{encoding}
> +Create or switch the current object attributes subsection to @var{name}. Valid
> +values for @var{name} are following the pattern @code{[a-zA-Z0-9_-]+}.
> +
> +The subsection property @var{comprehension} determines how a program processing
> +the attributes handles attributes that it does not recognize (perhaps because
> +the object file was generated by a different version of the toolchain). A
> +subsection that is marked @code{optional} can be skipped if it is not
> +understood. A subsection marked @code{required} implies that information
> +conveyed by the attribute is required for correct processing of the object file;
> +a fatal diagnostic must be generated if a tool does not recognize either the tag
> +or the value associated with it.
> +
> +@var{encoding} specifies the expected encoding of the attributes recorded in the
> +subsection. Currently supported values are @code{ULEB128} and @code{NTBS}
> +(null-terminated byte string).
> +
> +@cindex @code{.aeabi_attribute} @var{tag}, @var{value}
> +@item .aeabi_attribute @var{tag}, @var{value}
> +Create an attribute with the pair @var{tag}, @var{value} in the current
> +subsection. @var{tag} can either be an integer value, or a known named key.
> +@var{value} can either be an integer or a string.
> +
> +The complete list of subsections and tags supported on AArch64 is documented
> +in @cite{Build Attributes for the Arm 64-bit Architecture (AArch64)}.
Throughout you additions here I think it would be helpful if it was clarified
which of the items are case-sensitive, and which ones are not. For example
you spell "optional" and "required" all lower-case, but "ULEB128" and "NTBS"
all upper-case. Without disambiguation it doesn't become clear whether that
has any particular significance.
> --- a/include/elf/aarch64.h
> +++ b/include/elf/aarch64.h
> @@ -59,6 +59,19 @@
> #define STO_AARCH64_VARIANT_PCS 0x80 /* Symbol may follow different call
> convention from the base PCS. */
>
> +/* Tags used in aeabi_feature_and_bits subsection. */
As I started from the header files, I'll ask here: What is "feature and
bits" about? Why is one singular and the other plural? And what is "bits"
when presumably "feature" is ...
> +typedef enum Tag_Feature_XXX {
> + Tag_Feature_BTI = 0,
> + Tag_Feature_PAC = 1,
> + Tag_Feature_GCS = 2,
> +} Tag_Feature_XXX;
... anyone of these, with ...
> +/* Tags used in aeabi_pauthabi subsection. */
> +typedef enum Tag_PAuth_XXX {
> + Tag_PAuth_Platform = 1,
> + Tag_PAuth_Schema = 2,
> +} Tag_PAuth_XXX;
... these being sub-properties of one of the features.
Further, may I please ask to avoid XXX in code that is intended to go into
a public repo. Not only can this cause spam filters to detain mails, but
to me at least it's also an indication of something which yet needs dealing
with (not much else than TODO: or FIXME:).
Jan
More information about the Binutils
mailing list