[PATCH] D110304: [HIP] Fix linking of asanrt.bc
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Closed by commit rGc4afb5f81b62: [HIP] Fix linking of asanrt.bc (authored by
yaxunl).
Herald added a project: clang.
Changed prior to commit:
https://reviews.llvm.org/D110304?vs=374868&id=375329#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110304/new/
https://reviews.llvm.org/D110304
Files:
clang/include/clang/Driver/ToolChain.h
clang/lib/Driver/ToolChain.cpp
clang/lib/Driver/ToolChains/HIP.cpp
clang/lib/Driver/ToolChains/HIP.h
clang/test/CodeGenCUDA/Inputs/amdgpu-asanrtl.ll
clang/test/CodeGenCUDA/amdgpu-asan.cu
clang/test/Driver/hip-sanitize-options.hip
Index: clang/test/Driver/hip-sanitize-options.hip
===
--- clang/test/Driver/hip-sanitize-options.hip
+++ clang/test/Driver/hip-sanitize-options.hip
@@ -34,7 +34,7 @@
// CHECK-NOT: {{"[^"]*lld(\.exe){0,1}".* ".*hip.bc"}}
// CHECK: {{"[^"]*clang[^"]*".* "-triple" "x86_64-unknown-linux-gnu".* "-fsanitize=address"}}
-// NORDC: {{"[^"]*clang[^"]*".* "-emit-obj".* "-fcuda-is-device".* "-fsanitize=address".*}} "-o" "[[OUT:[^"]*.o]]"
+// NORDC: {{"[^"]*clang[^"]*".* "-emit-obj".* "-fcuda-is-device".* "-mlink-bitcode-file" ".*asanrtl.bc".* "-mlink-builtin-bitcode" ".*hip.bc".* "-fsanitize=address".*}} "-o" "[[OUT:[^"]*.o]]"
// NORDC: {{"[^"]*lld(\.exe){0,1}".*}} "[[OUT]]" {{".*asanrtl.bc" ".*hip.bc"}}
// NORDC: {{"[^"]*clang[^"]*".* "-triple" "x86_64-unknown-linux-gnu".* "-fsanitize=address"}}
Index: clang/test/CodeGenCUDA/amdgpu-asan.cu
===
--- clang/test/CodeGenCUDA/amdgpu-asan.cu
+++ clang/test/CodeGenCUDA/amdgpu-asan.cu
@@ -1,6 +1,20 @@
+// Create a sample address sanitizer bitcode library.
+
+// RUN: %clang_cc1 -x ir -fcuda-is-device -triple amdgcn-amd-amdhsa -emit-llvm-bc \
+// RUN: -disable-llvm-passes -o %t.asanrtl.bc %S/Inputs/amdgpu-asanrtl.ll
+
+// Check sanitizer runtime library functions survive
+// optimizations without being removed or parameters altered.
+
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=amdgcn-amd-amdhsa \
+// RUN: -fcuda-is-device -target-cpu gfx906 -fsanitize=address \
+// RUN: -mlink-bitcode-file %t.asanrtl.bc -x hip \
+// RUN: | FileCheck -check-prefix=ASAN %s
+
// RUN: %clang_cc1 %s -emit-llvm -o - -triple=amdgcn-amd-amdhsa \
// RUN: -fcuda-is-device -target-cpu gfx906 -fsanitize=address \
-// RUN: -x hip | FileCheck -check-prefix=ASAN %s
+// RUN: -O3 -mlink-bitcode-file %t.asanrtl.bc -x hip \
+// RUN: | FileCheck -check-prefix=ASAN %s
// RUN: %clang_cc1 %s -emit-llvm -o - -triple=amdgcn-amd-amdhsa \
// RUN: -fcuda-is-device -target-cpu gfx906 -x hip \
@@ -8,8 +22,10 @@
// REQUIRES: amdgpu-registered-target
-// ASAN-DAG: declare void @__amdgpu_device_library_preserve_asan_functions()
+// ASAN-DAG: define weak void @__amdgpu_device_library_preserve_asan_functions()
// ASAN-DAG: @__amdgpu_device_library_preserve_asan_functions_ptr = weak addrspace(1) constant void ()* @__amdgpu_device_library_preserve_asan_functions
// ASAN-DAG: @llvm.compiler.used = {{.*}}@__amdgpu_device_library_preserve_asan_functions_ptr
+// ASAN-DAG: define weak void @__asan_report_load1(i64 %{{.*}})
-// CHECK-NOT: @__amdgpu_device_library_preserve_asan_functions_ptr
+// CHECK-NOT: @__amdgpu_device_library_preserve_asan_functions
+// CHECK-NOT: @__asan_report_load1
Index: clang/test/CodeGenCUDA/Inputs/amdgpu-asanrtl.ll
===
--- /dev/null
+++ clang/test/CodeGenCUDA/Inputs/amdgpu-asanrtl.ll
@@ -0,0 +1,13 @@
+; Sample code for amdgpu address sanitizer runtime.
+
+; Note the runtime functions need to have weak linkage and default
+; visibility, otherwise they may be internalized and removed by GlobalOptPass.
+
+define weak void @__amdgpu_device_library_preserve_asan_functions() {
+ tail call void @__asan_report_load1(i64 0)
+ ret void
+}
+
+define weak void @__asan_report_load1(i64 %0) {
+ ret void
+}
Index: clang/lib/Driver/ToolChains/HIP.h
===
--- clang/lib/Driver/ToolChains/HIP.h
+++ clang/lib/Driver/ToolChains/HIP.h
@@ -83,7 +83,7 @@
llvm::opt::ArgStringList &CC1Args) const override;
void AddHIPIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const override;
- llvm::SmallVector
+ llvm::SmallVector
getHIPDeviceLibs(const llvm::opt::ArgList &Args) const override;
SanitizerMask getSupportedSanitizers() const override;
Index: clang/lib/Driver/ToolChains/HIP.cpp
===
--- clang/lib/Driver/ToolChains/HIP.cpp
+++ clang/lib/Driver/ToolChains/HIP.cpp
@@ -88,8 +88,8 @@
[PATCH] D110304: [HIP] Fix linking of asanrt.bc
yaxunl marked an inline comment as done.
yaxunl added inline comments.
Comment at: clang/lib/Driver/ToolChains/HIP.cpp:413
} else
-BCLibs.push_back(AsanRTL.str());
+BCLibs.push_back({AsanRTL.str(), false});
}
tra wrote:
> Nit: I'd add `/*ShouldInternalize=*/`
will do when committing
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110304/new/
https://reviews.llvm.org/D110304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110304: [HIP] Fix linking of asanrt.bc
tra accepted this revision.
tra added inline comments.
This revision is now accepted and ready to land.
Comment at: clang/lib/Driver/ToolChains/HIP.cpp:413
} else
-BCLibs.push_back(AsanRTL.str());
+BCLibs.push_back({AsanRTL.str(), false});
}
Nit: I'd add `/*ShouldInternalize=*/`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110304/new/
https://reviews.llvm.org/D110304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110304: [HIP] Fix linking of asanrt.bc
yaxunl updated this revision to Diff 374868.
yaxunl marked an inline comment as done.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Revised by Artem's comments
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110304/new/
https://reviews.llvm.org/D110304
Files:
clang/include/clang/Driver/ToolChain.h
clang/lib/Driver/ToolChain.cpp
clang/lib/Driver/ToolChains/HIP.cpp
clang/lib/Driver/ToolChains/HIP.h
clang/test/CodeGenCUDA/Inputs/amdgpu-asanrtl.ll
clang/test/CodeGenCUDA/amdgpu-asan.cu
clang/test/Driver/hip-sanitize-options.hip
Index: clang/test/Driver/hip-sanitize-options.hip
===
--- clang/test/Driver/hip-sanitize-options.hip
+++ clang/test/Driver/hip-sanitize-options.hip
@@ -34,7 +34,7 @@
// CHECK-NOT: {{"[^"]*lld(\.exe){0,1}".* ".*hip.bc"}}
// CHECK: {{"[^"]*clang[^"]*".* "-triple" "x86_64-unknown-linux-gnu".* "-fsanitize=address"}}
-// NORDC: {{"[^"]*clang[^"]*".* "-emit-obj".* "-fcuda-is-device".* "-fsanitize=address".*}} "-o" "[[OUT:[^"]*.o]]"
+// NORDC: {{"[^"]*clang[^"]*".* "-emit-obj".* "-fcuda-is-device".* "-mlink-bitcode-file" ".*asanrtl.bc".* "-mlink-builtin-bitcode" ".*hip.bc".* "-fsanitize=address".*}} "-o" "[[OUT:[^"]*.o]]"
// NORDC: {{"[^"]*lld(\.exe){0,1}".*}} "[[OUT]]" {{".*asanrtl.bc" ".*hip.bc"}}
// NORDC: {{"[^"]*clang[^"]*".* "-triple" "x86_64-unknown-linux-gnu".* "-fsanitize=address"}}
Index: clang/test/CodeGenCUDA/amdgpu-asan.cu
===
--- clang/test/CodeGenCUDA/amdgpu-asan.cu
+++ clang/test/CodeGenCUDA/amdgpu-asan.cu
@@ -1,6 +1,20 @@
+// Create a sample address sanitizer bitcode library.
+
+// RUN: %clang_cc1 -x ir -fcuda-is-device -triple amdgcn-amd-amdhsa -emit-llvm-bc \
+// RUN: -disable-llvm-passes -o %t.asanrtl.bc %S/Inputs/amdgpu-asanrtl.ll
+
+// Check sanitizer runtime library functions survive
+// optimizations without being removed or parameters altered.
+
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=amdgcn-amd-amdhsa \
+// RUN: -fcuda-is-device -target-cpu gfx906 -fsanitize=address \
+// RUN: -mlink-bitcode-file %t.asanrtl.bc -x hip \
+// RUN: | FileCheck -check-prefix=ASAN %s
+
// RUN: %clang_cc1 %s -emit-llvm -o - -triple=amdgcn-amd-amdhsa \
// RUN: -fcuda-is-device -target-cpu gfx906 -fsanitize=address \
-// RUN: -x hip | FileCheck -check-prefix=ASAN %s
+// RUN: -O3 -mlink-bitcode-file %t.asanrtl.bc -x hip \
+// RUN: | FileCheck -check-prefix=ASAN %s
// RUN: %clang_cc1 %s -emit-llvm -o - -triple=amdgcn-amd-amdhsa \
// RUN: -fcuda-is-device -target-cpu gfx906 -x hip \
@@ -8,8 +22,10 @@
// REQUIRES: amdgpu-registered-target
-// ASAN-DAG: declare void @__amdgpu_device_library_preserve_asan_functions()
+// ASAN-DAG: define weak void @__amdgpu_device_library_preserve_asan_functions()
// ASAN-DAG: @__amdgpu_device_library_preserve_asan_functions_ptr = weak addrspace(1) constant void ()* @__amdgpu_device_library_preserve_asan_functions
// ASAN-DAG: @llvm.compiler.used = {{.*}}@__amdgpu_device_library_preserve_asan_functions_ptr
+// ASAN-DAG: define weak void @__asan_report_load1(i64 %{{.*}})
-// CHECK-NOT: @__amdgpu_device_library_preserve_asan_functions_ptr
+// CHECK-NOT: @__amdgpu_device_library_preserve_asan_functions
+// CHECK-NOT: @__asan_report_load1
Index: clang/test/CodeGenCUDA/Inputs/amdgpu-asanrtl.ll
===
--- /dev/null
+++ clang/test/CodeGenCUDA/Inputs/amdgpu-asanrtl.ll
@@ -0,0 +1,13 @@
+; Sample code for amdgpu address sanitizer runtime.
+
+; Note the runtime functions need to have weak linkage and default
+; visibility, otherwise they may be internalized and removed by GlobalOptPass.
+
+define weak void @__amdgpu_device_library_preserve_asan_functions() {
+ tail call void @__asan_report_load1(i64 0)
+ ret void
+}
+
+define weak void @__asan_report_load1(i64 %0) {
+ ret void
+}
Index: clang/lib/Driver/ToolChains/HIP.h
===
--- clang/lib/Driver/ToolChains/HIP.h
+++ clang/lib/Driver/ToolChains/HIP.h
@@ -83,7 +83,7 @@
llvm::opt::ArgStringList &CC1Args) const override;
void AddHIPIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const override;
- llvm::SmallVector
+ llvm::SmallVector
getHIPDeviceLibs(const llvm::opt::ArgList &Args) const override;
SanitizerMask getSupportedSanitizers() const override;
Index: clang/lib/Driver/ToolChains/HIP.cpp
===
--- clang/lib/Driver/ToolChains/HIP.cpp
+++ clang/lib/Driver/ToolChains/HIP.cpp
@@ -88,8 +88,8 @@
if (Args.hasFlag(options::OPT_fgpu_sanitize, options::OPT_fno_gpu_sanitize,
false))
-llvm::for_each(TC.getHIPDeviceLibs(Args), [&](StringRef BCFile) {
- LldArgs.push_back(Args.MakeArgString(BCFile));
[PATCH] D110304: [HIP] Fix linking of asanrt.bc
yaxunl marked an inline comment as done.
yaxunl added inline comments.
Comment at: clang/include/clang/Driver/ToolChain.h:116-117
+ // Enums corresponding to clang options for linking bitcode, i.e.,
+ // -mlink-builtin-bitcode or -mlink-bitcode-file
+ enum BitCodeLinkOpt {
tra wrote:
> It appears that what we dealing with here is an on/off switch for whether we
> want to internalize symbols.
>
> Perhaps it should be implemented as such.
> getHIPDeviceLibs() will set the bool flag indicating *what* we want to do
> with the library and then the toolchain-specific code will decide *how* to
> make it happen. Currently it translates into specific linking option, but it
> may be something else. getHIPDeviceLibs does not need to know that.
>
> The code will remain essentially the same, it's mostly the naming exercise.
>
>
will do
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110304/new/
https://reviews.llvm.org/D110304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110304: [HIP] Fix linking of asanrt.bc
tra added inline comments.
Comment at: clang/include/clang/Driver/ToolChain.h:116-117
+ // Enums corresponding to clang options for linking bitcode, i.e.,
+ // -mlink-builtin-bitcode or -mlink-bitcode-file
+ enum BitCodeLinkOpt {
It appears that what we dealing with here is an on/off switch for whether we
want to internalize symbols.
Perhaps it should be implemented as such.
getHIPDeviceLibs() will set the bool flag indicating *what* we want to do with
the library and then the toolchain-specific code will decide *how* to make it
happen. Currently it translates into specific linking option, but it may be
something else. getHIPDeviceLibs does not need to know that.
The code will remain essentially the same, it's mostly the naming exercise.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110304/new/
https://reviews.llvm.org/D110304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D110304: [HIP] Fix linking of asanrt.bc
yaxunl created this revision.
yaxunl added a reviewer: tra.
Herald added subscribers: kerbowa, nhaehnle, jvesely.
yaxunl requested review of this revision.
HIP currently uses -mlink-builtin-bitcode to link all bitcode libraries, which
changes the linkage of functions to be internal once they are linked in. This
works for common bitcode libraries since these functions are not intended
to be exposed for external callers.
However, the functions in the sanitizer bitcode library is intended to be
called by instructions generated by the sanitizer pass. If their linkage is
changed to internal, their parameters may be altered by optimizations before
the sanitizer pass, which renders them unusable by the sanitizer pass.
To fix this issue, HIP toolchain links the sanitizer bitcode library with
-mlink-bitcode-file, which does not change the linkage.
An enum for bitcode link option and a struct BitCodeLibraryInfo is
introduced in ToolChain as a generic approach to pass the bitcode
linking option between ToolChain and Tool.
https://reviews.llvm.org/D110304
Files:
clang/include/clang/Driver/ToolChain.h
clang/lib/Driver/ToolChain.cpp
clang/lib/Driver/ToolChains/HIP.cpp
clang/lib/Driver/ToolChains/HIP.h
clang/test/CodeGenCUDA/Inputs/amdgpu-asanrtl.ll
clang/test/CodeGenCUDA/amdgpu-asan.cu
clang/test/Driver/hip-sanitize-options.hip
Index: clang/test/Driver/hip-sanitize-options.hip
===
--- clang/test/Driver/hip-sanitize-options.hip
+++ clang/test/Driver/hip-sanitize-options.hip
@@ -34,7 +34,7 @@
// CHECK-NOT: {{"[^"]*lld(\.exe){0,1}".* ".*hip.bc"}}
// CHECK: {{"[^"]*clang[^"]*".* "-triple" "x86_64-unknown-linux-gnu".* "-fsanitize=address"}}
-// NORDC: {{"[^"]*clang[^"]*".* "-emit-obj".* "-fcuda-is-device".* "-fsanitize=address".*}} "-o" "[[OUT:[^"]*.o]]"
+// NORDC: {{"[^"]*clang[^"]*".* "-emit-obj".* "-fcuda-is-device".* "-mlink-bitcode-file" ".*asanrtl.bc".* "-mlink-builtin-bitcode" ".*hip.bc".* "-fsanitize=address".*}} "-o" "[[OUT:[^"]*.o]]"
// NORDC: {{"[^"]*lld(\.exe){0,1}".*}} "[[OUT]]" {{".*asanrtl.bc" ".*hip.bc"}}
// NORDC: {{"[^"]*clang[^"]*".* "-triple" "x86_64-unknown-linux-gnu".* "-fsanitize=address"}}
Index: clang/test/CodeGenCUDA/amdgpu-asan.cu
===
--- clang/test/CodeGenCUDA/amdgpu-asan.cu
+++ clang/test/CodeGenCUDA/amdgpu-asan.cu
@@ -1,6 +1,20 @@
+// Create a sample address sanitizer bitcode library.
+
+// RUN: %clang_cc1 -x ir -fcuda-is-device -triple amdgcn-amd-amdhsa -emit-llvm-bc \
+// RUN: -disable-llvm-passes -o %t.asanrtl.bc %S/Inputs/amdgpu-asanrtl.ll
+
+// Check sanitizer runtime library functions survive
+// optimizations without being removed or parameters altered.
+
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=amdgcn-amd-amdhsa \
+// RUN: -fcuda-is-device -target-cpu gfx906 -fsanitize=address \
+// RUN: -mlink-bitcode-file %t.asanrtl.bc -x hip \
+// RUN: | FileCheck -check-prefix=ASAN %s
+
// RUN: %clang_cc1 %s -emit-llvm -o - -triple=amdgcn-amd-amdhsa \
// RUN: -fcuda-is-device -target-cpu gfx906 -fsanitize=address \
-// RUN: -x hip | FileCheck -check-prefix=ASAN %s
+// RUN: -O3 -mlink-bitcode-file %t.asanrtl.bc -x hip \
+// RUN: | FileCheck -check-prefix=ASAN %s
// RUN: %clang_cc1 %s -emit-llvm -o - -triple=amdgcn-amd-amdhsa \
// RUN: -fcuda-is-device -target-cpu gfx906 -x hip \
@@ -8,8 +22,10 @@
// REQUIRES: amdgpu-registered-target
-// ASAN-DAG: declare void @__amdgpu_device_library_preserve_asan_functions()
+// ASAN-DAG: define weak void @__amdgpu_device_library_preserve_asan_functions()
// ASAN-DAG: @__amdgpu_device_library_preserve_asan_functions_ptr = weak addrspace(1) constant void ()* @__amdgpu_device_library_preserve_asan_functions
// ASAN-DAG: @llvm.compiler.used = {{.*}}@__amdgpu_device_library_preserve_asan_functions_ptr
+// ASAN-DAG: define weak void @__asan_report_load1(i64 %{{.*}})
-// CHECK-NOT: @__amdgpu_device_library_preserve_asan_functions_ptr
+// CHECK-NOT: @__amdgpu_device_library_preserve_asan_functions
+// CHECK-NOT: @__asan_report_load1
Index: clang/test/CodeGenCUDA/Inputs/amdgpu-asanrtl.ll
===
--- /dev/null
+++ clang/test/CodeGenCUDA/Inputs/amdgpu-asanrtl.ll
@@ -0,0 +1,13 @@
+; Sample code for amdgpu address sanitizer runtime.
+
+; Note the runtime functions need to have weak linkage and default
+; visibility, otherwise they may be internalized and removed by GlobalOptPass.
+
+define weak void @__amdgpu_device_library_preserve_asan_functions() {
+ tail call void @__asan_report_load1(i64 0)
+ ret void
+}
+
+define weak void @__asan_report_load1(i64 %0) {
+ ret void
+}
Index: clang/lib/Driver/ToolChains/HIP.h
===
--- clang/lib/Driver/ToolChains/HIP.h
+++ clang/lib/Driver/ToolChains/HIP.h
@@ -83,7 +83,7 @@
llvm::opt::ArgStringLis
