[PATCH] gas/ELF: Assume merge entity size of 1 if it's missing

[Jan Beulich]

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