Page MenuHomeFreeBSD

[PPC64] Backport fix for missing IRELATIVE relocations
ClosedPublic

Authored by luporl on Jul 29 2019, 5:34 PM.

Details

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).

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

luporl created this revision.Jul 29 2019, 5:34 PM
emaste added a reviewer: dim.Jul 29 2019, 5:39 PM
emaste added a subscriber: markj.Jul 29 2019, 5:41 PM
markj added a comment.Jul 29 2019, 6:00 PM

Were there any conflicts in the merge?

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 added a comment.EditedJul 29 2019, 6:19 PM

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 added a comment.Jul 29 2019, 6:45 PM
In D21102#458084, @dim 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 updated this revision to Diff 60243.Jul 29 2019, 7:29 PM
  • Fix merge issues

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 accepted this revision.Jul 29 2019, 7:53 PM
markj added inline comments.
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.

This revision is now accepted and ready to land.Jul 29 2019, 7:53 PM
dim added a comment.Jul 29 2019, 9:28 PM
In D21102#458084, @dim 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 added a comment.Jul 29 2019, 9:31 PM
In D21102#458238, @dim wrote:
In D21102#458084, @dim 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 updated this revision to Diff 60279.Jul 30 2019, 12:29 PM
  • Match upstream behavior
This revision now requires review to proceed.Jul 30 2019, 12:29 PM
luporl marked an inline comment as done.Jul 30 2019, 12:40 PM

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 added a comment.Jul 30 2019, 2:09 PM

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?

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 updated this revision to Diff 60302.Jul 30 2019, 6:39 PM

Redo merge:

  • reverted previous changes
  • reverted -zifunc-noplt commit
  • applied IRELATIVE fix
  • applied upstream -zifunc-noplt commit
luporl updated this revision to Diff 60303.Jul 30 2019, 6:49 PM
  • Add missing space
markj accepted this revision.Jul 30 2019, 6:50 PM

Nice, thank you.

This revision is now accepted and ready to land.Jul 30 2019, 6:50 PM
markj added a comment.Jul 30 2019, 6:51 PM

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

markj added a comment.Jul 30 2019, 7:20 PM

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

LGTM

This revision was automatically updated to reflect the committed changes.