refactor code; support errors for explicit -Xarch_device; improve testing

Author: ro-i

clang/include/clang/Basic/DiagnosticDriverKinds.td

@@ -126,16 +126,23 @@ def err_drv_bad_offload_arch_combo : Error<
  "invalid offload arch combinations: '%0' and '%1' (for a specific processor, "
  "a feature should either exist in all offload archs, or not exist in any "
  "offload archs)">;
+def err_drv_unsupported_option_for_offload_arch_req_feature : Error<
+ "'%0' option for offload arch '%1' is not currently supported "
+ "there. Use it with an offload arch containing '%2' instead">;
 def warn_drv_unsupported_option_for_offload_arch_req_feature : Warning<
  "ignoring '%0' option for offload arch '%1' as it is not currently supported "
  "there. Use it with an offload arch containing '%2' instead">,
  InGroup<OptionIgnored>;
 def warn_drv_unsupported_option_for_target : Warning<
  "ignoring '%0' option as it is not currently supported for target '%1'">,
  InGroup<OptionIgnored>;
+def err_drv_unsupported_option_for_target : Error<
+ "'%0' option is not currently supported for target '%1'">;
 def warn_drv_unsupported_option_part_for_target : Warning<
  "ignoring '%0' in '%1' option as it is not currently supported for target '%2'">,
  InGroup<OptionIgnored>;
+def err_drv_unsupported_option_part_for_target : Error<
+ "'%0' in '%1' option is not currently supported for target '%2'">;
 def warn_drv_invalid_argument_for_flang : Warning<
  "'%0' is not valid for Fortran">,
  InGroup<OptionIgnored>;

clang/lib/Driver/ToolChains/AMDGPU.cpp

@@ -1074,32 +1074,12 @@ ROCMToolChain::getCommonDeviceLibNames(
  getSanitizerArgs(DriverArgs).needsAsanRt());
 }
 
