[PATCH] D69770: [APFloat] Add recoverable string parsing errors to APFloat

2020-01-09 Thread Ehud Katz via Phabricator via cfe-commits
ekatz marked an inline comment as done.
ekatz added inline comments.



Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:3133
   return TokError("invalid floating point literal");
-  } else if (Value.convertFromString(IDVal, APFloat::rmNearestTiesToEven) ==
- APFloat::opInvalidOp)
+  } else if (!Value.convertFromString(IDVal, APFloat::rmNearestTiesToEven))
 return TokError("invalid floating point literal");

smeenai wrote:
> I'm pretty certain this won't do what you want in an asserts build 
> (technically a build with `LLVM_ENABLE_ABI_BREAKING_CHECKS`). The destructor 
> of an `llvm::Expected` asserts that the Expected was checked, and evaluating 
> an Expected as a boolean only counts as checking it if there wasn't an error, 
> in the error case, you'll hit an assert failure instead of doing the return. 
> You'll need to capture the Expected and then do something like 
> `consumeError(expected.takeError())` to discard the error and avoid the 
> assertion.
I have commited a fix for this issue in rG24b326cc610d


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69770/new/

https://reviews.llvm.org/D69770



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D69770: [APFloat] Add recoverable string parsing errors to APFloat

2020-01-08 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.
Herald added a subscriber: herhut.



Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:3133
   return TokError("invalid floating point literal");
-  } else if (Value.convertFromString(IDVal, APFloat::rmNearestTiesToEven) ==
- APFloat::opInvalidOp)
+  } else if (!Value.convertFromString(IDVal, APFloat::rmNearestTiesToEven))
 return TokError("invalid floating point literal");

I'm pretty certain this won't do what you want in an asserts build (technically 
a build with `LLVM_ENABLE_ABI_BREAKING_CHECKS`). The destructor of an 
`llvm::Expected` asserts that the Expected was checked, and evaluating an 
Expected as a boolean only counts as checking it if there wasn't an error, in 
the error case, you'll hit an assert failure instead of doing the return. 
You'll need to capture the Expected and then do something like 
`consumeError(expected.takeError())` to discard the error and avoid the 
assertion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69770/new/

https://reviews.llvm.org/D69770



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D69770: [APFloat] Add recoverable string parsing errors to APFloat

2020-01-06 Thread Craig Topper via Phabricator via cfe-commits
craig.topper added inline comments.



Comment at: llvm/lib/Support/StringRef.cpp:593
+  if (!ErrOrStatus) {
+assert("Invalid floating point representation");
+return true;

This is an invalid assert.

lib/Support/StringRef.cpp:593:12: warning: implicit conversion turns string 
literal into bool: 'const char [38]' to 'bool' [-Wstring-conversion]


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69770/new/

https://reviews.llvm.org/D69770



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D69770: [APFloat] Add recoverable string parsing errors to APFloat

2020-01-06 Thread Ehud Katz via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc5fb73c5d1b3: [APFloat] Add recoverable string parsing 
errors to APFloat (authored by ekatz).

Changed prior to commit:
  https://reviews.llvm.org/D69770?vs=227917&id=236304#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69770/new/

https://reviews.llvm.org/D69770

Files:
  clang/lib/Lex/LiteralSupport.cpp
  llvm/include/llvm/ADT/APFloat.h
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/Support/APFloat.cpp
  llvm/lib/Support/StringRef.cpp
  llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
  llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
  llvm/unittests/ADT/APFloatTest.cpp

Index: llvm/unittests/ADT/APFloatTest.cpp
===
--- llvm/unittests/ADT/APFloatTest.cpp
+++ llvm/unittests/ADT/APFloatTest.cpp
@@ -10,8 +10,8 @@
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
-#include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 #include 
 #include 
@@ -20,9 +20,17 @@
 
 using namespace llvm;
 
-static double convertToDoubleFromString(const char *Str) {
+static std::string convertToErrorFromString(StringRef Str) {
   llvm::APFloat F(0.0);
-  F.convertFromString(Str, llvm::APFloat::rmNearestTiesToEven);
+  auto ErrOrStatus =
+  F.convertFromString(Str, llvm::APFloat::rmNearestTiesToEven);
+  EXPECT_TRUE(!ErrOrStatus);
+  return toString(ErrOrStatus.takeError());
+}
+
+static double convertToDoubleFromString(StringRef Str) {
+  llvm::APFloat F(0.0);
+  EXPECT_FALSE(!F.convertFromString(Str, llvm::APFloat::rmNearestTiesToEven));
   return F.convertToDouble();
 }
 
@@ -1156,172 +1164,172 @@
   EXPECT_DEATH(APFloat(APFloat::IEEEsingle(), 0).convertToDouble(), "Float semantics are not IEEEdouble");
   EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), 0).convertToFloat(),  "Float semantics are not IEEEsingle");
 }
