[clang] Fix D26214: Move error handling out of MC and to the callers.

[mgrang]

Summary

Related llvm patch: https://reviews.llvm.org/D27359

[compnerd]

I think that the argument parsing should really be hoisted into the driver rather than pushed down into the assembler. The driver can parse and validate the options before passing them down, so when cc1as gets it, it will simply set the value.

The newly introduced diagnostics, especially right now, are not driver related. Nor are they common. I would say that we would need a DiagnosticAssemblerKind.td for these.

[mgrang]

Moved error checking up to the driver and added another test case.

[compnerd]

Im not the biggest fan of this. However, having some validation is probably better than not. We can come up with a better way to address this in the future when needed I suppose.

include/clang/Basic/DiagnosticDriverKinds.td

def warn_drv_ps4_force_pic : Warning<
   "option '%0' was ignored by the PS4 toolchain, using '-fPIC'">,
   InGroup<OptionIgnored>;
 
 def warn_drv_ps4_sdk_dir : Warning<
   "environment variable SCE_ORBIS_SDK_DIR is set, but points to invalid or nonexistent directory '%0'">,
   InGroup<InvalidOrNonExistentDirectory>;
 
 def err_drv_unsupported_linker : Error<"unsupported value '%0' for -linker option">;
+def err_drv_defsym_invalid_format : Error<"defsym must be of the form: sym=value: %0">;
+def err_drv_defsym_invalid_symval : Error<"Value is not an integer: %0">;
 }

lib/Driver/Tools.cpp

for (StringRef Value : A->getValues()) {
           RenderDebugEnablingArgs(Args, CmdArgs,
                                   codegenoptions::LimitedDebugInfo,
                                   DwarfVersion, llvm::DebuggerKind::Default);
         }
       } else if (Value.startswith("-mcpu") || Value.startswith("-mfpu") ||
                  Value.startswith("-mhwdiv") || Value.startswith("-march")) {
         // Do nothing, we'll validate it later.
       } else if (Value == "-defsym") {
+          if (A->getNumValues() != 2) {
+            D.Diag(diag::err_drv_defsym_invalid_format) << Value;
+            break;
+          }
+          const char *S = A->getValue(1);
+          auto Pair = StringRef(S).split('=');
+          auto Sym = Pair.first;
+          auto SVal = Pair.second;
+
+          if (Sym.empty() || SVal.empty()) {
+            D.Diag(diag::err_drv_defsym_invalid_format) << S;
+            break;
+          }
+          int64_t IVal;
+          if (SVal.getAsInteger(0, IVal)) {
+            D.Diag(diag::err_drv_defsym_invalid_symval) << SVal;
+            break;
+          }
           CmdArgs.push_back(Value.data());
           TakeNextArg = true;
       } else {
         D.Diag(diag::err_drv_unsupported_option_argument)
             << A->getOption().getName() << Value;
       }
     }
   }

test/Driver/defsym.s

 // CHECK-DEFSYM1: "xyz=0xa"
 
 // RUN: not %clang -c -integrated-as -o /dev/null %s \
 // RUN: -Wa,-defsym,abc= \
 // RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR1
 // CHECK-DEFSYM-ERR1: error: defsym must be of the form: sym=value: abc=
 
 // RUN: not %clang -c -integrated-as -o /dev/null %s \
-// RUN: -Wa,-defsym,abc=1a2b3c \
+// RUN: -Wa,-defsym,=123 \
 // RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR2
-// CHECK-DEFSYM-ERR2: error: Value is not an integer: 1a2b3c
+// CHECK-DEFSYM-ERR2: error: defsym must be of the form: sym=value: =123
+
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -Wa,-defsym,abc=1a2b3c \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR3
+// CHECK-DEFSYM-ERR3: error: Value is not an integer: 1a2b3c
+
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -Wa,-defsym \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR4
+
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -Wa,-defsym, \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR4
+
+// CHECK-DEFSYM-ERR4: error: defsym must be of the form: sym=value: -defsym

tools/driver/cc1as_main.cpp

static bool ExecuteAssembler(AssemblerInvocation &Opts,
   MCTargetOptions Options;
   std::unique_ptr<MCTargetAsmParser> TAP(
       TheTarget->createMCAsmParser(*STI, *Parser, *MCII, Options));
   if (!TAP)
     Failed = Diags.Report(diag::err_target_unknown_triple) << Opts.Triple;
 
   // Set values for symbols, if any.
   for (auto &S : Opts.SymbolDefs) {
-    if (Ctx.setSymbolValue(Parser->getStreamer(), S)) {
-      Failed = true;
-      break;
-    }
+    auto Pair = StringRef(S).split('=');
+    auto Sym = Pair.first;
+    auto Val = Pair.second;
+    int64_t Value;
+    // We have already error checked this in the driver.
+    Val.getAsInteger(0, Value);
+    Ctx.setSymbolValue(Parser->getStreamer(), Sym, Value);
   }
 
   if (!Failed) {
     Parser->setTargetParser(*TAP.get());
     Failed = Parser->Run(Opts.NoInitialTextSection);
   }
 
   // Close Streamer first.