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

[Thiago Jung Bauermann]

References: <20250103032831.622617-1-thiago.bauermann@linaro.org>
	<6866522c-e094-42cc-af7d-b673deb36ee8@suse.com>
User-Agent: mu4e 1.12.8; emacs 29.4
Hello Jan,

Thanks for the review.

Jan Beulich <jbeulich@suse.com> writes:

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

That's a good point.

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

This is also a good point, though if this diagnostic is still a warning then
the patch loses most of its value, which is to keep existing behaviour of
previously released versions of GCC when built with current binutils.

Another alternative would be to backport the GCC configure.ac fix
to currently supported GCC branches.

>> --- 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 == ':')

BTW, this #include is spurious. It's from a previous version of the code.

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

I did it this way because get_absolute_expr is a static function. Would
it be acceptable if the patch exported it instead?

--
Thiago
Date: Tue, 07 Jan 2025 23:33:11 -0300