[ARM] Include operand class name in dev diags

[rengolin]

This feels like a simple patch that we should keep downstream.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

10216 ↗

(On Diff #119886)

that's a bit hackish, why not always?

[olista01]

Why do you think this should be kept downstream? It's a debug option which I've found useful while adding diagnostics for assembly options, none of which I plan on keeping downstream.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

10216 ↗

(On Diff #119886)

The getMatchClassName is only generated for builds with assertions. This code is only used when the -arm-arm-parser-dev-diags option is used, which is intended for LLVM developers only, so I think it makes sense to keep it this way.

[rengolin]

In D39194#904288, @olista01 wrote:

Why do you think this should be kept downstream? It's a debug option which I've found useful while adding diagnostics for assembly options, none of which I plan on keeping downstream.

Because it's an unusual pattern for assembly parsers in general and it may confuse the end user if they happen to build in assert/debug mode.

End user facing messages should be always the same. If you want a debug message, use DEBUG().

[olista01]

Normal users will only ever see the "invalid operand for instruction" part of the diagnostic, regardess of whether they have assertions enabled or not. The extra info is only enabled when the -arm-arm-parser-dev-diags option is used (the "if (DevDiags)" check above), which isn't even exposed as a clang option (you'd need to pass it through with -mllvm).

I made these development diagnostics part of the actual ones because otherwise it is hard to correlate the DEBUG/dbgs output with the emitted diagnostics, which they elaborate on.

[rengolin]

In D39194#904317, @olista01 wrote:

Normal users will only ever see the "invalid operand for instruction" part of the diagnostic, regardess of whether they have assertions enabled or not. The extra info is only enabled when the -arm-arm-parser-dev-diags option is used (the "if (DevDiags)" check above), which isn't even exposed as a clang option (you'd need to pass it through with -mllvm).

Isn't this going to be enabled by default some time in the near future?

[olista01]

Isn't this going to be enabled by default some time in the near future?

No, the -arm-arm-parser-dev-diags option is intended to permanently be a devlopment option, akin to -debug(-only) and the llvm-mc options like -show-inst, -show-encoding etc. It expands the diagnostics to include things like opcodes and enum values, to make it easier to work out why a more detailed diagnostic wasn't printed.

[rengolin]

Right, looking back, this is the only other use of DevDiags. At all.

I completely missed that when reviewing D31530, and I see now that this is the wrong implementation. We should not have the flag at all, and that should be just a simple usage of DEBUG().

We already have the stderr/stdout sync issue everywhere, and that's not a strong enough reason to add a flag for a single printf (or we wouldn't have DEBUG in the first place).

cheers,
--renato

[olista01]

• Remove the -arm-asm-parser-dev-diags option
• Print the extra info straight to dbgs(), rather than appending to the diagnostic

[rengolin]

LGTM. Thanks!

llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

 #include <cstdint>
 #include <iterator>
 #include <limits>
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
+#define DEBUG_TYPE "asm-parser"
+
 using namespace llvm;
 
 namespace {
 
 enum class ImplicitItModeTy { Always, Never, ARMOnly, ThumbOnly };
 
 static cl::opt<ImplicitItModeTy> ImplicitItMode(
     "arm-implicit-it", cl::init(ImplicitItModeTy::ARMOnly),
     cl::desc("Allow conditional instructions outdside of an IT block"),
     cl::values(clEnumValN(ImplicitItModeTy::Always, "always",
                           "Accept in both ISAs, emit implicit ITs in Thumb"),
                clEnumValN(ImplicitItModeTy::Never, "never",
                           "Warn in ARM, reject in Thumb"),
                clEnumValN(ImplicitItModeTy::ARMOnly, "arm",
                           "Accept in ARM, reject in Thumb"),
                clEnumValN(ImplicitItModeTy::ThumbOnly, "thumb",
                           "Warn in ARM, emit implicit ITs in Thumb")));
 
 static cl::opt<bool> AddBuildAttributes("arm-add-build-attributes",
                                         cl::init(false));
 
-cl::opt<bool>
-DevDiags("arm-asm-parser-dev-diags", cl::init(false),
-         cl::desc("Use extended diagnostics, which include implementation "
-                  "details useful for development"));
-
 enum VectorLaneTy { NoLanes, AllLanes, IndexedLane };
 
 class UnwindContext {
   using Locs = SmallVector<SMLoc, 4>;
 
   MCAsmParser &Parser;
   Locs FnStartLocs;
   Locs CantUnwindLocs;
case NearMissInfo::NearMissOperand: {
                                 (MatchClassKind)Pair.second);
           }))
         break;
       OperandMissesSeen.insert(
           std::make_pair(I.getOperandIndex(), DupCheckMatchClass));
 
       NearMissMessage Message;
       Message.Loc = OperandLoc;
-      raw_svector_ostream OS(Message.Message);
       if (OperandDiag) {
-        OS << OperandDiag;
+        Message.Message = OperandDiag;
       } else if (I.getOperandClass() == InvalidMatchClass) {
-        OS << "too many operands for instruction";
+        Message.Message = "too many operands for instruction";
       } else {
-        OS << "invalid operand for instruction";
-        if (DevDiags) {
-          OS << " class" << I.getOperandClass() << ", error "
-             << I.getOperandError() << ", opcode "
-             << MII.getName(I.getOpcode());
-        }
+        Message.Message = "invalid operand for instruction";
+        DEBUG(dbgs() << "Missing diagnostic string for operand class " <<
+              getMatchClassName((MatchClassKind)I.getOperandClass())
+              << I.getOperandClass() << ", error " << I.getOperandError()
+              << ", opcode " << MII.getName(I.getOpcode()) << "\n");
       }
       NearMissesOut.emplace_back(Message);
       break;
     }
     case NearMissInfo::NearMissFeature: {
       uint64_t MissingFeatures = I.getFeatures();
       // Don't report the same set of features twice.
       if (FeatureMissesSeen.count(MissingFeatures))