Fix LexFloatLiteral Lexing

[BrandonTJones]

Summary

Fix LexFloatLiteral Lexing to enforce the correct format before returning AsmToken::Real. It now reports an error on a wider range of invalid inputs. See test update for details.

[BrandonTJones]

I fixed the -U modifier on my diff as my diff was incorrect

[clayborg]

I don't think I am the right person to review this?

[BrandonTJones]

In D57321#1373767, @clayborg wrote:

I don't think I am the right person to review this?

This is correct, my apologies

[efriedma]

llvm/lib/MC/MCParser/AsmLexer.cpp

69

Spelling

76

The comment here seems to indicate the current behavior of LexFloatLiteral is intentional, and the caller (e.g. AsmParser::parseRealValue or AsmParser::parsePrimaryExpr) should handle ill-formed floats with a more specific error message. Do you think that design is wrong?

If you do think that design is wrong, please update the comments to describe what you think should happen instead.

[BrandonTJones]

I have fixed a spelling error and updated a comment to better reflect the change made.

[efriedma]

llvm/lib/MC/MCParser/AsmLexer.cpp

64

Probably should also fix this comment.

152

'e' and 'E' are identifier characters, so some of the checks here are redundant.

153

*CurPtr == 'e' && *CurPtr == 'E' is impossible.

We clearly need more test coverage given this issue wasn't caught by tests.

338

It's hard to follow this logic; when it's tangled together like this; does this accept 1.+1?

Need more test coverage to catch cases like this.

343

If we conclude the suffix doesn't qualify as a float, we apparently treat it the suffix as an identifier; is that right? Are the resulting diagnostics really going to be understandable? (I guess "unexpected token in '.double' directive" is okay, although not great.)

Should we worry about binutils compatibility at all? It apparently treats 1.e as equivalent to 1.e0.

[BrandonTJones]

llvm/lib/MC/MCParser/AsmLexer.cpp

343

I think the diagnostics should be okay.

For binutils compat, does it treat no exponent as "0" always, or only in the case of <digits>.e.
There seem to be tests in place that expect the program to die in response to these cases instead of handling them.

[BrandonTJones]

For binutils compat, does it treat no exponent as "0" always, or only in the case of <digits>.e.
There seem to be tests in place that expect the program to die in response to these cases instead of handling them.

[BrandonTJones]

Refined tests. Added binutils compat.

[efriedma]

llvm/include/llvm/MC/MCParser/AsmLexer.h

65

Spelling ("separated").

llvm/lib/MC/MCParser/AsmLexer.cpp

74

Is this early return necessary, or just to try to improve the error messages?

341

Instead of adding a boolean parameter to LexFloatLiteral, can we make the "++CurPtr" conditional? It's easier to follow the logic if CurPtr is always before the "E" when LexFloatLiteral is called.

[BrandonTJones]

Removed boolean param

[BrandonTJones]

llvm/lib/MC/MCParser/AsmLexer.cpp

74

Improve the error message, it feels ambiguous to allow the error message to be from the parser not expecting 2 floats in a row.

[efriedma]

So I guess overall, there are three fixes here:

1. Make AsmLexer::LexDigit handle floats without a decimal point more consistently.
2. Make AsmLexer::LexFloatLiteral print an error for floats which are apparently missing an "e".
3. Make APFloat::convertFromString use binutils-compatible exponent parsing.

Is that right?

llvm/lib/MC/MCParser/AsmLexer.cpp

82

Maybe update this comment?

151

This change doesn't do anything?

llvm/test/MC/AsmParser/floating-literals.s

62

We should probably have testcases for 1E1, 1e1e1, and 1e-1, since those don't work correctly without this patch.

llvm/unittests/ADT/APFloatTest.cpp

1196 ↗

(On Diff #187776)

We should probably keep these testcases, just change them to check for the new behavior (using ASSERT_EQ).

[BrandonTJones]

Added more complete test coverage.

[BrandonTJones]

llvm/lib/MC/MCParser/AsmLexer.cpp

151

This change arised from a merge conflict in my local repo, the new diff is more in keeping with the order from before the patch originally which is why I have kept it

[BrandonTJones]

ping

[efriedma]

llvm/test/MC/AsmParser/floating-literals.s

62

In this context, 1E1 is different from 1e1...

Probably best to check all of these with lowercase and uppercase "E".

[efriedma]

llvm/test/MC/AsmParser/floating-literals.s

62

Err, just realized my comment "1E1 is different from 1e1" might be unclear. They *should* be treated the same way, but LLVM without this patch treats them differently, so we should have test coverage.

[BrandonTJones]

Added a new test

[BrandonTJones]

Just to be clear, should I add a capital 'E' version of all the new tests I have added with this patch, or just the one mentioned?

[efriedma]

It would be better to add a capital E version of all the relevant tests in the file, I think.

[BrandonTJones]

Added test cases for 'E' cases.

[efriedma]

LGTM. Thanks for sticking with this for so many rounds of review.

[BrandonTJones]

Not a problem at all. Feel free to commit this on my behalf as I do not have permissions. Thanks!

llvm/include/llvm/MC/MCParser/AsmLexer.h

private:
   AsmToken ReturnError(const char *Loc, const std::string &Msg);
 
   AsmToken LexIdentifier();
   AsmToken LexSlash();
   AsmToken LexLineComment();
   AsmToken LexDigit();
   AsmToken LexSingleQuote();
   AsmToken LexQuote();
-  AsmToken LexFloatLiteral();
+  AsmToken LexFloatLiteral(bool isDotSeperated);
   AsmToken LexHexFloatLiteral(bool NoIntDigits);
 
   StringRef LexUntilEndOfLine();
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCPARSER_ASMLEXER_H

llvm/lib/MC/MCParser/AsmLexer.cpp

 }
 
 int AsmLexer::getNextChar() {
   if (CurPtr == CurBuf.end())
     return EOF;
   return (unsigned char)*CurPtr++;
 }
 
 /// LexFloatLiteral: [0-9]*[.][0-9]*([eE][+-]?[0-9]*)?
 ///
 /// The leading integral digit sequence and dot should have already been
 /// consumed, some or all of the fractional digit sequence *can* have been
 /// consumed.
