[PATCH v9 04/19] gas: implement parsing of object attributes v2
Matthieu Longo
matthieu.longo@arm.com
Tue Nov 4 11:52:50 GMT 2025
On 2025-10-24 15:19, Jan Beulich wrote:
> On 01.09.2025 18:56, Matthieu Longo wrote:
>> --- a/bfd/elf-attrs.c
>> +++ b/bfd/elf-attrs.c
>> @@ -255,8 +255,147 @@ 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, 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. */
>> +static const obj_attr_info_t known_attrs_gnu_testing[] =
>> +{
>> + {
>> + .tag = {"GNUTestTag_0", 0},
>> + .default_value.uint_val = 0,
>> + },
>> + {
>> + .tag = {"GNUTestTag_1", 1},
>> + .default_value.uint_val = 0,
>> + },
>> +};
>
> While discussing v8 you confirmed that these aren't part of the spec. Afaict the
> comment didn't change, though, and hence I'm still wondering where "known" comes
> from.
>
I mean "known" from the perspective of GNU binutils. The "known" here
does not relate to any spec. Depending on whether a tag is known or
unknown, ld's processing will vary between merging, dropping, and errorring.
Also in the context of the GNU testing tags, the tags above 1 are
considered "unknown", which means that ld can drop them if they are
inside an optional subsection, whereas ld will raise an error in a
required subsection.
I am open to change the name if you don't like it.
>> +/* List of known GNU subsections.
>> + Note: this list needs to be sorted. */
>> +static const known_subsection_v2 obj_attr_v2_known_gnu_subsections[] =
>> +{
>> + {
>> + /* Note: the currently set values for the subsection name, its optionality,
>> + and encoding are irrelevant for a testing subsection. These values are
>> + unused. This entry is only a placeholder for list of known GNU testing
>> + tags. */
>> + .subsec_name = NULL,
>
> When using dedicated initializers, NULL and 0 (and false) initializers can be
> omitted, and imo in most cases should be.
>
And so how does it work ? If you omit it, is it initialized to 0 or
uninitialized ?
The fact that I am wondering it means that it creates ambiguity. Then I
need to figure it out by searching what the C specs say.
Expressiveness is better than implicitness when the resulting behavior
is not obvious.
I am understanding from where you are coming from thanks to your
explanation of the Misra criteria. However, I am not convinced of the
benefit of this specific change.
>> +/* Search for a subsection matching NAME in the list of subsections known from
>> + bfd (generic or backend-specific). Return the subsection information if it
>> + is found, or NULL otherwise. */
>> +const known_subsection_v2 *
>> +obj_attr_v2_identify_subsection (const struct elf_backend_data *bed,
>> + const char *name)
>> +{
>> + /* Check known backend subsections. */
>> + const known_subsection_v2 *known_subsections
>> + = bed->obj_attr_v2_known_subsections;
>> + const size_t known_subsections_size = bed->obj_attr_v2_known_subsections_size;
>> +
>> + for (unsigned i = 0; i < known_subsections_size; ++i)
>> + {
>> + int cmp = strcmp (known_subsections[i].subsec_name, name);
>> + if (cmp == 0)
>> + return &known_subsections[i];
>> + else if (cmp > 0)
>
> Another such "else" (and more further down).
>
Given the discussion in patch 3/19, do you still request to change it ?
>> +/* To-string function for the pair <SUBSEC, TAG>.
>> + Returns the attribute information associated to TAG if it is found,
>> + or "Tag_unknown_<N>" otherwise. */
>> +const char *
>> +obj_attr_v2_tag_to_string (const struct elf_backend_data *bed,
>> + const char *subsec_name,
>> + obj_attr_tag_t tag)
>> +{
>> + const obj_attr_info_t *attr_info
>> + = obj_attr_v2_find_known_by_tag (bed, subsec_name, tag);
>> + if (attr_info != NULL)
>> + return attr_info->tag.name;
>> +
>> + /* 23 because "Tag_unknown_" + max(uint32_t), i.e 4294967295 + '\0'. */
>> + static char tag_s[23] = {'\0'};
>
> The initializer is not only pointless, but potentially misleading: On the
> 2nd and later invocations that's not what the variable holds upon entry
> into the function.
>
Agreed.
> There's also a more general problem here: Elsewhere iirc there's effort
> to make libbfd a proper library, i.e. usable by multiple threads. This is
> a step backwards in that regard.
>
If multithreading is a thing for binutils, I understand the issue.
I replaced it by a dynamic allocation in the next revision (see below).
/* To-string function for the pair <SUBSEC, TAG>.
Returns the attribute information associated to TAG if it is found,
or "Tag_unknown_<N>" otherwise. */
const char *
obj_attr_v2_tag_to_string (const struct elf_backend_data *bed,
const char *subsec_name,
obj_attr_tag_t tag)
{
const obj_attr_info_t *attr_info
= obj_attr_v2_find_known_by_tag (bed, subsec_name, tag);
if (attr_info != NULL)
return xstrdup (attr_info->tag.name);
/* 23 because "Tag_unknown_" + max(uint32_t), i.e 4294967295 + '\0'. */
char *tag_s = xmalloc (23);
snprintf (tag_s, 23, "Tag_unknown_%u", tag);
return tag_s;
}
And all the call sites have a corresponding free().
>> --- a/bfd/elf-bfd.h
>> +++ b/bfd/elf-bfd.h
>> @@ -1656,6 +1656,12 @@ struct elf_backend_data
>> /* Encode the object attributes version into the output object. */
>> uint8_t (*obj_attrs_version_enc) (obj_attr_version_t);
>>
>> + /* The known subsections and attributes (v2 only). */
>> + const known_subsection_v2 *obj_attr_v2_known_subsections;
>> +
>> + /* The size of the array of known subsections. */
>> + const size_t obj_attr_v2_known_subsections_size;
>> +
>> /* This function determines the order in which any attributes are
>> written. It must be defined for input in the range
>> LEAST_KNOWN_OBJ_ATTRIBUTE..NUM_KNOWN_OBJ_ATTRIBUTES-1 (this range
>> @@ -3092,6 +3098,7 @@ extern obj_attr_version_t _bfd_obj_attrs_version_dec (uint8_t);
>> extern uint8_t _bfd_obj_attrs_version_enc (obj_attr_version_t);
>> extern bfd_vma bfd_elf_obj_attr_size (bfd *);
>> extern void bfd_elf_set_obj_attr_contents (bfd *, bfd_byte *, bfd_vma);
>> +extern obj_attribute *elf_new_obj_attr (bfd *, obj_attr_vendor_t, obj_attr_tag_t);
>> extern int bfd_elf_get_obj_attr_int (bfd *, obj_attr_vendor_t, obj_attr_tag_t);
>> extern obj_attribute *bfd_elf_add_obj_attr_int
>> (bfd *, obj_attr_vendor_t, obj_attr_tag_t, unsigned int);
>
> When all adjacent functions have a bfd_ (or _bfd_) prefix, why would the new one
> not have one as well?
>
No good reason.
Fixed in the next revision.
>> --- 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,63 @@ _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 needs to be sorted. */
>> +static const obj_attr_info_t known_attrs_aeabi_feature_and_bits[] =
>> +{
>> + {
>> + .tag = {"Tag_Feature_BTI", Tag_Feature_BTI},
>> + .default_value.uint_val = 0,
>> + },
>> + {
>> + .tag = {"Tag_Feature_PAC", Tag_Feature_PAC},
>> + .default_value.uint_val = 0,
>> + },
>> + {
>> + .tag = {"Tag_Feature_GCS", Tag_Feature_GCS},
>> + .default_value.uint_val = 0,
>> + },
>> +};
>> +
>> +/* 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 needs to be sorted. */
>> +static const obj_attr_info_t known_attrs_aeabi_pauthabi[] =
>> +{
>> + {
>> + .tag = {"Tag_PAuth_Platform", Tag_PAuth_Platform},
>> + .default_value.uint_val = 0,
>> + },
>> + {
>> + .tag = {"Tag_PAuth_Schema", Tag_PAuth_Schema},
>> + .default_value.uint_val = 0,
>> + },
>> +};
>> +
>> +/* List of known subsections.
>> + Note: this array is exported by the backend, and needs to be sorted. */
>
> Sorted by what criteria?
>
For the attributes, the corresponding integer value (=the key) in the
specs. That's why Tag_Feature_GCS is after Tag_Feature_PAC.
In the next revision, I changed the sentence in both comments to:
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)".
For the subsections, I also changed the comment to:
This array is exported by the backend, and has to be sorted using the
same criteria as in _bfd_elf_obj_attr_subsection_v2_cmp().
Same comments for the GNU testing attributes and subsections.
diff --git a/bfd/elf-attrs.c b/bfd/elf-attrs.c
index df3d133842f..0d0afb99f92 100644
--- a/bfd/elf-attrs.c
+++ b/bfd/elf-attrs.c
@@ -259,7 +259,8 @@ bfd_elf_set_obj_attr_contents (bfd *abfd, bfd_byte
*buffer, bfd_vma size)
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. */
+ be set to obj_attr_subsection_v2_unknown.
+ Note: the array below has to be sorted by the tag's integer value. */
static const obj_attr_info_t known_attrs_gnu_testing[] =
{
{
@@ -273,7 +274,8 @@ static const obj_attr_info_t known_attrs_gnu_testing[] =
};
/* List of known GNU subsections.
- Note: this list needs to be sorted. */
+ Note: this array has to be sorted using the same criteria as in
+ _bfd_elf_obj_attr_subsection_v2_cmp(). */
static const known_subsection_v2 obj_attr_v2_known_gnu_subsections[] =
{
{
>> --- a/bfd/elfxx-target.h
>> +++ b/bfd/elfxx-target.h
>> @@ -563,6 +563,12 @@
>> #ifndef elf_backend_obj_attrs_version_enc
>> #define elf_backend_obj_attrs_version_enc _bfd_obj_attrs_version_enc
>> #endif
>> +#ifndef elf_backend_obj_attr_v2_known_subsections
>> +#define elf_backend_obj_attr_v2_known_subsections NULL
>> +#endif
>> +#ifndef elf_backend_obj_attr_v2_known_subsections_size
>> +#define elf_backend_obj_attr_v2_known_subsections_size 0
>> +#endif
>> #ifndef elf_backend_obj_attrs_order
>> #define elf_backend_obj_attrs_order NULL
>> #endif
>
> While you use tabs, adjacent entries use blanks after #ifndef.
>
Fixed in the next revision.
>> --- a/gas/config/obj-elf-attr.c
>> +++ b/gas/config/obj-elf-attr.c
>> @@ -22,8 +22,70 @@
>>
>> #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;
>> +
>> +typedef enum {
>> + OAv2_ASM_ID_VALUE_UNDEFINED = 0,
>> + OAv2_ASM_ID_VALUE_U8,
>> + OAv2_ASM_ID_VALUE_BOOL,
>> +} oav2_identifier_variant_type_info;
>> +
>> +typedef struct {
>> + oav2_identifier_variant_value val;
>> + oav2_identifier_variant_type_info vtype;
>> +} oav2_identifier_variant_t;
>> +
>> +typedef struct {
>> + const char *const name;
>> + const oav2_identifier_variant_t value;
>> +} oav2_identifier_t;
>> +
>> +/* A variant type to store the argument values of an assembly directive. */
>> +struct arg_variant_t;
>> +
>> +typedef struct {
>> + size_t len;
>> + struct arg_variant_t *elts;
>> +} arg_variant_list;
>> +
>> +typedef union {
>> + const char *string;
>> +#if (TC_OBJ_ATTR_v1)
>> + uint32_t u32;
>> +#endif
>
> How does v1 come into play here?
>
There is no good reason to keep this.
The tag is a uint32_t so I guess that at the beginning, I wanted to save
the parsed value as a uint32_t. Then I moved everything to use uint64_t,
keys and associated unsigned integer values.
Since it is inconsistent, I propose you to move the definition of
obj_attr_tag_t to uint64_t with the following changes (at the HEAD of my
branch, so the diff will be splitted to the respective patches).
diff --git a/bfd/elf-attrs.c b/bfd/elf-attrs.c
index eebd761d69c..273acb239d5 100644
--- a/bfd/elf-attrs.c
+++ b/bfd/elf-attrs.c
@@ -741,9 +741,12 @@ obj_attr_v2_tag_to_string (const struct
elf_backend_data *bed,
if (attr_info != NULL)
return xstrdup (attr_info->tag.name);
- /* 23 because "Tag_unknown_" + max(uint32_t), i.e 4294967295 + '\0'. */
- char *tag_s = xmalloc (23);
- snprintf (tag_s, 23, "Tag_unknown_%u", tag);
+ /* 33 because:
+ "Tag_unknown_" (12)
+ + max(uint64_t) (20) i.e 18,446,744,073,709,551,615
+ + '\0' (1) */
+ char *tag_s = xmalloc (33);
+ snprintf (tag_s, 33, "Tag_unknown_%lu", tag);
return tag_s;
}
diff --git a/bfd/elf-attrs.h b/bfd/elf-attrs.h
index c17e610c150..1a819d98377 100644
--- a/bfd/elf-attrs.h
+++ b/bfd/elf-attrs.h
@@ -68,7 +68,7 @@ typedef enum obj_attr_v2_status
obj_attr_v2_ok,
} obj_attr_v2_status;
-typedef uint32_t obj_attr_tag_t;
+typedef uint64_t obj_attr_tag_t;
typedef struct obj_attr_v2 {
/* The name/tag of an attribute. */
diff --git a/binutils/readelf.c b/binutils/readelf.c
index 1a29984b49a..8e3bfcac5d9 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -19973,7 +19973,7 @@ display_attr_v2 (unsigned char *cursor,
if (tag_info != NULL)
return display_arch_attr (cursor, end, tag_info, value_encoding);
- printf (" Tag_unknown_%u: ", tag);
+ printf (" Tag_unknown_%lu: ", tag);
return oav2_display_attr_value (cursor, end, value_encoding);
}
diff --git a/gas/config/obj-elf-attr.c b/gas/config/obj-elf-attr.c
index e386ee4ac33..d051b8c9bd5 100644
--- a/gas/config/obj-elf-attr.c
+++ b/gas/config/obj-elf-attr.c
@@ -57,9 +57,6 @@ typedef struct {
typedef union {
const char *string;
-#if (TC_OBJ_ATTR_v1)
- uint32_t u32;
-#endif
uint64_t u64;
int64_t i64;
arg_variant_list list;
@@ -813,10 +810,10 @@ vendor_attribute_parse_args (obj_attr_vendor_t
vendor ATTRIBUTE_UNUSED,
= (subsec != NULL)
? obj_attr_v2_get_arg_type (subsec->encoding)
: obj_attr_v1_get_arg_type (stdoutput, vendor,
- args_out[n-1].val.u32);
+ args_out[n-1].val.u64);
#elif (TC_OBJ_ATTR_v1)
= obj_attr_v1_get_arg_type (stdoutput, vendor,
- args_out[n-1].val.u32);
+ args_out[n-1].val.u64);
#else /* TC_OBJ_ATTR_v2 */
= obj_attr_v2_get_arg_type (subsec->encoding);
#endif
@@ -970,7 +967,7 @@ vendor_subsection_parse_args (unsigned int nargs, ...)
/* Record an attribute (object attribute v2 only). */
static void
-obj_attr_v2_record (uint64_t key, arg_t *arg_val)
+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);
@@ -999,7 +996,7 @@ obj_attr_v2_record (uint64_t key, arg_t *arg_val)
&& 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 %u cannot be redefined"), recorded_attr->tag);
+ as_bad (_("attribute %lu cannot be redefined"), recorded_attr->tag);
skip_recording = true;
}
>> + uint64_t u64;
>> + int64_t i64;
>> + arg_variant_list list;
>> +} arg_variant_value;
>> +
>> +typedef enum {
>> + VALUE_UNDEFINED = 0,
>> +#if (TC_OBJ_ATTR_v1)
>> + VALUE_U32,
>> +#endif
>> + VALUE_U64,
>> + VALUE_I64,
>> + VALUE_UNSIGNED_INTEGER = VALUE_U64,
>> + VALUE_SIGNED_INTEGER = VALUE_I64,
>> + VALUE_STRING,
>> + VALUE_LIST,
>> + VALUE_OPTIONAL_ABSENT,
>> +} arg_variant_type_info;
>> +
>> +typedef struct arg_variant_t {
>> + arg_variant_value val;
>> + arg_variant_type_info vtype;
>> +} arg_variant_t;
>> +
>> +typedef arg_variant_t arg_t;
>> +
>> #define skip_whitespace(str) do { if (is_whitespace (*(str))) ++(str); } while (0)
>>
>> static inline bool
>> @@ -38,6 +100,8 @@ skip_past_char (char **str, char c)
>> }
>> #define skip_past_comma(str) skip_past_char (str, ',')
>>
>> +#if (TC_OBJ_ATTR_v1)
>> +
>> /* A list of attributes that have been explicitly set by the assembly code.
>> VENDOR is the vendor id, BASE is the tag shifted right by the number
>> of bits in MASK, and bit N of MASK is set if tag BASE+N has been set. */
>> @@ -120,6 +184,997 @@ oav1_attr_seen (obj_attr_vendor_t vendor, obj_attr_tag_t tag)
>> return false;
>> }
>>
>> +#endif /* TC_OBJ_ATTR_v1 */
>> +
>> +/* Expected argument tokens for object attribute directives. */
>> +typedef enum {
>> + /* Base types. */
>> + IDENTIFIER = 0x1,
>> + UNSIGNED_INTEGER = 0x2,
>> + SIGNED_INTEGER = 0x4,
>> + STRING = 0x8,
>> + LIST = 0x10,
>> + LT_MASK = 0xFF,
>> + /* Higher types. */
>> + SUBSECTION_NAME = 0x100,
>> + SUBSECTION_OPTION_1 = 0x200,
>> + SUBSECTION_OPTION_2 = 0x400,
>> + ATTRIBUTE_KEY = 0x800,
>> + ATTRIBUTE_VALUE = 0x1000,
>> + HT_MASK = 0xFF00,
>> +} arg_token_t;
>> +
>> +/* Free an arguments list of size N. */
>> +static void
>> +args_list_free (arg_t *args, size_t n)
>> +{
>> + for (size_t i = 0; i < n; ++i)
>> + if (args[i].vtype == VALUE_STRING)
>> + free ((void *) args[i].val.string);
>> + else if (args[i].vtype == VALUE_LIST)
>> + args_list_free (args[i].val.list.elts, args[i].val.list.len);
>> + free (args);
>> +}
>> +
>> +/* Extract a string literal ("[^.]+") from the input. */
>> +static bool
>> +extract_string_literal (arg_t *arg_out)
>> +{
>> + skip_whitespace (input_line_pointer);
>> +
>> + if (*input_line_pointer != '"')
>> + {
>> + as_bad ("missing '\"', expected a string literal");
>> + return false;
>> + }
>> +
>> + int len;
>> + char *obstack_buf = demand_copy_C_string (&len);
>
> The function already skips whitespace and checks for a starting '"', so
> why open-code that here again?
>
I can remove the previous lines, but I cannot detect missing closing '"'
in my understanding. So I am losing the error message too.
The issue with the existing parsing utility functions is that they do
too much, or not enough. And their interface is not necessarily friendly
for what I want to do.
Would it be acceptable to keep it unchanged given the explanation above ?
>> + 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;
>> +}
>> +
>> +/* 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. */
>> +static bool
>> +extract_integer_literal (arg_t *arg_out,
>> + bool want_unsigned)
>> +{
>> + expressionS exp;
>> + expression_and_evaluate (&exp);
>> + if (exp.X_op != O_constant)
>> + {
>> + as_bad (_("invalid value, expected an integer literal"));
>> + goto bad;
>> + }
>> +
>> + int64_t val = (int64_t) exp.X_add_number;
>> + if (want_unsigned)
>> + {
>> + if (! exp.X_unsigned && val < 0)
>> + {
>> + as_bad (_("invalid negative value %" PRId64 ", expected an unsigned "
>> + "integer"), val);
>> + goto bad;
>> + }
>> + arg_out->val.u64 = val;
>> + arg_out->vtype = VALUE_UNSIGNED_INTEGER;
>> + }
>> + else
>> + {
>> + arg_out->val.i64 = val;
>> + arg_out->vtype = VALUE_SIGNED_INTEGER;
>> + }
>> + return true;
>> +
>> + bad:
>> + ignore_rest_of_line ();
>> + return false;
>> +}
>> +
>> +/* Extract an identifier based on the provided character matcher. */
>> +static bool
>> +extract_identifier (bool (*char_predicate) (char), arg_t *arg_out)
>> +{
>> + const char *s = input_line_pointer;
>> + unsigned int i = 0;
>> + for (; char_predicate (*input_line_pointer); ++input_line_pointer)
>> + i++;
>> + if (i == 0)
>> + {
>> + as_bad (_("invalid value '%c', expected an identifier"),
>> + *input_line_pointer);
>> + ignore_rest_of_line ();
>> + return false;
>> + }
>> +
>> + arg_out->vtype = VALUE_STRING;
>> + arg_out->val.string = xmemdup0 (s, i);
>> + return true;
>> +}
>> +
>> +#if (TC_OBJ_ATTR_v2)
>> +/* Resolve the identifier if it matches a known tag. */
>> +static bool
>> +resolve_if_matching_known_tag (const char *identifier,
>> + const obj_attr_tag_info_t *known_identifier,
>> + arg_t *val_out)
>> +{
>> + if (strcmp (known_identifier->name, identifier) != 0)
>> + return false;
>> +
>> + /* Free the identifier since we found the value. */
>> + free ((void *) val_out->val.string);
>
> The identifier you found is pointed to by "identifier", not
> val_out->val.string. What's the (hidden) relation between the two that
> justfies this freeing?
>
extract_identifier () sets the identifier value, i.e. the string in
'arg_out->val.string'. When the identifier is resolved, the string
corresponding to this identifier can be thrown away, and its owner is
'val_out' passed as argument. Hence the free (val_out->val.string).
See one of the next comments in lookup_symbol_table() where I proposed a
different approach that eliminates this confusion.
>> + val_out->val.u64 = known_identifier->value;
>> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
>> + return true;
>> +}
>> +
>> +/* 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)
>> +{
>> + if (strcmp (known_identifier->name, identifier) != 0)
>> + return false;
>> +
>> + /* Free the identifier since we found the value. */
>> + free ((void *) val_out->val.string);
>
> Same question here, obviously.
>
See previous answer.
>> + switch (known_identifier->value.vtype)
>> + {
>> + case OAv2_ASM_ID_VALUE_U8:
>> + val_out->val.u64 = known_identifier->value.val.b;
>> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
>> + break;
>> + case OAv2_ASM_ID_VALUE_BOOL:
>> + val_out->val.u64 = known_identifier->value.val.u8;
>> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
>> + break;
>> + default:
>> + abort ();
>> + }
>
> How come bool and u8 are all that need handling? Surely the reader could be
> helped by having a comment tio this effect?
>
Sorry, I don't understand the questions.
The only known identifiers are declared in
known_identifiers_subsection_optional and
known_identifiers_subsection_encoding.
Only bool and u8 are required, hence 3 values in
oav2_identifier_variant_type_info (u8, bool, undefined).
>> + return true;
>> +}
>> +#endif /* TC_OBJ_ATTR_v2 */
>> +
>> +#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,
>> + arg_token_t token_type,
>> + arg_t *val_out)
>> +{
>> +#ifndef CONVERT_SYMBOLIC_ATTRIBUTE
>> +#define CONVERT_SYMBOLIC_ATTRIBUTE(a) -1
>> + (void) identifier;
>
> As indicated in v8 review - the common way is use of ATTRIBUTE_UNUSED.
>
Fixed but unsure about the formatting (see below).
diff --git a/gas/config/obj-elf-attr.c b/gas/config/obj-elf-attr.c
index 82ec6a79783..b574d46b80c 100644
--- a/gas/config/obj-elf-attr.c
+++ b/gas/config/obj-elf-attr.c
@@ -354,15 +354,16 @@ resolve_if_matching (const char *identifier,
#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,
- arg_token_t token_type,
- arg_t *val_out)
-{
+obj_attr_v1_lookup_known_attr_tag_symbol (
#ifndef CONVERT_SYMBOLIC_ATTRIBUTE
-#define CONVERT_SYMBOLIC_ATTRIBUTE(a) -1
- (void) identifier;
+ #define CONVERT_SYMBOLIC_ATTRIBUTE(a) -1
+ const char *identifier ATTRIBUTE_UNUSED,
+#else
+ const char *identifier,
#endif
-
+ arg_token_t token_type,
+ arg_t *val_out)
+{
gas_assert (token_type & UNSIGNED_INTEGER);
int tag = CONVERT_SYMBOLIC_ATTRIBUTE (identifier);
>> +#endif
>> +
>> + gas_assert (token_type & UNSIGNED_INTEGER);
>> +
>> + int tag = CONVERT_SYMBOLIC_ATTRIBUTE (identifier);
>> + if (tag < 0)
>> + return false;
>> + val_out->val.u64 = tag;
>> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
>> + return true;
>> +}
>> +#endif /* TC_OBJ_ATTR_v1 */
>> +
>> +#if (TC_OBJ_ATTR_v2)
>> +/* Look up attribute tags defined in the backend (object attribute v2). */
>> +static bool
>> +obj_attr_v2_lookup_known_attr_tag_symbol (const char *identifier,
>> + arg_token_t token_type,
>> + arg_t *val_out)
>> +{
>> + obj_attr_subsection_v2 *subsec = elf_obj_attr_subsections (stdoutput).last;
>> + /* If there is no current subsection, this function was called wrongly before
>> + setting one (usually via the subsection directive). */
>> + gas_assert (subsec != NULL);
>> +
>> + /* An attribute tag is an unsigned integer, so the expected token type should
>> + always have the base type UNSIGNED_INTEGER. Otherwise, this function was
>> + called incorrectly. */
>> + gas_assert (token_type & UNSIGNED_INTEGER);
>> +
>> + bool resolved = false;
>> + const struct elf_backend_data *be = get_elf_backend_data (stdoutput);
>> + const known_subsection_v2 *known_subsec
>> + = obj_attr_v2_identify_subsection (be, subsec->name);
>> + if (known_subsec != NULL)
>> + {
>> + for (size_t i = 0; i < known_subsec->len && ! resolved; ++i)
>> + resolved
>> + = resolve_if_matching_known_tag (identifier,
>> + &known_subsec->known_attrs[i].tag,
>> + val_out);
>> + }
>> +
>> + if (resolved)
>> + /* An attribute tag is an unsigned integer, so the type of the found value
>> + should be VALUE_UNSIGNED_INTEGER. Otherwise, check if you set correctly
>> + the type of the value associated to the symbol. */
>> + gas_assert (val_out->vtype == VALUE_UNSIGNED_INTEGER);
>> +
>> + return resolved;
>> +}
>> +#endif /* TC_OBJ_ATTR_v2 */
>> +
>> +/* Look up known symbols, and try to resolve the given identifier. */
>> +static bool
>> +lookup_known_symbols (const char *identifier,
>> + arg_token_t token_type,
>> + arg_t *val_out)
>> +{
>> + if (identifier == NULL)
>> + return false;
>> +
>> + /* The identifier should match the value in val_out. */
>> + gas_assert (val_out->val.string == identifier);
>> +
>> + arg_token_t high_ttype = (token_type & HT_MASK);
>> +
>> +#if (TC_OBJ_ATTR_v2)
>> + static const oav2_identifier_t known_identifiers_subsection_optional[] = {
>> + { "optional", {
>> + .val.b = true,
>> + .vtype = OAv2_ASM_ID_VALUE_BOOL
>> + }
>> + },
>> + { "required", {
>> + .val.b = false,
>> + .vtype = OAv2_ASM_ID_VALUE_BOOL
>> + }
>> + },
>> + };
>
> Interesting - the "keywords" a user would specify need to be all lowercase here,
> but ...
>
>> + static const oav2_identifier_t known_identifiers_subsection_encoding[] = {
>> + { "ULEB128", {
>> + .val.u8 = obj_attr_encoding_v2_to_u8 (OA_ENC_ULEB128),
>> + .vtype = OAv2_ASM_ID_VALUE_U8
>> + }
>> + },
>> + { "uleb128", {
>> + .val.u8 = obj_attr_encoding_v2_to_u8 (OA_ENC_ULEB128),
>> + .vtype = OAv2_ASM_ID_VALUE_U8
>> + }
>> + },
>> + { "NTBS", {
>> + .val.u8 = obj_attr_encoding_v2_to_u8 (OA_ENC_NTBS),
>> + .vtype = OAv2_ASM_ID_VALUE_U8
>> + }
>> + },
>> + { "ntbs", {
>> + .val.u8 = obj_attr_encoding_v2_to_u8 (OA_ENC_NTBS),
>> + .vtype = OAv2_ASM_ID_VALUE_U8
>> + }
>> + },
>> + };
>
> ... can be all low or all upper case here (but then not e.g. Uleb128). Wouldn't
> case-insensitive comparison be preferable, leaving more freedom to users?
>
For reference, here below is the discussion we had about it in revision
8, patch 4.
>>>> --- a/gas/doc/c-aarch64.texi
>>>> +++ b/gas/doc/c-aarch64.texi
>>>> @@ -479,6 +479,33 @@ The AArch64 architecture uses @sc{ieee} floating-point numbers.
>>>>
>>>> @c AAAAAAAAAAAAAAAAAAAAAAAAA
>>>>
>>>> +@cindex @code{.aeabi_subsection} directive, AArch64
>>>> +@item .aeabi_subsection @var{name}, @var{comprehension}, @var{encoding}
>>>> +Create or switch the current object attributes subsection to @var{name}. Valid
>>>> +values for @var{name} are following the pattern @code{[a-zA-Z0-9_-]+}.
>>>> +
>>>> +The subsection property @var{comprehension} determines how a program processing
>>>> +the attributes handles attributes that it does not recognize (perhaps because
>>>> +the object file was generated by a different version of the toolchain). A
>>>> +subsection that is marked @code{optional} can be skipped if it is not
>>>> +understood. A subsection marked @code{required} implies that information
>>>> +conveyed by the attribute is required for correct processing of the object file;
>>>> +a fatal diagnostic must be generated if a tool does not recognize either the tag
>>>> +or the value associated with it.
>>>> +
>>>> +@var{encoding} specifies the expected encoding of the attributes recorded in the
>>>> +subsection. Currently supported values are @code{ULEB128} and @code{NTBS}
>>>> +(null-terminated byte string).
>>>> +
>>>> +@cindex @code{.aeabi_attribute} @var{tag}, @var{value}
>>>> +@item .aeabi_attribute @var{tag}, @var{value}
>>>> +Create an attribute with the pair @var{tag}, @var{value} in the current
>>>> +subsection. @var{tag} can either be an integer value, or a known named key.
>>>> +@var{value} can either be an integer or a string.
>>>> +
>>>> +The complete list of subsections and tags supported on AArch64 is documented
>>>> +in @cite{Build Attributes for the Arm 64-bit Architecture (AArch64)}.
>>>
>>> Throughout you additions here I think it would be helpful if it was clarified
>>> which of the items are case-sensitive, and which ones are not. For example
>>> you spell "optional" and "required" all lower-case, but "ULEB128" and "NTBS"
>>> all upper-case. Without disambiguation it doesn't become clear whether that
>>> has any particular significance.
>>>
>>
>> I personally don't like this choice between capitalized and
>> non-capitalized version for the encoding. The reason why capitalized
>> version exists for encoding is because ULEB128 and NTBS are acronyms,
>> and it is more natural in English to use the capitalized version for
>> those. I would have preferred only the capitalized version to exist, but
>> the spec proposed it so Gas supports it.
>>
>> I wrote this documentation in a prescriptive way and expect people to
>> use the version from the documentation. I omitted on purpose the
>> possibility of using lower case for the encoding because having several
>> options opens the door to inconsistencies across a code base.
>>
>> If you disagree, I can add it.
>
> Imo you want to state precisely what's accepted. If case doesn't matter,
> people should be free to use spellings of their choice. No matter what
> you or I like or dislike.
>
In the next revision, the identifiers are changed to lower case before
the comparison.
diff --git a/gas/config/obj-elf-attr.c b/gas/config/obj-elf-attr.c
index b574d46b80c..794a20f2505 100644
--- a/gas/config/obj-elf-attr.c
+++ b/gas/config/obj-elf-attr.c
@@ -327,7 +327,24 @@ resolve_if_matching (const char *identifier,
const oav2_identifier_t *known_identifier,
arg_t *val_out)
{
- if (strcmp (known_identifier->name, identifier) != 0)
+ /* Normalize the identifier before comparison by making it all lower
case. */
+ char normalized_identifier[100];
+ const char *identifier_ptr = identifier;
+ unsigned int i = 0;
+ while (*identifier_ptr != '\0' && i < sizeof (normalized_identifier))
+ {
+ normalized_identifier[i++] = TOLOWER (*identifier_ptr++);
+ //++i;
+ //++identifier_ptr
+ }
+ if (i >= sizeof (normalized_identifier))
+ /* Identifier is too long. */
+ return false;
+ else if (*identifier_ptr == '\0')
+ normalized_identifier[i] = '\0';
+
+ /* Comparison with normalized identifier. */
+ if (strcmp (known_identifier->name, normalized_identifier) != 0)
return false;
/* Free the identifier since we found the value. */
@@ -444,21 +461,11 @@ lookup_known_symbols (const char *identifier,
};
static const oav2_identifier_t
known_identifiers_subsection_encoding[] = {
- { "ULEB128", {
- .val.u8 = obj_attr_encoding_v2_to_u8 (OA_ENC_ULEB128),
- .vtype = OAv2_ASM_ID_VALUE_U8
- }
- },
{ "uleb128", {
.val.u8 = obj_attr_encoding_v2_to_u8 (OA_ENC_ULEB128),
.vtype = OAv2_ASM_ID_VALUE_U8
}
},
- { "NTBS", {
- .val.u8 = obj_attr_encoding_v2_to_u8 (OA_ENC_NTBS),
- .vtype = OAv2_ASM_ID_VALUE_U8
- }
- },
{ "ntbs", {
.val.u8 = obj_attr_encoding_v2_to_u8 (OA_ENC_NTBS),
.vtype = OAv2_ASM_ID_VALUE_U8
>> +#endif /* TC_OBJ_ATTR_v2 */
>> +
>> + bool resolved = false;
>> +
>> +#if (TC_OBJ_ATTR_v2)
>> + if (high_ttype == SUBSECTION_OPTION_1 || high_ttype == SUBSECTION_OPTION_2)
>> + {
>> + const oav2_identifier_t *known_identifiers
>> + = (high_ttype == SUBSECTION_OPTION_1
>> + ? known_identifiers_subsection_optional
>> + : known_identifiers_subsection_encoding);
>> + const size_t N_identifiers
>> + = (high_ttype == SUBSECTION_OPTION_1
>> + ? ARRAY_SIZE (known_identifiers_subsection_optional)
>> + : ARRAY_SIZE (known_identifiers_subsection_encoding));
>> +
>> + for (size_t i = 0; i < N_identifiers && ! resolved; ++i)
>> + resolved = resolve_if_matching (identifier,
>> + &known_identifiers[i],
>> + val_out);
>> + }
>> + else
>> +#endif /* TC_OBJ_ATTR_v2 */
>> + if (high_ttype == ATTRIBUTE_KEY)
>> + {
>> + obj_attr_version_t version = elf_obj_attr_version (stdoutput);
>> +#if (TC_OBJ_ATTR_v1)
>> + if (version == OBJ_ATTR_V1)
>> + resolved = obj_attr_v1_lookup_known_attr_tag_symbol (identifier,
>> + token_type, val_out);
>> +#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)
>> + resolved = obj_attr_v2_lookup_known_attr_tag_symbol (identifier,
>> + token_type, val_out);
>> +#endif /* TC_OBJ_ATTR_v2 */
>> + else
>> + abort ();
>> + }
>> + else
>> + abort ();
>> +
>> + return resolved;
>> +}
>> +
>> +/* Look up the symbol table of this compilation unit, and try to resolve the
>> + given identifier. */
>> +static bool
>> +lookup_symbol_table (const char *identifier,
>> + const arg_token_t expected_ttype,
>> + arg_t *val_out)
>> +{
>> + if (identifier == NULL)
>> + return false;
>> +
>> + /* Note: signed integer are unsupported for now. */
>> + gas_assert (expected_ttype & UNSIGNED_INTEGER);
>> + /* The identifier should match the value in val_out. */
>> + gas_assert (val_out->val.string == identifier);
>
> Hmm, maybe herein lies part of the answer to an earlier question. Yet then why
> would those functions take redundant parameters?
>
The wanted interface for such a function is:
- identifier: IN
- expected_type: IN
- val_out: OUT
but I acknowledge that the free() in those functions is misleading.
What about this ?
diff --git a/gas/config/obj-elf-attr.c b/gas/config/obj-elf-attr.c
index 94dfe0024f7..53b69c2be97 100644
--- a/gas/config/obj-elf-attr.c
+++ b/gas/config/obj-elf-attr.c
@@ -310,9 +310,6 @@ resolve_if_matching_known_tag (const char *identifier,
if (strcmp (known_identifier->name, identifier) != 0)
return false;
- /* Free the identifier since we found the value. */
- free ((void *) val_out->val.string);
-
val_out->val.u64 = known_identifier->value;
val_out->vtype = VALUE_UNSIGNED_INTEGER;
return true;
@@ -344,9 +341,6 @@ resolve_if_matching (const char *identifier,
if (strcmp (known_identifier->name, normalized_identifier) != 0)
return false;
- /* Free the identifier since we found the value. */
- free ((void *) val_out->val.string);
-
switch (known_identifier->value.vtype)
{
case OAv2_ASM_ID_VALUE_U8:
@@ -438,9 +432,6 @@ lookup_known_symbols (const char *identifier,
if (identifier == NULL)
return false;
- /* The identifier should match the value in val_out. */
- gas_assert (val_out->val.string == identifier);
-
arg_token_t high_ttype = (token_type & HT_MASK);
#if (TC_OBJ_ATTR_v2)
@@ -529,8 +520,6 @@ lookup_symbol_table (const char *identifier,
/* Note: signed integer are unsupported for now. */
gas_assert (expected_ttype & UNSIGNED_INTEGER);
- /* The identifier should match the value in val_out. */
- gas_assert (val_out->val.string == identifier);
symbolS *symbolP = symbol_find (identifier);
if (symbolP == NULL)
@@ -540,10 +529,6 @@ lookup_symbol_table (const char *identifier,
return false;
valueT val = S_GET_VALUE (symbolP);
-
- /* Free the identifier since we found the value. */
- free ((void *) val_out->val.string);
-
val_out->val.u64 = val;
val_out->vtype = VALUE_UNSIGNED_INTEGER;
@@ -602,23 +587,31 @@ obj_attr_parse_arg (arg_token_t expected_ttype,
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 (arg_out->val.string, expected_ttype, arg_out))
- return true;
+ 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 (arg_out->val.string, low_ttype, arg_out))
- return true;
+ if (lookup_symbol_table (identifier, low_ttype, arg_out))
+ goto free_identifier;
- as_bad ("unknown identifier '%s'", arg_out->val.string);
- free ((void *) arg_out->val.string);
+ as_bad ("unknown identifier '%s'", identifier);
arg_out->val.string = NULL;
arg_out->vtype = VALUE_UNDEFINED;
- return false;
+ resolved = false;
+
+free_identifier:
+ free ((char *) identifier);
+ return resolved;
}
/* Trim white spaces before a parameter.
>> + symbolS *symbolP = symbol_find (identifier);
>> + if (symbolP == NULL)
>> + return false;
>> +
>> + if (! S_IS_DEFINED (symbolP))
>> + return false;
>> +
>> + valueT val = S_GET_VALUE (symbolP);
>> +
>> + /* Free the identifier since we found the value. */
>> + free ((void *) val_out->val.string);
>> +
>> + val_out->val.u64 = val;
>> + val_out->vtype = VALUE_UNSIGNED_INTEGER;
>> +
>> + return true;
>> +}
>> +
>> +/* Return true if the next characters are suspected to represent an integer
>> + literal. */
>> +static bool
>> +look_like_integer_literal (const char *cursor)
>> +{
>> + if (ISDIGIT (*cursor))
>> + return true;
>> + if (! (*cursor == '+' || *cursor == '-' || *cursor == '~'))
>> + return false;
>> + ++cursor;
>> + while (ISSPACE (*cursor))
>> + ++cursor;
>> + return ISDIGIT (*cursor);
>> +}
>
> Hmm, now that I look at the result, maybe I misled you. An "integer literal"
> would perhaps indeed not allow for ~ and also not for a blank between sign
> and first digit. Yet then, as before, I'm having trouble seeing why proper
> expressions (involving perhaps more than just integer literals, e.g. also
> parentheses) shouldn't be acceptable.
>
I would like to revisit this parsing a bit later, and simply eliminate
this function.
I will send a detailed reply for it in a separate email.
>> +/* Skip white spaces + parameter separator after a parameter.
>> + Error if it does not meet a parameter separator after a parameter. */
>> +static bool
>> +skip_whitespaces_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 %d"), n);
>
> Nit: %u please for unsigned int values.
>
Fixed in the next revision.
>> + return false;
>> + }
>> + return true;
>> +}
>> +
>> +/* Can parse a list of arguments with variable length. */
>> +static bool
>> +obj_attr_parse_args (arg_token_t expected_ttype,
>> + bool (*match_identifier) (char c),
>> + bool resolve_identifier,
>> + arg_t *arg_out)
>> +{
>> + if ((expected_ttype & LIST) == 0)
>> + return obj_attr_parse_arg (expected_ttype, match_identifier,
>> + resolve_identifier, false, arg_out);
>> +
>> + static const size_t LIST_MAX_SIZE = 2;
>> + arg_t *arg_list = xcalloc (LIST_MAX_SIZE, sizeof (arg_t));
>
> As said in reply to at least one of the v8 patches - please avoid
> sizeof(<type>) when a specific variable or expression is what you're after
> (here: *arg_list).
>
Fixed in the next revision.
>> +
>> + /* We don't want to support recursive lists. */
>> + expected_ttype &= ~LIST;
>> +
>> + size_t n = 0;
>> + do {
>> + if (! trim_whitespaces_before_param (0))
>> + goto bad;
>> +
>> + if (! obj_attr_parse_arg (expected_ttype, match_identifier,
>> + 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_whitespaces_past_comma (0))
>> + goto bad;
>> +
>> + if (n >= LIST_MAX_SIZE)
>> + {
>> + as_bad ("too many arguments for a list (max: %lu)", LIST_MAX_SIZE);
>> + goto bad;
>> + }
>> + } while (n < LIST_MAX_SIZE);
>> +
>> + arg_out->vtype = VALUE_LIST;
>> + arg_out->val.list.len = n;
>> + arg_out->val.list.elts = arg_list;
>> + return true;
>> +
>> + bad:
>> + args_list_free (arg_list, n);
>> + return false;
>> +}
>> +
>> +#if (TC_OBJ_ATTR_v2)
>> +static bool
>> +is_valid_boolean (uint64_t value)
>> +{
>> + return value == 0 || value == 1;
>> +}
>> +
>> +#define is_valid_comprehension is_valid_boolean
>> +
>> +static bool
>> +is_valid_encoding (uint64_t value)
>> +{
>> + value = obj_attr_encoding_v2_from_u8 (value);
>> + return OA_ENC_UNSET < value && value <= OA_ENC_MAX;
>> +}
>> +
>> +static bool
>> +match_subsection_identifier (char c)
>> +{
>> + return ISALNUM (c) || c == '_' || c == '-';
>> +}
>
> Why "match" and not "is", like others above are?
>
Fixed in the next revision.
>> +#endif /* TC_OBJ_ATTR_v2 */
>> +
>> +static bool
>> +match_symbol (char c)
>> +{
>> + return ISALNUM (c) || c == '_';
>> +}
>
> Same here then, obviously.
>
Fixed in the next revision.
I also fixed match_tag_identifier.
>> --- a/gas/config/obj-elf-attr.h
>> +++ b/gas/config/obj-elf-attr.h
>> @@ -23,12 +23,36 @@
>>
>> #include "as.h"
>>
>> +#ifndef TC_OBJ_ATTR_V1
>> +#define TC_OBJ_ATTR_V1 0
>> +#endif
>> +#ifndef TC_OBJ_ATTR_V2
>> +#define TC_OBJ_ATTR_V2 0
>> +#endif
>> +
>> +#if (TC_OBJ_ATTR_v1 || TC_OBJ_ATTR_v2)
>> + #define TC_OBJ_ATTR 1
>> +#endif
>
> Given this, ...
>
>> #ifdef TC_OBJ_ATTR
>>
>> +#if (TC_OBJ_ATTR_v1)
>> extern void oav1_attr_info_init (void);
>> extern void oav1_attr_info_exit (void);
>> extern bool oav1_attr_seen (obj_attr_vendor_t, obj_attr_tag_t);
>> +#endif /* TC_OBJ_ATTR_v1 */
>> +
>> +/* Object attributes parsers. */
>> +
>> +#if (TC_OBJ_ATTR_v1)
>> extern obj_attr_tag_t obj_attr_v1_process_attribute (obj_attr_vendor_t);
>> +#endif /* (TC_OBJ_ATTR_v1) */
>> +#if (TC_OBJ_ATTR_v1 || TC_OBJ_ATTR_v2)
>> +extern obj_attr_tag_t obj_attr_process_attribute (obj_attr_vendor_t);
>> +#endif /* (TC_OBJ_ATTR_v1 || TC_OBJ_ATTR_v2) */
>
> ... is the #if really needed here? Or ...
>
It disappears in the next patch, but it should have been part of this patch.
Fixed in the next revision.
>> +#if (TC_OBJ_ATTR_v2)
>> +extern void obj_attr_process_subsection (void);
>> +#endif /* (TC_OBJ_ATTR_v2) */
>>
>> #endif /* TC_OBJ_ATTR */
>>
>> --- a/gas/config/obj-elf.c
>> +++ b/gas/config/obj-elf.c
>> @@ -72,7 +72,9 @@ static void obj_elf_symver (int);
>> static void obj_elf_subsection (int);
>> static void obj_elf_popsection (int);
>> #ifdef TC_OBJ_ATTR
>> +#if (TC_OBJ_ATTR_v1 || TC_OBJ_ATTR_v2)
>> static void obj_elf_gnu_attribute (int);
>> +#endif /* (TC_OBJ_ATTR_v1 || TC_OBJ_ATTR_v2) */
>> #endif /* TC_OBJ_ATTR */
>
> ... here?
>
This is fixed while addressing your comment in patch 2.
>> @@ -119,7 +121,9 @@ static const pseudo_typeS elf_pseudo_table[] =
>>
>> /* A GNU extension for object attributes. */
>> #ifdef TC_OBJ_ATTR
>> +#if (TC_OBJ_ATTR_v1 || TC_OBJ_ATTR_v2)
>> {"gnu_attribute", obj_elf_gnu_attribute, 0},
>> +#endif /* (TC_OBJ_ATTR_v1 || TC_OBJ_ATTR_v2) */
>> #endif /* TC_OBJ_ATTR */
>
> Same question here.
>
Fixed in the next revision.
>> @@ -3044,9 +3057,9 @@ elf_end (void)
>> free (groups.head);
>> }
>>
>> -#ifdef TC_OBJ_ATTR
>> +#if defined (TC_OBJ_ATTR) && TC_OBJ_ATTR_v1
>> oav1_attr_info_exit ();
>> -#endif /* TC_OBJ_ATTR */
>> +#endif /* defined (TC_OBJ_ATTR) && TC_OBJ_ATTR_v1 */
>> }
>
> And isn't a TC_OBJ_ATTR_v1 check alone sufficient here?
Fixed in the next revision.
>
> Jan
I added the diff with the next revision for some of the answers hoping
that it makes things clearer for you. Do you think that it overloads the
email, or is it ok ?
Matthieu
More information about the Binutils
mailing list