[PATCH 3/4] ELF: SHF_STRINGS isn't really tied to SHF_MERGE
Richard Earnshaw (lists)
Richard.Earnshaw@arm.com
Thu Jan 16 17:37:03 GMT 2025
On 16/01/2025 17:07, Jan Beulich wrote:
> On 16.01.2025 17:34, Richard Earnshaw (lists) wrote:
>> On 16/01/2025 16:26, Jan Beulich wrote:
>>> On 16.01.2025 17:08, Richard Earnshaw (lists) wrote:
>>>> On 16/01/2025 15:56, Jan Beulich wrote:
>>>>> 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.)
>>>>
>>>> On the other hand, we shouldn't reject existing code unless there are
>>>> really good reasons for doing so.
>>>
>>> We don't reject it, do we? It's an as_warn() only afaics.
>
> With this in mind, ...
>
>>>> I'm really not sure why the tools need to know element size on
>>>> non-mergeable strings anyway. It's not like they really need to parse
>>>> the strings themselves (they couldn't anyway as they don't know which
>>>> code table is being used).
>>>>
>>>> So does it *really* matter if the element size is guessed incorrectly
>>>> (as it always has been so far)?
>>>
>>> The spec is quite clear about it mattering - both flags have "The size of
>>> each {element,character} is specified in the section header's sh_entsize
>>> field." Hence the answer to your question is down to all possible consumers
>>> of ELF objects. It seems pretty likely to me that with what the spec says
>>> some may actually use sh_entsize for something even in the absence of
>>> SHF_MERGE.
>>
>> Maybe I'm missing something, but unless the strings are being merged, I
>> can't really see why they would possibly care if it wasn't right (I can
>> understand why it might be needed for merging).
>>
>> But we're not really talking about what goes in the *object* file, but
>> what is written in the *source* file. If the assembler can reasonably
>> fill in the blanks during assembly, it shouldn't force the programmer to
>> do it.
>>
>> I don't think it's unreasonable for the assembler to interpret missing
>> type and element sizes as being progbits and 1 for strings (the world is
>> still predominantly ASCII based, or at least byte-based. We provide a
>> mechanism to give other values, but I can't really see why we have to
>> insist on them being used in every case.
>
> ... why do you again say "insist"? Even if we guessed the value, we ought
> to indicate that by way of a warning. And it's a warning only now, unless
> I'm overlooking something. The difference is merely what to do in the
> case we're warning about: You suggest to default the entry size, whereas
> right now we clear SHF_MERGE and SHF_STRINGS.
>
The only way to stop the assembler from nagging is to change existing
code that previously worked without problems.
> Similarly I'm of the opinion that in principle we also ought to indicate
> (by way of a warning) when we guess the section type, for example. Except
> that I understand that such a warning would be too noisy for many peoples'
> tastes.
So it's a matter of degree. You're happy with the assembler silently
guessing when 'progbits' was implied, but not about guessing that the
element size of a string is 1. This is really about the lack of
evidence that the guess is leading to the tools processing code
incorrectly. I can see that if the user then puts in 'strings' with a
different size that might be considered wrong, but the problem then is
about the subsequent directives, not with the default behaviour for
handling omitted parameters.
Aside:
in C++ I can declare a function such as
int create_section(SecType t=progbits, int eltsize=1);
then simply call create_section() with no arguments. It doesn't
generate any warnings because it's just used the defaults; but if I want
a different value it's up to me to specify it. This is pretty much the
same sort of situation.
End-aside.
> Plus the doc explicitly names section type as optional, while at
> the same time it (now) properly says
>
> "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: ..."
>
> (Previously it said so only for M.)
Right, so you've changed the spec for assembler source code. I don't
really agree that is necessary.
Why can't the documentation be changed to say that if type and element
size are omitted for 'S' then these are defaulted to progbits and 1
rather than requiring programmers to 'fix' code that has always worked?
Making the assembler warn if, say, .asciz appeared in a string section
with a different element size would not be unreasonable. It's perhaps a
bit harder to warn for other element types, but we could certainly check
that a strings section had element size worth of zeros at the end and
warn if that were not the case.
R.
More information about the Binutils
mailing list