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

[Richard Earnshaw]

On 16/01/2025 11:34, Richard Earnshaw (lists) wrote:
> 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.
> 
> 

Or to put it another way, if 'S' is present in the flags, but 'M' is 
not, then the assembler defaults to progbits and element size 1.

R.