-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[RelLookupTableConverter] Drop unnamed_addr to avoid generating GOTPCREL relocations #142304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-llvm-transforms Author: dianqk (dianqk) ChangesFollow #72584 (comment), the patch will drop the But I don't think this will result in worse problems. > LLVM provides that the calculation of such a constant initializer will not overflow at link time under the medium code model if x is an unnamed_addr function. However, it does not provide this guarantee for a constant initializer folded into a function body. This intrinsic can be used to avoid the possibility of overflows when loading from such a constant. (‘llvm.load.relative’ Intrinsic) This is my concern. I'm not sure how unnamed_addr provides this guarantee, and I haven't found any test cases. Full diff: https://github.com/llvm/llvm-project/pull/142304.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp b/llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp
index 2700b4307308c..8bad67044bea4 100644
--- a/llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp
+++ b/llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp
@@ -110,6 +110,11 @@ static GlobalVariable *createRelLookupTable(Function &Func,
for (Use &Operand : LookupTableArr->operands()) {
Constant *Element = cast<Constant>(Operand);
+ // Drop unnamed_addr to avoid matching pattern in
+ // `handleIndirectSymViaGOTPCRel`, which creates GOTPCREL relocations not
+ // supported by the GNU linker and LLD versions below 18 on aarch64.
+ if (auto *GlobalElement = dyn_cast<GlobalValue>(Element))
+ GlobalElement->setUnnamedAddr(GlobalValue::UnnamedAddr::None);
Type *IntPtrTy = M.getDataLayout().getIntPtrType(M.getContext());
Constant *Base = llvm::ConstantExpr::getPtrToInt(RelLookupTable, IntPtrTy);
Constant *Target = llvm::ConstantExpr::getPtrToInt(Element, IntPtrTy);
diff --git a/llvm/test/Transforms/RelLookupTableConverter/unnamed_addr.ll b/llvm/test/Transforms/RelLookupTableConverter/unnamed_addr.ll
new file mode 100644
index 0000000000000..748d7522c1097
--- /dev/null
+++ b/llvm/test/Transforms/RelLookupTableConverter/unnamed_addr.ll
@@ -0,0 +1,88 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals all --version 5
+; RUN: opt < %s -passes=rel-lookup-table-converter -relocation-model=pic -mtriple=x86_64 -S | FileCheck -check-prefix=x86_64 %s
+; RUN: opt < %s -passes=rel-lookup-table-converter -relocation-model=pic -mtriple=aarch64 -S | FileCheck -check-prefix=aarch64 %s
+
+@a0 = private unnamed_addr constant i32 0
+@a1 = private unnamed_addr constant i32 1
+@a2 = private unnamed_addr constant i32 2
+@load_relative_1.table = private unnamed_addr constant [3 x ptr] [ptr @a0, ptr @a1, ptr @a2]
+
+@x0 = internal unnamed_addr constant i64 0
+@x1 = internal unnamed_addr constant i64 1
+@x2 = internal unnamed_addr constant i64 2
+@x3 = internal unnamed_addr constant i64 3
+@y0 = internal unnamed_addr constant ptr @x3
+@y1 = internal unnamed_addr constant ptr @x2
+@y2 = internal unnamed_addr constant ptr @x1
+@y3 = internal unnamed_addr constant ptr @x0
+@load_relative_2.table = private unnamed_addr constant [4 x ptr] [ptr @y3, ptr @y2, ptr @y1, ptr @y0]
+
+;.
+; x86_64: @a0 = private constant i32 0
+; x86_64: @a1 = private constant i32 1
+; x86_64: @a2 = private constant i32 2
+; x86_64: @load_relative_1.table.rel = private unnamed_addr constant [3 x i32] [i32 trunc (i64 sub (i64 ptrtoint (ptr @a0 to i64), i64 ptrtoint (ptr @load_relative_1.table.rel to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr @a1 to i64), i64 ptrtoint (ptr @load_relative_1.table.rel to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr @a2 to i64), i64 ptrtoint (ptr @load_relative_1.table.rel to i64)) to i32)], align 4
+; x86_64: @x0 = internal unnamed_addr constant i64 0
+; x86_64: @x1 = internal unnamed_addr constant i64 1
+; x86_64: @x2 = internal unnamed_addr constant i64 2
+; x86_64: @x3 = internal unnamed_addr constant i64 3
+; x86_64: @y0 = internal constant ptr @x3
+; x86_64: @y1 = internal constant ptr @x2
+; x86_64: @y2 = internal constant ptr @x1
+; x86_64: @y3 = internal constant ptr @x0
+; x86_64: @load_relative_2.table.rel = private unnamed_addr constant [4 x i32] [i32 trunc (i64 sub (i64 ptrtoint (ptr @y3 to i64), i64 ptrtoint (ptr @load_relative_2.table.rel to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr @y2 to i64), i64 ptrtoint (ptr @load_relative_2.table.rel to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr @y1 to i64), i64 ptrtoint (ptr @load_relative_2.table.rel to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr @y0 to i64), i64 ptrtoint (ptr @load_relative_2.table.rel to i64)) to i32)], align 4
+;.
+; aarch64: @a0 = private constant i32 0
+; aarch64: @a1 = private constant i32 1
+; aarch64: @a2 = private constant i32 2
+; aarch64: @load_relative_1.table.rel = private unnamed_addr constant [3 x i32] [i32 trunc (i64 sub (i64 ptrtoint (ptr @a0 to i64), i64 ptrtoint (ptr @load_relative_1.table.rel to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr @a1 to i64), i64 ptrtoint (ptr @load_relative_1.table.rel to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr @a2 to i64), i64 ptrtoint (ptr @load_relative_1.table.rel to i64)) to i32)], align 4
+; aarch64: @x0 = internal unnamed_addr constant i64 0
+; aarch64: @x1 = internal unnamed_addr constant i64 1
+; aarch64: @x2 = internal unnamed_addr constant i64 2
+; aarch64: @x3 = internal unnamed_addr constant i64 3
+; aarch64: @y0 = internal constant ptr @x3
+; aarch64: @y1 = internal constant ptr @x2
+; aarch64: @y2 = internal constant ptr @x1
+; aarch64: @y3 = internal constant ptr @x0
+; aarch64: @load_relative_2.table.rel = private unnamed_addr constant [4 x i32] [i32 trunc (i64 sub (i64 ptrtoint (ptr @y3 to i64), i64 ptrtoint (ptr @load_relative_2.table.rel to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr @y2 to i64), i64 ptrtoint (ptr @load_relative_2.table.rel to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr @y1 to i64), i64 ptrtoint (ptr @load_relative_2.table.rel to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr @y0 to i64), i64 ptrtoint (ptr @load_relative_2.table.rel to i64)) to i32)], align 4
+;.
+define ptr @load_relative_1(i64 %offset) {
+; x86_64-LABEL: define ptr @load_relative_1(
+; x86_64-SAME: i64 [[OFFSET:%.*]]) {
+; x86_64-NEXT: [[RELTABLE_SHIFT:%.*]] = shl i64 [[OFFSET]], 2
+; x86_64-NEXT: [[RELTABLE_INTRINSIC:%.*]] = call ptr @llvm.load.relative.i64(ptr @load_relative_1.table.rel, i64 [[RELTABLE_SHIFT]])
+; x86_64-NEXT: ret ptr [[RELTABLE_INTRINSIC]]
+;
+; aarch64-LABEL: define ptr @load_relative_1(
+; aarch64-SAME: i64 [[OFFSET:%.*]]) {
+; aarch64-NEXT: [[RELTABLE_SHIFT:%.*]] = shl i64 [[OFFSET]], 2
+; aarch64-NEXT: [[RELTABLE_INTRINSIC:%.*]] = call ptr @llvm.load.relative.i64(ptr @load_relative_1.table.rel, i64 [[RELTABLE_SHIFT]])
+; aarch64-NEXT: ret ptr [[RELTABLE_INTRINSIC]]
+;
+ %gep = getelementptr inbounds [3 x ptr], ptr @load_relative_1.table, i64 0, i64 %offset
+ %load = load ptr, ptr %gep
+ ret ptr %load
+}
+
+define ptr @load_relative_2(i64 %offset) {
+; x86_64-LABEL: define ptr @load_relative_2(
+; x86_64-SAME: i64 [[OFFSET:%.*]]) {
+; x86_64-NEXT: [[RELTABLE_SHIFT:%.*]] = shl i64 [[OFFSET]], 2
+; x86_64-NEXT: [[RELTABLE_INTRINSIC:%.*]] = call ptr @llvm.load.relative.i64(ptr @load_relative_2.table.rel, i64 [[RELTABLE_SHIFT]])
+; x86_64-NEXT: ret ptr [[RELTABLE_INTRINSIC]]
+;
+; aarch64-LABEL: define ptr @load_relative_2(
+; aarch64-SAME: i64 [[OFFSET:%.*]]) {
+; aarch64-NEXT: [[RELTABLE_SHIFT:%.*]] = shl i64 [[OFFSET]], 2
+; aarch64-NEXT: [[RELTABLE_INTRINSIC:%.*]] = call ptr @llvm.load.relative.i64(ptr @load_relative_2.table.rel, i64 [[RELTABLE_SHIFT]])
+; aarch64-NEXT: ret ptr [[RELTABLE_INTRINSIC]]
+;
+ %gep = getelementptr inbounds [4 x ptr], ptr @load_relative_2.table, i64 0, i64 %offset
+ %load = load ptr, ptr %gep
+ ret ptr %load
+}
+;.
+; x86_64: attributes #[[ATTR0:[0-9]+]] = { nocallback nofree nosync nounwind willreturn memory(argmem: read) }
+;.
+; aarch64: attributes #[[ATTR0:[0-9]+]] = { nocallback nofree nosync nounwind willreturn memory(argmem: read) }
+;.
|
@@ -110,6 +110,11 @@ static GlobalVariable *createRelLookupTable(Function &Func, | |||
|
|||
for (Use &Operand : LookupTableArr->operands()) { | |||
Constant *Element = cast<Constant>(Operand); | |||
// Drop unnamed_addr to avoid matching pattern in | |||
// `handleIndirectSymViaGOTPCRel`, which generates GOTPCREL relocations not | |||
// supported by the GNU linker and LLD versions below 18 on aarch64. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we are working around problems with specific linkers on specific platforms, we should add triple checks like you originally had, for aarch64 and for x86-darwin (and list which linker versions are affected, so this can be removed in the future).
Does BFD not support GOTPCREL on aarch64 even in the newest version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does BFD not support GOTPCREL on aarch64 even in the newest version?
Probably nobody noticed this.
/nix/...-binutils-aarch64-unknown-linux-gnu-2.44/bin/ld: /path/1.rcgu.o: unrecognized relocation type 0x13b in section `.rodata..Lswitch.table.bad.rel'
/nix/...-binutils-aarch64-unknown-linux-gnu-2.44/bin/ld: is this version of the linker - (GNU Binutils) 2.44 - out of date ?
/nix/...-binutils-aarch64-unknown-linux-gnu-2.44/bin/ld: final link failed: bad value
collect2: error: ld returned 1 exit status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. It would be good to file a bug for that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. It would be good to file a bug for that...
I'm still in the process of creating an account. :\ (https://sourceware.org/bugzilla/createaccount.cgi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/26573 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/16826 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/10902 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/6454 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/17506 Here is the relevant piece of the build log for the reference
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need REQUIRES: x86-registered-target
and REQUIRES: aarch64-registered-target
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, added: de7f2fb
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/24193 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/133/builds/17092 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/17728 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/20928 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/16878 Here is the relevant piece of the build log for the reference
|
…REL relocations (llvm#142304) Follow llvm#72584 (comment), the patch will drop the `unnamed_addr` attribute when generating relative lookup tables. I'm not very confident about this patch, but it does resolve rust-lang/rust#140686, rust-lang/rust#141306 and rust-lang/rust#141737. But I don't think this will result in worse problems. > LLVM provides that the calculation of such a constant initializer will not overflow at link time under the medium code model if x is an unnamed_addr function. However, it does not provide this guarantee for a constant initializer folded into a function body. This intrinsic can be used to avoid the possibility of overflows when loading from such a constant. ([‘llvm.load.relative’ Intrinsic](https://llvm.org/docs/LangRef.html#id2592)) This is my concern. I'm not sure how unnamed_addr provides this guarantee, and I haven't found any test cases. (cherry picked from commit aa09dbb)
Backport PR: #142311. |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/18582 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/18439 Here is the relevant piece of the build log for the reference
|
Rather than removing |
Does this sound like it will prevent the generation of all GOTPCRELs? This PR is to prevent "accidental" (we doesn't explicitly submit it) GOTPCREL generation. |
Oh yeah you're right. Disregard my last comment |
…REL relocations (llvm#142304) Follow llvm#72584 (comment), the patch will drop the `unnamed_addr` attribute when generating relative lookup tables. I'm not very confident about this patch, but it does resolve rust-lang/rust#140686, rust-lang/rust#141306 and rust-lang/rust#141737. But I don't think this will result in worse problems. > LLVM provides that the calculation of such a constant initializer will not overflow at link time under the medium code model if x is an unnamed_addr function. However, it does not provide this guarantee for a constant initializer folded into a function body. This intrinsic can be used to avoid the possibility of overflows when loading from such a constant. ([‘llvm.load.relative’ Intrinsic](https://llvm.org/docs/LangRef.html#id2592)) This is my concern. I'm not sure how unnamed_addr provides this guarantee, and I haven't found any test cases. (cherry picked from commit aa09dbb)
…REL relocations (llvm#142304) Follow llvm#72584 (comment), the patch will drop the `unnamed_addr` attribute when generating relative lookup tables. I'm not very confident about this patch, but it does resolve rust-lang/rust#140686, rust-lang/rust#141306 and rust-lang/rust#141737. But I don't think this will result in worse problems. > LLVM provides that the calculation of such a constant initializer will not overflow at link time under the medium code model if x is an unnamed_addr function. However, it does not provide this guarantee for a constant initializer folded into a function body. This intrinsic can be used to avoid the possibility of overflows when loading from such a constant. ([‘llvm.load.relative’ Intrinsic](https://llvm.org/docs/LangRef.html#id2592)) This is my concern. I'm not sure how unnamed_addr provides this guarantee, and I haven't found any test cases.
Follow #72584 (comment), the patch will drop the
unnamed_addr
attribute when generating relative lookup tables. I'm not very confident about this patch, but it does resolve rust-lang/rust#140686, rust-lang/rust#141306 and rust-lang/rust#141737.But I don't think this will result in worse problems.
This is my concern. I'm not sure how unnamed_addr provides this guarantee, and I haven't found any test cases.