Page MenuHomePhabricator

[APFloat] Add recoverable string parsing errors to APFloat
ClosedPublic

Authored by ekatz on Nov 3 2019, 12:13 PM.

Diff Detail

Event Timeline

ekatz created this revision.Nov 3 2019, 12:13 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 3 2019, 12:13 PM
arsenm added inline comments.Nov 3 2019, 6:54 PM
llvm/lib/Support/APFloat.cpp
270

Error message sounds like nonsense

llvm/unittests/ADT/APFloatTest.cpp
1322

It’s a gtestism that these operands should be swapped

ekatz added a reviewer: scanon.Nov 3 2019, 8:28 PM
ekatz marked 2 inline comments as done.Nov 5 2019, 10:28 AM
ekatz added inline comments.
llvm/lib/Support/APFloat.cpp
270

It is actually incorrect. There should not be an error returned, but absExponent should still be clamped.

llvm/unittests/ADT/APFloatTest.cpp
1322

Will do.

ekatz updated this revision to Diff 227917.Nov 5 2019, 10:31 AM

Fixed requested changes.

ekatz retitled this revision from Add recoverable string parsing errors to APFloat to [APFloat] Add recoverable string parsing errors to APFloat.Nov 24 2019, 11:10 AM
hfinkel accepted this revision.Dec 13 2019, 5:11 PM

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.

This revision is now accepted and ready to land.Dec 13 2019, 5:11 PM
This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.
llvm/lib/Support/StringRef.cpp
588

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]

smeenai added a subscriber: smeenai.Jan 8 2020, 2:37 PM
smeenai added inline comments.
llvm/lib/MC/MCParser/AsmParser.cpp
3141

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.

ekatz marked an inline comment as done.Jan 9 2020, 12:29 AM
ekatz added inline comments.
llvm/lib/MC/MCParser/AsmParser.cpp
3141

I have commited a fix for this issue in rG24b326cc610d