[PATCH v8 04/19] gas: implement parsing of object attributes v2
Matthieu Longo
matthieu.longo@arm.com
Wed Aug 6 14:02:57 GMT 2025
On 2025-07-31 15:40, Jan Beulich wrote:
> 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?
>
Yes.
>> - .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.
>
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.
>> ---
>> 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".
>
I know this patch series is really big. I tried to split the changes as
small as possible while keeping the logic.
Thanks a lot for your time and effort reviewing this patch series.
Regarding the correctness, the tests are a safety net. It might not
cover everything, but can give you a degree of confidence. If you think
that I missed some corner cases, please let me know, I will add them to
the test suite.
>> --- 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.
>> +/* List of known GNU subsections.
>> + Note: this list needs to be sorted. */
>> +static known_subsection_v2 obj_attr_v2_known_gnu_subsections[] =
>
> const?
>
Fixed.
>> +{
>> + {
>> + /* 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.
>
Fixed.
>> + },
>> + /* 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?
>
Yes, this is a prefix. People can do whatever they want with it.
For instance, for a test case only relying on parsing the assembly and
generating an object, and no merging is required, then it is a valid name.
However, if you want to test the merge, you need to follow this naming:
gnu-testing-<XXXXXX>-MERGE-<POLICY>
Otherwise you will get an error at runtime telling you that this
subsection is unknown, so merge policy is supported for it.
>> +/* 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.)
>
Fixed in the next revision.
>> + 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?
>
Fixed.
>> + }
>> + 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,
> > 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.
> 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.
>> + 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 ?
>> --- 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".
>
Regarding this comment and the next one, I decided to refactor those
types for two reasons:
1. I used the approach of the hammer, one size fits all, but this is
making the code less clear, and it removed some semantic informations.
This variant was used for too many different things.
2. This variant is not needed in elf-attrs.h, and can be segregated to
gas/config/obj-elf-attr.c because it is only needed for parsing. As soon
as I did this refactoring, the naming of the variant used for parsing
was clearer.
So, to summarize the changes that will appear in the next revision, the
only types appearing in elf-attrs.h are:
1.
typedef struct {
const char *const name;
obj_attr_tag_t value;
} obj_attr_tag_info_t;
This is used to declare an array of known attribute tags (=the keys) in
readelf.
2.
/* Attribute information. */
typedef struct {
const obj_attr_tag_info_t tag;
const obj_attr_value_v2 default_value;
} obj_attr_info_t;
typedef struct
{
const char *const subsec_name;
const obj_attr_info_t *known_tags;
const bool optional;
const obj_attr_encoding_v2 encoding;
const size_t len;
} known_subsection_v2;
This is used to declare known attributes, and is used for:
- parsing (the 'tag' part) to resolve tag identifiers,
- merging (the 'default_value') to generate a default value for a tag if
it is missing in REF or RHS.
Example:
static const obj_attr_info_t known_tags_aeabi_pauthabi[] =
{
{
.tag = {"Tag_PAuth_Platform", .value = Tag_PAuth_Platform},
.default_value = {.uint_val = 0},
},
{
.tag = {"Tag_PAuth_Schema", .value = Tag_PAuth_Schema},
.default_value = {.uint_val = 0},
},
};
In gas/config/obj-elf-attr.c, :
- The keywords used in .aeabi_subsection directive.
/* A variant type to store information about known OAv2 identifiers. */
typedef union {
uint8_t u8;
bool b;
} oav2_identifier_variant_value;
typedef enum {
OAv2_ASM_ID_VALUE_UNDEFINED = 0,
OAv2_ASM_ID_VALUE_U8,
OAv2_ASM_ID_VALUE_BOOL,
} oav2_identifier_variant_type_info;
typedef struct {
oav2_identifier_variant_value val;
oav2_identifier_variant_type_info vtype;
} oav2_identifier_variant_t;
typedef struct {
const char *const name;
const oav2_identifier_variant_t value;
} oav2_identifier_t;
Example:
static const oav2_identifier_t known_identifiers_subsection_optional[] = {
{ "optional", .value = {
.val.b = true,
.vtype = OAv2_ASM_ID_VALUE_BOOL
}
},
{ "required", .value = {
.val.b = false,
.vtype = OAv2_ASM_ID_VALUE_BOOL
}
},
};
- The parsing of the arguments of a directive.
/* A variant type to store the argument values of an assembly directive. */
struct arg_variant_t;
typedef struct {
size_t len;
struct arg_variant_t *elts;
} arg_variant_list;
typedef union {
const char *string;
#if (TC_OBJ_ATTR_v1)
uint32_t u32;
#endif
uint64_t u64;
int64_t i64;
arg_variant_list list;
} arg_variant_value;
typedef enum {
VALUE_UNDEFINED = 0,
#if (TC_OBJ_ATTR_v1)
VALUE_U32,
#endif
VALUE_U64,
VALUE_I64,
VALUE_UNSIGNED_INTEGER = VALUE_U64,
VALUE_SIGNED_INTEGER = VALUE_I64,
VALUE_STRING,
VALUE_LIST,
} arg_variant_type_info;
typedef struct arg_variant_t {
arg_variant_value val;
arg_variant_type_info vtype;
} arg_variant_t;
typedef arg_variant_t arg_t;
>> +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.
>> --- 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.
>> --- 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 ?
>> --- 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.
>> 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 ...
>
Fixed in the next revision.
>> + },
>> + {
>> + .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.
>
Fixed.
>> +/* 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"?
>
Fixed.
>> +{
>> + 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 ?
>> + 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).
>> + if (val < 0 && signedness_unsigned)
>> + {
>> + as_bad (_("invalid negative value %ld, expected an unsigned integer"),
>
> You can't use %ld to print int64_t.
>
I was ignoring it was not the right way.
I had a look at
https://en.cppreference.com/w/c/types/integer.html#Format_macro_constants,
and the doc suggests to use PRId64.
Fixed in the next revision using PRId64.
>> + 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.
>
Fixed in the next revision following your suggestion.
int64_t val = exp.X_add_number;
if (want_unsigned)
{
if (! exp.X_unsigned && val < 0)
{
as_bad (_("invalid negative value %" PRId64 ", expected an unsigned "
"integer"), val);
goto bad;
}
arg_out->val.u64 = val;
arg_out->vtype = VALUE_UNSIGNED_INTEGER;
}
else
{
arg_out->val.i64 = val;
arg_out->vtype = VALUE_SIGNED_INTEGER;
}
>> + 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.
>
Fixed.
>> + 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()
>
See my previous comment about refactoring the variant type, this line
disappeared.
>> +/* 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 ?
>> +/* 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);
}
>> +/* 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)
>> +/* 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.)
>
Fixed.
>> +/* 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.
>
Fixed.
>> + 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?
>
Fixed.
>> --- 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.
>> --- 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 ?
>> +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
Renamed to Tag_Feature_Set and Tag_PAuth_Info in the next revision.
Matthieu
More information about the Binutils
mailing list