Preserve backtraces across secondary error events #1349
+301
−16
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1266
What
This attempts to make the backtraces printed by the SDK while debugging unit tests more useful by presenting the backtrace from the first error raised during a contract call instead of a backtrace from a re-raised error.
The main change here is to add a
Host::secondary_error
function, and in several places that previously calledHost::error
, callHost::secondary_error
instead. WhereHost::error
receives aError
, collects diagnostics, and returns aHostError
,Host::secondary_error
receives aHostError
and returns aHostError
, collecting new diagnostics, but propagating the backtrace from the input error to the output error.To make this useful, to handle errors in test contracts
TestContractFrame
stores an entireHostError
, with backtrace, instead of aError
;call_n_internal
then reuses that error to callsecondary_error
and re-raise the original error.The VM dispatch macros also use
secondary_error
to preserve the original backtrace while also generating new diagnostic events.In addition to the unit tests I have manually tested this with the SDK.
Why
Background in #1266
In most of my experiences debugging contract unit tests, fuzz tests in particular, I have found the backtraces produced to not be useful because they are produced too late, having previously thrown away the backtrace from the originating error. As a result I have tended to make ad-hoc customizations to the soroban-env-host crate, like printlns in
Host::error
to figure out where the original error happened.Known limitations
Note the ugly impl of Hash for TestContractFrame. This type now contains a Backtrace (and diagnostic events) in the testutils config. This could be hashed, but it would be expensive and system-dependent. From looking at how this hash is used in the
trace
module, I am suspecting that hashing this backtrace isn't desirable nor needed for correctness.This has two test cases: one for wasm contracts and one for test contracts. These are brittle for two reasons:
This code introduces several expensive clones, but only in testutils configuration with diagnostics enabled.
It would be possible to preserve more backtraces if
HostError
was modified to contain a stack of them, but I don't think that would be useful enough to make it worthwile.