[PATCH v12 03/25] gas: implement parsing of object attributes v2

[Matthieu Longo]

On 19/01/2026 11:32, Jan Beulich wrote:
> On 16.01.2026 19:59, Matthieu Longo wrote:
>> --- a/gas/config/obj-elf-attr.c
>> +++ b/gas/config/obj-elf-attr.c
>> @@ -22,8 +22,63 @@
>>   
>>   #ifdef TC_OBJ_ATTR
>>   
>> +#include "obstack.h"
>>   #include "safe-ctype.h"
>>   
>> +/* A variant type to store information about known OAv2 identifiers.  */
>> +typedef union {
>> +  uint8_t u8;
>> +  bool b;
>> +} oav2_identifier_variant_value_t;
>> +
>> +typedef enum {
>> +  OAv2_ASM_ID_VALUE_UNDEFINED = 0,
>> +  OAv2_ASM_ID_VALUE_U8,
>> +  OAv2_ASM_ID_VALUE_BOOL,
>> +} oav2_identifier_variant_type_info_t;
>> +
>> +typedef struct {
>> +  oav2_identifier_variant_value_t val;
>> +  oav2_identifier_variant_type_info_t vtype;
>> +} oav2_identifier_variant_t;
>> +
>> +typedef struct {
>> +  const char *const name;
>> +  const oav2_identifier_variant_t value;
>> +} oav2_identifier_t;
>> +
>> +
>> +typedef struct {
> 
> Nit: No double blank lines please.
> 

Fixed.

>> +/* Resolve the identifier if it matches the given symbol.  */
>> +static bool
>> +resolve_if_matching (const char *identifier,
>> +		     const oav2_identifier_t *known_identifier,
>> +		     arg_t *val_out)
> 
> As to the comment: What does "given symbol" refer to? I'm inclined to guess
> it's "known_identifier", but then the comment may better say "... matches
> the given one"?
> 

Fixed.

>> +#if (TC_OBJ_ATTR_v1)
>> +/* Look up attribute tags defined in the backend (object attribute v1).  */
>> +static bool
>> +obj_attr_v1_lookup_known_attr_tag_symbol
>> +  (const char *identifier  ATTRIBUTE_UNUSED,
> 
> Nit: One too many padding blanks.
> 

Fixed.

>> +/* In the context of object attributes, an identifier is defined with the
>> +   following lexical constraint: [a-zA-z_][a-zA-Z0-9_].  An identifier can
> 
> Maybe better add a * or + at the end of the pattern, to make clear identifiers
> can consist of other than exactly two characters?
> 

Yes, there was a * missing. 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.  */
> 
> What is match_identifier()? Is this stale and means to say is_identifier_beginner()?
> 

Yes, it was stale. I meant is_identifier_beginner().
Fixed.

>> +      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;
> 
> With this, ...
> 
>> +      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;
> 
> ... why this?
> 

Indeed it is not needed, it is a duplicate and can be removed.
However, on the previous occurrence, it lacks 'arg_out->vtype = VALUE_UNDEFINED;' so I will move it next the previous occurrence of '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 success = extract_integer_literal (arg_out,
>> +					  (low_ttype & UNSIGNED_INTEGER),
>> +					  &signedness_issue);
>> +  if (success && (low_ttype & (UNSIGNED_INTEGER | SIGNED_INTEGER)))
>> +    return true;
>> +
>> +  char sbuffer[100];
>> +  if (success)
>> +    as_bad (_("unexpected integer '%lu', expected %s instead"),
>> +	    arg_out->val.u64,
> 
> As indicated before, %lu and alike aren't suitable to print uint64_t values.
> 

Fixed.

>> +/* 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 ())
>> +      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 ())
>> +      goto bad;
>> +
>> +    if (n >= LIST_MAX_SIZE)
>> +      {
>> +	as_bad ("too many arguments for a list (max: %lu)", LIST_MAX_SIZE);
> 
> %zu as LIST_MAX_SIZE is size_t.
> 

Fixed.

>> +/* 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, ...)
>> +{
>> +  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 ())
>> +	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.u64);
> 
> Nit: [n - 1] please.
> 

Fixed.

>> +#elif (TC_OBJ_ATTR_v1)
>> +	    = obj_attr_v1_get_arg_type (stdoutput, vendor,
>> +					args_out[n-1].val.u64);
> 
> Again.
> 

Fixed.

> As I'd like to save another round trip: Okay with all remarks suitably
> addressed (either verbally or by making respective adjustments). And again
> at least an Arm64 will be needed here as well (if you haven't got one
> already).
> 
> Jan

Matthieu