[PATCH 3/4] ELF: SHF_STRINGS isn't really tied to SHF_MERGE
Richard Earnshaw (lists)
Richard.Earnshaw@arm.com
Fri Jan 17 10:30:22 GMT 2025
On 17/01/2025 07:19, Jan Beulich wrote:
> On 16.01.2025 19:27, Richard Earnshaw wrote:
>> On 16/01/2025 17:37, Richard Earnshaw (lists) wrote:
>>> 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.
>
> Note what I said here, for ...
>
>>> 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.
>>
>> I've just noticed that in addition to the warning, you clear SHF_STRINGS
>> if a size hasn't been set, so your patch really has changed assembler
>> behaviour compared to the old code.
>
> ... what you now mention as well.
>
> As to your earlier reply: No, I indicated before that I'm not happy with
> the defaulting to @progbits. I merely, in my more recent reply, can see
> how changing that might cause too much fallout.
>
> To repeat in a more general manner what I said earlier: I'm generally
> unhappy about _any_ silent defaulting, unless that's clearly mandated by
> whatever spec. Another area where I mentioned this long ago is the
> defaulting of section attributes from section names - they're different
> and independent things.
I have no problem with defaulting some things (especially if there's a
way to override the default choices). There's a big difference between
what users need to write in assembly source files and what we eventually
have to encode in the object file. The former should be as succinct
(and where possible, portable) as possible, while the latter can have
the required, but largely obvious blanks filled in by the assembler as
it reads the sources.
Where I disagree with your approach is how to generate something
conforming from the 'incomplete' information. Your changes seem to be
taking the approach of *removing* things that the programmer has
explicitly stated (eg clearing SHF_MERGE/SHF_FLAGS) in order to create a
file that conforms to the spec, but doesn't conform to what the
programmer asked for. I think we should be filling in the missing
information where we can reasonably (or due to our assembler specs, not
the ELF specs) deduce what the programmer intended.
We can remove the guesswork by documenting what choices the assembler
will make when things are missing - selecting progbits is one such
choice, then it simply becomes a matter of fully documenting our choices
and completing the missing bits (or, from an alternative perspective
starting with a template with those bits filled in, and overwriting the
bits that the user was explicit about).
>
>> else if ((attr & (SHF_MERGE | SHF_STRINGS)) != 0)
>> {
>> as_warn (_("entity size for SHF_MERGE / SHF_STRINGS not specified"));
>> attr &= ~(SHF_MERGE | SHF_STRINGS);
>> }
>>
>>
>> how about, if instead we had
>>
>> else if ((attr & SHF_MERGE) != 0)
>> {
>> as_warn (_("entity size for SHF_MERGE not specified"));
>> attr &= ~(SHF_MERGE | SHF_STRINGS);
>> }
>> else if ((attr & SHF_STRINGS) != 0)
>> {
>> as_tsktsk (_"assuming entity size of 1 for SHF_STRINGS"));
>> entsize = 1;
>> }
>>
>> as_tsktsk is slightly weaker as a warning in that it doesn't cause
>> assembler failure with --fatal-warnings.
>
> After thinking this over some more I was going to suggest something
> similar, just not with as_tsktsk(): How about we introduce a separate
> control for pedantic warnings (e.g. --ped-warn and --no-ped-warn), to
> use here and maybe also for the @progbits defaulting (and anything
> alike)?
That sounds like a pretty fundamental change to be making this late in
the development cycle. I think we need something that can be fixed
before we branch and that sounds like quite a lot of additional change.
>
> However, while having a way to simply silence such warnings without
> silencing all warning may be viewed as desirable in some cases, the
> problem with downgrading such warnings further is that I'll then
> expect people to complain that the assembler silently did something
> the programmer didn't ask for. (I'd like to mention in this context
> that I find it odd that while as_warn() can be silenced, as_tsktsk()
> can't be. Hence why I didn't consider using it here.)
Yes, there are anomalies in the assembler behaviour, but tsktsk is
intended for cases where the assembler is pretty sure that it's
assumptions are safe or correct, whereas as_warn is intended for cases
where there is a guess, but it's very much a guess.
>
> What to fall back to is an orthogonal question: I can see your solution
> as being as good or bad as mine: Your suggestion would change assembler
> behavior, too - so far we didn't set sh_entsize to non-zero in such
> cases. The only thing I'll insist on is that we produce a spec-
> conforming object file.
Yes, in that we previously produced an object file that conformed to hte
programmers source code, but not to the spec. With your change it
no-longer produces something conforming to the source code (you've
cleared SHF_STRINGS) although it is conforming to the ELF spec. That,
IMO is simply wrong.
R.
More information about the Binutils
mailing list