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

67–68

Spelling

77–78

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

63–64

Probably should also fix this comment.

153

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

154

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

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

337

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.

339

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

339

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

73

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

337

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

73

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

83

Maybe update this comment?

152

This change doesn't do anything?

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

60

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

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

152

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

60

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

60

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

AsmToken AsmLexer::ReturnError(const char *Loc, const std::string &Msg) {
   return AsmToken(AsmToken::Error, StringRef(Loc, CurPtr - Loc));
 }
 
 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
+
+/// The leading integral digit sequence and dot/e should have already been
 /// consumed, some or all of the fractional digit sequence *can* have been
 /// consumed.
-AsmToken AsmLexer::LexFloatLiteral() {
-  // Skip the fractional digit sequence.
+AsmToken AsmLexer::LexFloatLiteral(bool isDotSeparated) {
+  // Skip the digit sequence.
   while (isDigit(*CurPtr))
     ++CurPtr;
+  if (*CurPtr == '-' || *CurPtr == '+')
+    return ReturnError(CurPtr, "Invalid sign in float literal");
+  // Check for exponent; we enforce rigidity on a correct
+  // literal as the upstream client cannot parse all invalid
+  // forms and crashes. (e.g. 1e8e8)
 
-  // 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 (isDotSeparated && (*CurPtr == 'e' || *CurPtr == 'E')){
     ++CurPtr;
+
     if (*CurPtr == '-' || *CurPtr == '+')
       ++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') &&
+         (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')) {
+    if (!isHex && (*CurPtr == '.' || *CurPtr == 'e' || *CurPtr == 'E')) {
       ++CurPtr;
-      return LexFloatLiteral();
+      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/lib/Support/APFloat.cpp

 static int
 readExponent(StringRef::iterator begin, StringRef::iterator end)
 {
   bool isNegative;
   unsigned int absExponent;
   const unsigned int overlargeExponent = 24000;  /* FIXME.  */
   StringRef::iterator p = begin;
 
-  assert(p != end && "Exponent has no digits");
+  // Treat no exponent as 0 to match binutils
+  if (p == end || ((*p == '-' || *p == '+') && (p + 1) == end)) {
+    return 0;
+  }
 
   isNegative = (*p == '-');
   if (*p == '-' || *p == '+') {
     p++;
     assert(p != end && "Exponent has no digits");
   }
 
   absExponent = decDigitValue(*p++);

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

 # CHECK: .quad  -4465782973978902528
 .double -1.2e+10
 # 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: .quad  4611686018427387904
+.double 2.e
+# CHECK: .quad  4611686018427387904
+.double 2.e+
+# CHECK: .quad  4611686018427387904
+.double 2.e-
+# CHECK: .quad  -4615288898129284301
+.double -1.2e
+# CHECK-ERROR: Invalid sign in float literal
+.double 2.+1
 
 # CHECK: .long 1310177520
 .float 0x12f7.1ep+17
 # CHECK: .long 1084227584
 .float 0x.ap+3
 # CHECK: .quad 4602678819172646912
 .double 0x2.p-2
 # CHECK: .long 1094713344

llvm/unittests/ADT/APFloatTest.cpp

TEST(APFloatTest, StringDecimalSignificandDeath) {
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "-.e1"), "Significand has no digits");
 
 
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  ".e"), "Significand has no digits");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "+.e"), "Significand has no digits");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "-.e"), "Significand has no digits");
 }
 
-TEST(APFloatTest, StringDecimalExponentDeath) {
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),   "1e"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "+1e"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "-1e"), "Exponent has no digits");
-
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),   "1.e"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "+1.e"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "-1.e"), "Exponent has no digits");
-
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),   ".1e"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "+.1e"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "-.1e"), "Exponent has no digits");
-
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),   "1.1e"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "+1.1e"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "-1.1e"), "Exponent has no digits");
-
-
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "1e+"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "1e-"), "Exponent has no digits");
-
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  ".1e"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), ".1e+"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), ".1e-"), "Exponent has no digits");
-
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "1.0e"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "1.0e+"), "Exponent has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "1.0e-"), "Exponent has no digits");
-}
-
 TEST(APFloatTest, StringHexadecimalDeath) {
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "0x"), "Invalid string");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "+0x"), "Invalid string");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "-0x"), "Invalid string");
 
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "0x0"), "Hex strings require an exponent");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "+0x0"), "Hex strings require an exponent");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "-0x0"), "Hex strings require an exponent");