[PPC64] Backport fix for missing IRELATIVE relocations

[luporl]

Summary

This is a backport of LLVM commit 8331f61a51a7a0a1efbf5ed398e181593023d151, llvm-svn: 353981:

ELF: Allow GOT relocs pointing to non-preemptable ifunc to resolve to an IRELATIVE where possible.

This is needed in order to make ifuncs work correctly on PPC64.

It fixes an issue with lld, in which it would skip emitting necessary IRELATIVE relocations.
Without this change, indirect calls to ifuncs would result in a segmentation fault, in static binaries or when defined in the main binary (outside shared libraries).

[markj]

Were there any conflicts in the merge?

[luporl]

In D21102#458061, @markj wrote:

Were there any conflicts in the merge?

There was only one, in line 1010. The original source didn't have the "!Config->ZIfuncnoplt" condition in it.
Actually, it seems I forgot to add this condition in the merged code. I'll fix it.

contrib/llvm/tools/lld/ELF/Relocations.cpp

1010 ↗

(On Diff #60237)

Add missing "!Config->ZIfuncnoplt" condition.

[dim]

In D21102#458082, @luporl wrote:

In D21102#458061, @markj wrote:

Were there any conflicts in the merge?

There was only one, in line 1010. The original source didn't have the "!Config->ZIfuncnoplt" condition in it.

Hmm, that reminds me, those changes have to be upstreamed at some point. Looks like this was rS38251, but the upstream discussion seems to have stalled:

http://lists.llvm.org/pipermail/llvm-dev/2018-August/125719.html

[markj]

In D21102#458084, @dim wrote:

In D21102#458082, @luporl wrote:

In D21102#458061, @markj wrote:

Were there any conflicts in the merge?

There was only one, in line 1010. The original source didn't have the "!Config->ZIfuncnoplt" condition in it.

Hmm, that reminds me, those changes have to be upstreamed at some point. Looks like this was rS38251, but the upstream discussion seems to have stalled:

http://lists.llvm.org/pipermail/llvm-dev/2018-August/125719.html

It was committed upstream: https://reviews.llvm.org/rGe041d15f5e326f7ee0b1ea9c7a988f8965ce7e6a

@luporl it may be easiest to revert our local changes which add -zifunc-noplt, and then apply the above-mentioned commit on top of the one that you are porting.

[luporl]

• Fix merge issues

[luporl]

I guess it is ok now.

I've also checked LLVM upstream version of Relocations.cpp and the logic matches, except for the part to display the warning message, in which the upstream version doesn't check for !Config->ZIfuncnoplt, while this patch does, preserving old behavior.

[markj]

contrib/llvm/tools/lld/ELF/Relocations.cpp

1010 ↗

(On Diff #60237)

I don't think we need to keep that piece of the diff. The warning is only printed if the warning flag is set, and I think it's a legitimate warning in most cases.

[dim]

In D21102#458090, @markj wrote:

In D21102#458084, @dim wrote:

In D21102#458082, @luporl wrote:

In D21102#458061, @markj wrote:

Were there any conflicts in the merge?

There was only one, in line 1010. The original source didn't have the "!Config->ZIfuncnoplt" condition in it.

Hmm, that reminds me, those changes have to be upstreamed at some point. Looks like this was rS38251, but the upstream discussion seems to have stalled:

http://lists.llvm.org/pipermail/llvm-dev/2018-August/125719.html

It was committed upstream: https://reviews.llvm.org/rGe041d15f5e326f7ee0b1ea9c7a988f8965ce7e6a

Aha, I had not yet noticed that! It will be interesting when I start merging lld 9.0.0... :)

Indeed it may be better if we attempt to undo our local changes, and adapt that upstream commit, both for this review and for later wholesale merges from upstream.

[markj]

In D21102#458238, @dim wrote:

In D21102#458090, @markj wrote:

In D21102#458084, @dim wrote:

In D21102#458082, @luporl wrote:

In D21102#458061, @markj wrote:

Were there any conflicts in the merge?

There was only one, in line 1010. The original source didn't have the "!Config->ZIfuncnoplt" condition in it.

Hmm, that reminds me, those changes have to be upstreamed at some point. Looks like this was rS38251, but the upstream discussion seems to have stalled:

http://lists.llvm.org/pipermail/llvm-dev/2018-August/125719.html

It was committed upstream: https://reviews.llvm.org/rGe041d15f5e326f7ee0b1ea9c7a988f8965ce7e6a

Aha, I had not yet noticed that! It will be interesting when I start merging lld 9.0.0... :)

Indeed it may be better if we attempt to undo our local changes, and adapt that upstream commit, both for this review and for later wholesale merges from upstream.

I think that's the right approach. Our version of the patch is functionally the same as upstream's, so we might as well drop our version now.

[luporl]

• Match upstream behavior

[luporl]

Now the merge result should match the behavior seen in upstream.

Was this what you had in mind? Or is the plan to revert the local changes and apply this patch on it instead?

[markj]

In D21102#458391, @luporl wrote:

Now the merge result should match the behavior seen in upstream.

Was this what you had in mind? Or is the plan to revert the local changes and apply this patch on it instead?

I think we should indeed revert the local changes and apply the two patches from upstream, but IMO it's fine to do that in a single commit. I believe that is effectively what you have done here?

[luporl]

In D21102#458426, @markj wrote:

In D21102#458391, @luporl wrote:

Now the merge result should match the behavior seen in upstream.

Was this what you had in mind? Or is the plan to revert the local changes and apply this patch on it instead?

I think we should indeed revert the local changes and apply the two patches from upstream, but IMO it's fine to do that in a single commit. I believe that is effectively what you have done here?

Actually, I've done it slightly different.
The first patch from upstream was applied without reverting the local changes, as there is only one place that both touch (Relocations.cpp).
The conflicting part was then resolved in a way that matches what is in upstream.

But this would indeed make it harder to update the second patch to the upstream version.
Let me redo it, in the way you have suggested.

[luporl]

Redo merge:

• reverted previous changes
• reverted -zifunc-noplt commit
• applied IRELATIVE fix
• applied upstream -zifunc-noplt commit

[luporl]

• Add missing space

[markj]

Nice, thank you.

[markj]

I'll give this patch a quick spin on amd64 and verify that -zifunc-noplt still works as expected.

[markj]

In D21102#458544, @markj wrote:

I'll give this patch a quick spin on amd64 and verify that -zifunc-noplt still works as expected.

LGTM