[PATCH v1 03/14] gas: implement parsing of object attributes v2

[Matthieu Longo]

On 2025-03-28 14:24, Jan Beulich wrote:
> On 21.03.2025 18:14, Matthieu Longo wrote:
>> --- a/gas/config/obj-elf.c
>> +++ b/gas/config/obj-elf.c
>> @@ -41,6 +41,10 @@
>>   #include "elf/alpha.h"
>>   #endif
>>   
>> +#ifdef TC_AARCH64
>> +#include "elf/aarch64.h"
>> +#endif
> 
> Why is this needed? It's imo bat practice to include arch-specific files here,
> even if ...
> 

Removed in the next revision.

>>   #ifdef TC_MIPS
>>   #include "elf/mips.h"
>>   #endif
> 
> ... context on both sides demonstrates that it was done before.
> 
> For most of the new code following in this file, I'm again not overly happy to
> see so much dead code added for non-Arm64. Can't interested tc-*.h files have a
> #define added controlling whether this code is actually to be compiled?
> 

I refactored the code in the next revision, and shared the parsing code 
between OAv1 and OAv2. So TC_* defines should not be necessary anymore.

There is actually a lot of code added here for object attributes (both 
v1 and v2). I am wondering if we could not move it to another file: 
obj-attr.c
What do you think ?

>> @@ -2127,6 +2131,413 @@ obj_elf_seen_attribute (int vendor, unsigned int tag)
>>     return false;
>>   }
>>   
>> +static bool
>> +extract_string_literal (attribute_arg_value *arg_out)
>> +{
>> +  skip_whitespace (input_line_pointer);
>> +
>> +  if (*input_line_pointer != '"')
>> +    {
>> +      as_bad ("Missing token '\"' for expected string: %c", *input_line_pointer);
> 
> Can we omit "token" here (and elsewhere)? In assembly it's not quite clear what
> tokens are. In C it would be the entire string literal here.
> 

Fixed in the next revision.

>> +      return false;
>> +    }
>> +
>> +  arg_out->type = STRING_VALUE;
>> +  int len;
>> +  arg_out->value.string = demand_copy_C_string (&len);
>> +  if (len > 0)
>> +    {
>> +      arg_out->optional_set = true;
>> +      return true;
> 
> Besides the pointer being non-NULL and the return value indicating success,
> why's the optional_set field also needed?
> 

This is not needed anymore in the next revision.

>> +    }
>> +
>> +  arg_out->value.string = NULL;
>> +  return false;
>> +}
>> +
>> +static bool
>> +extract_identifier (bool (*char_predicate) (char), attribute_arg_value *arg_out)
>> +{
>> +  expressionS exp;
>> +
>> +  /* Try to extract a value from the identifier.  */
>> +  if (ISDIGIT (*input_line_pointer))
> 
> Wait - numeric value or identifier?
> 

This was changed in the next revision.

>> +    {
>> +      expression (& exp);
>> +      if (exp.X_op != O_constant)
>> +	{
>> +	  as_bad (_("invalid value, expected an integer literal"));
> 
> O_constant isn't necessarily an integer literal. It can be an expression that
> was consisting of only constants, perhaps even including equates.
> 

Yes, I agree. This is taken from the existing code for OAv1.

>> +	  goto bad_integer_literal;
>> +	}
>> +      int val = exp.X_add_number;
> 
> Is silent truncation really okay here?
> 

Changed val to int64_t to avoid truncation.

>> +      arg_out->optional_set = true;
>> +      arg_out->type = ULEB128_VALUE;
>> +      arg_out->value.u64 = val;
>> +      return true;
>> +    }
>> +
>> +  /* Try to extract a symbol.  */
>> +  const char *s = input_line_pointer;
>> +  unsigned int i = 0;
>> +  for (; char_predicate (*input_line_pointer); ++input_line_pointer)
>> +    i++;
>> +  if (i == 0)
>> +    return false;
> 
> No diagnostic on this error path?
> 

Added a diagnostic message in the next revision.

>> +  char* val = xmemdup0 (s, i);
>> +  arg_out->optional_set = true;
>> +  arg_out->type = STRING_VALUE;
>> +  arg_out->value.string = val;
> 
> STRING_VALUE further up was used for string literals. How come it identifies
> what looks to be an identifier here? If it's an identifier, why would you not
> use one of the existing primitives to parse that, e.g. get_symbol_name()?
> 

get_symbol_name seems to do too much for what I needed, specifically the 
handling of double quotes.

>> +  return true;
>> +
>> +bad_integer_literal:
> 
> Please can labels be indented by (at least) one blank, to help GNU diff -p
> output? Then again this label is used exactly once, so can likely be avoided
> altogether.
> 

Fixed in the next revision.

>> +  ignore_rest_of_line ();
>> +  return false;
>> +}
>> +
>> +typedef struct obj_attr_v2_known_identifier
>> +{
>> +  const char *name;
>> +  const uint32_t val;
> 
> This const isn't very useful (or else "name" would also want to have one).
> You have ...
> 

Fixed in the next revision.

>> +} obj_attr_v2_known_identifier;
>> +
>> +static bool
>> +obj_attr_v2_lookup_known_symbol (
>> +  const char *identifier,
>> +  const obj_attr_v2_known_identifier known_identifiers[],
> 
> ... const here, which is entirely sufficient afaict.
> 

Fixed in the next revision.

>> +  const size_t N_identifiers,
>> +  uint32_t *val_out)
>> +{
>> +  if (identifier == NULL || known_identifiers == NULL || N_identifiers == 0)
>> +    return false;
> 
> Wouldn't callers better make sure of this? It's an internal function after all.
> Plus there's nothing wrong with N_identifiers == 0 (would be handled fine below
> anyway), and in that case known_identifiers == NULL also wouldn't be an issue.
> 

