[PATCH 3/4] ELF: SHF_STRINGS isn't really tied to SHF_MERGE

[Richard Earnshaw (lists)]

On 16/01/2025 07:50, Jan Beulich wrote:
> On 15.01.2025 19:03, Richard Sandiford wrote:
>> Jan Beulich <jbeulich@suse.com> writes:
>>> It's not overly useful without it, but the spec doesn't name any
>>> dependency between the two. People may want to use it for purely
>>> informational purposes, for example. Adjust, in particular, entity size
>>> processing to be engaged if either flag is set, as mandated by the spec.
>>>
>>> --- a/bfd/elf.c
>>> +++ b/bfd/elf.c
>>> @@ -846,7 +846,10 @@ _bfd_elf_make_section_from_shdr (bfd *ab
>>>         newsect->entsize = hdr->sh_entsize;
>>>       }
>>>     if ((hdr->sh_flags & SHF_STRINGS) != 0)
>>> -    flags |= SEC_STRINGS;
>>> +    {
>>> +      flags |= SEC_STRINGS;
>>> +      newsect->entsize = hdr->sh_entsize;
>>> +    }
>>>     if ((hdr->sh_flags & SHF_TLS) != 0)
>>>       flags |= SEC_THREAD_LOCAL;
>>>     if ((hdr->sh_flags & SHF_EXCLUDE) != 0)
>>> @@ -3709,7 +3712,10 @@ elf_fake_sections (bfd *abfd, asection *
>>>         this_hdr->sh_entsize = asect->entsize;
>>>       }
>>>     if ((asect->flags & SEC_STRINGS) != 0)
>>> -    this_hdr->sh_flags |= SHF_STRINGS;
>>> +    {
>>> +      this_hdr->sh_flags |= SHF_STRINGS;
>>> +      this_hdr->sh_entsize = asect->entsize;
>>> +    }
>>>     if ((asect->flags & SEC_GROUP) == 0 && elf_group_name (asect) != NULL)
>>>       this_hdr->sh_flags |= SHF_GROUP;
>>>     if ((asect->flags & SEC_THREAD_LOCAL) != 0)
>>> --- a/gas/doc/as.texi
>>> +++ b/gas/doc/as.texi
>>> @@ -7001,17 +7001,18 @@ Note - some sections, eg @code{.text} an
>>>   special and have fixed types.  Any attempt to declare them with a different
>>>   type will generate an error from the assembler.
>>>   
>>> -If @var{flags} contains the @code{M} symbol then the @var{type} argument must
>>> -be specified as well as an extra argument---@var{entsize}---like this:
>>> +If @var{flags} contains the @code{M} and/or @code{S} symbol then the @var{type}
>>> +argument must be specified as well as an extra argument---@var{entsize}---like
>>> +this:
>>>   
>>>   @smallexample
>>>   .section @var{name} , "@var{flags}"M, @@@var{type}, @var{entsize}
>>>   @end smallexample
>>>   
>>>   Sections with the @code{M} flag but not @code{S} flag must contain fixed size
>>> -constants, each @var{entsize} octets long. Sections with both @code{M} and
>>> -@code{S} must contain zero terminated strings where each character is
>>> -@var{entsize} bytes long. The linker may remove duplicates within sections with
>>> +constants, each @var{entsize} octets long. Sections with @code{S} must contain
>>> +zero terminated strings where each character is @var{entsize} bytes long. For
>>> +@code{M} sections the linker may remove duplicates within sections with
>>>   the same name, same entity size and same flags.  @var{entsize} must be an
>>>   absolute expression.  For sections with both @code{M} and @code{S}, a string
>>>   which is a suffix of a larger string is considered a duplicate.  Thus
>>
>> We have a codebase that made a lot of use of constructs like:
>>
>> 	.section .rodata.foo, "aS"
>> foo:
>>          .asciz "..."
>>
>> which after this patch need to become:
>>
>> 	.section .rodata.foo, "aS", @progbits, 1
>> foo:
>>          .asciz "..."
>>
>> The codebase is being changed to use the longer form.  But it kind-of
>> feels a bit redundant.  How about assuming that the entry size for
>> a string is 1 (i.e. that the string is has single-byte characters)
>> if no size is explicitly given, just like we assume @progbits for most
>> sections when no type is given?
> 
> Well. I've never liked the (silent) defaulting to @progbits. I therefore also
> wouldn't like silent defaulting to entry size 1 for strings. At the same time,
> if someone proposed a patch with reasonable justification, I also wouldn't
> mean to stand in the way of it going in.

I don't think it's unreasonable to assume that SHF_STRINGS defaults to 
progbits.

> 
>> If instead we require users to update their code, it would be worth
>> mentioning the change in gas/NEWS.
> 
> Hmm, maybe. I have to confess I didn't expect people would be taking shortcuts
> as you mention they do, so I didn't even consider the need for a NEWS entry.
> 

The real question is whether they have been taking shortcuts, though. 
Is there something that can't be intuited from the assembler source file 
that really needs this to be explicit every time?

I can't really see that. Yes the generated ELF file should have the 
relevant flags and fields set, but if the assembler can reasonably 
intuit that from the source code, that's not a problem (in my eyes).

The truth is that we've accepted this ever since we implemented ELF 
support in Gas, and I'm not aware of any user bug reports showing that 
it's led to situations where gas just can't get the output right.  Hence 
I think that having real-world examples of code that will have to be 
rewritten as a result of that change.

Are there bugs that I'm not aware of?  Or is this some rather arbitrary 
rewriting of the /assembler source code/ rules?

R.