-std::optional<std::string> AMDGPUToolChain::filterSanitizeOption(
+bool AMDGPUToolChain::shouldSkipSanitizeOption(
  const ToolChain &TC, const llvm::opt::ArgList &DriverArgs,
  StringRef TargetID, const llvm::opt::Arg *A) const {
- // For actions without targetID, do nothing.
- if (TargetID.empty())
- return std::nullopt;
- Option O = A->getOption();
-
- if (!O.matches(options::OPT_fsanitize_EQ))
- return std::nullopt;
-
- if (!DriverArgs.hasFlag(options::OPT_fgpu_sanitize,
- options::OPT_fno_gpu_sanitize, true))
- return "";
-
  auto &Diags = TC.getDriver().getDiags();
-
- // We only allow the address sanitizer and ignore all other sanitizers.
- SmallVector<std::string, 4> SupportedSanitizers;
- for (const char *Value : A->getValues()) {
- SanitizerMask K = parseSanitizerValue(Value, /*AllowGroups=*/false);
- if (K == SanitizerKind::Address)
- SupportedSanitizers.push_back(std::string(Value));
- }
- if (SupportedSanitizers.empty())
- return "";
+ bool IsExplicitDevice =
+ A->getBaseArg().getOption().matches(options::OPT_Xarch_device);
 
  // Check 'xnack+' availability by default
  llvm::StringRef Processor =
@@ -1111,7 +1091,7 @@ std::optional<std::string> AMDGPUToolChain::filterSanitizeOption(
  ? llvm::AMDGPU::getArchAttrAMDGCN(ProcKind)
  : llvm::AMDGPU::getArchAttrR600(ProcKind);
  if (Features & llvm::AMDGPU::FEATURE_XNACK_ALWAYS)
- return std::nullopt;
+ return false;
 
  // Look for the xnack feature in TargetID
  llvm::StringMap<bool> FeatureMap;
@@ -1120,11 +1100,17 @@ std::optional<std::string> AMDGPUToolChain::filterSanitizeOption(
  (void)OptionalGpuArch;
  auto Loc = FeatureMap.find("xnack");
  if (Loc == FeatureMap.end() || !Loc->second) {
- Diags.Report(
- clang::diag::warn_drv_unsupported_option_for_offload_arch_req_feature)
- << A->getAsString(DriverArgs) << TargetID << "xnack+";
- return "";
+ if (IsExplicitDevice) {
+ Diags.Report(
+ clang::diag::err_drv_unsupported_option_for_offload_arch_req_feature)
+ << A->getAsString(DriverArgs) << TargetID << "xnack+";
+ } else {
+ Diags.Report(
+ clang::diag::warn_drv_unsupported_option_for_offload_arch_req_feature)
+ << A->getAsString(DriverArgs) << TargetID << "xnack+";
+ }
+ return true;
  }
 
- return llvm::join(SupportedSanitizers, ",");
+ return false;
 }

clang/lib/Driver/ToolChains/AMDGPU.h

@@ -101,13 +101,11 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUToolChain : public Generic_ELF {
  /// Needed for translating LTO options.
  const char *getDefaultLinker() const override { return "ld.lld"; }
 
- /// Filter supported sanitizers from the sanitize option and return them. If
- /// there should be no filtering and Arg should be kept as-is, return
- /// std::nullopt. If no sanitizers are supported, return an empty string.
- std::optional<std::string>
- filterSanitizeOption(const ToolChain &TC,
- const llvm::opt::ArgList &DriverArgs, StringRef TargetID,
- const llvm::opt::Arg *A) const;
+ /// Should skip sanitize option.
+ bool shouldSkipSanitizeOption(const ToolChain &TC,
+ const llvm::opt::ArgList &DriverArgs,
+ StringRef TargetID,
+ const llvm::opt::Arg *A) const;
 
  /// Uses amdgpu-arch tool to get arch of the system GPU. Will return error
  /// if unable to find one.
@@ -157,19 +155,64 @@ class LLVM_LIBRARY_VISIBILITY ROCMToolChain : public AMDGPUToolChain {
  return SanitizerKind::Address;
  }
 
- void diagnoseUnsupportedSanitizers(const llvm::opt::ArgList &Args) const {
- if (!Args.hasFlag(options::OPT_fgpu_sanitize, options::OPT_fno_gpu_sanitize,
- true))
- return;
+ bool handleSanitizeOption(const ToolChain &TC, llvm::opt::DerivedArgList &DAL,
+ const llvm::opt::ArgList &DriverArgs,
+ StringRef TargetID, const llvm::opt::Arg *A) const {
+ if (TargetID.empty())
+ return false;
+ // If this isn't a sanitizer option, don't handle it.
+ if (!A->getOption().matches(options::OPT_fsanitize_EQ))
+ return false;
+ // If we shouldn't do sanitizing, skip it.
+ if (!DriverArgs.hasFlag(options::OPT_fgpu_sanitize,
+ options::OPT_fno_gpu_sanitize, true))
+ return true;
+
  auto &Diags = getDriver().getDiags();
- for (auto *A : Args.filtered(options::OPT_fsanitize_EQ)) {
- for (const char *Value : A->getValues()) {
- SanitizerMask K = parseSanitizerValue(Value, /*Allow Groups*/ false);
- if (K != SanitizerKind::Address)
+ bool IsExplicitDevice =
+ A->getBaseArg().getOption().matches(options::OPT_Xarch_device);
+
+ SmallVector<const char *, 4> SupportedSanitizers;
+ SmallVector<const char *, 4> UnSupportedSanitizers;
+
+ for (const char *Value : A->getValues()) {
+ SanitizerMask K = parseSanitizerValue(Value, /*Allow Groups*/ false);
+ if (K & ROCMToolChain::getSupportedSanitizers())
+ SupportedSanitizers.push_back(Value);
+ else
+ UnSupportedSanitizers.push_back(Value);
+ }
+
+ // If there are no supported sanitizers, drop the whole argument.
+ if (SupportedSanitizers.empty()) {
+ if (IsExplicitDevice) {
+ Diags.Report(clang::diag::err_drv_unsupported_option_for_target)
+ << A->getAsString(DAL) << getTriple().str();
+ } else {
+ Diags.Report(clang::diag::warn_drv_unsupported_option_for_target)
+ << A->getAsString(DAL) << getTriple().str();
+ }
+ return true;
+ }
+ // If only some sanitizers are unsupported, report each one individually.
+ if (!UnSupportedSanitizers.empty()) {
+ for (const char *Value : UnSupportedSanitizers) {
+ if (IsExplicitDevice) {
+ Diags.Report(clang::diag::err_drv_unsupported_option_part_for_target)
+ << Value << A->getAsString(DriverArgs) << getTriple().str();
+ } else {
  Diags.Report(clang::diag::warn_drv_unsupported_option_part_for_target)
- << Value << A->getAsString(Args) << getTriple().str();
+ << Value << A->getAsString(DriverArgs) << getTriple().str();
+ }
  }
  }
+ // If we know the target arch, check if the sanitizer is supported for it.
+ if (shouldSkipSanitizeOption(TC, DriverArgs, TargetID, A))
+ return true;
+
+ // Add a new argument with only the supported sanitizers.
+ DAL.AddJoinedArg(A, A->getOption(), llvm::join(SupportedSanitizers, ","));
+ return true;
  }
 };
 

clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp

@@ -28,8 +28,6 @@ AMDGPUOpenMPToolChain::AMDGPUOpenMPToolChain(const Driver &D,
  // Lookup binaries into the driver directory, this is used to
  // discover the 'amdgpu-arch' executable.
  getProgramPaths().push_back(getDriver().Dir);
- // Diagnose unsupported sanitizer options only once.
- diagnoseUnsupportedSanitizers(Args);
 }
 
 void AMDGPUOpenMPToolChain::addClangTargetOptions(
@@ -66,19 +64,10 @@ llvm::opt::DerivedArgList *AMDGPUOpenMPToolChain::TranslateArgs(
 
  const OptTable &Opts = getDriver().getOpts();
 
- // Skip sanitize options passed from the HostTC. Remove them early.
- // The decision to sanitize device code is computed only by
- // 'shouldSkipSanitizeOption'.
- if (DAL->hasArg(options::OPT_fsanitize_EQ))
- DAL->eraseArg(options::OPT_fsanitize_EQ);
-
  for (Arg *A : Args) {
- std::optional<std::string> SupportedSanitizers =
- filterSanitizeOption(*this, Args, BoundArch, A);
- if (!SupportedSanitizers)
+ // Filter unsupported sanitizers passed from the HostTC.
+ if (!handleSanitizeOption(*this, *DAL, Args, BoundArch, A))
  DAL->append(A);
- else if (!SupportedSanitizers->empty())
- DAL->AddJoinedArg(A, A->getOption(), *SupportedSanitizers);
  }
 
  if (!BoundArch.empty()) {
@@ -119,9 +108,8 @@ void AMDGPUOpenMPToolChain::AddIAMCUIncludeArgs(const ArgList &Args,
 SanitizerMask AMDGPUOpenMPToolChain::getSupportedSanitizers() const {
  // The AMDGPUOpenMPToolChain only supports sanitizers in the sense that it
  // allows sanitizer arguments on the command line if they are supported by the
- // host toolchain. The AMDGPUOpenMPToolChain will actually ignore any command
- // line arguments for any of these "supported" sanitizers. That means that no
- // sanitization of device code is actually supported at this time.
+ // host toolchain. The AMDGPUOpenMPToolChain will later filter unsupported
+ // sanitizers from the command line arguments.
  //
  // This behavior is necessary because the host and device toolchains
  // invocations often share the command line, so the device toolchain must

clang/lib/Driver/ToolChains/HIPAMD.cpp

@@ -219,8 +219,6 @@ HIPAMDToolChain::HIPAMDToolChain(const Driver &D, const llvm::Triple &Triple,
  // Lookup binaries into the driver directory, this is used to
  // discover the clang-offload-bundler executable.
  getProgramPaths().push_back(getDriver().Dir);
- // Diagnose unsupported sanitizer options only once.
- diagnoseUnsupportedSanitizers(Args);
 }
 
 void HIPAMDToolChain::addClangTargetOptions(
@@ -291,19 +289,10 @@ HIPAMDToolChain::TranslateArgs(const llvm::opt::DerivedArgList &Args,
 
  const OptTable &Opts = getDriver().getOpts();
 
- // Skip sanitize options passed from the HostTC. Remove them early.
- // The decision to sanitize device code is computed only by
- // 'shouldSkipSanitizeOption'.
- if (DAL->hasArg(options::OPT_fsanitize_EQ))
- DAL->eraseArg(options::OPT_fsanitize_EQ);
-
  for (Arg *A : Args) {
- std::optional<std::string> SupportedSanitizers =
- filterSanitizeOption(*this, Args, BoundArch, A);
- if (!SupportedSanitizers)
+ // Filter unsupported sanitizers passed from the HostTC.
+ if (!handleSanitizeOption(*this, *DAL, Args, BoundArch, A))
  DAL->append(A);
- else if (!SupportedSanitizers->empty())
- DAL->AddJoinedArg(A, A->getOption(), *SupportedSanitizers);
  }
 
  if (!BoundArch.empty()) {
@@ -358,9 +347,8 @@ void HIPAMDToolChain::AddHIPIncludeArgs(const ArgList &DriverArgs,
 SanitizerMask HIPAMDToolChain::getSupportedSanitizers() const {
  // The HIPAMDToolChain only supports sanitizers in the sense that it allows
  // sanitizer arguments on the command line if they are supported by the host
- // toolchain. The HIPAMDToolChain will actually ignore any command line
- // arguments for any of these "supported" sanitizers. That means that no
- // sanitization of device code is actually supported at this time.
+ // toolchain. The HIPAMDToolChain will later filter unsupported sanitizers
+ // from the command line arguments.
  //
  // This behavior is necessary because the host and device toolchains
  // invocations often share the command line, so the device toolchain must

clang/test/Driver/amdgpu-openmp-sanitize-options.c

@@ -57,26 +57,52 @@
 // (The address sanitizer enables the device sanitizer pipeline. The fuzzer
 // implicitly turns on LLVMs SanitizerCoverage, which the driver then forwards
 // to the device cc1. SanitizerCoverage is not supported on amdgcn.)
+
 // RUN: %clang -no-canonical-prefixes -### --target=x86_64-unknown-linux-gnu -fopenmp=libomp --offload-arch=gfx908:xnack+ -fsanitize=address,fuzzer --rocm-path=%S/Inputs/rocm %s 2>&1 \
 // RUN: | FileCheck -check-prefixes=HOSTSANCOMBINATION,INVALIDCOMBINATION1 %s
 // RUN: %clang -no-canonical-prefixes -### --target=x86_64-unknown-linux-gnu -fopenmp=libomp --offload-arch=gfx908:xnack+ -fsanitize=fuzzer,address --rocm-path=%S/Inputs/rocm %s 2>&1 \
 // RUN: | FileCheck -check-prefixes=HOSTSANCOMBINATION,INVALIDCOMBINATION2 %s
 
+// Do the same for multiple -fsanitize arguments and multi-arch scenarios.
+
+// RUN: %clang -no-canonical-prefixes -### --target=x86_64-unknown-linux-gnu -fopenmp=libomp --offload-arch=gfx908:xnack+ --offload-arch=gfx900:xnack- -fsanitize=address,fuzzer --rocm-path=%S/Inputs/rocm %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=HOSTSANCOMBINATION,INVALIDCOMBINATION1 %s
+// RUN: %clang -no-canonical-prefixes -### --target=x86_64-unknown-linux-gnu -fopenmp=libomp --offload-arch=gfx908:xnack+,gfx900:xnack- -fsanitize=address,fuzzer --rocm-path=%S/Inputs/rocm %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=HOSTSANCOMBINATION,INVALIDCOMBINATION1 %s
+// RUN: %clang -no-canonical-prefixes -### --target=x86_64-unknown-linux-gnu -fopenmp=libomp --offload-arch=gfx908:xnack+,gfx900:xnack- -fsanitize=fuzzer,address -fsanitize=leak --rocm-path=%S/Inputs/rocm %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=HOSTSANCOMBINATION2,NOTSUPPORTED-DAG,INVALIDCOMBINATION2 %s
+
+// Test -Xarch_device error scenario
+
+// RUN: not %clang -no-canonical-prefixes -### --target=x86_64-unknown-linux-gnu -fopenmp=libomp --offload-arch=gfx908:xnack+ -Xarch_device -fsanitize=leak --rocm-path=%S/Inputs/rocm %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=UNSUPPORTEDERROR %s
+
+// RUN: not %clang -no-canonical-prefixes -### --target=x86_64-unknown-linux-gnu -fopenmp=libomp --offload-arch=gfx908:xnack- -Xarch_device -fsanitize=address --rocm-path=%S/Inputs/rocm %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=XNACKERROR %s
+
+// RUN: not %clang -no-canonical-prefixes -### --target=x86_64-unknown-linux-gnu -fopenmp=libomp --offload-arch=gfx908:xnack+ -Xarch_device -fsanitize=fuzzer,address --rocm-path=%S/Inputs/rocm %s 2>&1 \
+// RUN: | FileCheck -check-prefixes=INVALIDCOMBINATIONERROR %s
+
 // INVALIDCOMBINATION1: warning: ignoring 'fuzzer' in '-fsanitize=address,fuzzer' option as it is not currently supported for target 'amdgcn-amd-amdhsa' [-Woption-ignored]
 // INVALIDCOMBINATION2: warning: ignoring 'fuzzer' in '-fsanitize=fuzzer,address' option as it is not currently supported for target 'amdgcn-amd-amdhsa' [-Woption-ignored]
 
 // FAIL-DAG: 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
-// NOTSUPPORTED-DAG: warning: ignoring 'leak' in '-fsanitize=leak' option as it is not currently supported for target 'amdgcn-amd-amdhsa'
+// NOTSUPPORTED-DAG: warning: ignoring '-fsanitize=leak' option as it is not currently supported for target 'amdgcn-amd-amdhsa'
 
 // NOXNACK: warning: ignoring '-fsanitize=address' option for offload arch 'gfx908' as it is not currently supported there. Use it with an offload arch containing 'xnack+' instead
 // XNACKNEG: warning: ignoring '-fsanitize=address' option for offload arch 'gfx908:xnack-' as it is not currently supported there. Use it with an offload arch containing 'xnack+' instead
 
 // HOSTSAN: {{"[^"]*clang[^"]*" "-cc1" "-triple" "x86_64-unknown-linux-gnu".* "-fopenmp".* "-fsanitize=address".* "--offload-targets=amdgcn-amd-amdhsa".* "-x" "c".*}}
 // HOSTSANCOMBINATION: {{"[^"]*clang[^"]*" "-cc1" "-triple" "x86_64-unknown-linux-gnu".* "-fopenmp".* "-fsanitize=address,fuzzer,fuzzer-no-link".* "--offload-targets=amdgcn-amd-amdhsa".* "-x" "c".*}}
+// HOSTSANCOMBINATION2: {{"[^"]*clang[^"]*" "-cc1" "-triple" "x86_64-unknown-linux-gnu".* "-fopenmp".* "-fsanitize=address,fuzzer,fuzzer-no-link,leak".* "--offload-targets=amdgcn-amd-amdhsa".* "-x" "c".*}}
 
 // GPUSAN: {{"[^"]*clang[^"]*" "-cc1" "-triple" "amdgcn-amd-amdhsa" "-aux-triple" "x86_64-unknown-linux-gnu".* "-emit-llvm-bc".* "-mlink-bitcode-file" "[^"]*asanrtl.bc".* "-mlink-bitcode-file" "[^"]*ockl.bc".* "-target-cpu" "(gfx908|gfx900|gfx1250|gfx1251)".* "-fopenmp".* "-fsanitize=address".* "-x" "c".*}}
 // NOGPUSAN: {{"[^"]*clang[^"]*" "-cc1" "-triple" "amdgcn-amd-amdhsa" "-aux-triple" "x86_64-unknown-linux-gnu".* "-emit-llvm-bc".* "-target-cpu" "(gfx908|gfx900)".* "-fopenmp".* "-x" "c".*}}
 
 // SAN: {{"[^"]*llvm-offload-binary[^"]*" "-o".* "--image=file=.*.bc,triple=amdgcn-amd-amdhsa,arch=(gfx908|gfx1250|gfx1251)(:xnack\-|:xnack\+)?,kind=openmp(,feature=(\-xnack|\+xnack))?"}}
 // SAN: {{"[^"]*clang[^"]*" "-cc1" "-triple" "x86_64-unknown-linux-gnu".* "-fopenmp".* "-fsanitize=address".* "--offload-targets=amdgcn-amd-amdhsa".* "-x" "ir".*}}
 // SAN: {{"[^"]*clang-linker-wrapper[^"]*".* "--host-triple=x86_64-unknown-linux-gnu".* "--linker-path=[^"]*".* "--whole-archive" "[^"]*(libclang_rt.asan_static.a|libclang_rt.asan_static-x86_64.a)".* "--whole-archive" "[^"]*(libclang_rt.asan.a|libclang_rt.asan-x86_64.a)".*}}
+
+// UNSUPPORTEDERROR: error: '-fsanitize=leak' option is not currently supported for target 'amdgcn-amd-amdhsa'
+// XNACKERROR: error: '-fsanitize=address' option for offload arch 'gfx908:xnack-' is not currently supported there. Use it with an offload arch containing 'xnack+' instead
+// INVALIDCOMBINATIONERROR: error: 'fuzzer' in '-fsanitize=fuzzer,address' option is not currently supported for target 'amdgcn-amd-amdhsa'