[PATCH v11 03/25] gas: implement parsing of object attributes v2
Matthieu Longo
matthieu.longo@arm.com
Fri Jan 16 18:26:19 GMT 2026
On 08/01/2026 13:04, Jan Beulich wrote:
> On 31.12.2025 00:05, Matthieu Longo wrote:
>> --- a/bfd/elf-attrs.c
>> +++ b/bfd/elf-attrs.c
>> @@ -258,6 +258,160 @@ 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 (from the perspective
>> + of GNU ld), 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.
>> + If the tag is unknown, ld can drop it if it is inside an optional subsection,
>> + whereas ld will raise an error in a required subsection.
>> + Note: the array below has to be sorted by the tag's integer value. */
>> +static const obj_attr_info_t known_attrs_gnu_testing[] =
>> +{
>> + {
>> + .tag = {"GNUTestTag_0", 0},
>> + .default_value = {},
>> + },
>> + {
>> + .tag = {"GNUTestTag_1", 1},
>> + .default_value = {},
>> + },
>> +};
> > As said on the v10 thread, aiui {} is a gcc extension (becoming standard C only
> in a version newer than C99). And as before - I don't see what good those two
> lines would do anyway. Simply omit them.
>
Fixed as follows:
static const obj_attr_info_t known_attrs_gnu_testing[] =
{
{ .tag = {"GNUTestTag_0", 0} },
{ .tag = {"GNUTestTag_1", 1} },
};
>> --- a/bfd/elf-attrs.h
>> +++ b/bfd/elf-attrs.h
>> @@ -21,6 +21,7 @@
>> #pragma once
>>
>> #include <stdint.h>
>> +#include "hidden.h"
>>
>> typedef enum obj_attr_version {
>> OBJ_ATTR_VERSION_NONE = 0,
>> @@ -47,6 +48,9 @@ typedef enum obj_attr_encoding_v2
>> #define obj_attr_encoding_v2_to_u8(value) \
>> ((uint8_t) ((value) - 1))
>>
>> +extern const char *
>> +bfd_oav2_encoding_to_string (obj_attr_encoding_v2_t);
>
> Okay, this now identifies itself as exported function.
> >> @@ -108,6 +112,9 @@ typedef struct obj_attr_subsection_v2 {
>> struct obj_attr_v2 *last;
>> } obj_attr_subsection_v2_t;
>>
>> +extern const char *
>> +oav2_comprehension_to_string (bool);
>
> What about this? It has no bfd_ prefix and no ATTRIBUTE_HIDDEN.
>
Fixed.
>> @@ -119,3 +126,37 @@ typedef struct obj_attr_subsection_list
>> /* The size of the list. */
>> unsigned int size;
>> } obj_attr_subsection_list_t;
>> +
>> +typedef struct {
>> + const char *const name;
>> + obj_attr_tag_t value;
>> +} obj_attr_tag_info_t;
>> +
>> +/* Attribute information. */
>> +typedef struct {
>> + obj_attr_tag_info_t tag;
>> + obj_attr_value_v2_t default_value;
>> +} obj_attr_info_t;
>> +
>> +typedef struct
>> +{
>> + const char *const subsec_name;
>> + const obj_attr_info_t *known_attrs;
>> + const bool optional;
>> + const obj_attr_encoding_v2_t encoding;
>> + const size_t len;
>> +} known_subsection_v2_t;
>> +
>> +struct elf_backend_data;
>> +
>> +extern const known_subsection_v2_t *
>> +bfd_obj_attr_v2_identify_subsection (const struct elf_backend_data *,
>> + const char*);
>> +
>> +extern const obj_attr_info_t *
>> +obj_attr_v2_find_known_by_tag (const struct elf_backend_data *,
>> + const char*, obj_attr_tag_t) ATTRIBUTE_HIDDEN;
>
> Nit: One blank too many ahead of ATTRIBUTE_HIDDEN, ...
>
Fixed.
>> +extern const char *
>> +obj_attr_v2_tag_to_string (const struct elf_backend_data *, const char*,
>> + obj_attr_tag_t) ATTRIBUTE_HIDDEN;
>
> ... also here. Also for all three above the pointer-to-const-char all
> (still) lack a blank ahead of the *. I'm sure I previously asked that
> you please go through the entire series to make respective adjustments.
>
Fixed.
>> --- a/bfd/elfxx-aarch64.c
>> +++ b/bfd/elfxx-aarch64.c
>> @@ -21,8 +21,10 @@
>> #include "sysdep.h"
>> #include "bfd.h"
>> #include "elf-bfd.h"
>> +#include "elf/aarch64.h"
>> #include "elfxx-aarch64.h"
>> #include "libbfd.h"
>> +#include "libiberty.h"
>> #include <stdarg.h>
>> #include <string.h>
>>
>> @@ -887,6 +889,68 @@ _bfd_aarch64_obj_attrs_version_enc (obj_attr_version_t version)
>> abort ();
>> }
>>
>> +/* List of known attributes in the subsection "aeabi_feature_and_bits".
>> + Note: the array below has to be sorted by the tag's integer value that can
>> + be found in the document "Build Attributes for the Arm® 64-bit Architecture
>> + (AArch64)". */
>> +static const obj_attr_info_t known_attrs_aeabi_feature_and_bits[] =
>> +{
>> + {
>> + .tag = {"Tag_Feature_BTI", Tag_Feature_BTI},
>> + .default_value.uint = 0,
>> + },
>> + {
>> + .tag = {"Tag_Feature_PAC", Tag_Feature_PAC},
>> + .default_value.uint = 0,
>> + },
>> + {
>> + .tag = {"Tag_Feature_GCS", Tag_Feature_GCS},
>> + .default_value.uint = 0,
>> + },
>> +};
>
> As per the earlier comment, no need for the .default_value lines.
>
Fixed.
>> +/* List of known attributes in the subsection "aeabi_pauthabi".
>> + Notes:
>> + - "aeabi_pauthabi" is a required subsection to use PAuthABI (which is
>> + today only supported by LLVM, unsupported by GCC 15 and lower. There is no
>> + plan to add support for it in the future). A value of 0 for any the tags
>> + below means that the user did not permit this entity to use the PAuthABI.
>> + - the array below has to be sorted by the tag's integer value that can be
>> + found in the document "Build Attributes for the Arm® 64-bit Architecture
>> + (AArch64)". */
>> +static const obj_attr_info_t known_attrs_aeabi_pauthabi[] =
>> +{
>> + {
>> + .tag = {"Tag_PAuth_Platform", Tag_PAuth_Platform},
>> + .default_value.uint = 0,
>> + },
>> + {
>> + .tag = {"Tag_PAuth_Schema", Tag_PAuth_Schema},
>> + .default_value.uint = 0,
>> + },
>> +};
>
> Again.
>
Fixed.
>> +/* Extract a string literal ("[^.]+") from the input. */
>
> What is ("[^.]+") about? ISTR you mentioning . to be illegal in certain places,
> so that may mean to be a regular expression. Except that ...
>
I am myself confused by this comment.
I think it is a typo because the regular expression should have been something like "([^"]+)".
Retrospectively, I don't think that this comment need a regexp inside.
Changed to the simpler:
/* Extract a string literal from the input. */
>> +static bool
>> +extract_string_literal (arg_t *arg_out)
>> +{
>> + int len;
>> + char *obstack_buf = demand_copy_C_string (&len);
>> + if (obstack_buf != NULL)
>> + {
>> + arg_out->val.string = xstrdup (obstack_buf);
>> + obstack_free (¬es, obstack_buf);
>> + arg_out->vtype = VALUE_STRING;
>> + return true;
>> + }
>> +
>> + arg_out->val.string = NULL;
>> + return false;
>> +}
>
> ... no exclusion of . is done anywhere 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.
>> + Return true on success, false otherwise. If a signedness issue is detected,
>> + 'signedness_issue' is also set to true. */
>> +static bool
>> +extract_integer_literal (arg_t *arg_out,
>> + bool want_unsigned,
>> + bool *signedness_issue)
>> +{
>> + const char *cursor_begin = input_line_pointer;
>> + expressionS exp;
>> + expression_and_evaluate (&exp);
>> + if (exp.X_op != O_constant)
>> + {
>> + char backup_c = *input_line_pointer;
>> + *input_line_pointer = '\0';
>> + as_bad (_("expression '%s' does not resolve to an integer literal"),
>
> "integer literal" is a plain number in whatever source. The result of evaluating
> an expression isn't a "literal" anymore. Just drop the word.
>
Fixed.
>> + cursor_begin);
>> + /* Restore the character pointed by the current cursor position,
>> + otherwise '\0' misleads ignore_rest_of_line(). */
>> + *input_line_pointer = backup_c;
>> + return false;
>> + }
>> +
>> + int64_t val = (int64_t) exp.X_add_number;
>> + if (want_unsigned)
>> + {
>> + if (! exp.X_unsigned && val < 0)
>> + {
>> + as_bad (_("unexpected value %" PRId64 ", expected `unsigned integer'"
>> + " instead"), val);
>
> Even if this goes beyond 80 chars, please don't wrap format string unless
> they're really long. Also please recall what I said about not putting further
> arguments after an already wrapped one (of course right here this won't be an
> issue anymore once the wrapping disappears).
>
Fixed.
>> +/* 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 resolve_identifier,
>> + bool optional,
>> + arg_t *arg_out)
>> +{
>> + const arg_token_t low_ttype = (expected_ttype & LT_MASK);
>> +
>> + if (optional && is_end_of_stmt (*input_line_pointer))
>> + {
>> + arg_out->vtype = VALUE_OPTIONAL_ABSENT;
>> + return true;
>> + }
>> +
>> + /* Check whether this looks like a string literal
>> + Note: symbol look-up for string literals is not available. */
>> + if (*input_line_pointer == '"')
>> + {
>> + bool status = extract_string_literal (arg_out);
>> + if (status && (low_ttype & STRING))
>> + return true;
>> +
>> + if (status)
>> + {
>> + char sbuffer[100];
>> + as_bad (_("unexpected `string' \"%s\", expected %s instead"),
>> + arg_out->val.string,
>> + expectations_to_string (low_ttype, sbuffer, sizeof(sbuffer)));
>> + free ((char *) arg_out->val.string);
>> + arg_out->val.string = NULL;
>> + arg_out->vtype = VALUE_UNDEFINED;
>> + }
>> + return false;
>> + }
>> +
>> + /* Check whether this looks like an identifier. */
>> + if (is_identifier_beginner (*input_line_pointer))
>> + {
>> + bool status = extract_identifier (is_part_of_identifier, arg_out);
>> + /* match_identifier() confirmed that it was the beginning of an
>> + identifier, so we don't expect the extraction to fail. */
>> + gas_assert (status);
>> + gas_assert (arg_out->vtype == VALUE_STRING);
>> +
>> + if (! (low_ttype & IDENTIFIER))
>> + {
>> + char sbuffer[100];
>> + as_bad (_("unexpected `identifier' \"%s\", expected %s instead"),
>> + arg_out->val.string,
>> + expectations_to_string (low_ttype, sbuffer, sizeof(sbuffer)));
>> + free ((char *) arg_out->val.string);
>> + arg_out->val.string = NULL;
>> + arg_out->vtype = VALUE_UNDEFINED;
>> + return false;
>> + }
>> +
>> + /* In some cases, we don't want to resolve the identifier because it is the
>> + actual value. */
>> + if (! resolve_identifier)
>> + return true;
>> +
>> + /* Move the identifier out of arg_out. */
>> + const char *identifier = arg_out->val.string;
>> + arg_out->val.string = NULL;
>> + bool resolved = true;
>> +
>> + /* The identifier is a symbol, let's try to resolve it by:
>> + 1. using the provided list of known symbols.
>> + a) backend-independent
>> + b) backend-specific. */
>> + if (lookup_known_symbols (identifier, expected_ttype, arg_out))
>> + goto free_identifier;
>> +
>> + /* 2. using the symbol table for this compilation unit.
>> + Note: this is the last attempt before failure. */
>> + if (lookup_symbol_table (identifier, low_ttype, arg_out))
>> + goto free_identifier;
>> +
>> + as_bad (_("unknown identifier '%s' in this context"), identifier);
>> + arg_out->val.string = NULL;
>> + arg_out->vtype = VALUE_UNDEFINED;
>> + resolved = false;
>> +
>> + free_identifier:
>> + free ((char *) identifier);
>> + return resolved;
>> + }
>> +
>> + /* If it is neither a string nor an identifier, it must be an expression. */
>> + bool signedness_issue = false;
>> + bool status = extract_integer_literal (arg_out,
>> + (low_ttype & UNSIGNED_INTEGER),
>> + &signedness_issue);
>> + if (status && (low_ttype & (UNSIGNED_INTEGER | SIGNED_INTEGER)))
>> + return true;
>> +
>> + char sbuffer[100];
>> + if (status)
>
> The use of "status" here is suboptimal, as - without looking at
> extract_integer_literal() itself - it's not clear whether "true" means
> success or failure. Not the least because it's "success" yet ...
>
>> + as_bad (_("unexpected integer %lu, expected %s instead"),
>> + arg_out->val.u64,
>> + expectations_to_string (low_ttype, sbuffer, sizeof(sbuffer)));
>
> ... it results in an error here. Would you mind renaming to "success" or
> "okay"?
>
Renamed to "success".
>> + else if ((low_ttype & UNSIGNED_INTEGER) && signedness_issue) {}
>> + /* Already handled by extract_integer_literal(), nothing to do. */
>
> Having the (empty) braces on the same line as the if() may be okay when the
> body is really empty. With the comment, however, please place the braces as
> usual.
>
Fixed in the next revision.
Just to make sure that this is what you want:
else if ((low_ttype & UNSIGNED_INTEGER) && signedness_issue)
{
/* Already handled by extract_integer_literal(), nothing to do. */
}
not:
else if ((low_ttype & UNSIGNED_INTEGER) && signedness_issue)
/* Already handled by extract_integer_literal(), nothing to do. */
{}
>> +/* Trim white spaces before a parameter.
>> + Error if it meets a parameter separator before a parameter. */
>> +static bool
>> +trim_whitespace_before_param (unsigned int n)
>
> You dropped the use of plural in the function name, but for some reason it
> stayed in the comment. (Again for th following function.)
>
Fixed.
>> +{
>> + 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 %u"), n);
>> + skip_past_comma (&input_line_pointer);
>> + }
>> + }
>> + while (is_whitespace (*input_line_pointer));
>
> Why does this need to be a loop? Can't you bail after encountering the first
> error, rather than perhaps printing many (unhelpful, simply by their amount)
> diagnostics?
>
Changed to remove the loop and bail out after meeting the first issue.
static bool
trim_whitespace_before_param (void)
{
skip_whitespace (input_line_pointer);
if (*input_line_pointer == ',')
{
as_bad (_("syntax error, comma not expected here"));
return false;
}
return true;
}
>> +/* Skip white spaces + parameter separator after a parameter.
>> + Error if it does not meet a parameter separator after a parameter. */
>> +static bool
>> +skip_whitespace_past_comma (unsigned int n)
>> +{
>> + skip_whitespace (input_line_pointer);
>> + if (! skip_past_comma (&input_line_pointer))
>> + {
>> + if (n == 0)
>> + as_bad (_("unexpected comma after value"));
>> + else
>> + as_bad (_("missing comma after parameter %u"), n);
>
> Something's wrong with the error messages here: skip_past_comma() returning
> "false" can't mean there was a comma in one case and there was none in the
> other.
Simplified to:
static bool
skip_whitespace_past_comma (void)
{
skip_whitespace (input_line_pointer);
if (! skip_past_comma (&input_line_pointer))
{
as_bad (_("syntax error, comma missing here"));
return false;
}
return true;
}
> I wonder if the function can be called at all when n == 0. Hmm, ...
No.
>> +/* Can parse a list of arguments with variable length. */
>> +static bool
>> +obj_attr_parse_args (arg_token_t expected_ttype,
>> + bool resolve_identifier,
>> + arg_t *arg_out)
>> +{
>> + if ((expected_ttype & LIST) == 0)
>> + return obj_attr_parse_arg (expected_ttype, resolve_identifier, false,
>> + arg_out);
>> +
>> + static const size_t LIST_MAX_SIZE = 2;
>> + arg_t *arg_list = xcalloc (LIST_MAX_SIZE, sizeof (*arg_list));
>> +
>> + /* We don't want to support recursive lists. */
>> + expected_ttype &= ~LIST;
>> +
>> + size_t n = 0;
>> + do {
>> + if (! trim_whitespace_before_param (0))
>> + goto bad;
>> +
>> + if (! obj_attr_parse_arg (expected_ttype, resolve_identifier, false,
>> + &arg_list[n]))
>> + goto bad;
>> +
>> + ++n;
>> + skip_whitespace (input_line_pointer);
>> + if (is_end_of_stmt (*input_line_pointer))
>> + break;
>> +
>> + if (! skip_whitespace_past_comma (0))
>> + goto bad;
>
> ... yes, it can, but why? Don't you mean to pass n here (which already is non-
> zero)? And don't you also mean to pass n to trim_whitespace_before_param()?
>
I removed the parameter in the next revision, so this is not an issue anymore.
>> +#if (TC_OBJ_ATTR_v1)
>> +/* Determine the expected argument type based on the tag ID. */
>> +static arg_token_t
>> +obj_attr_v1_get_arg_type (bfd *abfd,
>> + obj_attr_vendor_t vendor,
>> + obj_attr_tag_t tag)
>> +{
>> + int attr_type = bfd_elf_obj_attrs_arg_type (abfd, vendor, tag);
>> + arg_token_t arg_type;
>> + if (attr_type == (ATTR_TYPE_FLAG_STR_VAL | ATTR_TYPE_FLAG_INT_VAL))
>> + arg_type = LIST | UNSIGNED_INTEGER | STRING;
>> + else if (attr_type == ATTR_TYPE_FLAG_STR_VAL)
>> + arg_type = STRING;
>> + else
>> + /* Covers the remaning cases:
>> + - ATTR_TYPE_FLAG_INT_VAL.
>> + - ATTR_TYPE_FLAG_INT_VAL | ATTR_TYPE_FLAG_NO_DEFAULT. */
>> + arg_type = UNSIGNED_INTEGER;
>> + return arg_type;
>> +}
>> +#endif /* TC_OBJ_ATTR_v1 */
>> +
>> +#if (TC_OBJ_ATTR_v2)
>> +/* Determine the expected argument type based on the subsection encoding. */
>> +static arg_token_t
>> +obj_attr_v2_get_arg_type (obj_attr_encoding_v2_t subsec_encoding)
>> +{
>> + arg_token_t arg_type;
>> + switch (subsec_encoding)
>> + {
>> + case OA_ENC_ULEB128:
>> + arg_type = UNSIGNED_INTEGER;
>> + break;
>> + case OA_ENC_NTBS:
>> + arg_type = STRING;
>> + break;
>> + case OA_ENC_UNSET:
>> + default:
>> + abort ();
>> + }
>> + return arg_type;
>> +}
>> +#endif /* TC_OBJ_ATTR_v2 */
>> +
>> +/* Parse the arguments of [vendor]_attribute directive. */
>> +static arg_t *
>> +vendor_attribute_parse_args (obj_attr_vendor_t vendor ATTRIBUTE_UNUSED,
>> + const obj_attr_subsection_v2_t *subsec ATTRIBUTE_UNUSED,
>> + unsigned int nargs, ...)
>> +{
>> + bool parsing_err = false;
>> + 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_whitespace_before_param (n + 1))
>> + {
>> + ignore_rest_of_line ();
>> + 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,
>
> These two lines want indenting by two more positions.
>
Fixed.
>> + args_out[n-1].val.u64);
>> +#elif (TC_OBJ_ATTR_v1)
>> + = obj_attr_v1_get_arg_type (stdoutput, vendor,
>> + args_out[n-1].val.u64);
>> +#else /* TC_OBJ_ATTR_v2 */
>> + = obj_attr_v2_get_arg_type (subsec->encoding);
>> +#endif
>
> The #ifdef-ary here isn't very nice. Couldn't you drop all of it here,
> leaving just the #if part, and have obj_attr_v{1,2}_get_arg_type()
> return suitable "nothing" values when TC_OBJ_ATTR_v{1,2} isn't set?
>
This "nothing" value does not exist currently in arg_token_t.
It means that I need to add new code to handle it.
I would prefer to keep this #ifdefery rather than adding this magic value.
Is it ok to keep it as it currently is ?
>> + expected_ttype |= type_attr_value;
>> + }
>> +
>> + if (! obj_attr_parse_args (expected_ttype, true, arg_out))
>> + {
>> + if (high_ttype == ATTRIBUTE_KEY)
>> + {
>> + as_bad (_("could not parse attribute tag"));
>> + parsing_err = true;
>> + }
>> + else
>> + {
>> + as_bad (_("could not parse attribute value"));
>> + ignore_rest_of_line ();
>> + goto bad;
>
> Why would you bail here, but not for the ATTRIBUTE_KEY case? As said
> in an earlier comment, once there was an error it's hard to see why
> you would want to continue parsing.
>
In the next revision, there is only one ignore_rest_of_line() at the end of the function at 'bad', and both error cases, for the attribute and the key, jump to 'bad'.
It actually makes the implementation simpler, and solve the issue you raised in the next comment.
>> + }
>> + }
>> +
>> + if (n + 1 < nargs && !skip_whitespace_past_comma (n + 1))
>> + goto bad;
>> + }
>> +
>> + if (! parsing_err)
>> + {
>> + demand_empty_rest_of_line ();
>> + va_end (args);
>> + return args_out;
>> + }
>> +
>> + bad:
>> + args_list_free (args_out, nargs);
>> + va_end (args);
>> + return NULL;
>
> The reason you don't have ignore_rest_of_line() here is that
> skip_whitespace_past_comma() already invokes it. Imo that's wrong, though
> demonstrated also by vendor_subsection_parse_args() getting it wrong (in
> invoking ignore_rest_of_line() twice in that case). obj_attr_parse_args()
> also is exiting with inconsistent state because of this - it may or may
> not have invoked ignore_rest_of_line().
>
I fixed the inconsistency between skip_whitespace_past_comma() and trim_whitespace_before_param(), none calls ignore_rest_of_line() now.
I also removed ignore_rest_of_line() in obj_attr_parse_args(), and extract_identifier().
The error is forwarded down the call stack, and only vendor_attribute_parse_args() and vendor_subsection_parse_args() need to call ignore_rest_of_line().
The bail out for an issue on the attribute key greatly simplified the problem.
Thanks for reading this code carefully. I realize that continuing the parsing after meeting an error complexified the problem, and caused confusion for myself. All those ignore_rest_of_line() didn't make any sense.
>> +#if (TC_OBJ_ATTR_v1)
>> +/* Record an attribute (object attribute v1 only). */
>> +static obj_attribute *
>> +obj_attr_v1_record (bfd *abfd,
>> + const obj_attr_vendor_t vendor,
>> + const obj_attr_tag_t tag,
>> + arg_t *parsed_arg)
>> +{
>> + obj_attribute *attr = bfd_elf_new_obj_attr (abfd, vendor, tag);
>> + if (attr != NULL)
>> + {
>> + int tag_type = bfd_elf_obj_attrs_arg_type (abfd, vendor, tag);
>> + if (parsed_arg->vtype == VALUE_LIST)
>> + {
>> + arg_variant_list_t *plist = &parsed_arg->val.list;
>> + gas_assert (plist->len == 2
>> + && plist->elts[0].vtype == VALUE_UNSIGNED_INTEGER
>> + && plist->elts[1].vtype == VALUE_STRING);
>> + attr->type = tag_type;
>> + attr->i = plist->elts[0].val.u64;
>> + attr->s = (char *) plist->elts[1].val.string;
>> + plist->elts[1].val.string = NULL;
>> + }
>> + else if (parsed_arg->vtype == VALUE_STRING)
>> + {
>> + attr->type = tag_type;
>> + attr->s = (char *) parsed_arg->val.string;
>> + parsed_arg->val.string = NULL;
>> + }
>> + else
>> + {
>> + attr->type = tag_type;
>> + attr->i = parsed_arg->val.u64;
>> + }
>
> Why is the setting of attr->type not done once, outside the if/else chain?
> With that arguably the need for the local "tag_type" would disappear. (My
> attention was mainly caught by the suspicious use of plain int there. The
> magic values 1, 2, and 3 really would better be enumerators, I guess. Not
> something you want or need to address here, of course.)
>
Thanks for noticing that.
In the next revision, I directly assigned attr->type with the returned value of bfd_elf_obj_attrs_arg_type ().
Regarding the usage of integers in gnu_obj_attrs_arg_type(), yes, it would be preferrable to use enumerators. However, this is OAv1 code, and I don't plan to change the existing code for it.
>> +/* Record an attribute (object attribute v2 only). */
>> +static void
>> +obj_attr_v2_record (obj_attr_tag_t key, arg_t *arg_val)
>> +{
>> + /* An OAv2 cannot be recorded unless a subsection has been recorded. */
>> + gas_assert (elf_obj_attr_subsections (stdoutput).last != NULL);
>> +
>> + union obj_attr_value_v2 obj_attr_val;
>> + if (arg_val->vtype == VALUE_UNSIGNED_INTEGER)
>> + obj_attr_val.uint = arg_val->val.u64;
>> + else
>> + {
>> + /* Move the string. */
>> + obj_attr_val.string = arg_val->val.string;
>> + arg_val->val.string = NULL;
>> + }
>> +
>> + obj_attr_v2_t *obj_attr = bfd_elf_obj_attr_v2_init (key, obj_attr_val);
>> + gas_assert (obj_attr != NULL);
>> +
>> + /* Go over the list of already recorded attributes and check for
>> + redefinitions (which are forbidden). */
>> + bool skip_recording = false;
>> + obj_attr_v2_t *recorded_attr = obj_attr_v2_find_by_tag
>> + (elf_obj_attr_subsections (stdoutput).last, obj_attr->tag, false);
>> + if (recorded_attr != NULL)
>> + {
>> + if ((arg_val->vtype == VALUE_UNSIGNED_INTEGER
>> + && recorded_attr->val.uint != obj_attr->val.uint)
>> + || (arg_val->vtype == VALUE_STRING
>> + && strcmp (recorded_attr->val.string, obj_attr->val.string) != 0))
>> + as_bad (_("attribute %lu cannot be redefined"), recorded_attr->tag);
>> + skip_recording = true;
>> + }
>> +
>> + if (skip_recording)
>> + {
>> + if (arg_val->vtype == VALUE_STRING)
>> + free ((void *) obj_attr->val.string);
>> + free (obj_attr);
>> + return;
>> + }
>> +
>> + LINKED_LIST_APPEND(obj_attr_v2_t) (
>
> Nit (style): Blank before opening paren. (More of these elsewhere with the
> use of linked list macros.)
>
Fixed.
>> +/* Parse an attribute directive (supports both v1 & v2). */
>> +obj_attr_tag_t
>> +obj_attr_process_attribute (obj_attr_vendor_t vendor)
>> +{
>> + obj_attr_version_t version = elf_obj_attr_version (stdoutput);
>> + obj_attr_subsection_v2_t *subsec = NULL;
>> +
>> +#if (TC_OBJ_ATTR_v2)
>> + if (version == OBJ_ATTR_V2)
>> + {
>> + subsec = elf_obj_attr_subsections (stdoutput).last;
>> + if (subsec == NULL)
>> + {
>> + as_bad (_("declaration of an attribute outside the scope of an "
>> + "attribute subsection"));
>> + ignore_rest_of_line ();
>> + return 0;
>> + }
>> + }
>> +#endif /* TC_OBJ_ATTR_v2 */
>> +
>> + const size_t N_ARGS = 2;
>> + arg_t *args = vendor_attribute_parse_args (
>> + vendor, subsec, N_ARGS,
>> + ATTRIBUTE_KEY | IDENTIFIER | UNSIGNED_INTEGER,
>> + ATTRIBUTE_VALUE);
>> +
>> + if (args == NULL)
>> + return 0;
>> +
>> + obj_attr_tag_t tag = args[0].val.u64;
>> +#if (TC_OBJ_ATTR_v1)
>> + if (version == OBJ_ATTR_V1)
>> + {
>> + oav1_attr_record_seen (vendor, tag);
>> + obj_attr_v1_record (stdoutput, vendor, tag, &args[1]);
>> + }
>> +#endif /* TC_OBJ_ATTR_v1 */
>> +#if (TC_OBJ_ATTR_v2)
>> + #if (TC_OBJ_ATTR_v1)
>> + else
>> + #endif /* TC_OBJ_ATTR_v1 */
>> + if (version == OBJ_ATTR_V2)
>> + obj_attr_v2_record (tag, &args[1]);
>> +#endif /* TC_OBJ_ATTR_v2 */
>> + else
>> + abort ();
>
> Wouldn't using switch() make this at least a little more readable? In any
> event, please keep #-es in the first column (inserting indenting blanks
> afterwards as desired).
>
Yes, it makes the code more readable to use a switch().
I also fixed the same issue in lookup_known_symbols().
Thanks for the suggestion.
The code looks like this now:
switch (version)
{
#if (TC_OBJ_ATTR_v1)
case OBJ_ATTR_V1:
oav1_attr_record_seen (vendor, tag);
obj_attr_v1_record (stdoutput, vendor, tag, &args[1]);
break;
#endif /* TC_OBJ_ATTR_v1 */
#if (TC_OBJ_ATTR_v2)
case OBJ_ATTR_V2:
obj_attr_v2_record (tag, &args[1]);
break;
#endif /* TC_OBJ_ATTR_v2 */
default:
abort ();
}
>> +#if (TC_OBJ_ATTR_v2)
>> +/* Parse an object attribute v2's subsection directive. */
>> +void
>> +obj_attr_process_subsection ()
>
> void please as (fake) function parameter.
>
>> @@ -234,13 +1378,19 @@ obj_attr_v1_process_attribute (obj_attr_vendor_t vendor)
>> ignore_rest_of_line ();
>> return 0;
>> }
>> +#endif /* TC_OBJ_ATTR_v1 */
>>
>> /* Parse a .gnu_attribute directive. */
>>
>> void
>> obj_elf_gnu_attribute (int ignored ATTRIBUTE_UNUSED)
>> {
>> +#if (TC_OBJ_ATTR_v1)
>> obj_attr_v1_process_attribute (OBJ_ATTR_GNU);
>> +#endif /* TC_OBJ_ATTR_v1 */
>> +#if (TC_OBJ_ATTR_v2)
>> + obj_attr_process_attribute (OBJ_ATTR_GNU);
>> +#endif /* TC_OBJ_ATTR_v2 */
>> }
>
> Further up you also allow for TC_OBJ_ATTR_v1 and TC_OBJ_ATTR_v2 both to
> be set at the same time. While the code here "looks" fine, it really
> isn't, as in that case obj_attr_v1_process_attribute() will already have
> consumed a line, and obj_attr_process_attribute() will consume the next
> one.
Fixed with the below:
void
obj_elf_gnu_attribute (int ignored ATTRIBUTE_UNUSED)
{
+#if (TC_OBJ_ATTR_v1 && !TC_OBJ_ATTR_v2)
obj_attr_v1_process_attribute (OBJ_ATTR_GNU);
+#else
+ obj_attr_process_attribute (OBJ_ATTR_GNU);
+#endif
}
> (Btw, why not obj_attr_v2_process_attribute()?)
The next patch removes obj_attr_v1_process_attribute() and only keeps obj_attr_process_attribute() for both OAv1 and OAv2.
Consequently, the version number in the name is irrelevant.
>
> Jan
Matthieu
More information about the Binutils
mailing list