[PATCH] D41613: [ELF] - Do not use HeaderSize for conditions in PltSection.

Rui Ueyama via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 10 23:35:03 PST 2018


On Wed, Jan 10, 2018 at 9:01 AM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> Rui Ueyama <ruiu at google.com> writes:
>
> > On Tue, Jan 9, 2018 at 5:47 PM, Rafael Avila de Espindola <
> > rafael.espindola at gmail.com> wrote:
> >
> >> Rui Ueyama <ruiu at google.com> writes:
> >>
> >> > On Tue, Jan 9, 2018 at 4:52 PM, Rafael Avila de Espindola <
> >> > rafael.espindola at gmail.com> wrote:
> >> >
> >> >> Rui Ueyama via Phabricator <reviews at reviews.llvm.org> writes:
> >> >>
> >> >> > ruiu added a comment.
> >> >> >
> >> >> > Isn't it an ABI violation not to write the PLT header for PPC64? If
> >> so,
> >> >> we fix that ABI violation rather than "correctly" emitting
> ABI-violating
> >> >> executables.
> >> >>
> >> >> The PPC64 implementation is incomplete. Finishing it would be nice,
> but
> >> >> independent of this change.
> >> >>
> >> >> Without this change a target must set Target->PltHeaderSize to avoid
> >> >> having lld confuse the plt and iplt sections, which is not a good
> design
> >> >> IMHO.
> >> >>
> >> >
> >> > It is okay to be incomplete, but I'm not very fond of making a change
> >> that
> >> > is not needed for the proper ABI. How about setting a non-zero value
> to
> >> the
> >> > PPC64 PLT header? We don't have to fill the actual instruction
> sequence.
> >> > Setting a non-zero value should fix the issue, right?
> >>
> >> I think the way to look at this patch is a cleanup, not a fix.
> >>
> >> I find
> >>
> >>   if (IsIplt) {
> >>
> >> A lot easier to read than
> >>
> >>   if (HeaderSize == 0) {
> >>
> >> I does happen to fix a problem with the ppc64 target, but that is almost
> >> incidental. And yes, it could be made a pure cleanup by setting the
> >> ppc64 PltHeaderSize to a nonzero value first.
> >>
> >
> > Sure. But if PLTs for all targets have the PLT header for lazy symbol
> > resolution (I believe that's the case), and IPLT doesn't have one (I
> > believe that's true too), then we can use `bool is Iplt() { return
> > HeaderSize == 0; }` instead of adding a new boolean flag to the ctor.
>
> But that is a coincidence that happens to be true is most targets, no?
>

I don't think that's a coincidence. If you implement lazy resolution, it is
a natural consequence to have a PLT header. It can be PLT trailer or can
reside in other section, I doubt if such ABI exists.

Even if we consider this patch as a cleanup, this patch is incomplete. I'll
send follow-up comments.

Why are all targets required to have a plt header? Some as George point
> out don't even have a plt, let alone lazy resolution.
>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180110/c0399383/attachment.html>


More information about the llvm-commits mailing list