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

[Rui Ueyama via llvm-commits]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180109/5ab00241/attachment.html>