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

[Mandeep Singh Grang via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL288762: [clang] Fix D26214: Move error handling out of MC 
and to the callers. (authored by mgrang).

Changed prior to commit:
  https://reviews.llvm.org/D27360?vs=80314=80369#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27360

Files:
  cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
  cfe/trunk/lib/Driver/Tools.cpp
  cfe/trunk/test/Driver/defsym.s
  cfe/trunk/tools/driver/cc1as_main.cpp


Index: cfe/trunk/lib/Driver/Tools.cpp
===
--- cfe/trunk/lib/Driver/Tools.cpp
+++ cfe/trunk/lib/Driver/Tools.cpp
@@ -3116,6 +3116,24 @@
  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 {
Index: cfe/trunk/tools/driver/cc1as_main.cpp
===
--- cfe/trunk/tools/driver/cc1as_main.cpp
+++ cfe/trunk/tools/driver/cc1as_main.cpp
@@ -426,10 +426,13 @@
 
   // Set values for symbols, if any.
   for (auto  : 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) {
Index: cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
@@ -277,4 +277,6 @@
   InGroup;
 
 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">;
 }
Index: cfe/trunk/test/Driver/defsym.s
===
--- cfe/trunk/test/Driver/defsym.s
+++ cfe/trunk/test/Driver/defsym.s
@@ -17,6 +17,21 @@
 // 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


Index: cfe/trunk/lib/Driver/Tools.cpp
===
--- cfe/trunk/lib/Driver/Tools.cpp
+++ cfe/trunk/lib/Driver/Tools.cpp
@@ -3116,6 +3116,24 @@
  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());
 

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

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.


https://reviews.llvm.org/D27360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mandeep Singh Grang via Phabricator via cfe-commits]

mgrang updated this revision to Diff 80314.

https://reviews.llvm.org/D27360

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  lib/Driver/Tools.cpp
  test/Driver/defsym.s
  tools/driver/cc1as_main.cpp


Index: tools/driver/cc1as_main.cpp
===
--- tools/driver/cc1as_main.cpp
+++ tools/driver/cc1as_main.cpp
@@ -426,10 +426,13 @@
 
   // Set values for symbols, if any.
   for (auto  : 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) {
Index: test/Driver/defsym.s
===
--- test/Driver/defsym.s
+++ test/Driver/defsym.s
@@ -17,6 +17,21 @@
 // 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
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -3116,6 +3116,24 @@
  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 {
Index: include/clang/Basic/DiagnosticDriverKinds.td
===
--- include/clang/Basic/DiagnosticDriverKinds.td
+++ include/clang/Basic/DiagnosticDriverKinds.td
@@ -277,4 +277,6 @@
   InGroup;
 
 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">;
 }


Index: tools/driver/cc1as_main.cpp
===
--- tools/driver/cc1as_main.cpp
+++ tools/driver/cc1as_main.cpp
@@ -426,10 +426,13 @@
 
   // Set values for symbols, if any.
   for (auto  : 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) {
Index: test/Driver/defsym.s
===
--- test/Driver/defsym.s
+++ test/Driver/defsym.s
@@ -17,6 +17,21 @@
 // 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
+
+// 

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

[Mandeep Singh Grang via Phabricator via cfe-commits]

mgrang added a comment.

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


https://reviews.llvm.org/D27360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

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.


https://reviews.llvm.org/D27360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits