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

[Jan Beulich]

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 ...

>  #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?

> @@ -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.

> +      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?

> +    }
> +
> +  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?

> +    {
> +      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.

> +	  goto bad_integer_literal;
> +	}
> +      int val = exp.X_add_number;

Is silent truncation really okay here?

> +      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?

> +  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()?

> +  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.

> +  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 ...

> +} 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.

> +  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.

> +  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.

> +{
> +  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.

> +  return val;

Silent truncation again, and signed-ness change as well.

> +}
> +
> +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.

> +		    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.

> +  /* 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).

> +  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. ...

> +{
> +  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.

> +    {
> +      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?

> +	  {
> +	    {"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()?

> +  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.

> +      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.

> +  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?

> +      /* 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?

> +      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?

> +    }
> +
> +  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?

> +  /* 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.

> +  gas_assert (obj_attr != NULL);

Similarly here - the call to obj_attr_v2_find_by_tag() already dereferenced
the pointer.

> --- 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.

> @@ -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.

> +    {"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)?

> +  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.

Jan