[PATCH v8 04/19] gas: implement parsing of object attributes v2
Jan Beulich
jbeulich@suse.com
Wed Aug 6 15:05:58 GMT 2025
On 06.08.2025 16:02, Matthieu Longo wrote:
> On 2025-07-31 15:40, Jan Beulich wrote:
>> On 15.07.2025 13:39, Matthieu Longo wrote:
>>> 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.
>>
>
> Unlike code where there is a genuine usecase of switching to and from
> sections, all the build attributes are in a file scope so there is not
> really much need to split them up. For instance, GCC on AArch64 targets
> will dump them all in one chunk.
> No strong opinion against the idea, so I don't mind adding it if you
> think that it would be valuable for users.
>
> NB: This would require amending the spec, and discussing it with LLVM.
Well, I don't know how things will typically look in assembly files not
produced by a compiler or alike. If "re-declarations" aren't expected to
be common, perhaps leave as is. Question then is whether to permit
"re-declarations" at all; disallowing them would simplify a few things.
>>> --- 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.
>>
>
> Those two tags were added for testing purpose only, they are not defined
> in the specifications. Ideally they should only be available for testing
> via a runtime flag, or maybe developer mode and disabled for release.
>
> Richard Earnshaw and I discussed this issue off-line previously, he
> agreed that it is not ideal to do a release build with those.
>
> However, there is neither existing define to check whether the project
> was compiled in maintainer mode or release mode, nor a runtime flag for
> a testing mode. We decided to postpone this discussion later as it would
> involve introducing new flags or add a new define for the build mode.
> This would extend the scope of the patch series again, and we certainly
> don't want that given the size of this patch series.
In a recent patch of mine I had the need to tell release builds from debug
ones, and I came up with the check you can now find in gas'es
perform_an_assembly_pass(). See commit 7b40f4c6587c ("gas: add a means to
programmatically determine the assembler version").
>>> +/* 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,
> > > 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.
>>
>
> The "identifier associated to tag" refers to the array in
> bfd/elfxx-aarch64.c for instance.
>
> 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,
> },
> ...
>
> The enum value Tag_Feature_BTI is associated to the identifier
> `Tag_Feature_BTI'.
> A subsection contains tags which are pairs of <tag_key: uleb128,
> tag_value: type_specified_in_subsection_header>.
>
> Tag here means the key. I agree that the naming is not great because it
> creates a confusion between the value and the key of a tag.
> As a rule of thumb for the whole patch series, "tag" always refers to
> the tag key, and "value" always refer to the tag value when it is
> mentioned in the context of a tag.
Might it be better to (uniformly) refer to the key as "key" then, not
as "tag" or "identifier"?
Also in structure initializers please be consistent with the style used:
Either all positional or (preferably) all designated initializers.
> > Is "string" a good name when an identifier is meant, and not an NTBS
> value?
> >
>
> It seems to me acceptable if you understand "tag" as key.
Well, see above.
>>> + 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.
>>
>
> "optional" and "required" are keywords so I would not expect them to be
> translated. And even if "comprehension" and "encoding" are not keywords,
> I would also expect them not to be translated as they refer to the spec.
>
> For instance, with the code below:
> .aeabi_subsection aeabi_feature_and_bits, optional, ULEB128
> ...
> .aeabi_subsection aeabi_feature_and_bits, required, ULEB128
>
> I would expect the following message:
> "incompatible redeclaration of subsection `aeabi_feature_and_bits'.
> Previous declaration had properties: comprehension=`optional',
> encoding=`uleb128'."
> Translated to another language, the comprehension= and encoding= should
> also be included in the do-not-translate bit.
>
> I am not sure how to reflect this intent inside the string, and how
> as_bad does not seem to have support for something like below.
>
> incompatible redeclaration of subsection %<`%s'%>. Previous declaration
> had properties: <%comprehension=`%s', encoding=`%s'%>.
>
> I guess that for now, the best solution consists in adding the `...'
> around the keywords, and ignoring the translation issue for
> comprehension= and encoding=.
>
> Do you know a better solution ?
How about the simplistic
as_bad (_("incompatible redeclaration of subsection %s. Previous "
"declaration had properties: %s=%s, %s=%s."),
name, "comprehension", prev_comprehension,
"encoding", prev_encoding);
then? (Btw, I don't think there should be full stops in gas diagnostics.)
>>> +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.
>
> See my answer to the previous comment.
Except that the same union was still there in the proposed new code, just
(aiui) moved to gas.
>>> --- 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 *.
>
> If you look in others places, it does not seem to be the style used in
> this file.
I fear I don't understand: Even right here in context, in the preceding
patch hunk, you add a well-formed construct without a blank following the
*.
>>> --- 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.
>
> I understand the benefit and agree with you.
> However, how would you suggest to do that ?
Along the lines of other projects' BUILD_BUG_ON(). Or see the top of
opcodes/i386-gen.c for a fallback when there's no _Static_assert().
>>> --- 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?
>>
>
> I just wanted to say that the GNU toolchain does not support PAuthABI.
>
> The only reason why those tags are "known" is because they are listed in
> the specification, and since the GNU toolchain does not support them,
> the merge of those attributes consists in detecting whether the value is
> set to something other than 0, and raising an error.
But a comment lie this is very easy to go stale. At the very least you'd
need to replace "currently" with something more firm.
>>> +/* 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?
This was left unaddressed (unless ...
>>> +static bool
>>> +extract_integer_literal (arg_t *arg_out,
>>> + bool signedness_unsigned)
>>
>> Maybe shorter as "want_unsigned"?
>
> Fixed.
... this was meant to cover both).
>>> +{
>>> + expressionS exp;
>>> + expression (&exp);
>>
>> As you want to use the value right away, don't you mean expression_and_evaluate()?
>>
>
> I changed it in the next revision.
> To be honest, the difference between the different modes is not clear to
> me. Please could you explain to me the difference between those ?
After parsing, an expression can be stored as-was, with just literal number
expressions resolved. Or it can be evaluated right away, also resolving non-
literal-number values as far as available at that point.
>>> + 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)?
>
> offsetT lets think that we are dealing with an offset here whereas we
> are not. This type makes the intent of the code more obscure, so I
> preferred to use int64_t (offsetT -> bfd_signed_vma -> int64_t).
But hidden type conversions can be problematic, too. We use offsetT and
its siblings in many places where it's not offsets we're dealing with.
>>> +/* 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.
>
> Not sure I follow here.
> valueT -> addressT -> bfd_vma -> uint64_t
> Where is the signed value ?
I'm sorry, I must have got confused by something.
>>> +/* 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 ~?
>>
>
> Does this look better ?
>
> static bool
> look_like_integer_literal (char const* const cursor)
> {
> if (ISDIGIT (*cursor))
> return true;
> if (! (*cursor == '+' || *cursor == '-' || *cursor == '~'))
> return false;
> char const *cursor_cpy = cursor + 1;
> while (ISSPACE (*cursor_cpy))
> ++cursor_cpy;
> return ISDIGIT (*cursor_cpy);
> }
Yes, with two nits: I don't see why you would need "cursor_cpy", and in
the function parameter the * is again misplaced.
However, as to my remark regarding ~ : That's not truly an integer literal
anymore then. Other unary operators may then also want recognizing here.
But really, aiui, what you're after is telling an expression (i.e. more
than just literal numbers) that resolve to a constant from plain symbols
(which may want using as such). This, in other words, is the custom
parsing that I'm concerned about here, and that I would rather see go
away. (We sadly have ample examples in the assembler, and about zero of
them can deal with even just mild corner cases.)
>>> +/* 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?
>
> What do you mean ?
> Something like the below ?
> .aeabi_attribute (123 + 546), ~(2-1)
That's expressions, which - yes - also ought to work imo. What I meant
with the comment is
.equ x, 1
.eqv y, 2
.aeabi_attribute x, y
(as one of the most simple examples).
>>> --- 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.
>>
>
> I personally don't like this choice between capitalized and
> non-capitalized version for the encoding. The reason why capitalized
> version exists for encoding is because ULEB128 and NTBS are acronyms,
> and it is more natural in English to use the capitalized version for
> those. I would have preferred only the capitalized version to exist, but
> the spec proposed it so Gas supports it.
>
> I wrote this documentation in a prescriptive way and expect people to
> use the version from the documentation. I omitted on purpose the
> possibility of using lower case for the encoding because having several
> options opens the door to inconsistencies across a code base.
>
> If you disagree, I can add it.
Imo you want to state precisely what's accepted. If case doesn't matter,
people should be free to use spellings of their choice. No matter what
you or I like or dislike.
>>> --- 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 ...
>>
>
> The subsection "aeabi_feature_and_bits" contains this list of tags
> which, if you OR them all, represents a bit field, hence "bits".
>
> I think that the name is rather cryptic and "aeabi_features" might have
> been better.
>
> Since this name comes straight from the spec, maybe a reference to the
> spec for more information would do instead of an introduction ?
Yes, that would do.
Jan
More information about the Binutils
mailing list