| Message ID | 20250103032831.622617-1-thiago.bauermann@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gas/ELF: Assume merge entity size of 1 if it's missing | expand |
On 03.01.2025 04:28, Thiago Jung Bauermann wrote: > commit d5cbf916be4a ("gas/ELF: also reject merge entity size being zero") > made gas stricter when an entity size isn't specified (and thus assumed to > be zero). > > Unfortunately this mistake can happen in 32-bit Arm assembly, where the @ > character is the start of a comment. E.g.: > > .section .rodata.str, "aMS", @progbits, 1 And what if it's .section .rodata.str, "aMS", @progbits, 4 ? You just can't ... > The aforementioned commit actually pointed out a binutils testcase with > this problem, as well as a bug in GCC's configure script. > > Therefore, change the parser to accept the specific case of an absent > entsize with "M" and assume 1, as suggested by Alan Modra. ... make such an assumption (but see below). > With this change, a version of GCC's configure script with the bug > recognizes support for section merging: > > configure:27013: checking assembler for section merging support > configure:27022: /path/to/bin/as --fatal-warnings -o conftest.o conftest.s >&5 > conftest.s: Assembler messages: > conftest.s:1: entity size for SHF_MERGE / SHF_STRINGS not specified. Assuming 1. > configure:27025: $? = 0 > configure:27036: result: yes Nor do I agree on this aspect. I'm sure --fatal-warnings is passed here for a reason. Yet you make the diagnostics ... > --- a/gas/config/obj-elf.c > +++ b/gas/config/obj-elf.c > @@ -1318,15 +1318,22 @@ obj_elf_section (int push) > if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0 > && *input_line_pointer == ',') > { > + operatorT kind; > + > ++input_line_pointer; > SKIP_WHITESPACE (); > if (inherit && *input_line_pointer == ',' > && (bfd_section_flags (now_seg) > & (SEC_MERGE | SEC_STRINGS)) != 0) > goto fetch_entsize; > - entsize = get_absolute_expression (); > + entsize = get_absolute_expression_kind (&kind); > SKIP_WHITESPACE (); > - if (entsize <= 0) > + if (kind == O_absent) > + { > + as_tsktsk (_("entity size for SHF_MERGE / SHF_STRINGS not specified. Assuming 1.")); > + entsize = 1; > + } > + else if (entsize <= 0) > { > as_warn (_("invalid merge / string entity size")); > attr &= ~(SHF_MERGE | SHF_STRINGS); > @@ -1342,8 +1349,8 @@ obj_elf_section (int push) > } > else if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0) > { > - as_warn (_("entity size for SHF_MERGE / SHF_STRINGS not specified")); > - attr &= ~(SHF_MERGE | SHF_STRINGS); > + as_tsktsk (_("entity size for SHF_MERGE / SHF_STRINGS not specified. Assuming 1.")); > + entsize = 1; > } ... not be warnings anymore. With them being warnings, the defaulting may then be acceptable (albeit still somewhat questionable). > --- a/gas/read.c > +++ b/gas/read.c > @@ -45,6 +45,7 @@ > #include "ginsn.h" > > #include <limits.h> > +#include <stdbool.h> > > #ifndef TC_START_LABEL > #define TC_START_LABEL(STR, NUL_CHAR, NEXT_CHAR) (NEXT_CHAR == ':') > @@ -569,6 +570,26 @@ get_absolute_expression (void) > return get_absolute_expr (&exp); > } > > +/* Return value of absolute expression starting at INPUT_LINE_POINTER and > + set KIND to either O_constant, O_absent or O_illegal depending on > + whether the expression was constant, absent or something else > + entirely. */ > + > +offsetT > +get_absolute_expression_kind (operatorT *kind) > +{ > + expressionS exp; > + offsetT value; > + > + value = get_absolute_expr (&exp); > + if (exp.X_op == O_absent || exp.X_op == O_constant) > + *kind = exp.X_op; > + else > + *kind = O_illegal; > + > + return value; > +} Personally I question such oddly named, overly special purpose helpers. Callers needing details on the expression simply should be using the respective underlying functions. Jan
diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c index c4af01808b6a..d5b558210919 100644 --- a/gas/config/obj-elf.c +++ b/gas/config/obj-elf.c @@ -1318,15 +1318,22 @@ obj_elf_section (int push) if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0 && *input_line_pointer == ',') { + operatorT kind; + ++input_line_pointer; SKIP_WHITESPACE (); if (inherit && *input_line_pointer == ',' && (bfd_section_flags (now_seg) & (SEC_MERGE | SEC_STRINGS)) != 0) goto fetch_entsize; - entsize = get_absolute_expression (); + entsize = get_absolute_expression_kind (&kind); SKIP_WHITESPACE (); - if (entsize <= 0) + if (kind == O_absent) + { + as_tsktsk (_("entity size for SHF_MERGE / SHF_STRINGS not specified. Assuming 1.")); + entsize = 1; + } + else if (entsize <= 0) { as_warn (_("invalid merge / string entity size")); attr &= ~(SHF_MERGE | SHF_STRINGS); @@ -1342,8 +1349,8 @@ obj_elf_section (int push) } else if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0) { - as_warn (_("entity size for SHF_MERGE / SHF_STRINGS not specified")); - attr &= ~(SHF_MERGE | SHF_STRINGS); + as_tsktsk (_("entity size for SHF_MERGE / SHF_STRINGS not specified. Assuming 1.")); + entsize = 1; } if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0 && type == SHT_NOBITS) diff --git a/gas/read.c b/gas/read.c index 6d0d4b5e31a1..9468277bc609 100644 --- a/gas/read.c +++ b/gas/read.c @@ -45,6 +45,7 @@ #include "ginsn.h" #include <limits.h> +#include <stdbool.h> #ifndef TC_START_LABEL #define TC_START_LABEL(STR, NUL_CHAR, NEXT_CHAR) (NEXT_CHAR == ':') @@ -569,6 +570,26 @@ get_absolute_expression (void) return get_absolute_expr (&exp); } +/* Return value of absolute expression starting at INPUT_LINE_POINTER and + set KIND to either O_constant, O_absent or O_illegal depending on + whether the expression was constant, absent or something else + entirely. */ + +offsetT +get_absolute_expression_kind (operatorT *kind) +{ + expressionS exp; + offsetT value; + + value = get_absolute_expr (&exp); + if (exp.X_op == O_absent || exp.X_op == O_constant) + *kind = exp.X_op; + else + *kind = O_illegal; + + return value; +} + static int pop_override_ok; static const char *pop_table_name; diff --git a/gas/read.h b/gas/read.h index d11daf13034f..bc236523a89e 100644 --- a/gas/read.h +++ b/gas/read.h @@ -117,6 +117,7 @@ extern char *demand_copy_string (int *lenP); extern char *demand_copy_C_string (int *len_pointer); extern char get_absolute_expression_and_terminator (long *val_pointer); extern offsetT get_absolute_expression (void); +extern offsetT get_absolute_expression_kind (operatorT *kind); extern unsigned int next_char_of_string (void); extern void s_mri_sect (char *); extern char *mri_comment_field (char *);
commit d5cbf916be4a ("gas/ELF: also reject merge entity size being zero") made gas stricter when an entity size isn't specified (and thus assumed to be zero). Unfortunately this mistake can happen in 32-bit Arm assembly, where the @ character is the start of a comment. E.g.: .section .rodata.str, "aMS", @progbits, 1 The aforementioned commit actually pointed out a binutils testcase with this problem, as well as a bug in GCC's configure script. Therefore, change the parser to accept the specific case of an absent entsize with "M" and assume 1, as suggested by Alan Modra. With this change, a version of GCC's configure script with the bug recognizes support for section merging: configure:27013: checking assembler for section merging support configure:27022: /path/to/bin/as --fatal-warnings -o conftest.o conftest.s >&5 conftest.s: Assembler messages: conftest.s:1: entity size for SHF_MERGE / SHF_STRINGS not specified. Assuming 1. configure:27025: $? = 0 configure:27036: result: yes PR gas/32491 * gas/config/obj-elf.c (obj_elf_section): Assume entsize of 1 if not specified. * gas/read.c (get_absolute_expression_kind): New function. * gas/read.h (get_absolute_expression_kind): Declare. --- gas/config/obj-elf.c | 15 +++++++++++---- gas/read.c | 21 +++++++++++++++++++++ gas/read.h | 1 + 3 files changed, 33 insertions(+), 4 deletions(-) Also, note that I posted a patch to fix GCC's configure script here: https://inbox.sourceware.org/gcc-patches/20241227214756.1059146-1-thiago.bauermann@linaro.org/ base-commit: f087e2e3f6dece21c5cb6049b4531bf762a3068f