Fixed in the next revision.

>> +  for (size_t i = 0; i < N_identifiers; ++i)
>> +    if (!strcmp (known_identifiers[i].name, identifier))
>> +      {
>> +	*val_out = known_identifiers[i].val;
>> +	return true;
>> +      }
>> +  return false;
>> +}
>> +
>> +static int
>> +obj_attr_v2_look_symbol_table (const char *name)
> 
> The name doesn't quite describe what the function does.
> 

I restructure the code in the next revision. I hope it will be clearer.

>> +{
>> +  if (name == NULL)
>> +    return -1;
>> +
>> +  symbolS *symbolP;
>> +  symbolP = symbol_find (name);
> 
>    symbolS *symbolP = symbol_find (name);
> 
> ?
> 
>> +  if (symbolP == NULL)
>> +    return -1;
>> +
>> +  if (! S_IS_DEFINED (symbolP))
>> +    return -1;
>> +
>> +  valueT val = S_GET_VALUE (symbolP);
>> +  DEBUG_TRACE ("Resolved symbol '%s' to %lu", name, val);
> 
> This doesn't look like it will compile for non-Arm64, as it's tc-aarch64.h
> which defines the macro.
> 

Fixed in the next revision. I tested the new revision on several 
backends (see testing in commit message).

>> +  return val;
> 
> Silent truncation again, and signed-ness change as well.
> 

Fixed in the next revision.

>> +}
>> +
>> +static bool
>> +obj_attr_parse_arg (attribute_arg_type arg_type,
>> +		    const obj_attr_v2_known_identifier known_identifiers[],
>> +		    const size_t N_identifiers,
>> +		    bool skip_backend_symbol_attribute_lookup,
> 
> I'm puzzled by this name: What backend? And then there's nothing really
> skipped when this comes in as true. Instead something extra is being done.
> 

I changed the logic in the next revision. I don't think this comment is 
relevant anymore.

>> +		    attribute_arg_value *arg_out)
>> +{
>> +  bool match_symbol (char c)
>> +  {
>> +    return ISALNUM (c) || c == '_';
>> +  }
> 
> Please can we avoid nested functions? For this particular one I wonder why
> it's needed anyway. As indicated above, if you look for an identifier, parse
> it using one of the existing interfaces. For match_subsection_char() further
> down I can see that things might be getting more difficult, if you also
> want to permit dashes. (And perhaps also other characters?) Yet then recall
> that quoted identifiers have been supported for a while, which may be an
> option there to limit the amount of custom code.
> 

I removed the nested functions.
Dashes are not allowed for the key words of the directive. I should 
maybe use the word "key word" here ?
Please have a look at the next revision as the code was heavily 
restructured.

>> +  /* Note: for now, symbol look-up for string literals is not available.  */
>> +  if (arg_type == STRING_VALUE)
>> +    return extract_string_literal (arg_out);
>> +
>> +  /* Try to extract an identifier (symbol, integer literal).  */
>> +  if (! extract_identifier (&match_symbol, arg_out))
>> +    return false;
>> +
>> +  /* The identifier is fully resolved.  */
>> +  if (arg_out->type == ULEB128_VALUE)
>> +    return true;
>> +
>> +  /* The identifier is a symbol, let's try to resolve it using the provided list
>> +     of known symbols.  */
>> +  gas_assert (arg_out->type == STRING_VALUE);
>> +  uint32_t resolved_symbol_value;
>> +  if (obj_attr_v2_lookup_known_symbol (arg_out->value.string,
>> +				       known_identifiers,
>> +				       N_identifiers,
>> +				       &resolved_symbol_value))
>> +    {
>> +      free ((void *) arg_out->value.string);
>> +      arg_out->value.u64 = resolved_symbol_value;
>> +      arg_out->type = ULEB128_VALUE;
>> +      return true;
>> +    }
>> +
>> +  int resolved_symbol;
>> +  if (skip_backend_symbol_attribute_lookup)
>> +    {
>> +      /* Try to resolve the name using the symbol table for this compilation
>> +	 unit.  */
>> +      resolved_symbol = obj_attr_v2_look_symbol_table (arg_out->value.string);
>> +      if (resolved_symbol == -1)
>> +	goto bad;
>> +    }
> 
> This body looks to exactly match ...
> >> +#ifndef CONVERT_SYMBOLIC_ATTRIBUTE
>> +  #define CONVERT_SYMBOLIC_ATTRIBUTE(a) -1
>> +#endif
>> +
>> +  /* Note: for now, CONVERT_SYMBOLIC_ATTRIBUTE can only handle integers.  */
>> +  resolved_symbol = CONVERT_SYMBOLIC_ATTRIBUTE (arg_out->value.string);
>> +  if (resolved_symbol == -1)
>> +    {
>> +      /* Try to resolve the name using the symbol table for this compilation
>> +	 unit.  */
>> +      resolved_symbol = obj_attr_v2_look_symbol_table (arg_out->value.string);
>> +      if (resolved_symbol == -1)
>> +	goto bad;
>> +    }
> 
> ... this one. Better fold such duplicate code?
> 
> Also what's calculated in the first if() is then overwritten again right
> afterwards (unless the error path is taken).
> 

I restructured all this code in the next revision to share a lot of 
parsing code between OAv1 and OAv2.

>> +  free ((void *) arg_out->value.string);
>> +  arg_out->value.u64 = resolved_symbol;
>> +  arg_out->type = ULEB128_VALUE;
>> +  return true;
>> +
>> +bad:
>> +  as_bad ("unknown symbol: %s", arg_out->value.string);
>> +  free ((void *) arg_out->value.string);
>> +  return false;
>> +}
>> +
>> +void
>> +obj_elf_obj_attr_v2_free_args (attribute_arg_value *args, size_t n)
>> +{
>> +  for (size_t i = 0; i < n; ++i)
>> +    if (args[i].type == STRING_VALUE && args[i].value.string != NULL)
>> +      free ((void *) args[i].value.string);
>> +  free (args);
>> +}
>> +
>> +attribute_arg_value *
>> +obj_elf_subsection_obj_attr_v2_parse_args (int nargs, ...)
> 
> It is generally advisable that variables which can't hold negative values,
> i.e. ...
> 

