[PATCH] gas: elf: Relax rules for SHF_STRING sections

[Richard Earnshaw (lists)]

On 18/01/2025 09:37, Alan Modra wrote:
> On Fri, Jan 17, 2025 at 03:25:19PM +0000, Richard Earnshaw wrote:
>> Commit af3394d97a8c5187085c0eec5fb03e8da88db5fb allowed sections
>> declared with "S" (SHF_STRING) to specify the entity size, but then
>> would warn if the entity size was omitted, as with the old syntax.
>>
>> Unfortunately, since specifying the entity size is incompatible with
>> binutils 2.43 or earlier, this makes it impossible to specify a
>> strings section in source code without generating an assembly warning
>> (the new syntax isn't supported in older assemblers and the old syntax
>> generates warnings).
>>
>> Nevertheless, the old code was wrong in that it did not set the entity
>> size at all, in contravention of the ELF specification (though to date
>> there are no known cases where this mattered outside of mergeable
>> sections).
>>
>> Fix this by permitting the original syntax without a warning again,
>> but by defaulting the entity size to 1.  This is compatible with the
>> most common case of strings being byte-based.
> 
> I think backward compatibility matters, and this change doesn't negate
> Jan's fix for bad entsize, but rather allows a reasonable default.
> 
>> --- /dev/null
>> +++ b/gas/testsuite/gas/elf/string.d
>> @@ -0,0 +1,9 @@
>> +#readelf: -S
> 
> You'll need -W or --wide here.

For ELF64, thanks.  Of course, I managed to start from elf/section3.d 
which is pretty much the only example that doesn't have -W.  Testing on 
Arm I didn't notice that the output would differ for 64-bit targets.

I've taken the above comment as assent and pushed the patch with that 
minor tweak.  Hopefully that is OK.

R.

> 
> I also think we should do something about backwards compatibilty for
> the ARM compiler configure bug, so that people can build released
> versions of gcc.  This is what I have in mind, simpler than the patch
> Thiago submitted.
> 
> diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
> index 6670b9add0f..b3b2025938b 100644
> --- a/gas/config/obj-elf.c
> +++ b/gas/config/obj-elf.c
> @@ -1324,13 +1324,27 @@ obj_elf_section (int push)
>   		  && (bfd_section_flags (now_seg)
>   		      & (SEC_MERGE | SEC_STRINGS)) != 0)
>   		goto fetch_entsize;
> -	      entsize = get_absolute_expression ();
> -	      SKIP_WHITESPACE ();
> -	      if (entsize <= 0)
> +	      if (is_end_of_line[(unsigned char) *input_line_pointer])
> +		{
> +		  /* ??? This is here for older versions of gcc that
> +		     test for gas string merge support with
> +		     '.section .rodata.str, "aMS", @progbits, 1'
> +		     Unfortunately '@' begins a comment on arm.
> +		     This isn't as_warn because gcc tests with
> +		     --fatal-warnings. */
> +		  as_tsktsk (_("missing merge / string entity size, 1 assumed"));
> +		  entsize = 1;
> +		}
> +	      else
>   		{
> -		  as_warn (_("invalid merge / string entity size"));
> -		  attr &= ~(SHF_MERGE | SHF_STRINGS);
> -		  entsize = 0;
> +		  entsize = get_absolute_expression ();
> +		  SKIP_WHITESPACE ();
> +		  if (entsize <= 0)
> +		    {
> +		      as_warn (_("invalid merge / string entity size"));

I might even say that an invalid value in the source (as opposed to a 
missing one) for the entity size should be an error not a warning.

> +		      attr &= ~(SHF_MERGE | SHF_STRINGS);
> +		      entsize = 0;
> +		    }
>   		}
>   	    }
>   	  else if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0 && inherit
> 

That seems reasonable to me, but it's probably independent of my change.


R.