Skip to content

[Driver][ASan] Refactor Clang-Driver "Sanitizer Bitcode" linking. #123922

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 30, 2025
Merged

[Driver][ASan] Refactor Clang-Driver "Sanitizer Bitcode" linking. #123922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5ec4303
[Driver][ASan] Refactor Clang-Driver "Sanitizer Bitcode" linking.
ampandey-1995 Jan 22, 2025
cd81973
[OpenMP][ASan] Enable ASan Instrumentation for AMDGPUOpenMPToolChain.
ampandey-1995 Jan 28, 2025
46ea14d
[OpenMP][ASan] Enable ASan Instrumentation for AMDGPUOpenMPToolChain.…
ampandey-1995 Jan 28, 2025
71e42ce
Revert "[OpenMP][ASan] Enable ASan Instrumentation for AMDGPUOpenMPTo…
ampandey-1995 Jan 28, 2025
5d727c0
Merge pull request #3 from ampandey-1995/revert-2-omp-asan-driver
ampandey-1995 Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 41 additions & 17 deletions clang/lib/Driver/ToolChains/AMDGPU.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,6 +950,11 @@ void ROCMToolChain::addClangTargetOptions(
ABIVer))
return;

std::tuple<bool, const SanitizerArgs> GPUSan(
DriverArgs.hasFlag(options::OPT_fgpu_sanitize,
options::OPT_fno_gpu_sanitize, true),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot be default true, it included ockl.bc which breaks OpenMP's printf and includes some other conflicting definitions. Furthermore you disable internalization so they stick around in every single OpenMP program now.

Copy link
Contributor Author

@ampandey-1995 ampandey-1995 Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes , sorry one misstep is here is that GPUSan object created by default while not checking that -fsanitize=address available in Args list. This GPUSan should be a pointer and must be initialized when -fsanitize=address is detected on Args.

getSanitizerArgs(DriverArgs));

bool Wave64 = isWave64(DriverArgs, Kind);

// TODO: There are way too many flags that change this. Do we need to check
Expand All @@ -965,21 +970,19 @@ void ROCMToolChain::addClangTargetOptions(
DriverArgs.hasArg(options::OPT_cl_fp32_correctly_rounded_divide_sqrt);

// Add the OpenCL specific bitcode library.
llvm::SmallVector<std::string, 12> BCLibs;
BCLibs.push_back(RocmInstallation->getOpenCLPath().str());
llvm::SmallVector<BitCodeLibraryInfo, 12> BCLibs;
BCLibs.emplace_back(RocmInstallation->getOpenCLPath().str());

// Add the generic set of libraries.
BCLibs.append(RocmInstallation->getCommonBitcodeLibs(
DriverArgs, LibDeviceFile, Wave64, DAZ, FiniteOnly, UnsafeMathOpt,
FastRelaxedMath, CorrectSqrt, ABIVer, false));
FastRelaxedMath, CorrectSqrt, ABIVer, GPUSan, false));

if (getSanitizerArgs(DriverArgs).needsAsanRt()) {
CC1Args.push_back("-mlink-bitcode-file");
CC1Args.push_back(
DriverArgs.MakeArgString(RocmInstallation->getAsanRTLPath()));
}
for (StringRef BCFile : BCLibs) {
CC1Args.push_back("-mlink-builtin-bitcode");
for (auto [BCFile, Internalize] : BCLibs) {
if (Internalize)
CC1Args.push_back("-mlink-builtin-bitcode");
else
CC1Args.push_back("-mlink-bitcode-file");
CC1Args.push_back(DriverArgs.MakeArgString(BCFile));
}
}
Expand All @@ -1002,18 +1005,35 @@ bool RocmInstallationDetector::checkCommonBitcodeLibs(
return true;
}

