[PATCH 3/4] ELF: SHF_STRINGS isn't really tied to SHF_MERGE
Richard Earnshaw (lists)
Richard.Earnshaw@arm.com
Fri Jan 17 11:21:54 GMT 2025
On 17/01/2025 11:13, H.J. Lu wrote:
> On Fri, Jan 17, 2025 at 6:31 PM Richard Earnshaw (lists)
> <Richard.Earnshaw@arm.com> wrote:
>>
>> 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.
>
> It is very undesirable to require changing existing assembly codes
> in the real world. We shouldn't make such assembler changes.
> It may be OK for the assembler to issue a warning. But it should
> keep generating correct and working object files.
>
Just to be clear, I'm not disputing with Jan that we were previously
generating invalid object files; the discussion is over how to address
that. I think we should set the ENTSIZE field to address this problem.
Gas is currently (after Jan's patch) removing SHF_STRINGS, which IMO is
not the right solution.
R.
More information about the Binutils
mailing list