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

[Jan Beulich]

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.

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

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

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

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

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

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

> +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 (&notes, 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.

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

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

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

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

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

> +/* 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. I wonder if the function can be called at all when n == 0. Hmm, ...

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

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

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

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

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

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

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

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

> +#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. (Btw, why not obj_attr_v2_process_attribute()?)

Jan