X86AsmParser.cpp asserts: OperandStack.size() > 1 && "Too few operands."

[avt77]

Summary

This patch should close PR22004.

[RKSimon]

@probinson Any comments?

lib/Target/X86/AsmParser/X86AsmParser.cpp

1321

Update the comment

[avt77]

I simply commented the check becuase it hides the case of identificator. There are no regression failed tests that's why I hope it's acceptable.

mcrosier, it seems you are "blame" in this code. Could you review it?

[mcrosier]

I'm happy to help, but it's been many years since I looked at this code. Can you be more verbose in your description of the problem as well as describe how this patch addresses the issue?

[avt77]

The initial problem was described in PR22004. The problem raises because this check does not allow using of identifiers starting from dot. But such IDs are legal for asm, e.g as local labels. As result we have (in this exactly case) the binary expression without RHS operand. It means "too few operands" like assertion says. If I comment out the check then everything works without any problems including all regression tests. Could I remove the check from the code?

[RKSimon]

@mcrosier Any comments?

lib/Target/X86/AsmParser/X86AsmParser.cpp

1319

Did this actually need moving?

1325

Don't comment out code - delete it.

[avt77]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1319

It depends on decision about the check below: if we can remove the check then we should not move that but if we have to keep the modified version of the check then we have to move that.

1325

I'm waiting for the answer from @mcrosier: if we don't need this check at all then I'll remove the commented code and return the local variable initialization above to its origin place.

@mcrosier, could you comment this patch now?

[mcrosier]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1325

Can you think of an example where Tok starts with '.' and is *not* an identifier?

In other words, I'm thinking your change is causing this condition to be unconditionally false. If that's the case, then the right solution might be to remove this check entirely.

Unfortunately, we don't have a lot of lit tests that cover the dot operator. Have you tested this chance on something more substantial than the llvm test suite?

[avt77]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1325

No, I tested it as "check-all" only. Could you suggest something suitable?
And of course I'm ready to keep the check like it's written inside the commented code. I don't know the example of such token but it was written for some reason that's why I suggest to use the commented version. What do you think about?

[mcrosier]

Added @rnk as a reviewer as he's largely taken over the MS style inline assembly, IIRC.

@rnk: Any suggestions beyond the LIT tests for testing? Any opinions on this patch?

[rnk]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1325

Make sure check-clang passes, it has the interesting checks. If you already have clang checked out and configured, that would've been included in check-all. These test cases have this corner case:

../clang/test/CodeGen/ms-inline-asm-64.c:       mov eax, [ebx].foo.a
../clang/test/CodeGen/ms-inline-asm-64.c:     mov [ebx].foo.b, ecx

[avt77]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1325

Yes, I always have configured clang in my repository and I always use "check-all" which includes "check-clang". It passes. And your test is included and passes.

[mcrosier]

LGTM, but please remove the commented out code.

[avt77]

I restored the condition like here:

if (PrevTK != AsmToken::Error && Tok.getString().startswith(".") &&
    TK != AsmToken::Identifier)

I merged with trunk (both LLVM and Clang) and launched "check-all": there are no failed tests.
Reid, Chad - could you give me LGTM?

[rnk]

Sure, lgtm.

[mcrosier]

LGTM.

[avt77]

The revision was committed.

lib/Target/X86/AsmParser/X86AsmParser.cpp

bool X86AsmParser::ParseIntelExpression(IntelExprStateMachine &SM, SMLoc &End) {
   MCAsmParser &Parser = getParser();
   const AsmToken &Tok = Parser.getTok();
 
   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
     // identifier.  Don't try an parse it as a register.
-    if (PrevTK != AsmToken::Error && Tok.getString().startswith("."))
+    if (PrevTK != AsmToken::Error && Tok.getString().startswith(".") &&
+        TK != AsmToken::Identifier)
       break;
 
     // If we're parsing an immediate expression, we don't expect a '['.
     if (SM.getStopOnLBrac() && getLexer().getKind() == AsmToken::LBrac)
       break;
 
-    AsmToken::TokenKind TK = getLexer().getKind();
     switch (TK) {
     default: {
       if (SM.isValidEndState()) {
         Done = true;
         break;
       }
       return Error(Tok.getLoc(), "unknown token in expression");
     }

test/MC/X86/pr22004.s

+// RUN: llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel %s
+
+lea rax, qword ptr [rip + .L.str]