This is an archive of the discontinued LLVM Phabricator instance.

lld/ELF/Arch/PPC64.cpp
1018	@MaskRay comments on the other patch about lowercase hex literals applies here as well. Ditto for the suggested test change (ie labels instead of sections)
lld/test/ELF/ppc64-got-pcrel34-reloc.s
33 ↗	(On Diff #271128)	It this test just to show the linker doesn't modify the load instruction that follows the PC-relative load from the got?

MaskRay added a subscriber: grimar.Jun 17 2020, 11:33 AM

MaskRay added inline comments.

lld/ELF/Arch/PPC64.cpp
1018	In https://reviews.llvm.org/D81986#inline-753874 @grimar asked whether we should use `const`. It seems to me that is a good idea. Many other components of LLVM use `const` whenever appropriate.
lld/test/ELF/ppc64-got-pcrel34-reloc.s
1 ↗	(On Diff #271128)	The naming is `ppc64-reloc-*` (x86-64-reloc- tests stick with this rule more consistently)
18 ↗	(On Diff #271128)	Consider using a label instead of a `.section` as the anchor in the output. This allows you to use `-NEXT:` which makes tests easier to update when something goes off. # CHECK: <foo>: # CHECK-NEXT: pld 3, ... # I tend to indent instructions to make it clear the region is covered by the previous label Omit unneeded addresses unless you do some arithmetic and somewhere else in the test requires exact addresses. In that case, you should write the formula in a comment (example: ppc64-tls-*.s)
33 ↗	(On Diff #271128)	If you want to check not TOC (a variant of GOT) optimization is performed, use -no-pie (default) or -pie, instead of -shared.
44 ↗	(On Diff #271128)	This creates a large file. Use a linker script like ppc32-long-thunk.s

stefanp marked an inline comment as done.Jun 18 2020, 4:16 AM

stefanp added inline comments.

lld/test/ELF/ppc64-got-pcrel34-reloc.s
33 ↗	(On Diff #271128)	Yes, this test is just to make sure that we don't make changes to the `lwa`. It may not be necessary from that perspective because that instruction does not have a relocation on it so there really is no risk of this problem. However, in my head they are the part of the same set so I want to check them both. I'm not trying to check for not TOC. We have removed all of the TOC references on the compiler side. Or, maybe I didn't understand your comment...

Addressed review comments.

LGTM, with one last nit. Please wait for @sfertile's approval.

lld/test/ELF/ppc64-reloc-got-pcrel34.s
29	It seems strange that `lwa` is indented differently. Please fix it.

This revision is now accepted and ready to land.Jun 18 2020, 12:57 PM

LGTM.

lld/ELF/Arch/PPC64.cpp
1018	Big +1 to judiciously adding const wherever appropriate.
lld/test/ELF/ppc64-got-pcrel34-reloc.s
33 ↗	(On Diff #271128)	If it was to test we don't try to perform toc-optimizations then I was going to say you need to make it an executable like MaskRay suggested. But since its to make sure we don't modify past the end of the prefixed instructions its good the way you have it, this test won't change when you do go to implement the got optimizations which is how we want it I think 👍
lld/test/ELF/ppc64-reloc-got-pcrel34.s
19	If the 'SYMBOL' checks are to verify the referenced symbols make it into the dynamic symbol table maybe have a first check line: `SYMBOL: Symbol table '.dynsym' contains 4 entries:`

Updated with last nits.

Closed by commit rG8131ef5d635b: [LLD][PowerPC] Add support for R_PPC64_GOT_PCREL34 (authored by stefanp, committed by kamaub). · Explain WhyJun 24 2020, 5:54 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents

Path

Size

lld/

ELF/

Arch/

PPC64.cpp

13 lines

test/

ELF/

ppc64-reloc-got-pcrel34.s

51 lines

Diff 273005

lld/ELF/Arch/PPC64.cpp

Show First 20 Lines • Show All 665 Lines • ▼ Show 20 Lines	RelExpr PPC64::getRelExpr(RelType type, const Symbol &s,
case R_PPC64_GOT16_LO:		case R_PPC64_GOT16_LO:
case R_PPC64_GOT16_LO_DS:		case R_PPC64_GOT16_LO_DS:
return R_GOT_OFF;		return R_GOT_OFF;
case R_PPC64_TOC16:		case R_PPC64_TOC16:
case R_PPC64_TOC16_DS:		case R_PPC64_TOC16_DS:
case R_PPC64_TOC16_HI:		case R_PPC64_TOC16_HI:
case R_PPC64_TOC16_LO:		case R_PPC64_TOC16_LO:
return R_GOTREL;		return R_GOTREL;
		case R_PPC64_GOT_PCREL34:
		return R_GOT_PC;
case R_PPC64_TOC16_HA:		case R_PPC64_TOC16_HA:
case R_PPC64_TOC16_LO_DS:		case R_PPC64_TOC16_LO_DS:
return config->tocOptimize ? R_PPC64_RELAX_TOC : R_GOTREL;		return config->tocOptimize ? R_PPC64_RELAX_TOC : R_GOTREL;
case R_PPC64_TOC:		case R_PPC64_TOC:
return R_PPC64_TOCBASE;		return R_PPC64_TOCBASE;
case R_PPC64_REL14:		case R_PPC64_REL14:
case R_PPC64_REL24:		case R_PPC64_REL24:
return R_PPC64_CALL_PLT;		return R_PPC64_CALL_PLT;
▲ Show 20 Lines • Show All 325 Lines • ▼ Show 20 Lines	case R_PPC64_PCREL34: {
const uint64_t fullMask = 0x0003ffff0000ffff;		const uint64_t fullMask = 0x0003ffff0000ffff;
checkInt(loc, val, 34, rel);		checkInt(loc, val, 34, rel);

uint64_t instr = readPrefixedInstruction(loc) & ~fullMask;		uint64_t instr = readPrefixedInstruction(loc) & ~fullMask;
writePrefixedInstruction(loc, instr \| ((val & si0Mask) << 16) \|		writePrefixedInstruction(loc, instr \| ((val & si0Mask) << 16) \|
(val & si1Mask));		(val & si1Mask));
break;		break;
}		}
		case R_PPC64_GOT_PCREL34: {
		const uint64_t si0Mask = 0x00000003ffff0000;
		sfertileUnsubmitted Not Done Reply Inline Actions @MaskRay comments on the other patch about lowercase hex literals applies here as well. Ditto for the suggested test change (ie labels instead of sections) sfertile: @MaskRay comments on the other patch about lowercase hex literals applies here as well. Ditto…
		MaskRayUnsubmitted Not Done Reply Inline Actions In https://reviews.llvm.org/D81986#inline-753874 @grimar asked whether we should use `const`. It seems to me that is a good idea. Many other components of LLVM use `const` whenever appropriate. MaskRay: In https://reviews.llvm.org/D81986#inline-753874 @grimar asked whether we should use `const`.
		sfertileUnsubmitted Not Done Reply Inline Actions Big +1 to judiciously adding const wherever appropriate. sfertile: Big +1 to judiciously adding const wherever appropriate.
		const uint64_t si1Mask = 0x000000000000ffff;
		const uint64_t fullMask = 0x0003ffff0000ffff;
		checkInt(loc, val, 34, rel);

		uint64_t instr = readPrefixedInstruction(loc) & ~fullMask;
		writePrefixedInstruction(loc, instr \| ((val & si0Mask) << 16) \|
		(val & si1Mask));
		break;
		}
default:		default:
llvm_unreachable("unknown relocation");		llvm_unreachable("unknown relocation");
}		}
}		}

bool PPC64::needsThunk(RelExpr expr, RelType type, const InputFile *file,		bool PPC64::needsThunk(RelExpr expr, RelType type, const InputFile *file,
uint64_t branchAddr, const Symbol &s, int64_t a) const {		uint64_t branchAddr, const Symbol &s, int64_t a) const {
if (type != R_PPC64_REL14 && type != R_PPC64_REL24)		if (type != R_PPC64_REL14 && type != R_PPC64_REL24)
▲ Show 20 Lines • Show All 199 Lines • Show Last 20 Lines

lld/test/ELF/ppc64-reloc-got-pcrel34.s

This file was added.

				# REQUIRES: ppc
				# RUN: echo 'SECTIONS { \
				# RUN: .text_low 0x10010000: { *(.text_low) } \
				# RUN: .text_high 0x10080000 : { *(.text_high) } \
				# RUN: }' > %t.script

				# RUN: llvm-mc -filetype=obj -triple=powerpc64le %s -o %t.o
				# RUN: ld.lld -T %t.script --shared %t.o -o %t
				# RUN: llvm-readelf -s %t \| FileCheck %s --check-prefix=SYMBOL
				# RUN: llvm-readelf -r %t \| FileCheck %s --check-prefix=RELA
				# RUN: llvm-objdump -d --no-show-raw-insn --mcpu=future %t \| FileCheck %s

				# RUN: llvm-mc -filetype=obj -triple=powerpc64 %s -o %t.o
				# RUN: ld.lld -T %t.script --shared %t.o -o %t
				# RUN: llvm-readelf -s %t \| FileCheck %s --check-prefix=SYMBOL
				# RUN: llvm-readelf -r %t \| FileCheck %s --check-prefix=RELA
				# RUN: llvm-objdump -d --no-show-raw-insn --mcpu=future %t \| FileCheck %s

				.text
				sfertileUnsubmitted Not Done Reply Inline Actions If the 'SYMBOL' checks are to verify the referenced symbols make it into the dynamic symbol table maybe have a first check line: `SYMBOL: Symbol table '.dynsym' contains 4 entries:` sfertile: If the 'SYMBOL' checks are to verify the referenced symbols make it into the dynamic symbol…
				.section .text_low, "ax", %progbits
				# CHECK-LABEL: <GlobIntPCRel>:
				# CHECK-NEXT: pld 3, 458928(0), 1
				# CHECK-NEXT: lwa 3, 0(3)
				# SYMBOL: Symbol table '.dynsym' contains 4 entries:
				# SYMBOL: 00000000 0 NOTYPE GLOBAL DEFAULT UND glob_int
				# RELA: 100800b0 0000000100000014 R_PPC64_GLOB_DAT 0000000000000000 glob_int + 0
				GlobIntPCRel:
				pld 3, glob_int@got@PCREL(0), 1
				lwa 3, 0(3)
				MaskRayUnsubmitted Not Done Reply Inline Actions It seems strange that `lwa` is indented differently. Please fix it. MaskRay: It seems strange that `lwa` is indented differently. Please fix it.
				blr

				# CHECK-LABEL: <GlobIntPCRelOffset>:
				# CHECK-NEXT: pld 3, 458920(0), 1
				# CHECK-NEXT: lwa 3, 8(3)
				# SYMBOL: 00000000 0 NOTYPE GLOBAL DEFAULT UND glob_int8
				# RELA: 100800b8 0000000200000014 R_PPC64_GLOB_DAT 0000000000000000 glob_int8 + 0
				GlobIntPCRelOffset:
				pld 3, glob_int8@got@PCREL(0), 1
				lwa 3, 8(3)
				blr

				# CHECK-LABEL: <GlobIntPCRelBigOffset>:
				# CHECK-NEXT: pld 3, 192(0), 1
				# CHECK-NEXT: lwa 3, 64(3)
				# SYMBOL: 00000000 0 NOTYPE GLOBAL DEFAULT UND glob_int8_big
				# RELA: 100800c0 0000000300000014 R_PPC64_GLOB_DAT 0000000000000000 glob_int8_big + 0
				.section .text_high, "ax", %progbits
				GlobIntPCRelBigOffset:
				pld 3, glob_int8_big@got@PCREL(0), 1
				lwa 3, 64(3)
				blr

This is an archive of the discontinued LLVM Phabricator instance.

[LLD][PowerPC] Add support for R_PPC64_GOT_PCREL34ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents

Diff 273005

lld/ELF/Arch/PPC64.cpp

lld/test/ELF/ppc64-reloc-got-pcrel34.s

[LLD][PowerPC] Add support for R_PPC64_GOT_PCREL34
ClosedPublic