Fixed in the next revision.

>> +{
>> +  bool match_subsection_char (char c)
>> +  {
>> +    return ISALNUM (c) || c == '_' || c == '-';
>> +  }
>> +
>> +  va_list args;
>> +  va_start (args, nargs);
>> +
>> +  attribute_arg_value* attrs = calloc (nargs, sizeof (attribute_arg_value));
>> +
>> +  for (int count = 0; count < nargs; ++count)
> 
> ... also this one, be of an unsigned type.
> 

Fixed in the next revision.

>> +    {
>> +      skip_whitespace (input_line_pointer);
>> +
>> +      attribute_arg_type type_next_arg = va_arg (args, attribute_arg_type);
>> +
>> +      if (type_next_arg == SECTION_IDENTIFIER)
>> +	{
>> +	  if (!extract_identifier (&match_subsection_char, &attrs[count]))
>> +	    {
>> +	      as_bad (_("Invalid token for section name: %c"), *input_line_pointer);
>> +	      goto bad;
>> +	    }
>> +	  gas_assert (attrs[count].type == STRING_VALUE);
>> +	  attrs[count].type = SECTION_IDENTIFIER;
>> +	}
>> +      else if (type_next_arg == ULEB128_VALUE || type_next_arg == STRING_VALUE)
>> +	{
>> +	  const obj_attr_v2_known_identifier known_identifiers[] =
> 
> static?
> 

Fixed in the next revision.

>> +	  {
>> +	    {"ULEB128", 0},
>> +	    {"uleb128", 0},
>> +	    {"NTBS", 1},
>> +	    {"ntbs", 1},
>> +	    {"optional", 1},
>> +	    {"required", 0},
>> +	  };
>> +	  if (! obj_attr_parse_arg (
>> +		  type_next_arg, known_identifiers,
>> +		  ARRAY_SIZE (known_identifiers),
>> +		  /* skip_backend_symbol_attribute_lookup */ true,
>> +		  &attrs[count]))
>> +	    goto bad;
>> +	}
>> +
>> +      if (count + 1 < nargs && skip_past_comma (&input_line_pointer) == -1)
>> +	goto bad;
>> +    }
>> +
>> +  va_end (args);
>> +  demand_empty_rest_of_line ();
>> +
>> +  return attrs;
>> +
>> +bad:
>> +  obj_elf_obj_attr_v2_free_args (attrs, nargs);
>> +  va_end (args);
>> +  as_bad (_("Expected <subsection_name>, <optional>, <format>"));
>> +  ignore_rest_of_line ();
>> +  return NULL;
>> +}
>> +
>> +attribute_arg_value *
>> +obj_elf_obj_attr_v2_parse_args (int nargs, ...)
>> +{
>> +  if (elf_obj_attr_subsections (stdoutput).last_ == NULL)
>> +    {
>> +      as_bad (_("A subsection has to be declared before declaring attributes"));
>> +      ignore_rest_of_line ();
>> +      return NULL;
>> +    }
>> +
>> +  va_list args;
>> +  va_start (args, nargs);
>> +
>> +  attribute_arg_value* attrs = calloc (nargs, sizeof (attribute_arg_value));
> 
> This can fail, yet you happily use the result further down. Better use
> xcalloc()?
> 

Fixed in the next revision.

>> +  for (int count = 0; count < nargs; ++count)
>> +  {
>> +    skip_whitespace (input_line_pointer);
>> +
>> +    attribute_arg_type type_next_arg = va_arg (args, attribute_arg_type);
>> +
>> +    gas_assert (type_next_arg == ATTRIBUTE_KEY
>> +	     || type_next_arg == ATTRIBUTE_VALUE);
>> +
>> +    if (type_next_arg == ATTRIBUTE_KEY)
>> +      {
>> +	if (! obj_attr_parse_arg (
>> +		type_next_arg, NULL, 0,
>> +		/* skip_backend_symbol_attribute_lookup */ false,
>> +		&attrs[count]))
>> +	  goto bad;
>> +      }
>> +    else if (type_next_arg == ATTRIBUTE_VALUE)
>> +      {
>> +	obj_attr_encoding_v2 expected_value_type =
>> +	  elf_obj_attr_subsections (stdoutput).last_->encoding;
>> +	gas_assert (expected_value_type == ULEB128
>> +		 || expected_value_type == NTBS);
>> +
>> +	if (! obj_attr_parse_arg (
>> +		expected_value_type == ULEB128 ? ULEB128_VALUE : STRING_VALUE,
>> +		NULL, 0, /* skip_backend_symbol_attribute_lookup */ true,
>> +		&attrs[count]))
>> +	  goto bad;
>> +      }
>> +
>> +    if ((count + 1 < nargs) && (skip_past_comma (&input_line_pointer) == -1))
> 
> Within a single patch, can you please be consistent with the use of parentheses?
> See the similar construct (without inner parentheses) further up.
> 

Sorry again for this. I struggle a lot with the GNU coding style, and 
mix a lot of things up. Fixed in the next revision I hope.

>> +      goto bad;
>> +  }
>> +
>> +  va_end (args);
>> +  demand_empty_rest_of_line ();
>> +
>> +  return attrs;
>> +
>> +bad:
>> +  obj_elf_obj_attr_v2_free_args (attrs, nargs);
>> +
>> +  va_end (args);
>> +  as_bad (_("expected <tag> , <value>"));
> 
> This sounds as if they should be alternating, yet the code further up looks
> to be accepting things in any order.
> 

Code was heavily changed in the next revision. Order is not taken into 
account.

>> +  ignore_rest_of_line ();
>> +  return NULL;
>> +}
>> +
>> +void
>> +obj_elf_record_subsection_obj_attr_v2 (const char *name,
>> +				       bool optional,
>> +				       obj_attr_encoding_v2 encoding)
>> +{
>> +  obj_attr_subsection_v2* already_recorded_subsec =
>> +    obj_attr_subsection_v2_find_by_name
>> +      (elf_obj_attr_subsections (stdoutput).first_, name, false);
>> +
>> +  if (already_recorded_subsec != NULL)
>> +    {
>> +      /* Check for mismatching redefinition of the subsection, i.e. the names
>> +	 match but the properties are different.  */
>> +      if ((already_recorded_subsec->optional != optional)
>> +       || (already_recorded_subsec->encoding != encoding))
>> +	{
>> +	  as_bad (_("Recalled subsections must have the same parameters"));
>> +	  return;
>> +	}
> 
> Wouldn't a secondary use of the respective directive better also permit
> simply omitting the extra attributes?
> 

We discussed this in the specs, and we opted for keeping everything 
explicit. This is an optimization anyway, and could be implemented later 
if this is considered very useful for the users.

>> +      /* Move the existing subsection to the last position.  */
>> +      LINKED_LIST_REMOVE(obj_attr_subsection_v2) (
>> +	&elf_obj_attr_subsections (stdoutput), already_recorded_subsec);
>> +      LINKED_LIST_APPEND(obj_attr_subsection_v2) (
>> +	&elf_obj_attr_subsections (stdoutput), already_recorded_subsec);
>> +    }
>> +  else
>> +    {
>> +      const char *vendor_name =
>> +	get_elf_backend_data (stdoutput)->obj_attrs_vendor;
> 
> A comment in patch 2 says this is unused by Arm64. Yet here it's used?
> 

Yes, the comment needs to be updated. This is used to determine whether 
the subsection is public or private. Everything starting with the vendor 
prefix should be considered public.
I updated the comment in the next revision.

>> +      obj_attr_subsection_scope_v2 scope =
>> +	(strncmp (name, vendor_name, strlen (vendor_name)) == 0)
>> +	? SUBSEC_PUBLIC
>> +	: SUBSEC_PRIVATE;
>> +
>> +      obj_attr_subsection_v2* new_subsection =
>> +	_bfd_elf_obj_attr_subsection_v2_init (name, scope, optional, encoding);
>> +      LINKED_LIST_APPEND(obj_attr_subsection_v2) (
>> +	&elf_obj_attr_subsections (stdoutput), new_subsection);
>> +    }
>> +}
>> +
>> +void
>> +obj_elf_record_obj_attr_v2 (uint64_t key, attribute_arg_value *value)
>> +{
>> +  union obj_attr_value_v2 obj_attr_vals;
>> +  if (value->type == ULEB128_VALUE)
>> +    obj_attr_vals.uint_val = value->value.u64;
>> +  else
>> +    {
>> +      /* Move the string.  */
>> +      obj_attr_vals.string_val = xstrdup (value->value.string);
>> +      value->value.string = NULL;
> 
> If that string was also allocated, aren't you leaking it here?
> 

I changed this in the next revision, and freed the string with 
obstack_free().

>> +    }
>> +
>> +  obj_attr_v2* obj_attr = _bfd_elf_obj_attr_v2_init (key, obj_attr_vals);
>> +  if (value->type == STRING_VALUE)
>> +    obj_attr_vals.string_val = NULL;
> 
> Why is this? _bfd_elf_obj_attr_v2_init() already does so, doesn't it?
> 

I did this because I moved the string value from obj_attr_vals to 
obj_attr, but obj_attr_vals is on the stack and not freed, so risk of 
double-free.
Fixed in the next revision.

>> +  /* Go over the list of already recorded attributes and check for
>> +     redefinitions (which are forbidden).  */
>> +  bool skip_recording = false;
>> +  obj_attr_v2* recorded_attr = obj_attr_v2_find_by_tag
>> +    (elf_obj_attr_subsections (stdoutput).last_, obj_attr->tag, false);
>> +  if (recorded_attr != NULL)
>> +    {
>> +      if ((value->type == ULEB128_VALUE
>> +	   && recorded_attr->vals.uint_val != obj_attr->vals.uint_val) ||
>> +	  (value->type == STRING_VALUE
>> +	   && strcmp (recorded_attr->vals.string_val, obj_attr->vals.string_val) != 0))
>> +	as_bad (_("Attribute %u cannot be redefined"), recorded_attr->tag);
>> +      skip_recording = true;
>> +    }
>> +
>> +  if (skip_recording)
>> +    {
>> +      if (value->type == STRING_VALUE && obj_attr->vals.string_val != NULL)
>> +	free ((void *) obj_attr->vals.string_val);
>> +      free (obj_attr);
>> +      return;
>> +    }
>> +
>> +  gas_assert (elf_obj_attr_subsections (stdoutput).last_ != NULL);
> 
> This comes a little too late - obj_attr_v2_find_by_tag() already dereferenced
> the pointer.
> 

This assert checks that obj_elf_record_obj_attr_v2() is not called 
whereas no subsection has been previously registered.
It makes more sense to move it to the top. Fixed in the next revision.

>> +  gas_assert (obj_attr != NULL);
> 
> Similarly here - the call to obj_attr_v2_find_by_tag() already dereferenced
> the pointer.
> 

Fixed in the next revision.

>> --- a/gas/config/tc-aarch64.c
>> +++ b/gas/config/tc-aarch64.c
>> @@ -2401,6 +2401,56 @@ s_tlsdescldr (int ignored ATTRIBUTE_UNUSED)
>>   
>>     demand_empty_rest_of_line ();
>>   }
>> +
>> +/* Parse a .aeabi_subsection directive.  */
>> +static void
>> +s_aarch64_aeabi_subsection (int ignored ATTRIBUTE_UNUSED)
>> +{
>> +  const size_t N_ARGS = 3;
>> +  attribute_arg_value* args =
>> +    obj_elf_subsection_obj_attr_v2_parse_args (
>> +      N_ARGS, SECTION_IDENTIFIER, ULEB128_VALUE, ULEB128_VALUE);
>> +
>> +  bool is_valid_boolean (uint64_t value)
>> +  {
>> +    return value == 0 || value == 1;
>> +  }
>> +
>> +  bool is_valid_optional (uint64_t value)
>> +  {
>> +    return value == 0 || value == 1;
>> +  }
>> +
>> +  if (args == NULL)
>> +    return;
>> +
>> +  if (! is_valid_boolean (args[1].value.u64))
>> +    as_bad (("Expected subsection values for <optional> are 0 or 1"));
>> +  if (! is_valid_optional (args[2].value.u64))
>> +    as_bad (("Expected subsection values for <format> are 0 or 1"));
> 
> The user doesn't specify 0 or 1 there, I thought? The two values are
> internal representations of what the user supplies? is_valid_optional()
> probably better also wouldn't use literal numbers, but the enumerators
> that I think you did introduce somewhere.
> 

The user can either specify a keyword ('optional' or 'required'), or an 
integer (hence the check on the value).

>> @@ -11320,4 +11372,54 @@ aarch64_elf_copy_symbol_attributes (symbolS *dest, symbolS *src)
>>         S_SET_SIZE (dest, S_GET_SIZE (src));
>>       }
>>   }
>> +
>> +static bool
>> +aarch64_obj_attr_v2_lookup_known_symbol (const char *identifier, uint32_t *val)
>> +{
>> +  if (identifier == NULL)
>> +    return false;
>> +
>> +  /* IMPORTANT: This list must be alphabetically sorted.  */
> 
> This comment may better go ...
> 
>> +  static const struct
>> +  {
>> +    const char *name;
>> +    const uint32_t val;
>> +  } known_identifiers[] =
>> +  {
> 
> ... here.
> 

I changed the code significantly in the next revision, but took into 
account this comment.

>> +    {"Tag_Feature_BTI", 0},
>> +    {"Tag_Feature_GCS", 2},
>> +    {"Tag_Feature_PAC", 1},
>> +    {"Tag_PAuth_Platform", 1},
>> +    {"Tag_PAuth_Schema", 2},
>> +  };
> 
> Multiple names for the same value, when the names don't look to mean the
> same (albeit I lack Arm64 knowledge to be fully sure about this)?
> 

Tags are tied to their subsection, so some tags might have the same 
value like Tag_Feature_PAC and Tag_PAuth_Platform.
Contrarily to OAv1, a tag is not only identified by its ID, but by 
(subsection_name, ID).

>> +  for (unsigned i = 0; i < ARRAY_SIZE (known_identifiers); ++i)
>> +    {
>> +      int cmp = strcmp (known_identifiers[i].name, identifier);
>> +      if (cmp == 0)
>> +	{
>> +	  *val = known_identifiers[i].val;
>> +	  return true;
>> +	}
>> +      else if (cmp > 0)
>> +	break;
>> +    }
>> +  return false;
>> +}
>> +
>> +/* Convert a symbol related to object attribute v2 to its corresponding
>> +   integer value.  */
>> +int
>> +aarch64_convert_symbolic_attribute (const char *name)
>> +{
>> +  if (name == NULL)
>> +    return -1;
>> +
>> +  uint32_t value = 0;
>> +  /* Try to resolve the name against a known list of identifiers.  */
>> +  if (aarch64_obj_attr_v2_lookup_known_symbol (name, &value))
>> +    return value;
> 
> Silent signed-ness change again.

Fixed in the next revision.

> 
> Jan