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

[Jan Beulich]

On 16.01.2025 16:11, Richard Earnshaw wrote:
> On 16/01/2025 15:08, Jan Beulich wrote:
>> On 16.01.2025 12:39, Richard Earnshaw wrote:
>>> 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?
>>>
>>> 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.
>>
>> Which may or may not match section contents. The section could very well
>> contain UCS-2 or UCS-4 strings, for example. We wouldn't know that until
>> we actually get to see directives filling the section, which is after we
>> parse the section directive.
> 
> And at which point you could legitimately emit an error.  But if the 
> section contained .asciz, there's no need to.

Yet deferring the diagnostic from parsing the .section until (perhaps much)
later isn't quite straightforward, I'm afraid. Like elsewhere, for the time
being we need to take the .section directive all by itself ... (It's also
not clear to me what exact heuristics we might want to employ to suppress
the diagnostic: Imo we ought to emit it unless there's no doubt at all.)

Jan