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 ↗

(On Diff #183829)

Spelling

76 ↗

(On Diff #183829)

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 ↗

(On Diff #185029)

Probably should also fix this comment.

152 ↗

(On Diff #185029)

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

153 ↗

(On Diff #185029)

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

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

338 ↗

(On Diff #185029)

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 ↗

(On Diff #185029)

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 ↗

(On Diff #185029)

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 ↗

(On Diff #187199)

Spelling ("separated").

llvm/lib/MC/MCParser/AsmLexer.cpp

73 ↗

(On Diff #187199)

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

337 ↗

(On Diff #187199)

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 ↗

(On Diff #187199)

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

77 ↗

(On Diff #187776)

Maybe update this comment?

153 ↗

(On Diff #187776)

This change doesn't do anything?

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

60 ↗

(On Diff #187776)

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

153 ↗

(On Diff #187776)

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 ↗

(On Diff #187776)

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 ↗

(On Diff #187776)

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/trunk/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() {
   // 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 (*CurPtr == '-' || *CurPtr == '+')
+    return ReturnError(CurPtr, "Invalid sign in float literal");
+
+  // Check for exponent
+  if ((*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))
+
+    if (!IsIdentifierChar(*CurPtr, AllowAtInIdentifier) ||
+        *CurPtr == 'e' || *CurPtr == 'E')
       return LexFloatLiteral();
   }
 
   while (IsIdentifierChar(*CurPtr, AllowAtInIdentifier))
     ++CurPtr;
 
   // Handle . as a special case.
   if (CurPtr == TokStart+1 && TokStart[0] == '.')
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')) {
+      if (*CurPtr == '.')
         ++CurPtr;
       return LexFloatLiteral();
     }
 
     StringRef Result(TokStart, CurPtr - TokStart);
 
     APInt Value(128, 0, true);
     if (Result.getAsInteger(Radix, Value))
       return ReturnError(TokStart, !isHex ? "invalid decimal number" :

llvm/trunk/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/trunk/test/MC/AsmParser/floating-literals.s

 # CHECK: .long  2147483647
 .single nAN
 
 # CHECK: .quad  9223372036854775807
 .double NaN
 
 # CHECK: .long  1067928519
 .float 1.307
-        
 # CHECK: .quad  4617315517961601024
 # CHECK: .quad  4597526701198935065
 # CHECK: .quad  -4600933674317040845
 .double 5, .232, -11.1
 
 # CHECK: .quad  0
 .double 0.0
 
 # CHECK: .quad  -4570379565595099136
 .double -1.2e3
 # CHECK: .quad  -4690170861623122860
 .double -1.2e-5
 # CHECK: .quad  -4465782973978902528
 .double -1.2e+10
 # CHECK: .quad  4681608360884174848
 .double 1e5
 # CHECK: .quad  4681608360884174848
 .double 1.e5
 # CHECK: .quad  4611686018427387904
 .double 2.
+# 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: .quad  4621819117588971520
+.double 1e1
+# CHECK: .quad  4591870180066957722
+.double 1e-1
 
-// APFloat should reject these with an error, not crash:
-//.double -1.2e+
-//.double -1.2e
+
+# CHECK: .quad  -4570379565595099136
+.double -1.2E3
+# CHECK: .quad  -4690170861623122860
+.double -1.2E-5
+# CHECK: .quad  -4465782973978902528
+.double -1.2E+10
+# CHECK: .quad  4681608360884174848
+.double 1E5
+# CHECK: .quad  4681608360884174848
+.double 1.E5
+# 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: .quad  4621819117588971520
+.double 1E1
+# CHECK: .quad  4591870180066957722
+.double 1E-1
+
+
+#CHECK-ERROR: unexpected token in '.double' directive
+.double 1e1e
+# 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

 # CHECK-ERROR: invalid hexadecimal floating-point constant: expected at least one significand digit
 .float 0xp2
 
 # CHECK-ERROR: invalid hexadecimal floating-point constant: expected at least one significand digit
 .float 0x.p5
 
 # CHECK-ERROR: error: invalid hexadecimal floating-point constant: expected exponent part 'p'
 .float 0x1.2
+

llvm/trunk/unittests/ADT/APFloatTest.cpp

TEST(APFloatTest, fromDecimalString) {
   EXPECT_EQ(2.0e-2,   APFloat(APFloat::IEEEdouble(), "2.e-2").convertToDouble());
   EXPECT_EQ(2.05e2,    APFloat(APFloat::IEEEdouble(), "002.05000e2").convertToDouble());
   EXPECT_EQ(2.05e+2,   APFloat(APFloat::IEEEdouble(), "002.05000e+2").convertToDouble());
   EXPECT_EQ(2.05e-2,   APFloat(APFloat::IEEEdouble(), "002.05000e-2").convertToDouble());
   EXPECT_EQ(2.05e12,   APFloat(APFloat::IEEEdouble(), "002.05000e12").convertToDouble());
   EXPECT_EQ(2.05e+12,  APFloat(APFloat::IEEEdouble(), "002.05000e+12").convertToDouble());
   EXPECT_EQ(2.05e-12,  APFloat(APFloat::IEEEdouble(), "002.05000e-12").convertToDouble());
 
+  EXPECT_EQ(1.0,      APFloat(APFloat::IEEEdouble(), "1e").convertToDouble());
+  EXPECT_EQ(1.0,      APFloat(APFloat::IEEEdouble(), "+1e").convertToDouble());
+  EXPECT_EQ(-1.0,      APFloat(APFloat::IEEEdouble(), "-1e").convertToDouble());
+
+  EXPECT_EQ(1.0,      APFloat(APFloat::IEEEdouble(), "1.e").convertToDouble());
+  EXPECT_EQ(1.0,      APFloat(APFloat::IEEEdouble(), "+1.e").convertToDouble());
+  EXPECT_EQ(-1.0,      APFloat(APFloat::IEEEdouble(), "-1.e").convertToDouble());
+
+  EXPECT_EQ(0.1,      APFloat(APFloat::IEEEdouble(), ".1e").convertToDouble());
+  EXPECT_EQ(0.1,      APFloat(APFloat::IEEEdouble(), "+.1e").convertToDouble());
+  EXPECT_EQ(-0.1,      APFloat(APFloat::IEEEdouble(), "-.1e").convertToDouble());
+
+  EXPECT_EQ(1.1,      APFloat(APFloat::IEEEdouble(), "1.1e").convertToDouble());
+  EXPECT_EQ(1.1,      APFloat(APFloat::IEEEdouble(), "+1.1e").convertToDouble());
+  EXPECT_EQ(-1.1,      APFloat(APFloat::IEEEdouble(), "-1.1e").convertToDouble());
+
+  EXPECT_EQ(1.0,      APFloat(APFloat::IEEEdouble(), "1e+").convertToDouble());
+  EXPECT_EQ(1.0,      APFloat(APFloat::IEEEdouble(), "1e-").convertToDouble());
+
+  EXPECT_EQ(0.1,      APFloat(APFloat::IEEEdouble(), ".1e").convertToDouble());
+  EXPECT_EQ(0.1,      APFloat(APFloat::IEEEdouble(), ".1e+").convertToDouble());
+  EXPECT_EQ(0.1,      APFloat(APFloat::IEEEdouble(), ".1e-").convertToDouble());
+
+  EXPECT_EQ(1.0,      APFloat(APFloat::IEEEdouble(), "1.0e").convertToDouble());
+  EXPECT_EQ(1.0,      APFloat(APFloat::IEEEdouble(), "1.0e+").convertToDouble());
+  EXPECT_EQ(1.0,      APFloat(APFloat::IEEEdouble(), "1.0e-").convertToDouble());
+
   // These are "carefully selected" to overflow the fast log-base
   // calculations in APFloat.cpp
   EXPECT_TRUE(APFloat(APFloat::IEEEdouble(), "99e99999").isInfinity());
   EXPECT_TRUE(APFloat(APFloat::IEEEdouble(), "-99e99999").isInfinity());
   EXPECT_TRUE(APFloat(APFloat::IEEEdouble(), "1e-99999").isPosZero());
   EXPECT_TRUE(APFloat(APFloat::IEEEdouble(), "-1e-99999").isNegZero());
 
   EXPECT_EQ(2.71828, convertToDoubleFromString("2.71828"));
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");