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 25590
Build 24191: 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

Were there any conflicts in the merge?

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
1007	Add missing "!Config->ZIfuncnoplt" condition.

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

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.

Fix merge issues

Harbormaster completed remote builds in B25569: Diff 60243.Jul 29 2019, 7:29 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
1007	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#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.

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.

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

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?

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?

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.

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

Add missing space

Harbormaster completed remote builds in B25604: Diff 60303.Jul 30 2019, 6:49 PM

Nice, thank you.

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.

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

Diff 60279

View Options

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

This file was added.

Property	Old Value	New Value
svn:eol-style	null	native \ No newline at end of property
svn:keywords	null	FreeBSD=%H \ No newline at end of property
svn:mime-type	null	text/plain \ No newline at end of property

				/*-
				* Copyright (c) 2014 The FreeBSD Foundation
				* All rights reserved.
				*
				* This software was developed by Andrew Turner under
				* sponsorship from the FreeBSD Foundation.
				*
				* Redistribution and use in source and binary forms, with or without
				* modification, are permitted provided that the following conditions
				* are met:
				* 1. Redistributions of source code must retain the above copyright
				* notice, this list of conditions and the following disclaimer.
				* 2. Redistributions in binary form must reproduce the above copyright
				* notice, this list of conditions and the following disclaimer in the
				* documentation and/or other materials provided with the distribution.
				*
				* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
				* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
				* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
				* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
				* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
				* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
				* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
				* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
				* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
				* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
				* SUCH DAMAGE.
				*/

				#include <machine/asm.h>
				__FBSDID("$FreeBSD$");

				ENTRY(.rtld_start)
				mov x19, x0 /* Put ps_strings in a callee-saved register */
				mov x20, sp /* And the stack pointer */

				sub x8, x20, #16 /* Make room for obj_main & exit proc */
				mov sp, x8 /* Update the stack pointer */

				adr x3, .Ldelta
				ldr x2, [x3]
				sub x1, x3, x2 /* Load the relocation delta */
				adr x0, _DYNAMIC /* Load &_DYNAMIC */
				bl reloc_non_plt_self

				mov x0, x20 /* Pass the stack we were given to _rtld */
				mov x1, sp /* exit_proc */
				add x2, x1, #8 /* obj_main */
				/* Good */
				bl _rtld /* Call the loader */
				mov x8, x0 /* Backup the entry point */

				ldr x2, [sp] /* Load cleanup */
				ldr x1, [sp, #8] /* Load obj_main */
				mov x0, x19 /* Restore ps_strings */
				mov sp, x20 /* Restore the stack pointer */
				br x8 /* Jump to the entry point */
				.Ldelta:
				.word .
				.word 0
				END(.rtld_start)

				/*
				* uint64_t _rtld_tlsdesc(struct tlsdesc *);
				*
				* struct tlsdesc {
				* uint64_t ptr;
				* uint64_t data;
				* };
				*
				* Returns the data.
				*/
				ENTRY(_rtld_tlsdesc)
				ldr x0, [x0, #8]
				RET
				END(_rtld_tlsdesc)

View Options

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

This file was added.

Property	Old Value	New Value
svn:eol-style	null	native \ No newline at end of property
svn:keywords	null	FreeBSD=%H \ No newline at end of property
svn:mime-type	null	text/plain \ No newline at end of property

				/*-
				* Copyright (c) 2014 The FreeBSD Foundation
				* All rights reserved.
				*
				* This software was developed by Andrew Turner under
				* sponsorship from the FreeBSD Foundation.
				*
				* Redistribution and use in source and binary forms, with or without
				* modification, are permitted provided that the following conditions
				* are met:
				* 1. Redistributions of source code must retain the above copyright
				* notice, this list of conditions and the following disclaimer.
				* 2. Redistributions in binary form must reproduce the above copyright
				* notice, this list of conditions and the following disclaimer in the
				* documentation and/or other materials provided with the distribution.
				*
				* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
				* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
				* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
				* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
				* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
				* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
				* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
				* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
				* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
				* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
				* SUCH DAMAGE.
				*/

				#include <machine/asm.h>
				__FBSDID("$FreeBSD$");

				ENTRY(.rtld_start)
				mov x19, x0 /* Put ps_strings in a callee-saved register */
				mov x20, sp /* And the stack pointer */

				sub x8, x20, #16 /* Make room for obj_main & exit proc */
				mov sp, x8 /* Update the stack pointer */

				adr x3, .Ldelta
				ldr x2, [x3]
				sub x1, x3, x2 /* Load the relocation delta */
				adr x0, _DYNAMIC /* Load &_DYNAMIC */
				bl reloc_non_plt_self

				mov x0, x20 /* Pass the stack we were given to _rtld */
				mov x1, sp /* exit_proc */
				add x2, x1, #8 /* obj_main */
				/* Good */
				bl _rtld /* Call the loader */
				mov x8, x0 /* Backup the entry point */

				ldr x2, [sp] /* Load cleanup */
				ldr x1, [sp, #8] /* Load obj_main */
				mov x0, x19 /* Restore ps_strings */
				mov sp, x20 /* Restore the stack pointer */
				br x8 /* Jump to the entry point */
				.Ldelta:
				.word .
				.word 0
				END(.rtld_start)

				/*
				* uint64_t _rtld_tlsdesc(struct tlsdesc *);
				*
				* struct tlsdesc {
				* uint64_t ptr;
				* uint64_t data;
				* };
				*
				* Returns the data.
				*/
				ENTRY(_rtld_tlsdesc)
				ldr x0, [x0, #8]
				RET
				END(_rtld_tlsdesc)

View Options

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

This file was added.

Property	Old Value	New Value
svn:eol-style	null	native \ No newline at end of property
svn:keywords	null	FreeBSD=%H \ No newline at end of property
svn:mime-type	null	text/plain \ No newline at end of property

				/*-
				* Copyright (c) 2014 The FreeBSD Foundation
				* All rights reserved.
				*
				* This software was developed by Andrew Turner under
				* sponsorship from the FreeBSD Foundation.
				*
				* Redistribution and use in source and binary forms, with or without
				* modification, are permitted provided that the following conditions
				* are met:
				* 1. Redistributions of source code must retain the above copyright
				* notice, this list of conditions and the following disclaimer.
				* 2. Redistributions in binary form must reproduce the above copyright
				* notice, this list of conditions and the following disclaimer in the
				* documentation and/or other materials provided with the distribution.
				*
				* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
				* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
				* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
				* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
				* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
				* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
				* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
				* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
				* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
				* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
				* SUCH DAMAGE.
				*/

				#include <machine/asm.h>
				__FBSDID("$FreeBSD$");

				ENTRY(.rtld_start)
				mov x19, x0 /* Put ps_strings in a callee-saved register */
				mov x20, sp /* And the stack pointer */

				sub x8, x20, #16 /* Make room for obj_main & exit proc */
				mov sp, x8 /* Update the stack pointer */

				adr x3, .Ldelta
				ldr x2, [x3]
				sub x1, x3, x2 /* Load the relocation delta */
				adr x0, _DYNAMIC /* Load &_DYNAMIC */
				bl reloc_non_plt_self

				mov x0, x20 /* Pass the stack we were given to _rtld */
				mov x1, sp /* exit_proc */
				add x2, x1, #8 /* obj_main */
				/* Good */
				bl _rtld /* Call the loader */
				mov x8, x0 /* Backup the entry point */

				ldr x2, [sp] /* Load cleanup */
				ldr x1, [sp, #8] /* Load obj_main */
				mov x0, x19 /* Restore ps_strings */
				mov sp, x20 /* Restore the stack pointer */
				br x8 /* Jump to the entry point */
				.Ldelta:
				.word .
				.word 0
				END(.rtld_start)

				/*
				* uint64_t _rtld_tlsdesc(struct tlsdesc *);
				*
				* struct tlsdesc {
				* uint64_t ptr;
				* uint64_t data;
				* };
				*
				* Returns the data.
				*/
				ENTRY(_rtld_tlsdesc)
				ldr x0, [x0, #8]
				RET
				END(_rtld_tlsdesc)

View Options

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

This file was added.

Property	Old Value	New Value
svn:eol-style	null	native \ No newline at end of property
svn:keywords	null	FreeBSD=%H \ No newline at end of property
svn:mime-type	null	text/plain \ No newline at end of property

				/*-
				* Copyright (c) 2014 The FreeBSD Foundation
				* All rights reserved.
				*
				* This software was developed by Andrew Turner under
				* sponsorship from the FreeBSD Foundation.
				*
				* Redistribution and use in source and binary forms, with or without
				* modification, are permitted provided that the following conditions
				* are met:
				* 1. Redistributions of source code must retain the above copyright
				* notice, this list of conditions and the following disclaimer.
				* 2. Redistributions in binary form must reproduce the above copyright
				* notice, this list of conditions and the following disclaimer in the
				* documentation and/or other materials provided with the distribution.
				*
				* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
				* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
				* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
				* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
				* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
				* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
				* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
				* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
				* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
				* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
				* SUCH DAMAGE.
				*/

				#include <machine/asm.h>
				__FBSDID("$FreeBSD$");

				ENTRY(.rtld_start)
				mov x19, x0 /* Put ps_strings in a callee-saved register */
				mov x20, sp /* And the stack pointer */

				sub x8, x20, #16 /* Make room for obj_main & exit proc */
				mov sp, x8 /* Update the stack pointer */

				adr x3, .Ldelta
				ldr x2, [x3]
				sub x1, x3, x2 /* Load the relocation delta */
				adr x0, _DYNAMIC /* Load &_DYNAMIC */
				bl reloc_non_plt_self

				mov x0, x20 /* Pass the stack we were given to _rtld */
				mov x1, sp /* exit_proc */
				add x2, x1, #8 /* obj_main */
				/* Good */
				bl _rtld /* Call the loader */
				mov x8, x0 /* Backup the entry point */

				ldr x2, [sp] /* Load cleanup */
				ldr x1, [sp, #8] /* Load obj_main */
				mov x0, x19 /* Restore ps_strings */
				mov sp, x20 /* Restore the stack pointer */
				br x8 /* Jump to the entry point */
				.Ldelta:
				.word .
				.word 0
				END(.rtld_start)

				/*
				* uint64_t _rtld_tlsdesc(struct tlsdesc *);
				*
				* struct tlsdesc {
				* uint64_t ptr;
				* uint64_t data;
				* };
				*
				* Returns the data.
				*/
				ENTRY(_rtld_tlsdesc)
				ldr x0, [x0, #8]
				RET
				END(_rtld_tlsdesc)

View Options

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

View Options

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

View Options

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

[PPC64] Backport fix for missing IRELATIVE relocations
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 60279

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

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

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

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

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

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

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

[PPC64] Backport fix for missing IRELATIVE relocationsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 60279

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

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

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

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

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

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

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

[PPC64] Backport fix for missing IRELATIVE relocations
ClosedPublic
Actions

Revision Contents
Changeset List