llvm::SmallVector<std::string, 12>
llvm::SmallVector<ToolChain::BitCodeLibraryInfo, 12>
RocmInstallationDetector::getCommonBitcodeLibs(
const llvm::opt::ArgList &DriverArgs, StringRef LibDeviceFile, bool Wave64,
bool DAZ, bool FiniteOnly, bool UnsafeMathOpt, bool FastRelaxedMath,
bool CorrectSqrt, DeviceLibABIVersion ABIVer, bool isOpenMP = false) const {
llvm::SmallVector<std::string, 12> BCLibs;

auto AddBCLib = [&](StringRef BCFile) { BCLibs.push_back(BCFile.str()); };
bool CorrectSqrt, DeviceLibABIVersion ABIVer,
const std::tuple<bool, const SanitizerArgs> &GPUSan,
bool isOpenMP = false) const {
llvm::SmallVector<ToolChain::BitCodeLibraryInfo, 12> BCLibs;

auto GPUSanEnabled = [GPUSan]() { return std::get<bool>(GPUSan); };
auto AddBCLib = [&](ToolChain::BitCodeLibraryInfo BCLib,
bool Internalize = true) {
BCLib.ShouldInternalize = Internalize;
BCLibs.emplace_back(BCLib);
};
auto AddSanBCLibs = [&]() {
if (GPUSanEnabled()) {
auto SanArgs = std::get<const SanitizerArgs>(GPUSan);
if (SanArgs.needsAsanRt())
AddBCLib(getAsanRTLPath(), false);
}
};

AddSanBCLibs();
AddBCLib(getOCMLPath());
if (!isOpenMP)
AddBCLib(getOCKLPath());
else if (GPUSanEnabled() && isOpenMP)
AddBCLib(getOCKLPath(), false);
AddBCLib(getDenormalsAreZeroPath(DAZ));
AddBCLib(getUnsafeMathPath(UnsafeMathOpt || FastRelaxedMath));
AddBCLib(getFiniteOnlyPath(FiniteOnly || FastRelaxedMath));
Expand All @@ -1027,7 +1047,7 @@ RocmInstallationDetector::getCommonBitcodeLibs(
return BCLibs;
}

llvm::SmallVector<std::string, 12>
llvm::SmallVector<ToolChain::BitCodeLibraryInfo, 12>
ROCMToolChain::getCommonDeviceLibNames(const llvm::opt::ArgList &DriverArgs,
const std::string &GPUArch,
bool isOpenMP) const {
Expand All @@ -1044,6 +1064,10 @@ ROCMToolChain::getCommonDeviceLibNames(const llvm::opt::ArgList &DriverArgs,
// If --hip-device-lib is not set, add the default bitcode libraries.
// TODO: There are way too many flags that change this. Do we need to check
// them all?
std::tuple<bool, const SanitizerArgs> GPUSan(
DriverArgs.hasFlag(options::OPT_fgpu_sanitize,
options::OPT_fno_gpu_sanitize, true),
getSanitizerArgs(DriverArgs));
bool DAZ = DriverArgs.hasFlag(options::OPT_fgpu_flush_denormals_to_zero,
options::OPT_fno_gpu_flush_denormals_to_zero,
getDefaultDenormsAreZeroForTarget(Kind));
Expand All @@ -1061,7 +1085,7 @@ ROCMToolChain::getCommonDeviceLibNames(const llvm::opt::ArgList &DriverArgs,

return RocmInstallation->getCommonBitcodeLibs(
DriverArgs, LibDeviceFile, Wave64, DAZ, FiniteOnly, UnsafeMathOpt,
FastRelaxedMath, CorrectSqrt, ABIVer, isOpenMP);
FastRelaxedMath, CorrectSqrt, ABIVer, GPUSan, isOpenMP);
}

bool AMDGPUToolChain::shouldSkipSanitizeOption(
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Driver/ToolChains/AMDGPU.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class LLVM_LIBRARY_VISIBILITY ROCMToolChain : public AMDGPUToolChain {
Action::OffloadKind DeviceOffloadKind) const override;

// Returns a list of device library names shared by different languages
llvm::SmallVector<std::string, 12>
llvm::SmallVector<BitCodeLibraryInfo, 12>
getCommonDeviceLibNames(const llvm::opt::ArgList &DriverArgs,
const std::string &GPUArch,
bool isOpenMP = false) const;
Expand Down
7 changes: 1 addition & 6 deletions clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#include "AMDGPUOpenMP.h"
#include "AMDGPU.h"
#include "CommonArgs.h"
#include "ToolChains/ROCm.h"
#include "ROCm.h"
#include "clang/Basic/DiagnosticDriver.h"
#include "clang/Driver/Compilation.h"
#include "clang/Driver/Driver.h"
Expand Down Expand Up @@ -159,11 +159,6 @@ AMDGPUOpenMPToolChain::getDeviceLibs(const llvm::opt::ArgList &Args) const {
if (Args.hasArg(options::OPT_nogpulib))
return {};

if (!RocmInstallation->hasDeviceLibrary()) {
getDriver().Diag(diag::err_drv_no_rocm_device_lib) << 0;
return {};
}

StringRef GpuArch = getProcessorFromTargetID(
getTriple(), Args.getLastArgValue(options::OPT_march_EQ));

Expand Down
25 changes: 4 additions & 21 deletions clang/lib/Driver/ToolChains/HIPAMD.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ HIPAMDToolChain::getDeviceLibs(const llvm::opt::ArgList &DriverArgs) const {
llvm::sys::path::append(Path, BCName);
FullName = Path;
if (llvm::sys::fs::exists(FullName)) {
BCLibs.push_back(FullName);
BCLibs.emplace_back(FullName);
return;
}
}
Expand All @@ -396,28 +396,11 @@ HIPAMDToolChain::getDeviceLibs(const llvm::opt::ArgList &DriverArgs) const {
StringRef GpuArch = getGPUArch(DriverArgs);
assert(!GpuArch.empty() && "Must have an explicit GPU arch.");

// If --hip-device-lib is not set, add the default bitcode libraries.
if (DriverArgs.hasFlag(options::OPT_fgpu_sanitize,
options::OPT_fno_gpu_sanitize, true) &&
getSanitizerArgs(DriverArgs).needsAsanRt()) {
auto AsanRTL = RocmInstallation->getAsanRTLPath();
if (AsanRTL.empty()) {
unsigned DiagID = getDriver().getDiags().getCustomDiagID(
DiagnosticsEngine::Error,
"AMDGPU address sanitizer runtime library (asanrtl) is not found. "
"Please install ROCm device library which supports address "
"sanitizer");
getDriver().Diag(DiagID);
return {};
} else
BCLibs.emplace_back(AsanRTL, /*ShouldInternalize=*/false);
}

// Add the HIP specific bitcode library.
BCLibs.push_back(RocmInstallation->getHIPPath());
BCLibs.emplace_back(RocmInstallation->getHIPPath());

// Add common device libraries like ocml etc.
for (StringRef N : getCommonDeviceLibNames(DriverArgs, GpuArch.str()))
for (auto N : getCommonDeviceLibNames(DriverArgs, GpuArch.str()))
BCLibs.emplace_back(N);

// Add instrument lib.
Expand All @@ -426,7 +409,7 @@ HIPAMDToolChain::getDeviceLibs(const llvm::opt::ArgList &DriverArgs) const {
if (InstLib.empty())
return BCLibs;
if (llvm::sys::fs::exists(InstLib))
BCLibs.push_back(InstLib);
BCLibs.emplace_back(InstLib);
else
getDriver().Diag(diag::err_drv_no_such_file) << InstLib;
}
Expand Down
12 changes: 6 additions & 6 deletions clang/lib/Driver/ToolChains/ROCm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "clang/Basic/LLVM.h"
#include "clang/Driver/Driver.h"
#include "clang/Driver/Options.h"
#include "clang/Driver/SanitizerArgs.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/Option/ArgList.h"
Expand Down Expand Up @@ -173,12 +174,11 @@ class RocmInstallationDetector {

/// Get file paths of default bitcode libraries common to AMDGPU based
/// toolchains.
llvm::SmallVector<std::string, 12>
getCommonBitcodeLibs(const llvm::opt::ArgList &DriverArgs,
StringRef LibDeviceFile, bool Wave64, bool DAZ,
bool FiniteOnly, bool UnsafeMathOpt,
bool FastRelaxedMath, bool CorrectSqrt,
DeviceLibABIVersion ABIVer, bool isOpenMP) const;
llvm::SmallVector<ToolChain::BitCodeLibraryInfo, 12> getCommonBitcodeLibs(
const llvm::opt::ArgList &DriverArgs, StringRef LibDeviceFile,
bool Wave64, bool DAZ, bool FiniteOnly, bool UnsafeMathOpt,
bool FastRelaxedMath, bool CorrectSqrt, DeviceLibABIVersion ABIVer,
const std::tuple<bool, const SanitizerArgs> &GPUSan, bool isOpenMP) const;
/// Check file paths of default bitcode libraries common to AMDGPU based
/// toolchains. \returns false if there are invalid or missing files.
bool checkCommonBitcodeLibs(StringRef GPUArch, StringRef LibDeviceFile,
Expand Down
8 changes: 4 additions & 4 deletions clang/test/Driver/hip-sanitize-options.hip
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
// RUN: -nogpuinc --rocm-path=%S/Inputs/rocm \
// RUN: %s 2>&1 | FileCheck -check-prefixes=RDC %s

// RUN: not %clang -### --target=x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
// RUN: not %clang -### --target=x86_64-unknown-linux-gnu -mcode-object-version=5 --offload-arch=gfx900:xnack+ \
// RUN: -fsanitize=address -fgpu-sanitize \
// RUN: -nogpuinc --rocm-path=%S/Inputs/rocm-invalid \
// RUN: %s 2>&1 | FileCheck -check-prefixes=FAIL %s
Expand Down Expand Up @@ -52,15 +52,15 @@
// 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".* "-mlink-bitcode-file" ".*asanrtl.bc".* "-mlink-builtin-bitcode" ".*hip.bc".* "-fsanitize=address".*}} "-o" "[[OUT:[^"]*.o]]"
// NORDC: {{"[^"]*clang[^"]*".* "-emit-obj".* "-fcuda-is-device".* "-mlink-builtin-bitcode" ".*hip.bc".* "-mlink-bitcode-file" ".*asanrtl.bc".* "-fsanitize=address".*}} "-o" "[[OUT:[^"]*.o]]"
// NORDC-NOT: {{"[^"]*lld(\.exe){0,1}".*}} "[[OUT]]" {{".*asanrtl.bc" ".*hip.bc"}}
// NORDC: {{"[^"]*clang[^"]*".* "-triple" "x86_64-unknown-linux-gnu".* "-fsanitize=address"}}

// RDC: {{"[^"]*clang[^"]*".* "-triple" "x86_64-unknown-linux-gnu".* "-fsanitize=address"}}
// RDC: {{"[^"]*clang[^"]*".* "-emit-llvm-bc".* "-fcuda-is-device".* "-mlink-bitcode-file" ".*asanrtl.bc".* "-mlink-builtin-bitcode" ".*hip.bc".* "-fsanitize=address".*}} "-o" "[[OUT:[^"]*.bc]]"
// RDC: {{"[^"]*clang[^"]*".* "-emit-llvm-bc".* "-fcuda-is-device".* "-mlink-builtin-bitcode" ".*hip.bc".* "-mlink-bitcode-file" ".*asanrtl.bc".* "-fsanitize=address".*}} "-o" "[[OUT:[^"]*.bc]]"
// RDC-NOT: {{"[^"]*lld(\.exe){0,1}".*}} "[[OUT]]" {{".*asanrtl.bc" ".*hip.bc"}}

// FAIL: AMDGPU address sanitizer runtime library (asanrtl) is not found. Please install ROCm device library which supports address sanitizer
// FAIL: error: cannot find ROCm device library for ABI version 5; provide its path via '--rocm-path' or '--rocm-device-lib-path', or pass '-nogpulib' to build without ROCm device library

// XNACK-DAG: warning: ignoring '-fsanitize=leak' option as it is not currently supported for target 'amdgcn-amd-amdhsa'
// XNACK-DAG: warning: ignoring '-fsanitize=address' option for offload arch 'gfx900:xnack-' as it is not currently supported there. Use it with an offload arch containing 'xnack+' instead
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Driver/rocm-device-libs.cl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@
// RUN: 2>&1 | FileCheck --check-prefixes=NOASAN %s

// COMMON: "-triple" "amdgcn-amd-amdhsa"
// ASAN-SAME: "-mlink-bitcode-file" "{{.*}}/amdgcn/bitcode/asanrtl.bc"
// COMMON-SAME: "-mlink-builtin-bitcode" "{{.*}}/amdgcn/bitcode/opencl.bc"
// ASAN-SAME: "-mlink-bitcode-file" "{{.*}}/amdgcn/bitcode/asanrtl.bc"
// COMMON-SAME: "-mlink-builtin-bitcode" "{{.*}}/amdgcn/bitcode/ocml.bc"
// COMMON-SAME: "-mlink-builtin-bitcode" "{{.*}}/amdgcn/bitcode/ockl.bc"

Expand Down