Page MenuHomeFreeBSD

[PPC64] Backport fix for missing IRELATIVE relocations
ClosedPublic

Authored by luporl on Jul 29 2019, 5:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 24, 7:24 AM
Unknown Object (File)
Sat, Mar 23, 7:52 AM
Unknown Object (File)
Jan 29 2024, 10:22 AM
Unknown Object (File)
Jan 29 2024, 10:21 AM
Unknown Object (File)
Jan 29 2024, 10:21 AM
Unknown Object (File)
Jan 27 2024, 10:09 PM
Unknown Object (File)
Jan 27 2024, 10:09 PM
Unknown Object (File)
Jan 27 2024, 10:03 PM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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

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.

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

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.

This revision now requires review to proceed.Jul 30 2019, 12:29 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?

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.

Redo merge:

  • reverted previous changes
  • reverted -zifunc-noplt commit
  • applied IRELATIVE fix
  • applied upstream -zifunc-noplt commit
This revision is now accepted and ready to land.Jul 30 2019, 6:50 PM

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

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.