-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Error and ICE reporting tweaks #15907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
51640f6
to
c860c2d
Compare
c860c2d
to
064d44d
Compare
util::AnsiColorized scope( | ||
util::AnsiColorized formattedStream( | ||
_stream, | ||
_formatted, | ||
{BOLD, SourceReferenceFormatter::errorTextColor(Error::errorSeverity(error.type))} | ||
); | ||
_stream << _linePrefix << Error::formatErrorType(error.type); | ||
formattedStream << _linePrefix << Error::formatErrorType(error.type); | ||
if (error.errorId.has_value()) | ||
_stream << ' ' << error.errorId->error; | ||
_stream << ": "; | ||
formattedStream << ' ' << error.errorId->error; | ||
formattedStream << ": "; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one does not actually fix anything but I decided to change it to be less of a head-scratcher.
We usually output to the ANSI stream using <<
, but here it's just created and we ignore it. I initially thought it was a bug and that coloring must be broken because of it, but it works. Apparently the control codes are issued at construction and destruction time.
@@ -36,7 +36,7 @@ namespace solidity::yul | |||
struct YulException: virtual util::Exception {}; | |||
struct OptimizerException: virtual YulException {}; | |||
struct CodegenException: virtual YulException {}; | |||
struct YulAssertion: virtual YulException {}; | |||
struct YulAssertion: virtual util::Exception {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should simplify this one day and make all assert macros throw InternalCompilerError
. Having different types and even documenting them in Error types just unnecessarily exposes implementation details to the user. We should only have compilation errors there. It's especially confusing when things like SMTLogicError
are actually failed assertions.
"formattedMessage": "DeclarationError: Declaration \"A\" not found in \"\" (referenced as \".\"). | ||
--> :2:24: | ||
| | ||
2 | pragma solidity >=0.0; import {A} from \".\"; | ||
| ^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this affects also the errors reported by the assert in CompilerStack::loadGeneratedIR()
.
Instead of this:
Invalid IR generated:
Error (6913): Call or assignment expected.
Error (8143): Expected keyword "data" or "object" or "}".
we'll now see this:
Invalid IR generated:
Error (6913): Call or assignment expected.
--> :7:9:
|
7 | mstore(64, memoryguard(128))
| ^^^^^^
Error (8143): Expected keyword "data" or "object" or "}".
--> :7:9:
|
7 | mstore(64, memoryguard(128))
| ^^^^^^
Another thing it affects are Yul test case variants that use the empty name for the source they parse and don't allow errors in expectations. The formatted errors are instead included in the exception they throw.
064d44d
to
f751dd3
Compare
Error: Cannot swap Variable value23 with Slot 0x20: too deep in the stack by 9 slots in [ RET value23 value22 value21 value20 value19 value18 value17 value16 value15 value14 value13 value12 value11 value10 value9 value8 value7 value6 value5 value4 value3 value2 value1 value0 pos 0x20 ] | ||
memoryguard was present. | ||
--> stack_too_deep_from_code_transform/input.sol:1:1: | ||
| | ||
1 | contract C { | ||
| ^ (Relevant source part starts here and spans across multiple lines). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what it looked like before the change:
Uncaught exception:
/solidity/libyul/backends/evm/EVMObjectCompiler.cpp(110): Throw in function void solidity::yul::EVMObjectCompiler::run(const solidity::yul::Object&, bool)
Dynamic exception type:
std::exception::what: Cannot swap Variable value23 with Slot 0x20: too deep in the stack by 9 slots in [ RET value23 value22 value21 value20 value19 value18 value17 value16 value15 value14 value13 value12 value11 value10 value9 value8 value7 value6 value5 value4 value3 value2 value1 value0 pos 0x20 ]
memoryguard was present.
[solidity::util::tag_comment*] = Cannot swap Variable value23 with Slot 0x20: too deep in the stack by 9 slots in [ RET value23 value22 value21 value20 value19 value18 value17 value16 value15 value14 value13 value12 value11 value10 value9 value8 value7 value6 value5 value4 value3 value2 value1 value0 pos 0x20 ]
memoryguard was present.
[solidity::langutil::tag_sourceLocation*] = stack_too_deep_from_code_transform/input.sol[0,206]
f751dd3
to
3580a7a
Compare
…he input file is an empty string
…en analysis fails
3580a7a
to
c422fa5
Compare
This is an assortment of small improvements in error reporting:
AsmAnalyzer::analyzeStrictAssertCorrect()
now includes pretty printed source and the error list in the assertion message. This will make it easier to triage cases like #15889 and is the most important change here.SourceReferenceFormatter
was not printing source locations for files with an empty name. This is a perfectly valid case in Standard JSON. We also use such names for internally generated Yul and in test suites.langutil
StackTooDeep errors; WTF) are now handled properly in Solidity mode: the former as ICE, the latter as a normal error.