llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Matt Arsenault (arsenm) <details> <summary>Changes</summary> Previously the AMDGPU toolchains hackily handled -fsanitize arguments. They would lie and report that all host side sanitizers are available, then TranslateArgs would filter out the device side cases that do not work, providing diagnostics for the skipped cases. Move that logic into the base sanitizer argument parsing. This makes the produced diagnostics more consistent. Previously we would get repeated warnings when a sanitizer is fully unsupported by amdgpu, which should now be once for the toolchain. These could be further improved; we're printing the specific field of -fsanitize in more cases where it could be skipped. In other cases we have the opposite problem, where we aren't reporting the exact sanitizer from the -f flag in the case that depends on a subtarget feature. This will help fix other broken target specific flag forwarding bugs in the future. Co-authored-by: Claude Sonnet 4 <noreply@<!-- -->anthropic.com> --- Patch is 40.32 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/196737.diff 14 Files Affected: - (modified) clang/include/clang/Driver/SanitizerArgs.h (+4-1) - (modified) clang/include/clang/Driver/ToolChain.h (+18-1) - (modified) clang/lib/Driver/SanitizerArgs.cpp (+84-10) - (modified) clang/lib/Driver/ToolChain.cpp (+9-2) - (modified) clang/lib/Driver/ToolChains/AMDGPU.cpp (+55-46) - (modified) clang/lib/Driver/ToolChains/AMDGPU.h (+7-75) - (modified) clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp (+17-21) - (modified) clang/lib/Driver/ToolChains/AMDGPUOpenMP.h (+1-4) - (modified) clang/lib/Driver/ToolChains/Clang.cpp (+3-1) - (modified) clang/lib/Driver/ToolChains/HIPAMD.cpp (+21-24) - (modified) clang/lib/Driver/ToolChains/HIPAMD.h (+1-4) - (modified) clang/test/Driver/amdgpu-openmp-sanitize-options.c (+2-2) - (added) clang/test/Driver/amdgpu-validate-sanitize.cl (+23) - (modified) clang/test/Driver/hip-sanitize-options.hip (+14-14) ``````````diff diff --git a/clang/include/clang/Driver/SanitizerArgs.h b/clang/include/clang/Driver/SanitizerArgs.h index ed2eb6852b124..d4ee17802fd8e 100644 --- a/clang/include/clang/Driver/SanitizerArgs.h +++ b/clang/include/clang/Driver/SanitizerArgs.h @@ -9,6 +9,7 @@ #define LLVM_CLANG_DRIVER_SANITIZERARGS_H #include "clang/Basic/Sanitizers.h" +#include "clang/Driver/Action.h" #include "clang/Driver/Types.h" #include "llvm/Option/Arg.h" #include "llvm/Option/ArgList.h" @@ -85,7 +86,9 @@ class SanitizerArgs { public: /// Parses the sanitizer arguments from an argument list. SanitizerArgs(const ToolChain &TC, const llvm::opt::ArgList &Args, - bool DiagnoseErrors = true); + bool DiagnoseErrors = true, bool DiagnoseBoundArchErrors = true, + StringRef BoundArch = "", + Action::OffloadKind DeviceOffloadKind = Action::OFK_None); bool needsSharedRt() const { return SharedRuntime; } bool needsStableAbi() const { return StableABI; } diff --git a/clang/include/clang/Driver/ToolChain.h b/clang/include/clang/Driver/ToolChain.h index 8676318d6f9d4..cbdec7843876c 100644 --- a/clang/include/clang/Driver/ToolChain.h +++ b/clang/include/clang/Driver/ToolChain.h @@ -18,6 +18,7 @@ #include "llvm/ADT/APFloat.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/FloatingPointMode.h" +#include "llvm/ADT/SmallSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "llvm/Frontend/Debug/Options.h" @@ -185,8 +186,13 @@ class ToolChain { Tool *getOffloadPackager() const; Tool *getLinkerWrapper() const; + /// Track if diagnostics have been emitted for sanitizer arguments already to + /// avoid duplicate diagnostics. mutable bool SanitizerArgsChecked = false; + /// Set of BoundArch values which have already had diagnostics emitted. + mutable llvm::SmallSet<StringRef, 4> BoundArchSanitizerArgsChecked; + /// The effective clang triple for the current Job. mutable llvm::Triple EffectiveTriple; @@ -338,7 +344,18 @@ class ToolChain { /// -print-multi-flags-experimental argument. Multilib::flags_list getMultilibFlags(const llvm::opt::ArgList &) const; - SanitizerArgs getSanitizerArgs(const llvm::opt::ArgList &JobArgs) const; + virtual SanitizerArgs getSanitizerArgs( + const llvm::opt::ArgList &JobArgs, StringRef BoundArch = "", + Action::OffloadKind DeviceOffloadKind = Action::OFK_None) const; + + /// Returns the feature requirement for a sanitizer on a specific arch for + /// diagnostic purposes. Returns the required feature name (e.g., "xnack+") if + /// the sanitizer is generally supported but requires a specific feature for + /// the given BoundArch, or an empty StringRef otherwise. + virtual StringRef getSanitizerRequirement(SanitizerMask Kinds, + StringRef BoundArch) const { + return {}; + } const XRayArgs getXRayArgs(const llvm::opt::ArgList &) const; diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp index 4bf011ab3dedf..31660dd29407c 100644 --- a/clang/lib/Driver/SanitizerArgs.cpp +++ b/clang/lib/Driver/SanitizerArgs.cpp @@ -398,7 +398,9 @@ bool SanitizerArgs::needsLTO() const { SanitizerArgs::SanitizerArgs(const ToolChain &TC, const llvm::opt::ArgList &Args, - bool DiagnoseErrors) { + bool DiagnoseErrors, bool DiagnoseBoundArchErrors, + StringRef BoundArch, + Action::OffloadKind DeviceOffloadKind) { SanitizerMask AllRemove; // During the loop below, the accumulated set of // sanitizers disabled by the current sanitizer // argument or any argument after it. @@ -412,8 +414,16 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, SanitizerMask IgnoreForUbsanFeature; // Accumulated set of values passed to // `-fsanitize-ignore-for-ubsan-feature`. SanitizerMask Kinds; - const SanitizerMask Supported = - setGroupBits(TC.getSupportedSanitizers("", Action::OFK_None)); + + // Figure out the base toolchain's sanitizer support so we can diagnose the + // diff for a specific BoundArch. + const SanitizerMask ToolChainSupported = + setGroupBits(TC.getSupportedSanitizers("", DeviceOffloadKind)); + + const SanitizerMask BoundArchSupported = + BoundArch.empty() ? ToolChainSupported + : setGroupBits(TC.getSupportedSanitizers( + BoundArch, DeviceOffloadKind)); CfiCrossDso = Args.hasFlag(options::OPT_fsanitize_cfi_cross_dso, options::OPT_fno_sanitize_cfi_cross_dso, false); @@ -540,15 +550,79 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, DiagnosedKinds |= SanitizerKind::CFIMFCall; } - if (SanitizerMask KindsToDiagnose = Add & ~Supported & ~DiagnosedKinds) { - if (DiagnoseErrors) { - std::string Desc = describeSanitizeArg(Arg, KindsToDiagnose); + // Check for sanitizers that are supported by the toolchain but not for + // this specific arch (e.g., AMDGPU requires specific subtarget features + // for address sanitizer.) + if (SanitizerMask ArchSpecificUnsupported = + Add & ToolChainSupported & ~BoundArchSupported & ~DiagnosedKinds; + ArchSpecificUnsupported && DiagnoseBoundArchErrors) { + // Upgrade the warning to an error if the unsupported sanitizer was + // explicitly specified for the bound arch. + + // FIXME: There are additional options which explicitly bind to this + // device. + bool IsExplicitDevice = + Arg->getBaseArg().getOption().matches(options::OPT_Xarch_device); + + // Check if the toolchain provides a feature requirement hint for + // any of the unsupported sanitizers + StringRef Requirement = + TC.getSanitizerRequirement(ArchSpecificUnsupported, BoundArch); + if (!Requirement.empty()) { + // Emit diagnostic with feature requirement + // + // TODO: Use variant of unsupported_option_part_for_target that + // includes offload_arch_req_feature + D.Diag( + IsExplicitDevice + ? diag:: + err_drv_unsupported_option_for_offload_arch_req_feature + : diag:: + warn_drv_unsupported_option_for_offload_arch_req_feature) + << Arg->getAsString(Args) << BoundArch << Requirement; + } else { + // Fall back to generic diagnostic if no requirement was provided + SanitizerSet UnsupportedSet; + UnsupportedSet.Mask = ArchSpecificUnsupported; + D.Diag(diag::warn_drv_unsupported_option_part_for_target) + << toString(UnsupportedSet) << Arg->getAsString(Args) + << Triple.str(); + } + + DiagnosedKinds |= ArchSpecificUnsupported; + } + + // Check for sanitizers that are not supported at all by the toolchain + if (SanitizerMask KindsToDiagnose = + Add & ~ToolChainSupported & ~DiagnosedKinds; + DiagnoseErrors && KindsToDiagnose) { + bool IsExplicitDevice = + Arg->getBaseArg().getOption().matches(options::OPT_Xarch_device); + // For device offload compilation, emit a warning since the sanitizer + // may still work on the host. For non-offload compilation or explicit + // device specification, emit an error. + if (DeviceOffloadKind != Action::OFK_None && + DeviceOffloadKind != Action::OFK_Host) { + // For warnings, extract just the sanitizer names (e.g., "fuzzer") + // instead of the full argument (e.g., "-fsanitize=fuzzer") + SanitizerSet KindSet; + KindSet.Mask = KindsToDiagnose; + D.Diag(IsExplicitDevice + ? diag::err_drv_unsupported_option_part_for_target + : diag::warn_drv_unsupported_option_part_for_target) + << toString(KindSet) << Arg->getAsString(Args) + << TC.getTriple().str(); + } else { + // For non-offload targets, use the shorter diagnostic format D.Diag(diag::err_drv_unsupported_opt_for_target) - << Desc << TC.getTriple().str(); + << describeSanitizeArg(Arg, KindsToDiagnose) + << TC.getTriple().str(); } + DiagnosedKinds |= KindsToDiagnose; } - Add &= Supported; + + Add &= BoundArchSupported; // Test for -fno-rtti + explicit -fsanitizer=vptr before expanding groups // so we don't error out if -fno-rtti and -fsanitize=undefined were @@ -599,7 +673,7 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, options::OPT_fno_wrapv_pointer, S)) Add &= ~SanitizerKind::PointerOverflow; } - Add &= Supported; + Add &= BoundArchSupported; if (Add & SanitizerKind::Fuzzer) Add |= SanitizerKind::FuzzerNoLink; @@ -714,7 +788,7 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC, // c++abi-specific parts of UBSan runtime, and they are not provided by the // toolchain. We don't have a good way to check the latter, so we just // check if the toolchan supports vptr. - if (~Supported & SanitizerKind::Vptr) { + if (~BoundArchSupported & SanitizerKind::Vptr) { SanitizerMask KindsToDiagnose = Kinds & ~TrappingKinds & NeedsUbsanCxxRt; // The runtime library supports the Microsoft C++ ABI, but only well enough // for CFI. FIXME: Remove this once we support vptr on Windows. diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp index 6e5148101d4ec..e0ff70c32ddfd 100644 --- a/clang/lib/Driver/ToolChain.cpp +++ b/clang/lib/Driver/ToolChain.cpp @@ -516,8 +516,15 @@ ToolChain::getMultilibFlags(const llvm::opt::ArgList &Args) const { } SanitizerArgs -ToolChain::getSanitizerArgs(const llvm::opt::ArgList &JobArgs) const { - SanitizerArgs SanArgs(*this, JobArgs, !SanitizerArgsChecked); +ToolChain::getSanitizerArgs(const llvm::opt::ArgList &JobArgs, + StringRef BoundArch, + Action::OffloadKind DeviceOffloadKind) const { + SanitizerArgs SanArgs(*this, JobArgs, + /*DiagnoseErrors=*/!SanitizerArgsChecked, + /*DiagnoseBoundArchErrors=*/ + BoundArchSanitizerArgsChecked.insert(BoundArch).second, + BoundArch, DeviceOffloadKind); + SanitizerArgsChecked = true; return SanArgs; } diff --git a/clang/lib/Driver/ToolChains/AMDGPU.cpp b/clang/lib/Driver/ToolChains/AMDGPU.cpp index b457ee2cde1c3..7f59d59585eb0 100644 --- a/clang/lib/Driver/ToolChains/AMDGPU.cpp +++ b/clang/lib/Driver/ToolChains/AMDGPU.cpp @@ -908,6 +908,9 @@ AMDGPUToolChain::getGPUArch(const llvm::opt::ArgList &DriverArgs) const { AMDGPUToolChain::ParsedTargetIDType AMDGPUToolChain::getParsedTargetID(const llvm::opt::ArgList &DriverArgs) const { StringRef TargetID = DriverArgs.getLastArgValue(options::OPT_mcpu_EQ); + if (TargetID.empty()) + TargetID = DriverArgs.getLastArgValue(options::OPT_march_EQ); + if (TargetID.empty()) return {std::nullopt, std::nullopt, std::nullopt}; @@ -980,15 +983,14 @@ void ROCMToolChain::addClangTargetOptions( if (TT.getEnvironment() == llvm::Triple::LLVM) return; - // Get the device name and canonicalize it - const StringRef GpuArch = getGPUArch(DriverArgs); - auto Kind = llvm::AMDGPU::parseArchAMDGCN(GpuArch); - const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind); - StringRef LibDeviceFile = RocmInstallation->getLibDeviceFile(CanonArch); + AMDGPUToolChain::ParsedTargetIDType TargetID = getParsedTargetID(DriverArgs); + StringRef GpuArch = + TargetID.OptionalGPUArch ? *TargetID.OptionalGPUArch : StringRef(); + + StringRef LibDeviceFile = RocmInstallation->getLibDeviceFile(GpuArch); auto ABIVer = DeviceLibABIVersion::fromCodeObjectVersion( getAMDGPUCodeObjectVersion(getDriver(), DriverArgs)); - if (!RocmInstallation->checkCommonBitcodeLibs(CanonArch, LibDeviceFile, - ABIVer)) + if (!RocmInstallation->checkCommonBitcodeLibs(GpuArch, LibDeviceFile, ABIVer)) return; // Add the OpenCL specific bitcode library. @@ -998,7 +1000,9 @@ void ROCMToolChain::addClangTargetOptions( // Add the generic set of libraries. BCLibs.append(RocmInstallation->getCommonBitcodeLibs( DriverArgs, LibDeviceFile, GpuArch, DeviceOffloadingKind, - getSanitizerArgs(DriverArgs).needsAsanRt())); + getSanitizerArgs(DriverArgs, TargetID.OptionalTargetID.value_or(""), + DeviceOffloadingKind) + .needsAsanRt())); for (auto [BCFile, Internalize] : BCLibs) { if (Internalize) @@ -1074,8 +1078,8 @@ RocmInstallationDetector::getCommonBitcodeLibs( llvm::SmallVector<ToolChain::BitCodeLibraryInfo, 12> ROCMToolChain::getCommonDeviceLibNames( - const llvm::opt::ArgList &DriverArgs, llvm::StringRef GPUArch, - Action::OffloadKind DeviceOffloadingKind) const { + const llvm::opt::ArgList &DriverArgs, llvm::StringRef TargetID, + llvm::StringRef GPUArch, Action::OffloadKind DeviceOffloadingKind) const { auto Kind = llvm::AMDGPU::parseArchAMDGCN(GPUArch); const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind); @@ -1088,46 +1092,51 @@ ROCMToolChain::getCommonDeviceLibNames( return RocmInstallation->getCommonBitcodeLibs( DriverArgs, LibDeviceFile, GPUArch, DeviceOffloadingKind, - getSanitizerArgs(DriverArgs).needsAsanRt()); + getSanitizerArgs(DriverArgs, TargetID, DeviceOffloadingKind) + .needsAsanRt()); } -bool AMDGPUToolChain::shouldSkipSanitizeOption( - const ToolChain &TC, const llvm::opt::ArgList &DriverArgs, - StringRef TargetID, const llvm::opt::Arg *A) const { - auto &Diags = TC.getDriver().getDiags(); - bool IsExplicitDevice = - A->getBaseArg().getOption().matches(options::OPT_Xarch_device); - - // Check 'xnack+' availability by default - llvm::StringRef Processor = - getProcessorFromTargetID(TC.getTriple(), TargetID); - auto ProcKind = TC.getTriple().isAMDGCN() - ? llvm::AMDGPU::parseArchAMDGCN(Processor) - : llvm::AMDGPU::parseArchR600(Processor); - auto Features = TC.getTriple().isAMDGCN() - ? llvm::AMDGPU::getArchAttrAMDGCN(ProcKind) - : llvm::AMDGPU::getArchAttrR600(ProcKind); - if (Features & llvm::AMDGPU::FEATURE_XNACK_ALWAYS) - return false; +static bool isXnackAvailable(const llvm::Triple &TT, llvm::StringRef TargetID) { + // Arch-specific check - only report as supported if arch has xnack+ + llvm::StringRef Processor = getProcessorFromTargetID(TT, TargetID); + auto ProcKind = TT.isAMDGCN() ? llvm::AMDGPU::parseArchAMDGCN(Processor) + : llvm::AMDGPU::parseArchR600(Processor); + auto Features = TT.isAMDGPU() ? llvm::AMDGPU::getArchAttrAMDGCN(ProcKind) + : llvm::AMDGPU::getArchAttrR600(ProcKind); + + // If processor has xnack always on, Address sanitizer is supported + bool XnackAvailable = (Features & llvm::AMDGPU::FEATURE_XNACK_ALWAYS); + if (XnackAvailable) + return true; - // Look for the xnack feature in TargetID + // Otherwise, check if xnack+ is explicitly enabled in the target ID llvm::StringMap<bool> FeatureMap; - auto OptionalGpuArch = parseTargetID(TC.getTriple(), TargetID, &FeatureMap); - assert(OptionalGpuArch && "Invalid Target ID"); - (void)OptionalGpuArch; + auto OptionalGpuArch = parseTargetID(TT, TargetID, &FeatureMap); + if (!OptionalGpuArch) + return false; auto Loc = FeatureMap.find("xnack"); - if (Loc == FeatureMap.end() || !Loc->second) { - 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 (Loc != FeatureMap.end() && Loc->second); +} - return false; +SanitizerMask AMDGPUToolChain::getSupportedSanitizers( + StringRef BoundArch, Action::OffloadKind DeviceOffloadKind) const { + SanitizerMask SupportedMask = + SanitizerKind::Undefined | SanitizerKind::UndefinedGroup; + + // Address sanitizer is potentially supported, but depends on the exact target + // arch xnack support. + if (BoundArch.empty() || isXnackAvailable(getTriple(), BoundArch)) + SupportedMask |= SanitizerKind::Address; + + return SupportedMask; +} + +StringRef AMDGPUToolChain::getSanitizerRequirement(SanitizerMask Kinds, + StringRef BoundArch) const { + // Address sanitizer requires xnack+ feature + if ((Kinds & SanitizerKind::Address) && !BoundArch.empty() && + !isXnackAvailable(getTriple(), BoundArch)) { + return "xnack+"; + } + return ""; } diff --git a/clang/lib/Driver/ToolChains/AMDGPU.h b/clang/lib/Driver/ToolChains/AMDGPU.h index 1698786f0e27a..2a5e28224808c 100644 --- a/clang/lib/Driver/ToolChains/AMDGPU.h +++ b/clang/lib/Driver/ToolChains/AMDGPU.h @@ -101,11 +101,8 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUToolChain : public Generic_ELF { /// Needed for translating LTO options. const char *getDefaultLinker() const override { return "ld.lld"; } - /// Should skip sanitize option. - bool shouldSkipSanitizeOption(const ToolChain &TC, - const llvm::opt::ArgList &DriverArgs, - StringRef TargetID, - const llvm::opt::Arg *A) const; + StringRef getSanitizerRequirement(SanitizerMask Kinds, + StringRef BoundArch) const override; /// Uses amdgpu-arch tool to get arch of the system GPU. Will return error /// if unable to find one. @@ -134,6 +131,10 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUToolChain : public Generic_ELF { /// Common warning options shared by AMDGPU HIP, OpenCL and OpenMP toolchains. /// Language specific warning options should go to derived classes. void addClangWarningOptions(llvm::opt::ArgStringList &CC1Args) const override; + + SanitizerMask + getSupportedSanitizers(StringRef BoundArch, + Action::OffloadKind DeviceOffloadKind) const override; }; class LLVM_LIBRARY_VISIBILITY ROCMToolChain : public AMDGPUToolChain { @@ -148,23 +149,9 @@ class LLVM_LIBRARY_VISIBILITY ROCMToolChain : public AMDGPUToolChain { // Returns a list of device library names shared by different languages llvm::SmallVector<BitCodeLibraryInfo, 12> getCommonDeviceLibNames(const llvm::opt::ArgList &DriverArgs, - llvm::StringRef GPUArch, + llvm::StringRef TargetID, llvm::StringRef GPUArch, Action::OffloadKind DeviceOffloadingKind) const; - // FIXME: Remove this and make use of OffloadKind argument to - // getSupportedSanitizers - static constexpr SanitizerMask getOffloadSupportedSanitizers() { - return SanitizerKind::Address | SanitizerKind::Undefined | - SanitizerKind::UndefinedGroup; - } - - SanitizerMask - getSupportedSanitizers(StringRef BoundArch, - Action::OffloadKind DeviceOffloadKind) const override { - assert(DeviceOffloadKind == Action::OFK_None); - return getOffloadSupportedSanitizers(); - } - bool diagnoseUnsupportedOption(const llvm::opt::Arg *A, const llvm::opt::DerivedArgList &DAL, const llvm::opt::ArgList &DriverArgs, @@ -189,61 +176,6 @@ class LLVM_LIBRARY_VISIBILITY ROCMToolChain : public AMDGPUToolChain { } return true; } - - bool handleSanitizeOption(const ToolChain &TC, llvm::opt::DerivedArgList &DAL, - const llvm::opt::ArgList &DriverArgs, - StringRef TargetID, const llvm::opt::Arg *A) const { - if (TargetID.em... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/196737 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
