[PATCH v9 04/19] gas: implement parsing of object attributes v2
Matthieu Longo
matthieu.longo@arm.com
Tue Nov 4 17:32:27 GMT 2025
On 24/10/2025 15:19, Jan Beulich wrote:
> On 01.09.2025 18:56, Matthieu Longo wrote:
>> +
>> +/* 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.
>
> Jan
I decided to remove this function, and change the implementation of
obj_attr_parse_arg(). Please have a look at it and let me know if you
think that it looks better.
Please keep in mind that I try not to reinvent the wheel and not to do
open coding, but the existing parsing utilities in binutils are not very
helpful for what I try to achieve.
Here are the major changes compare to the previous implementation:
- move the erroring back into obj_attr_parse_arg () as much as possible.
- better detection of the expected type VS the found type.
- improvement of the error messages.
- move most of the calls to ignore_rest_of_line(), when a error is met,
into vendor_attribute_parse_args() and vendor_subsection_parse_args().
It is a pity that binutils does not have a recursive parser that would
return tokens, it would really make things easier and would avoid this
open-coding.
Is the absence of such a parser due to the legacy parsing scattered
everywhere ? and the code change would be too disruptive ?
Matthieu
diff --git a/gas/config/obj-elf-attr.c b/gas/config/obj-elf-attr.c
index 4cc8c025cc6..923fd9a532a 100644
--- a/gas/config/obj-elf-attr.c
+++ b/gas/config/obj-elf-attr.c
@@ -242,17 +242,27 @@ extract_string_literal (arg_t *arg_out)
/* 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. */
+ 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 want_unsigned,
+ bool *signedness_issue)
{
+ const char *cursor_begin = input_line_pointer;
expressionS exp;
expression_and_evaluate (&exp);
if (exp.X_op != O_constant)
{
- as_bad (_("invalid value, expected an integer literal"));
- goto bad;
+ char backup_c = *input_line_pointer;
+ *input_line_pointer = '\0';
+ as_bad (_("expression '%s' does not resolve to an integer literal"),
+ 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;
@@ -260,9 +270,10 @@ extract_integer_literal (arg_t *arg_out,
{
if (! exp.X_unsigned && val < 0)
{
- as_bad (_("invalid negative value %" PRId64 ", expected an unsigned "
- "integer"), val);
- goto bad;
+ as_bad (_("unexpected value %" PRId64 ", expected `unsigned integer'"
+ " instead"), val);
+ *signedness_issue = true;
+ return false;
}
arg_out->val.u64 = val;
arg_out->vtype = VALUE_UNSIGNED_INTEGER;
@@ -273,10 +284,6 @@ extract_integer_literal (arg_t *arg_out,
arg_out->vtype = VALUE_SIGNED_INTEGER;
}
return true;
-
- bad:
- ignore_rest_of_line ();
- return false;
}
/* Extract an identifier based on the provided character matcher. */
@@ -289,7 +296,7 @@ extract_identifier (bool (*char_predicate) (char),
arg_t *arg_out)
i++;
if (i == 0)
{
- as_bad (_("invalid value '%c', expected an identifier"),
+ as_bad (_("invalid character '%c' in identifier"),
*input_line_pointer);
ignore_rest_of_line ();
return false;
@@ -535,19 +542,58 @@ lookup_symbol_table (const char *identifier,
return true;
}
-/* Return true if the next characters are suspected to represent an integer
- literal. */
-static bool
-look_like_integer_literal (const char *cursor)
+/* Function similar to snprintf from the standard library, except that it
+ also updates the buffer pointer to point to the last written character,
+ and length to match the remaining space in the buffer.
+ Return the number of bytes printed. The function is assumed to
always be
+ successful, and a failure with vnsprintf() will trigger an assert(). */
+static size_t
+snprintf_append (char **sbuffer, size_t *length,
+ const char *format, ...)
{
- if (ISDIGIT (*cursor))
- return true;
- if (! (*cursor == '+' || *cursor == '-' || *cursor == '~'))
- return false;
- ++cursor;
- while (ISSPACE (*cursor))
- ++cursor;
- return ISDIGIT (*cursor);
+ va_list args;
+ va_start (args, format);
+ int rval = vsnprintf (*sbuffer, *length, format, args);
+ va_end (args);
+
+ gas_assert (rval >= 0);
+ size_t written = rval;
+ *length -= written;
+ *sbuffer += written;
+ return written;
+}
+
+/* Return the list of comma-separated strings matching the expected types
+ (i.e. the flags set in low_ttype). */
+static const char *
+expectations_to_string (const arg_token_t low_ttype,
+ char *sbuffer, size_t length)
+{
+ unsigned match_n = 0;
+ char *sbuffer_start = sbuffer;
+ size_t total = 0;
+ if (low_ttype & IDENTIFIER)
+ {
+ ++match_n;
+ total += snprintf_append (&sbuffer, &length, "`%s'", "identifier");
+ }
+
+#define EXP_APPEND_TYPE_STR(type_flag, type_str) \
+ if (low_ttype & type_flag) \
+ { \
+ if (match_n >= 1) \
+ total += snprintf_append (&sbuffer, &length, "%s", ", or "); \
+ ++match_n; \
+ total += snprintf_append (&sbuffer, &length, "`%s'", type_str); \
+ }
+
+ EXP_APPEND_TYPE_STR (STRING, "string");
+ EXP_APPEND_TYPE_STR (UNSIGNED_INTEGER, "unsigned integer");
+ EXP_APPEND_TYPE_STR (SIGNED_INTEGER, "signed integer");
+#undef APPEND_TYPE_STR
+
+ gas_assert (total <= length);
+ return sbuffer_start;
}
/* Parse an argument, and set its type accordingly depending on the input
@@ -567,51 +613,102 @@ obj_attr_parse_arg (arg_token_t expected_ttype,
return true;
}
- /* Note: symbol look-up for string literals is not available. */
- if (((low_ttype & STRING) && *input_line_pointer == '"')
- || !(low_ttype & ~STRING))
- return extract_string_literal (arg_out);
-
- if (((low_ttype & (UNSIGNED_INTEGER | SIGNED_INTEGER))
- && look_like_integer_literal (input_line_pointer))
- || !(low_ttype & ~(UNSIGNED_INTEGER | SIGNED_INTEGER)))
- return extract_integer_literal (arg_out, (low_ttype &
UNSIGNED_INTEGER));
-
- /* Try to extract an identifier. */
- if (! extract_identifier (match_identifier, arg_out))
- return false;
- gas_assert (arg_out->vtype == VALUE_STRING);
-
- /* 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;
+ /* 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;
- /* 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;
+ 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. */
+ else if (ISALPHA (*input_line_pointer) || *input_line_pointer == '_')
+ {
+ bool status = extract_identifier (match_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);
- /* 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;
+ if (status && ! (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;
+ }
- as_bad ("unknown identifier '%s'", identifier);
- arg_out->val.string = NULL;
- arg_out->vtype = VALUE_UNDEFINED;
- resolved = 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;
+ free ((char *) identifier);
+ return resolved;
+ }
+ /* If it is neither a string nor an identifier, it must be an
expression. */
+ else
+ {
+ 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)
+ as_bad (_("unexpected integer %lu, expected %s instead"),
+ arg_out->val.u64,
+ expectations_to_string (low_ttype, sbuffer, sizeof(sbuffer)));
+ else if ((low_ttype & UNSIGNED_INTEGER) && signedness_issue) {}
+ /* Already handled by extract_integer_literal(), nothing to do. */
+ else
+ as_bad (_("fell back to integer literal extraction from expression, "
+ "but expected %s instead"),
+ expectations_to_string (low_ttype, sbuffer, sizeof(sbuffer)));
+ arg_out->vtype = VALUE_UNDEFINED;
+ return false;
+ }
}
/* Trim white spaces before a parameter.
@@ -634,6 +731,7 @@ trim_whitespaces_before_param (unsigned int n)
}
}
while (is_whitespace (*input_line_pointer));
+
return !error;
}
@@ -649,6 +747,7 @@ skip_whitespaces_past_comma (unsigned int n)
as_bad (_("unexpected comma after value"));
else
as_bad (_("missing comma after parameter %u"), n);
+ ignore_rest_of_line ();
return false;
}
return true;
@@ -786,6 +885,7 @@ vendor_attribute_parse_args (obj_attr_vendor_t
vendor ATTRIBUTE_UNUSED,
const obj_attr_subsection_v2 *subsec ATTRIBUTE_UNUSED,
unsigned int nargs, ...)
{
+ bool parsing_err = false;
va_list args;
va_start (args, nargs);
@@ -794,7 +894,10 @@ vendor_attribute_parse_args (obj_attr_vendor_t
vendor ATTRIBUTE_UNUSED,
for (unsigned int n = 0; n < nargs; ++n)
{
if (! trim_whitespaces_before_param (n + 1))
- goto bad;
+ {
+ ignore_rest_of_line ();
+ goto bad;
+ }
arg_t *arg_out = &args_out[n];
@@ -826,25 +929,33 @@ vendor_attribute_parse_args (obj_attr_vendor_t
vendor ATTRIBUTE_UNUSED,
arg_out))
{
if (high_ttype == ATTRIBUTE_KEY)
- as_bad (_("could not parse attribute tag"));
+ {
+ as_bad (_("could not parse attribute tag"));
+ parsing_err = true;
+ }
else
- as_bad (_("could not parse attribute value"));
- goto bad;
+ {
+ as_bad (_("could not parse attribute value"));
+ ignore_rest_of_line ();
+ goto bad;
+ }
}
if (n + 1 < nargs && !skip_whitespaces_past_comma (n + 1))
goto bad;
}
- va_end (args);
- demand_empty_rest_of_line ();
-
- return args_out;
+ if (! parsing_err)
+ {
+ va_end (args);
+ if (! demand_empty_rest_of_line ())
+ goto bad;
+ return args_out;
+ }
bad:
args_list_free (args_out, nargs);
va_end (args);
- ignore_rest_of_line ();
return NULL;
}
@@ -955,14 +1066,16 @@ vendor_subsection_parse_args (unsigned int nargs,
...)
}
va_end (args);
- demand_empty_rest_of_line ();
+ if (! demand_empty_rest_of_line ())
+ goto bad_no_line_claim;
return args_out;
bad:
+ ignore_rest_of_line ();
+ bad_no_line_claim:
args_list_free (args_out, nargs);
va_end (args);
- ignore_rest_of_line ();
return NULL;
}
diff --git a/gas/read.c b/gas/read.c
index 4ba71f99250..62c702f3e5d 100644
--- a/gas/read.c
+++ b/gas/read.c
@@ -4056,15 +4056,17 @@ s_weakref (int ignore ATTRIBUTE_UNUSED)
dereference input_line_pointer unconditionally. Note that when the
gas parser is switched to handling a string (where buffer_limit
should be the size of the string excluding the NUL terminator) this
- will be one past the NUL; is_end_of_line(0) returns true. */
+ will be one past the NUL; is_end_of_line(0) returns true.
+ Return true on success, false when if junks at the end of line is
found. */
-void
+bool
demand_empty_rest_of_line (void)
{
+ bool ret = true;
SKIP_WHITESPACE ();
if (input_line_pointer > buffer_limit)
- return;
- if (is_end_of_stmt (*input_line_pointer))
+ {}
+ else if (is_end_of_stmt (*input_line_pointer))
input_line_pointer++;
else
{
@@ -4075,8 +4077,10 @@ demand_empty_rest_of_line (void)
as_bad (_("junk at end of line, first unrecognized character valued
0x%x"),
*input_line_pointer);
ignore_rest_of_line ();
+ ret = false;
}
/* Return pointing just after end-of-line. */
+ return ret;
}
/* Silently advance to the end of a statement. Use this after already
having
diff --git a/gas/read.h b/gas/read.h
index 72b66cdd952..7ea31b2711d 100644
--- a/gas/read.h
+++ b/gas/read.h
@@ -132,7 +132,7 @@ extern void init_include_dir (void);
extern void add_include_dir (char *);
extern FILE *search_and_open (const char *, char *);
extern void cons (int nbytes);
-extern void demand_empty_rest_of_line (void);
+extern bool demand_empty_rest_of_line (void);
extern void emit_expr (expressionS *exp, unsigned int nbytes);
extern void emit_expr_with_reloc (expressionS *exp, unsigned int nbytes,
TC_PARSE_CONS_RETURN_TYPE);
diff --git a/gas/testsuite/gas/arm/attr-syntax.d
b/gas/testsuite/gas/arm/attr-syntax.d
index 2c0ef1b9680..1b358fd9984 100644
--- a/gas/testsuite/gas/arm/attr-syntax.d
+++ b/gas/testsuite/gas/arm/attr-syntax.d
@@ -2,7 +2,7 @@
#notarget: *-*-pe
#as:
#error: \A[^\n]+: Assembler messages:
-#error: \n[^\n]+: Error: unknown identifier 'made_up_tag'
+#error: \n[^\n]+: Error: unknown identifier 'made_up_tag' in this context
#error: \n[^\n]+: Error: could not parse attribute tag
#error: \n[^\n]+: Error: unexpected comma before parameter 1
#error: \n[^\n]+: Error: missing comma after parameter 1\Z
diff --git a/gas/testsuite/gas/aarch64/build-attributes/ba-failures-1.d
b/gas/testsuite/gas/aarch64/build-attributes/ba-failures-1.d
index 14d8639ff3c..a14d3848e13 100644
--- a/gas/testsuite/gas/aarch64/build-attributes/ba-failures-1.d
+++ b/gas/testsuite/gas/aarch64/build-attributes/ba-failures-1.d
@@ -4,29 +4,40 @@
#error: \A[^\n]+: Assembler messages:
#error: \n[^\n]+: Error: declaration of an attribute outside the scope
of an attribute subsection
#error: \n[^\n]+: Error: attribute 1 cannot be redefined
-#error: \n[^\n]+: Error: invalid negative value -1, expected an
unsigned integer
+#error: \n[^\n]+: Error: unexpected value -1, expected `unsigned
integer' instead
#error: \n[^\n]+: Error: could not parse attribute tag
-#error: \n[^\n]+: Error: unknown identifier 'Tag_Unknown'
+#error: \n[^\n]+: Error: unknown identifier 'Tag_Unknown' in this context
#error: \n[^\n]+: Error: could not parse attribute tag
-#error: \n[^\n]+: Error: invalid negative value -1, expected an
unsigned integer
+#error: \n[^\n]+: Error: unknown identifier 'Tag_Unknown' in this context
+#error: \n[^\n]+: Error: could not parse attribute tag
+#error: \n[^\n]+: Error: unexpected `string' "plop", expected `unsigned
integer' instead
+#error: \n[^\n]+: Error: could not parse attribute value
+#error: \n[^\n]+: Error: unexpected value -1, expected `unsigned
integer' instead
+#error: \n[^\n]+: Error: could not parse attribute value
+#error: \n[^\n]+: Error: unexpected `string' "foo", expected `unsigned
integer' instead
#error: \n[^\n]+: Error: could not parse attribute value
-#error: \n[^\n]+: Error: missing '"', expected a string literal
+#error: \n[^\n]+: Error: unexpected integer 1, expected `string' instead
#error: \n[^\n]+: Error: could not parse attribute value
#error: \n[^\n]+: Error: attribute 4 cannot be redefined
#error: \n[^\n]+: Error: comprehension and encoding of a subsection
cannot be omitted on the first declaration
#error: \n[^\n]+: Error: incompatible redeclaration of subsection
vendor_1_subsection_3\. Previous declaration had properties:
comprehension=required, encoding=NTBS
#error: \n[^\n]+: Error: incompatible redeclaration of subsection
vendor_1_subsection_3\. Previous declaration had properties:
comprehension=required, encoding=NTBS
#error: \n[^\n]+: Error: incompatible redeclaration of subsection
vendor_1_subsection_3\. Previous declaration had properties:
comprehension=required, encoding=NTBS
-#error: \n[^\n]+: Error: unknown identifier 'ntbs'
-#error: \n[^\n]+: Error: unknown identifier 'uleb128'
-#error: \n[^\n]+: Error: invalid value '.', expected an identifier
+#error: \n[^\n]+: Error: unknown identifier 'ntbs' in this context
+#error: \n[^\n]+: Error: unknown identifier 'uleb128' in this context
+#error: \n[^\n]+: Error: expression '.vendor' does not resolve to an
integer literal
+#error: \n[^\n]+: Error: fell back to integer literal extraction from
expression, but expected `identifier' instead
#error: \n[^\n]+: Error: expected <subsection_name>, <comprehension>,
<encoding>
-#error: \n[^\n]+: Error: unknown identifier 'uleb128'
-#error: \n[^\n]+: Error: unknown identifier 'optial'
-#error: \n[^\n]+: Error: unknown identifier 'ul128'
+#error: \n[^\n]+: Error: unknown identifier 'uleb128' in this context
+#error: \n[^\n]+: Error: unknown identifier 'optial' in this context
+#error: \n[^\n]+: Error: unknown identifier 'ul128' in this context
#error: \n[^\n]+: Error: invalid value 2, expected values for
<comprehension> are 0 \(=`required'\) or 1 \(=`optional'\)
#error: \n[^\n]+: Error: invalid value 2, expected values for
<encoding> are 0 \(=`ULEB128'\) or 1 \(=`NTBS'\)
#error: \n[^\n]+: Error: unexpected comma before parameter 2
#error: \n[^\n]+: Error: junk at end of line, first unrecognized
character is `,'
#error: \n[^\n]+: Error: unexpected comma before parameter 1
-#error: \n[^\n]+: Error: missing comma after parameter 2\Z
+#error: \n[^\n]+: Error: missing comma after parameter 2
+#error: \n[^\n]+: Error: unexpected comma before parameter 2
+#error: \n[^\n]+: Error: junk at end of line, first unrecognized
character is `,'
+#error: \n[^\n]+: Error: unexpected comma before parameter 1
+#error: \n[^\n]+: Error: junk at end of line, first unrecognized
character is `1'\Z
diff --git a/gas/testsuite/gas/aarch64/build-attributes/ba-failures-1.s
b/gas/testsuite/gas/aarch64/build-attributes/ba-failures-1.s
index 0e65499e860..d66ab894672 100644
--- a/gas/testsuite/gas/aarch64/build-attributes/ba-failures-1.s
+++ b/gas/testsuite/gas/aarch64/build-attributes/ba-failures-1.s
@@ -14,6 +14,8 @@
/* Only unsigned integer are allowed for attribute keys. */
.aeabi_attribute -1, 1
/* Unknown tag identifier. */
+.aeabi_attribute Tag_Unknown, 1
+/* 2 issues on the same line, both should be reported. */
.aeabi_attribute Tag_Unknown, "plop"
/* Mismatch between the type expected from the subsection definition,
and the
@@ -55,8 +57,27 @@
.aeabi_subsection vendor_1_subsection_4, 2, 1
.aeabi_subsection vendor_1_subsection_4, 1, 2
-/* Wrong comma. */
+/* Wrong comma in the declaration of a subsection. */
.aeabi_subsection vendor_1_subsection_4, , 1
.aeabi_subsection vendor_1_subsection_4, 1, 1, 1
.aeabi_subsection , vendor_1_subsection_4, 1
.aeabi_subsection vendor_1_subsection_4, 1 1
+
+/* The purpose of this line is not to detect a parsing error on this
line, but
+ a previous one that would have declared a subsection with the same name.
+ It makes sure that the junk at the end of line was correctly
detected, and
+ the subsections were correctly not recorded. If it had been
recorded, this
+ line would trigger an error for redeclaration of a subsection with
different
+ parameters. */
+.aeabi_subsection vendor_1_subsection_4, 0, 1
+
+/* Wrong comma in the declaration of an attribute. */
+.aeabi_attribute 1, , 1
+.aeabi_attribute 1, "dead", 1, 1
+.aeabi_attribute , 1, 1
+.aeabi_attribute 1, "beef" 1
+
+/* The purpose of this line is similar to the last subsection
declaration. It
+ would trigger an error if one of the previous declaration attempts had
+ succeeded by ignoring the junk at the end of line. */
+.aeabi_attribute 1, "ok"
More information about the Binutils
mailing list