[TableGen][CodeEmitterGen] Do not crash on insufficient positional instruction operands.

[lattner]

I'm not up to speed on this code: is the current convention for tblgen to exit(1) after errors? If so, this looks reasonable to me.

[kosarev]

Right, reworked to use TableGen's PrintFatalError(). Thanks.

[kosarev]

Adding more reviewers in hope someone may have a chance to take a look.

[kosarev]

Ping.

[foad]

llvm/utils/TableGen/CodeEmitterGen.cpp

124–131

Do I understand correctly: the fix is to do this check even if the "while" loop above executed 0 times?

[kosarev]

llvm/utils/TableGen/CodeEmitterGen.cpp

124–131

Yes. With the original code control may reach the OpIdx = NumberedOp++; below several times. As the index gets increased every time, it can at some point become equal to the total number of operands, meaning we would never enter the loop.

Doing PrintFatalError() as the patch intends makes it impossible to catch the error twice, but it still feels better to have the check outside of the loop.

llvm/test/TableGen/InsufficientPositionalOperands.td

+// RUN: not llvm-tblgen -gen-emitter -I %p/../../include %s 2>&1 | FileCheck %s
+
+// Check that TableGen doesn't crash on insufficient positional
+// instruction operands.
+
+include "llvm/Target/Target.td"
+
+def ArchInstrInfo : InstrInfo { }
+
+def Arch : Target {
+  let InstructionSet = ArchInstrInfo;
+}
+
+def Reg : Register<"reg">;
+
+def Regs : RegisterClass<"foo", [i32], 0, (add Reg)>;
+
+def foo : Instruction {
+  bits<3> rd;
+  bits<3> rs;
+
+  bits<8> Inst;
+  let Inst{1-0} = 0;
+  let Inst{4-2} = rd;
+  let Inst{7-5} = rs;
+
+// CHECK: Too few operands in record foo (no match for variable rs)
+  let OutOperandList = (outs Regs:$xd);
+  let InOperandList = (ins);
+}

llvm/utils/TableGen/CodeEmitterGen.cpp

 #include "SubtargetFeatureInfo.h"
 #include "Types.h"
 #include "VarLenCodeEmitterGen.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
 #include "llvm/TableGen/TableGenBackend.h"
 #include <cassert>
 #include <cstdint>
 #include <map>
 #include <set>
 #include <string>
 #include <utility>
if (CGI.Operands.hasOperandNamed(VarName, OpIdx)) {
     unsigned NumberOps = CGI.Operands.size();
     /// If this operand is not supposed to be emitted by the
     /// generated emitter, skip it.
     while (NumberedOp < NumberOps &&
            (CGI.Operands.isFlatOperandNotEmitted(NumberedOp) ||
               (!NamedOpIndices.empty() && NamedOpIndices.count(
                 CGI.Operands.getSubOperandNumber(NumberedOp).first)))) {
       ++NumberedOp;
-
-      if (NumberedOp >= CGI.Operands.back().MIOperandNo +
-                        CGI.Operands.back().MINumOperands) {
-        errs() << "Too few operands in record " << R->getName() <<
-                  " (no match for variable " << VarName << "):\n";
-        errs() << *R;
-        errs() << '\n';
-
-        return;
-      }
+
+    if (NumberedOp >=
+        CGI.Operands.back().MIOperandNo + CGI.Operands.back().MINumOperands) {
+      std::string E;
+      raw_string_ostream S(E);
+      S << "Too few operands in record " << R->getName()
+        << " (no match for variable " << VarName << "):\n";
+      S << *R;
+      PrintFatalError(R, E);
     }
 
     OpIdx = NumberedOp++;
   }
   
   std::pair<unsigned, unsigned> SO = CGI.Operands.getSubOperandNumber(OpIdx);
   std::string &EncoderMethodName = CGI.Operands[SO.first].EncoderMethodName;