[Clang] Do not crash when an invalid offload architecture is set

[jhuber6]

Summary

If an invalid architecture is set we currently return an empty string.
This will cause the offloading toolchain to continue to be built and
hit an assertion elsewhere due to the invalid architecture. This patch
fixes that so we now correctly exit.

[jhuber6]

Fix missing None return on HIP.

[tra]

LGTM with few nits.

clang/lib/Driver/Driver.cpp

4194

Nit. Optional is fine, but we could've just checked returned value for Arch.empty().

4288

Nit: do we need explicit StringRef() here? If implicit type conversion does not work, we could use Arch.equals("all").
The current code is fine, but it does stick out a bit and makes me wonder if there's something interesting going on there.

[jhuber6]

clang/lib/Driver/Driver.cpp

4194

Probably the better option. I originally used Optional for the implicit boolean conversion, but I don't think it'll make the code that much more complicated.

4288

You're right, previously this used Arg-getValue() but then when it changed to llvm::split it got converted to a StringRef so this can be updated now. Thanks for pointing it out.

[jhuber6]

Making suggested changes.

[tra]

clang/lib/Driver/Driver.cpp

4211

This can be undone now, too.

4224

ditto.

4293–4296

Nit: we could save one line here:

if(ArchStr.empty()) 
  return Archs;
Archs.erase(ArchStr);

[jhuber6]

Making suggested changes, Early exists is part of the LLVM style so it's definitely better.

clang/lib/Driver/Driver.cpp

void Driver::BuildActions(Compilation &C, DerivedArgList &Args,
   Args.ClaimAllArgs(options::OPT_cl_ignored_Group);
 }
 
 /// Returns the canonical name for the offloading architecture when using a HIP
 /// or CUDA architecture.
 static StringRef getCanonicalArchString(Compilation &C,
                                         const llvm::opt::DerivedArgList &Args,
                                         StringRef ArchStr,
                                         const llvm::Triple &Triple) {
   // Lookup the CUDA / HIP architecture string. Only report an error if we were
   // expecting the triple to be only NVPTX / AMDGPU.
   CudaArch Arch = StringToCudaArch(getProcessorFromTargetID(Triple, ArchStr));
   if (Triple.isNVPTX() &&
       (Arch == CudaArch::UNKNOWN || !IsNVIDIAGpuArch(Arch))) {
     C.getDriver().Diag(clang::diag::err_drv_offload_bad_gpu_arch)
         << "CUDA" << ArchStr;
     return StringRef();
   } else if (Triple.isAMDGPU() &&
              (Arch == CudaArch::UNKNOWN || !IsAMDGpuArch(Arch))) {
     C.getDriver().Diag(clang::diag::err_drv_offload_bad_gpu_arch)
         << "HIP" << ArchStr;
     return StringRef();
   }
 
   if (IsNVIDIAGpuArch(Arch))
     return Args.MakeArgStringRef(CudaArchToString(Arch));
 
   if (IsAMDGpuArch(Arch)) {
     llvm::StringMap<bool> Features;
     auto HIPTriple = getHIPOffloadTargetTriple(C.getDriver(), C.getInputArgs());
     if (!HIPTriple)
       return StringRef();
     auto Arch = parseTargetID(*HIPTriple, ArchStr, &Features);
     if (!Arch) {
       C.getDriver().Diag(clang::diag::err_drv_bad_target_id) << ArchStr;
       C.setContainsError();
       return StringRef();
     }
     return Args.MakeArgStringRef(getCanonicalTargetID(*Arch, Features));
   }
 
   // If the input isn't CUDA or HIP just return the architecture.
   return ArchStr;
 }
 
 /// Checks if the set offloading architectures does not conflict. Returns the
 /// incompatible pair if a conflict occurs.
for (auto *Arg : Args) {
     if (Arg->getOption().matches(options::OPT_Xopenmp_target_EQ) &&
         ToolChain::getOpenMPTriple(Arg->getValue(0)) == TC->getTriple()) {
       Arg->claim();
       unsigned Index = Args.getBaseArgs().MakeIndex(Arg->getValue(1));
       ExtractedArg = getOpts().ParseOneArg(Args, Index);
       Arg = ExtractedArg.get();
     }
 
+    // Add or remove the seen architectures in order of appearance. If an
+    // invalid architecture is given we simply exit.
     if (Arg->getOption().matches(options::OPT_offload_arch_EQ)) {
-      for (StringRef Arch : llvm::split(Arg->getValue(), ","))
-        Archs.insert(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+      for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
+        StringRef ArchStr =
+            getCanonicalArchString(C, Args, Arch, TC->getTriple());
+        if (ArchStr.empty())
+          return Archs;
+        Archs.insert(ArchStr);
+      }
     } else if (Arg->getOption().matches(options::OPT_no_offload_arch_EQ)) {
       for (StringRef Arch : llvm::split(Arg->getValue(), ",")) {
-        if (Arch == StringRef("all"))
+        if (Arch == "all") {
           Archs.clear();
-        else
-          Archs.erase(getCanonicalArchString(C, Args, Arch, TC->getTriple()));
+        } else {
+          StringRef ArchStr =
+              getCanonicalArchString(C, Args, Arch, TC->getTriple());
+          if (ArchStr.empty())
+            return Archs;
+          Archs.erase(ArchStr);
+        }
       }
     }
   }
 
   if (auto ConflictingArchs = getConflictOffloadArchCombination(Archs, Kind)) {
     C.getDriver().Diag(clang::diag::err_drv_bad_offload_arch_combo)
         << ConflictingArchs->first << ConflictingArchs->second;
     C.setContainsError();

clang/test/Driver/cuda-phases.cu

 // LTO-NEXT: 9: preprocessor, {8}, cuda-cpp-output, (device-cuda, sm_70)
 // LTO-NEXT: 10: compiler, {9}, ir, (device-cuda, sm_70)
 // LTO-NEXT: 11: backend, {10}, lto-bc, (device-cuda, sm_70)
 // LTO-NEXT: 12: offload, "device-cuda (nvptx64-nvidia-cuda:sm_70)" {11}, lto-bc
 // LTO-NEXT: 13: clang-offload-packager, {7, 12}, image, (device-cuda)
 // LTO-NEXT: 14: offload, "host-cuda (powerpc64le-ibm-linux-gnu)" {2}, "device-cuda (powerpc64le-ibm-linux-gnu)" {13}, ir
 // LTO-NEXT: 15: backend, {14}, assembler, (host-cuda)
 // LTO-NEXT: 16: assembler, {15}, object, (host-cuda)
+
+//
+// Test that the new driver does not create actions for invalid architectures.
+//
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:        -ccc-print-phases --offload-arch=sm_999 -fgpu-rdc -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-ARCH %s
+//      INVALID-ARCH: error: unsupported CUDA gpu architecture: sm_999
+// INVALID-ARCH-NEXT: 0: input, "[[INPUT:.+]]", cuda, (host-cuda)
+// INVALID-ARCH-NEXT: 1: preprocessor, {0}, cuda-cpp-output, (host-cuda)
+// INVALID-ARCH-NEXT: 2: compiler, {1}, ir, (host-cuda)
+// INVALID-ARCH-NEXT: 3: backend, {2}, assembler, (host-cuda)
+// INVALID-ARCH-NEXT: 4: assembler, {3}, object, (host-cuda)