+#endif
+#endif
 
-TEST(APFloatTest, StringDecimalDeath) {
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  ""), "Invalid string length");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "+"), "String has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "-"), "String has no digits");
+TEST(APFloatTest, StringDecimalError) {
+  EXPECT_EQ("Invalid string length", convertToErrorFromString(""));
+  EXPECT_EQ("String has no digits", convertToErrorFromString("+"));
+  EXPECT_EQ("String has no digits", convertToErrorFromString("-"));
 
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("\0", 1)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1\0", 2)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1" "\0" "2", 3)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1" "\0" "2e1", 5)), "Invalid character in significand");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1e\0", 3)), "Invalid character in exponent");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1e1\0", 4)), "Invalid character in exponent");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), StringRef("1e1" "\0" "2", 5)), "Invalid character in exponent");
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("\0", 1)));
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("1\0", 2)));
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("1" "\0" "2", 3)));
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString(StringRef("1" "\0" "2e1", 5)));
+  EXPECT_EQ("Invalid character in exponent", convertToErrorFromString(StringRef("1e\0", 3)));
+  EXPECT_EQ("Invalid character in exponent", convertToErrorFromString(StringRef("1e1\0", 4)));
+  EXPECT_EQ("Invalid character in exponent", convertToErrorFromString(StringRef("1e1" "\0" "2", 5)));
 
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "1.0f"), "Invalid character in significand");
+  EXPECT_EQ("Invalid character in significand", convertToErrorFromString("1.0f"));
 
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), ".."), "String contains multiple dots");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "..0"), "String contains multiple dots");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "1.0.0"), "String contains multiple dots");
+  EXPECT_EQ("String contains multiple dots", convertToErrorFromString(".."));
+  EXPECT_EQ("String contains multiple dots", convertToErrorFromString("..0"));
+  EXPECT_EQ("String contains multiple dots", convertToErrorFromString("1.0.0"));
 }
 
-TEST(APFloatTest, StringDecimalSignificandDeath) {
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(),  "."), "Significand has no digits");
-  EXPECT_DEATH(APFloat(APFloat::IEEEdouble(), "+."), "Significand has no digits");
-  EXPEC

[PATCH] D69770: [APFloat] Add recoverable string parsing errors to APFloat

2019-12-13 Thread Hal Finkel via Phabricator via cfe-commits
hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

If we really want to be confident that this is robust, we should probably 
fuzz-test it. Regardless, this seems like a definite improvement. LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69770/new/

https://reviews.llvm.org/D69770



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D69770: [APFloat] Add recoverable string parsing errors to APFloat

2019-12-11 Thread Ehud Katz via Phabricator via cfe-commits
ekatz added a comment.

Ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69770/new/

https://reviews.llvm.org/D69770



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D69770: [APFloat] Add recoverable string parsing errors to APFloat

2019-12-02 Thread Ehud Katz via Phabricator via cfe-commits
ekatz added a comment.

Ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69770/new/

https://reviews.llvm.org/D69770



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits