X86 Asm should produce error messages instead of assertions when it's possible

[avt77]

Summary

If we use Intal syntax assembler and deal with negative value of Scale in LEA instruction then Clang crashes (see pr33661 for details).

[RKSimon]

lib/Target/X86/AsmParser/X86AsmParser.cpp

532 ↗

(On Diff #105609)

Add newlines before/after the function and pass it through clang-format

553 ↗

(On Diff #105609)

if (checkScale(ErrMsg))
  return true;

605 ↗

(On Diff #105609)

if (checkScale(ErrMsg))
  return true;

test/CodeGen/ARM/arguments-nosplit-double.ll

11 ↗

(On Diff #105609)

NFC commit?

test/CodeGen/ARM/arguments-nosplit-i64.ll

11 ↗

(On Diff #105609)

NFC commit?

[avt77]

I removed NFCs from this patch.

[RKSimon]

test/MC/X86/intel-syntax-invalid-scale.s

12 ↗

(On Diff #105645)

Any idea why this error message is different? Is it worth fixing?

[avt77]

test/MC/X86/intel-syntax-invalid-scale.s

12 ↗

(On Diff #105645)

gcc produces the same message like above 3 ones but Clang does what we see. Clang does it automatically (without any my intervation) that's why I decided don't touch it. But I could try to fix.

[RKSimon]

test/MC/X86/intel-syntax-invalid-scale.s

12 ↗

(On Diff #105645)

Do you think you can easily add it to this patch or would it be better to separate it as a follow up patch?

[avt77]

test/MC/X86/intel-syntax-invalid-scale.s

12 ↗

(On Diff #105645)

I'm no sure it's easy but I'll try to do it tomorrow. If I fail...

[avt77]

test/MC/X86/intel-syntax-invalid-scale.s

12 ↗

(On Diff #105645)

I need some more time to fix it that's why it's better to do it in the follow up patch.

[RKSimon]

lib/Target/X86/AsmParser/X86AsmParser.cpp

129 ↗

(On Diff #105645)

Improve the comment to explain that the invalid value will be caught later by checkScale

[avt77]

This patch replaces assertions with normal diagnostic when it's possible. Now we see normal error messages instead of compiler crashes. It should close PR33861, PR33862 and PR33661. In addition the patch prepares tests for D35621.

[davide]

LGTM but please test with assertions on :)

[RKSimon]

LGTM with one minor

test/MC/X86/intel-syntax2.s

2 ↗

(On Diff #107675)

Very minor but please call this file intel-syntax-3.s (we already have a intel-syntax-2.s).

llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp

 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <memory>
 
 using namespace llvm;
 
+static bool checkScale(unsigned Scale, StringRef &ErrMsg) {
+  if (Scale != 1 && Scale != 2 && Scale != 4 && Scale != 8) {
+    ErrMsg = "scale factor in address must be 1, 2, 4 or 8";
+    return true;
+  }
+  return false;
+}
+
 namespace {
 
 static const char OpPrecedence[] = {
   0, // IC_OR
   1, // IC_XOR
   2, // IC_AND
   3, // IC_LSHIFT
   3, // IC_RSHIFT
class InfixCalculator {
     bool isUnaryOperator(const InfixCalculatorTok Op) {
       return Op == IC_NEG || Op == IC_NOT;
     }
 
   public:
     int64_t popOperand() {
       assert (!PostfixStack.empty() && "Poped an empty stack!");
       ICToken Op = PostfixStack.pop_back_val();
-      assert ((Op.first == IC_IMM || Op.first == IC_REGISTER)
-              && "Expected and immediate or register!");
+      if (!(Op.first == IC_IMM || Op.first == IC_REGISTER))
+        return -1; // The invalid Scale value will be caught later by checkScale
       return Op.second;
     }
     void pushOperand(InfixCalculatorTok Op, int64_t Val = 0) {
       assert ((Op == IC_IMM || Op == IC_REGISTER) &&
               "Unexpected operand!");
       PostfixStack.push_back(std::make_pair(Op, Val));
     }
 
void onRShift() {
       case IES_RPAREN:
       case IES_REGISTER:
         State = IES_RSHIFT;
         IC.pushOperator(IC_RSHIFT);
         break;
       }
       PrevState = CurrState;
     }
-    void onPlus() {
+    bool onPlus(StringRef &ErrMsg) {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_INTEGER:
       case IES_RPAREN:
       case IES_REGISTER:
         State = IES_PLUS;
         IC.pushOperator(IC_PLUS);
         if (CurrState == IES_REGISTER && PrevState != IES_MULTIPLY) {
           // If we already have a BaseReg, then assume this is the IndexReg with
           // a scale of 1.
           if (!BaseReg) {
             BaseReg = TmpReg;
           } else {
-            assert (!IndexReg && "BaseReg/IndexReg already set!");
+            if (IndexReg) {
+              ErrMsg = "BaseReg/IndexReg already set!";
+              return true;
+            }
             IndexReg = TmpReg;
             Scale = 1;
           }
         }
         break;
       }
       PrevState = CurrState;
+      return false;
     }
-    void onMinus() {
+    bool onMinus(StringRef &ErrMsg) {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_OR:
       case IES_XOR:
       case IES_AND:
bool onMinus(StringRef &ErrMsg) {
       case IES_RBRAC:
       case IES_INTEGER:
       case IES_REGISTER:
         State = IES_MINUS;
         // push minus operator if it is not a negate operator
         if (CurrState == IES_REGISTER || CurrState == IES_RPAREN ||
             CurrState == IES_INTEGER  || CurrState == IES_RBRAC)
           IC.pushOperator(IC_MINUS);
-        else
+        else if (PrevState == IES_REGISTER && CurrState == IES_MULTIPLY) {
+          // We have negate operator for Scale: it's illegal
+          ErrMsg = "Scale can't be negative";
+          return true;
+        } else
           IC.pushOperator(IC_NEG);
         if (CurrState == IES_REGISTER && PrevState != IES_MULTIPLY) {
           // If we already have a BaseReg, then assume this is the IndexReg with
           // a scale of 1.
           if (!BaseReg) {
             BaseReg = TmpReg;
           } else {
-            assert (!IndexReg && "BaseReg/IndexReg already set!");
+            if (IndexReg) {
+              ErrMsg = "BaseReg/IndexReg already set!";
+              return true;
+            }
             IndexReg = TmpReg;
             Scale = 1;
           }
         }
         break;
       }
       PrevState = CurrState;
+      return false;
     }
     void onNot() {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_OR:
void onNot() {
       case IES_LPAREN:
       case IES_LBRAC:
         State = IES_NOT;
         IC.pushOperator(IC_NOT);
         break;
       }
       PrevState = CurrState;
     }
-    void onRegister(unsigned Reg) {
+
+    bool onRegister(unsigned Reg, StringRef &ErrMsg) {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_PLUS:
       case IES_LPAREN:
         State = IES_REGISTER;
         TmpReg = Reg;
         IC.pushOperand(IC_REGISTER);
         break;
       case IES_MULTIPLY:
         // Index Register - Scale * Register
         if (PrevState == IES_INTEGER) {
-          assert (!IndexReg && "IndexReg already set!");
+          if (IndexReg) {
+            ErrMsg = "BaseReg/IndexReg already set!";
+            return true;
+          }
           State = IES_REGISTER;
           IndexReg = Reg;
           // Get the scale and replace the 'Scale * Register' with '0'.
           Scale = IC.popOperand();
+          if (checkScale(Scale, ErrMsg))
+            return true;
           IC.pushOperand(IC_IMM);
           IC.popOperator();
         } else {
           State = IES_ERROR;
         }
         break;
       }
       PrevState = CurrState;
+      return false;
     }
     void onIdentifierExpr(const MCExpr *SymRef, StringRef SymRefName) {
       PrevState = State;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_PLUS:
bool onInteger(int64_t TmpInt, StringRef &ErrMsg) {
       case IES_RSHIFT:
       case IES_DIVIDE:
       case IES_MOD:
       case IES_MULTIPLY:
       case IES_LPAREN:
         State = IES_INTEGER;
         if (PrevState == IES_REGISTER && CurrState == IES_MULTIPLY) {
           // Index Register - Register * Scale
-          assert (!IndexReg && "IndexReg already set!");
+          if (IndexReg) {
+            ErrMsg = "BaseReg/IndexReg already set!";
+            return true;
+          }
           IndexReg = TmpReg;
           Scale = TmpInt;
-          if(Scale != 1 && Scale != 2 && Scale != 4 && Scale != 8) {
-            ErrMsg = "scale factor in address must be 1, 2, 4 or 8";
+          if (checkScale(Scale, ErrMsg))
             return true;
-          }
           // Get the scale and replace the 'Register * Scale' with '0'.
           IC.popOperator();
         } else {
           IC.pushOperand(IC_IMM, TmpInt);
         }
         break;
       }
       PrevState = CurrState;

 
 /// @name Auto-generated Match Functions
 /// {
 
 static unsigned MatchRegisterName(StringRef Name);
 
 /// }
 
-static bool CheckBaseRegAndIndexReg(unsigned BaseReg, unsigned IndexReg,
-                                    StringRef &ErrMsg) {
+static bool CheckBaseRegAndIndexRegAndScale(unsigned BaseReg, unsigned IndexReg,
+                                            unsigned Scale, StringRef &ErrMsg) {
   // If we have both a base register and an index register make sure they are
   // both 64-bit or 32-bit registers.
   // To support VSIB, IndexReg can be 128-bit or 256-bit registers.
 
   if ((BaseReg == X86::RIP && IndexReg != 0) || (IndexReg == X86::RIP)) {
     ErrMsg = "invalid base+index expression";
     return true;
   }
if (X86MCRegisterClasses[X86::GR16RegClassID].contains(BaseReg)) {
            IndexReg != X86::SI && IndexReg != X86::DI) ||
           ((BaseReg == X86::SI || BaseReg == X86::DI) &&
            IndexReg != X86::BX && IndexReg != X86::BP)) {
         ErrMsg = "invalid 16-bit base/index register combination";
         return true;
       }
     }
   }
-  return false;
+  return checkScale(Scale, ErrMsg);
 }
 
 bool X86AsmParser::ParseRegister(unsigned &RegNo,
                                  SMLoc &StartLoc, SMLoc &EndLoc) {
   MCAsmParser &Parser = getParser();
   RegNo = 0;
   const AsmToken &PercentTok = Parser.getTok();
   StartLoc = PercentTok.getLoc();
bool X86AsmParser::ParseIntelNamedOperator(StringRef Name, IntelExprStateMachine &SM) {
   else
     return false;
   return true;
 }
 
 bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End) {
   MCAsmParser &Parser = getParser();
   const AsmToken &Tok = Parser.getTok();
+  StringRef ErrMsg;
 
   AsmToken::TokenKind PrevTK = AsmToken::Error;
   bool Done = false;
   while (!Done) {
     bool UpdateLocLex = true;
 
     AsmToken::TokenKind TK = getLexer().getKind();
     // The period in the dot operator (e.g., [ebx].foo.bar) is parsed as an
while (!Done) {
     case AsmToken::Identifier: {
       // This could be a register or a symbolic displacement.
       unsigned TmpReg;
       const MCExpr *Val;
       SMLoc IdentLoc = Tok.getLoc();
       StringRef Identifier = Tok.getString();
       UpdateLocLex = false;
       if (TK != AsmToken::String && !ParseRegister(TmpReg, IdentLoc, End)) {
-        SM.onRegister(TmpReg);
+        if (SM.onRegister(TmpReg, ErrMsg))
+          return Error(Tok.getLoc(), ErrMsg);
       } else if (ParseIntelNamedOperator(Identifier, SM)) {
         UpdateLocLex = true;
       } else if (!isParsingInlineAsm()) {
         if (getParser().parsePrimaryExpr(Val, End))
           return Error(Tok.getLoc(), "Unexpected identifier!");
         SM.onIdentifierExpr(Val, Identifier);
       } else if (unsigned OpKind = IdentifyIntelOperator(Identifier)) {
         if (OpKind == IOK_OFFSET) 
           return Error(IdentLoc, "Dealing OFFSET operator as part of"
             "a compound immediate expression is yet to be supported");
         int64_t Val = ParseIntelOperator(OpKind,SM.getAddImmPrefix());
         if (!Val)
           return true;
-        StringRef ErrMsg;
         if (SM.onInteger(Val, ErrMsg))
           return Error(IdentLoc, ErrMsg);
       } else if (Identifier.find('.') != StringRef::npos &&
             PrevTK == AsmToken::RBrac) {
           return false;
       } else {
         InlineAsmIdentifierInfo Info;
         if (ParseIntelIdentifier(Val, Identifier, Info,
case AsmToken::Identifier: {
           InlineAsmIdentifierInfo &SMInfo = SM.getIdentifierInfo();
           SMInfo = Info;
           SM.onIdentifierExpr(Val, Identifier);
         }
       }
       break;
     }
     case AsmToken::Integer: {
-      StringRef ErrMsg;
       if (isParsingInlineAsm() && SM.getAddImmPrefix())
         InstInfo->AsmRewrites->emplace_back(AOK_ImmPrefix, Tok.getLoc());
       // Look for 'b' or 'f' following an Integer as a directional label
       SMLoc Loc = getTok().getLoc();
       int64_t IntVal = getTok().getIntVal();
       End = consumeToken();
       UpdateLocLex = false;
       if (getLexer().getKind() == AsmToken::Identifier) {
case AsmToken::Integer: {
             return Error(Loc, ErrMsg);
         }
       } else {
         if (SM.onInteger(IntVal, ErrMsg))
           return Error(Loc, ErrMsg);
       }
       break;
     }
-    case AsmToken::Plus:    SM.onPlus(); break;
-    case AsmToken::Minus:   SM.onMinus(); break;
+    case AsmToken::Plus:
+      if (SM.onPlus(ErrMsg))
+        return Error(getTok().getLoc(), ErrMsg);
+      break;
+    case AsmToken::Minus:
+      if (SM.onMinus(ErrMsg))
+        return Error(getTok().getLoc(), ErrMsg);
+      break;
     case AsmToken::Tilde:   SM.onNot(); break;
     case AsmToken::Star:    SM.onStar(); break;
     case AsmToken::Slash:   SM.onDivide(); break;
     case AsmToken::Pipe:    SM.onOr(); break;
     case AsmToken::Caret:   SM.onXor(); break;
     case AsmToken::Amp:     SM.onAnd(); break;
     case AsmToken::LessLess:
                             SM.onLShift(); break;
if (!isParsingInlineAsm()) {
     // handle [-42]
     if (!BaseReg && !IndexReg) {
       if (!SegReg)
         return X86Operand::CreateMem(getPointerWidth(), Disp, Start, End, Size);
       return X86Operand::CreateMem(getPointerWidth(), SegReg, Disp, 0, 0, 1,
                                    Start, End, Size);
     }
     StringRef ErrMsg;
-    if (CheckBaseRegAndIndexReg(BaseReg, IndexReg, ErrMsg)) {
+    if (CheckBaseRegAndIndexRegAndScale(BaseReg, IndexReg, Scale, ErrMsg)) {
       Error(StartInBrac, ErrMsg);
       return nullptr;
     }
     return X86Operand::CreateMem(getPointerWidth(), SegReg, Disp, BaseReg,
                                  IndexReg, Scale, Start, End, Size);
   }
 
   InlineAsmIdentifierInfo &Info = SM.getIdentifierInfo();
std::unique_ptr<X86Operand> X86AsmParser::ParseMemOperand(unsigned SegReg,
   }
   if (BaseReg == 0 &&
       X86MCRegisterClasses[X86::GR16RegClassID].contains(IndexReg)) {
     Error(IndexLoc, "16-bit memory operand may not include only index register");
     return nullptr;
   }
 
   StringRef ErrMsg;
-  if (CheckBaseRegAndIndexReg(BaseReg, IndexReg, ErrMsg)) {
+  if (CheckBaseRegAndIndexRegAndScale(BaseReg, IndexReg, Scale, ErrMsg)) {
     Error(BaseLoc, ErrMsg);
     return nullptr;
   }
 
   if (SegReg || BaseReg || IndexReg)
     return X86Operand::CreateMem(getPointerWidth(), SegReg, Disp, BaseReg,
                                  IndexReg, Scale, MemStart, MemEnd);
   return X86Operand::CreateMem(getPointerWidth(), Disp, MemStart, MemEnd);

llvm/trunk/test/MC/X86/intel-syntax-3.s

+// RUN: not llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel %s 2> %t.err
+// RUN: FileCheck --check-prefix=CHECK-STDERR < %t.err %s
+
+_test:
+// CHECK-LABEL: _test:
+// CHECK: xorl    %eax, %eax
+
+  xor EAX, EAX
+  ret
+
+.set  number, 8
+.global _foo
+
+.text
+  .global main
+main:
+
+// CHECK-STDERR:  error: unknown token in expression
+  lea RDX, [RAX * number + RBX + _foo]
+
+// CHECK-STDERR:  error: unknown token in expression
+  lea RDX, [_foo + RAX * number + RBX]
+
+// CHECK-STDERR:  error: unknown token in expression
+  lea RDX, [number + RAX * number + RCX]
+
+// CHECK-STDERR:  error: unknown token in expression
+  lea RDX, [_foo + RAX * number]
+
+// CHECK-STDERR:  error: unknown token in expression
+  lea RDX, [_foo + RAX * number + RBX]
+
+// CHECK-STDERR: scale factor in address must be 1, 2, 4 or 8
+  lea RDX, [number * RAX + RBX + _foo]
+
+// CHECK-STDERR: scale factor in address must be 1, 2, 4 or 8
+  lea RDX, [_foo + number * RAX + RBX]
+
+// CHECK-STDERR: scale factor in address must be 1, 2, 4 or 8
+  lea RDX, [8 + number * RAX + RCX]
+
+// CHECK-STDERR: scale factor in address must be 1, 2, 4 or 8
+lea RDX, [unknown_number * RAX + RBX + _foo]
+
+// CHECK-STDERR: error: BaseReg/IndexReg already set!
+lea RDX, [4 * RAX + 27 * RBX + _pat]

llvm/trunk/test/MC/X86/intel-syntax-invalid-scale.s

 // RUN: not llvm-mc -triple x86_64-unknown-unknown %s 2> %t.err
 // RUN: FileCheck < %t.err %s
 
 .intel_syntax
 
 // CHECK: error: scale factor in address must be 1, 2, 4 or 8
     lea rax, [rdi + rdx*64]
 // CHECK: error: scale factor in address must be 1, 2, 4 or 8
     lea rax, [rdi + rdx*32]
 // CHECK: error: scale factor in address must be 1, 2, 4 or 8
     lea rax, [rdi + rdx*16]
+// CHECK: error: Scale can't be negative
+    lea rax, [rdi + rdx*-8]
+// CHECK: error: scale factor in address must be 1, 2, 4 or 8
+    lea rax, [rdi + -1*rdx]

llvm/trunk/test/MC/X86/intel-syntax.s

 // RUN: llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel %s > %t 2> %t.err
 // RUN: FileCheck < %t %s
 // RUN: FileCheck --check-prefix=CHECK-STDERR < %t.err %s
 
 _test:
   xor EAX, EAX
   ret
 
-_main:
+.set  number, 8
+.global _foo
+
+.text
+  .global main
+main:
+
+// CHECK: leaq    _foo(%rbx,%rax,8), %rdx
+  lea RDX, [8 * RAX + RBX      + _foo]
+
+// CHECK: leaq _foo(%rbx,%rax,8), %rdx
+  lea RDX, [_foo + 8 * RAX + RBX]
+
+// CHECK: leaq 8(%rcx,%rax,8), %rdx
+  lea RDX, [8 + RAX * 8 + RCX]
+
+// CHECK: leaq 8(%rcx,%rax,8), %rdx
+  lea RDX, [number + 8 * RAX + RCX]
+
+// CHECK: leaq _foo(,%rax,8), %rdx
+  lea RDX, [_foo + RAX * 8]
+
+// CHECK:  leaq _foo(%rbx,%rax,8), %rdx
+  lea RDX, [_foo + RAX * 8 + RBX]
+
+// CHECK: leaq 8(%rax), %rdx
+  lea RDX, [RAX - number]
+// CHECK: leaq -8(%rax), %rdx
+  lea RDX, [RAX - 8]
+
+// CHECK: leaq    _foo(%rax), %rdx
+  lea RDX, [RAX + _foo]
+// CHECK: leaq    8(%rax), %rdx
+  lea RDX, [RAX + number]
+// CHECK: leaq    8(%rax), %rdx
+  lea RDX, [RAX + 8]
+
+// CHECK: leaq    _foo(%rax), %rdx
+  lea RDX, [_foo + RAX]
+// CHECK: leaq    8(%rax), %rdx
+  lea RDX, [number + RAX]
+// CHECK: leaq    8(%rax), %rdx
+  lea RDX, [8 + RAX]
+
 // CHECK: movl  $257, -4(%rsp)
   mov DWORD PTR [RSP - 4], 257
 // CHECK: movl  $258, 4(%rsp)
   mov DWORD PTR [RSP + 4], 258
 // CHECK: movq  $123, -16(%rsp)
   mov QWORD PTR [RSP - 16], 123
 // CHECK: movb  $97, -17(%rsp)
   mov BYTE PTR [RSP - 17], 97