Page MenuHomeFreeBSD
Differential D21102

[PPC64] Backport fix for missing IRELATIVE relocations
ClosedPublic
Actions

Authored by luporl on Jul 29 2019, 5:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jun 3, 5:52 PM
Unknown Object (File)
Wed, May 28, 6:06 AM
Unknown Object (File)
Mon, May 19, 12:04 PM
Unknown Object (File)
Sat, May 17, 2:03 AM
Unknown Object (File)
Sun, May 11, 7:28 PM
Unknown Object (File)
Sat, May 10, 7:29 PM
Unknown Object (File)
May 9 2025, 10:26 AM
Unknown Object (File)
May 7 2025, 8:15 AM
View All Files
Subscribers
alfredo
imp
markj

Details

Reviewers
emaste
jhibbits
bdragon
dim
markj
Commits
rS350467: [PPC64] Backport fix for missing IRELATIVE relocations
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

Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 25604
Build 24204: arc lint + arc unit

Event Timeline

luporl created this revision.Jul 29 2019, 5:34 PM
Harbormaster completed remote builds in B25565: Diff 60237.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?

luporl added a comment.Jul 29 2019, 6:17 PM
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
999

Add missing "!Config->ZIfuncnoplt" condition.

dim added a comment.EditedJul 29 2019, 6:19 PM
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 added a comment.Jul 29 2019, 6:45 PM
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 updated this revision to Diff 60243.Jul 29 2019, 7:29 PM
  • Fix merge issues
Harbormaster completed remote builds in B25569: Diff 60243.Jul 29 2019, 7:29 PM
luporl added a comment.Jul 29 2019, 7:36 PM

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
999

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#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 added a comment.Jul 29 2019, 9:31 PM
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 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
Harbormaster completed remote builds in B25590: Diff 60279.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
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 added a comment.Jul 30 2019, 5:15 PM
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 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
Harbormaster completed remote builds in B25603: Diff 60302.Jul 30 2019, 6:39 PM
luporl updated this revision to Diff 60303.Jul 30 2019, 6:49 PM
  • Add missing space
Harbormaster completed remote builds in B25604: Diff 60303.Jul 30 2019, 6:49 PM
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
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

Closed by commit rS350467: [PPC64] Backport fix for missing IRELATIVE relocations (authored by luporl). · Explain WhyJul 31 2019, 4:22 PM
This revision was automatically updated to reflect the committed changes.
luporl added a commit: rS350467: [PPC64] Backport fix for missing IRELATIVE relocations.
Herald added a subscriber: imp. · View Herald TranscriptJul 31 2019, 4:22 PM
dim mentioned this in rS360658: Merge llvm, clang, compiler-rt, libc++, libunwind, lld, lldb and openmp.May 5 2020, 1:05 PM

Revision Contents
Changeset List

PathSize
contrib/
llvm/
tools/
lld/
ELF/
2 lines
8 lines
6 lines
10 lines
264 lines
10 lines
10 lines
5 lines
10 lines

Diff 60303

View Options

contrib/llvm/tools/lld/ELF/Config.h

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

contrib/llvm/tools/lld/ELF/Symbols.h

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...