[PATCH 3/4] ELF: SHF_STRINGS isn't really tied to SHF_MERGE
H.J. Lu
hjl.tools@gmail.com
Fri Jan 17 11:13:38 GMT 2025
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.
--
H.J.
More information about the Binutils
mailing list