-AsmToken AsmLexer::LexFloatLiteral() {
+AsmToken AsmLexer::LexFloatLiteral(bool isDotSeperated) {
   // Skip the fractional digit sequence.
   while (isDigit(*CurPtr))
     ++CurPtr;
 
   // Check for exponent; we intentionally accept a slighlty wider set of
   // literals here and rely on the upstream client to reject invalid ones (e.g.,
   // "1e+").
-  if (*CurPtr == 'e' || *CurPtr == 'E') {
+  if (isDotSeperated && (*CurPtr == 'e' || *CurPtr == 'E') &&
+      (isDigit(CurPtr[1]) ||
+       ((CurPtr[1] == '-' || CurPtr[1] == '+') && isDigit(CurPtr[2])))) {
     ++CurPtr;
-    if (*CurPtr == '-' || *CurPtr == '+')
+    if ((*CurPtr == '-' || *CurPtr == '+') && isDigit(CurPtr[1]))
       ++CurPtr;
     while (isDigit(*CurPtr))
       ++CurPtr;
   }
 
   return AsmToken(AsmToken::Real,
                   StringRef(TokStart, CurPtr - TokStart));
 }
 

 }
 
 AsmToken AsmLexer::LexIdentifier() {
   // Check for floating point literals.
   if (CurPtr[-1] == '.' && isDigit(*CurPtr)) {
     // Disambiguate a .1243foo identifier from a floating literal.
     while (isDigit(*CurPtr))
       ++CurPtr;
-    if (*CurPtr == 'e' || *CurPtr == 'E' ||
-        !IsIdentifierChar(*CurPtr, AllowAtInIdentifier))
-      return LexFloatLiteral();
+    if ((!IsIdentifierChar(*CurPtr, AllowAtInIdentifier) &&
+         (*CurPtr != 'e' && *CurPtr != 'E')) ||
+        (!IsIdentifierChar(*CurPtr, AllowAtInIdentifier) &&
+         (*CurPtr == 'e' && *CurPtr == 'E') &&
+         (isDigit(CurPtr[1]) ||
+          ((CurPtr[1] == '-' || CurPtr[1] == '+') && isDigit(CurPtr[2])))))
+      return LexFloatLiteral(false);
   }
 
   while (IsIdentifierChar(*CurPtr, AllowAtInIdentifier))
     ++CurPtr;
 
   // Handle . as a special case.
   if (CurPtr == TokStart+1 && TokStart[0] == '.')
     return AsmToken(AsmToken::Dot, StringRef(TokStart, 1));
if (LexMasmIntegers && isdigit(CurPtr[-1])) {
     CurPtr = OldCurPtr;
   }
 
   // Decimal integer: [1-9][0-9]*
   if (CurPtr[-1] != '0' || CurPtr[0] == '.') {
     unsigned Radix = doHexLookAhead(CurPtr, 10, LexMasmIntegers);
     bool isHex = Radix == 16;
     // Check for floating point literals.
-    if (!isHex && (*CurPtr == '.' || *CurPtr == 'e')) {
-      ++CurPtr;
-      return LexFloatLiteral();
+    if ((!isHex && (*CurPtr == '.' || *CurPtr == 'e')) &&
+        (isDigit(CurPtr[1]) || ((CurPtr[1] == '-' || CurPtr[1] == '+' ||
+                                 (*CurPtr == '.' && CurPtr[1] == 'e')) &&
+                                isDigit(CurPtr[2])))) {
+      ++CurPtr;
+      if (CurPtr[-1] == '.')
+        return LexFloatLiteral(true);
+      return LexFloatLiteral(false);
     }
 
     StringRef Result(TokStart, CurPtr - TokStart);
 
     APInt Value(128, 0, true);
     if (Result.getAsInteger(Radix, Value))
       return ReturnError(TokStart, !isHex ? "invalid decimal number" :
                            "invalid hexdecimal number");
 

llvm/test/MC/AsmParser/floating-literals.s

 # CHECK: .quad  4681608360884174848
 .double 1e5
 # CHECK: .quad  4681608360884174848
 .double 1.e5
 # CHECK: .quad  4611686018427387904
 .double 2.
 
 // APFloat should reject these with an error, not crash:
-//.double -1.2e+
-//.double -1.2e
+
+#CHECK-ERROR: unexpected token in '.double' directive
+.double -1.2e+
+#CHECK-ERROR: unexpected token in '.double' directive
+.double -1.2e
 
 # CHECK: .long 1310177520
 .float 0x12f7.1ep+17
 # CHECK: .long 1084227584
 .float 0x.ap+3
 # CHECK: .quad 4602678819172646912
 .double 0x2.p-2
 # CHECK: .long 1094713344
 .float 0x3p2
 # CHECK: .long 872284160
 .float 0x7fp-30
 # CHECK: .long 3212836864
 .float -0x1.0p0