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

[Rafael Avila de Espindola via llvm-commits]

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?
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