Skip to content

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

Merged
merged 7 commits into from
Mar 5, 2025
Merged

Conversation

cameel
Copy link
Member

@cameel cameel commented Mar 2, 2025

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.
  • Failed Yul assertions and Yul StackTooDeep errors (not to be confused with completely separate langutil StackTooDeep errors; WTF) are now handled properly in Solidity mode: the former as ICE, the latter as a normal error.
    • ICEs in Standard JSON now properly include all the same diagnostic info we provide on the CLI. Until now this was being done only for uncaught exceptions, not failed assertions.
    • StackTooDeep errors on the CLI now show a proper error with location rather than the hairy diagnostic info meant for ICEs. This fixes Stack too deep exception is not being caught #15740.
    • Note that this is still a mess in many ways. E.g. we have separate exception handlers for Yul and I did not touch those.
  • The assertion that's triggered by invalid IR coming out the generator now shows error codes (and source locations due to the other fix mentioned above).

Comment on lines -200 to 209
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 << ": ";
}
Copy link
Member Author

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 {};
Copy link
Member Author

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.

Comment on lines 6 to +10
"formattedMessage": "DeclarationError: Declaration \"A\" not found in \"\" (referenced as \".\").
--> :2:24:
|
2 | pragma solidity >=0.0; import {A} from \".\";
| ^^^^^^^^^^^^^^^^^^^^
Copy link
Member Author

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.

@cameel cameel force-pushed the error-and-ice-reporting-tweaks branch from 064d44d to f751dd3 Compare March 2, 2025 09:44
Comment on lines +1 to +6
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).
Copy link
Member Author

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]

@cameel cameel force-pushed the error-and-ice-reporting-tweaks branch from f751dd3 to 3580a7a Compare March 2, 2025 10:26
@cameel cameel force-pushed the error-and-ice-reporting-tweaks branch from 3580a7a to c422fa5 Compare March 5, 2025 04:19
@cameel cameel merged commit ddc9f0d into develop Mar 5, 2025
74 checks passed
@cameel cameel deleted the error-and-ice-reporting-tweaks branch March 5, 2025 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack too deep exception is not being caught
2 participants