[PATCH v8 04/19] gas: implement parsing of object attributes v2
Matthieu Longo
matthieu.longo@arm.com
Wed Aug 27 15:17:12 GMT 2025
On 2025-08-06 16:05, Jan Beulich wrote:
> 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.
>
Note: addressed in another reply.
>>>> --- 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").
>
I tried to address this concern as a part of
https://inbox.sourceware.org/binutils/20250814151629.174026-1-matthieu.longo@arm.com/.
I am not sure how to progress on this after the concern you raised on
potential conflicts with some distro's versioning, and that I cannot
rely on either gas or ld to determine the release mode.
How about keeping the code as it is now ? It does not have any
significant impact at runtime.
>>>> +/* 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.
>
For consistency, I adopted the naming from the specs.
<attribute> is <tag, value> pair. Where tag is encoded using
unsigned LEB128 encoding (ULEB128), and value is encoded as
described by parameter type.
, and went through the patch series and uniformized the naming.
So as a summary, I replaced reference of "key" by "tag", and "tag" by
"attribute".
>> > 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.
>
obj_attr_v2_tag_to_string converts a tag (a number) to its string
representation. I don't see a case where a tag would have a different
representation for its identifier and its string representation.
Additionally, the spec states clearly that a tag cannot be an NTBS.
> 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 apparent hole in my understanding of what this
> is about.
I am not sure that I understand what is not clear here. The spec lists
tags with their associated numeric value. The name associated to the
number is what I call an identifier. In the commit description, I
haven't used the word "identifier" but there is this sentence:
Tag can either be an integer, or one of the defined symbols in the
backend.
I can change "symbol" by "identifier" if you think that it is clearer.
Please point to me any other gap so that I can fix them. Given that I
have been working on this patch series for a long time now, I might have
missed things assuming that they were already written somewhere else.
>>>> + 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.)
>
Fixed in the next revision.
>>>> +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.
>
Please, have a look at the next revision so that you are able to comment
on the code directly if there is an issue with it. It starts being
difficult to track your comments on things that I have not published yet.
>>>> --- 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
> *.
>
I saw it now. Fixed in the next revision.
>>>> --- 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().
>
I fixed the issue in the next revision by declaring the array with an
explicit length. This length has to match between the header file and
the implementation file declaring it, otherwise the compiler complains
with an explicit error message.
>>>> --- 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.
>
I replaced it with a reference to GCC 15 and earlier.
>>>> +/* 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 ...
>
Yes, it is not permitted.
>>>> +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.
>
When you say "hidden type conversion", do you mean that it would not be
obvious that a conversion happened if one day for instance, someone was
changing offsetT to be unsigned, or being a structure, or something
similar ?
I could add an explicit cast as below to really make it clear that a
conversion could potentially happen:
int64_t val = (int64_t) exp.X_add_number;
Using offsetT (a higher abstraction level type) in this part of the code
does not really make sense as I mentioned previously. The code for OAv2
is dealing with raw integers, strings, then up to the backend to
interpret them how it needs.
What would the real benefit of using offsetT be here ?
>>>> +/* 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.
>
Fixed in the next revision.
> 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.)
>
The original code for OAv1 was doing its own parsing. I unified the
parsing of OAv1 and OAv2, but indeed it is still a custom parsing.
Unifying further the parsing increases the scope of the work again. The
situation of the parsing is actually not worse than before. That's
definitely not ideal, but not worse.
Sorry, I need to think more about all that. I will try to address it in
another message.
>>>> +/* 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).
>
The equate is already supported. See the test
gas/testsuite/gas/aarch64/build-attributes/ba-1.s in patch
"Serialization and dumping tests of Object Attributes v2" as an example.
>>>> --- 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.
>
Fixed in the next revision.
>>>> --- 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.
Fixed in the next revision.
>
> Jan
More information about the Binutils
mailing list