This repository has been archived by the owner on Feb 26, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Have debugger skip anything called from a generated source while stepping #6138
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gnidan
approved these changes
Jul 13, 2023
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.
Just a minor docs point. Looks good!
/** | ||
* stacktrace.current.callstack.preupdated | ||
*/ | ||
preupdated: createLeaf( |
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 probably warrants a note about what "preupdated" means
This was referenced Nov 27, 2023
This was referenced Mar 22, 2024
This was referenced Mar 25, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
PR description
@gnidan noticed a problem whereby a generated source could appear in a stacktrace even though generated sources were turned off. What's up with that?
Well, turns out that in this case, a function in a generated source called another function in that generated source, and due to optimization part of the latter function ended up sourcemapped to a user source! So when you stepped, you would hit that source range, and see that you were now inside a user source called from a generated source. Yikes!
The fix for this is that, if generated sources are turned off, you shouldn't hit such a source range in the first place, as logically it should really be considered to be within a generated source. So I changed it so that, while stepping, the debugger won't stop while there are generated sources anywhere on the callstack (unless you started in a generated source or generated sources are turned on).
Now, I wanted to restrict this new check specifically to generated sources. Normally in the debugger we consider a source to be "internal" if it's either generated, or unmapped, or we just don't know what it is. I was a little worried that this last case could cause some problems, though. And while ideally maybe we'd do the first two but not the third, the thing is that distinguishing the second from the third would require making some changes to the debugger's internals that I didn't want to bother with. (Although, now that I think about it, it actually wouldn't be that difficult, so I can go back and change that if you want.) So this new check only checks specifically for generated sources.
(The existing check regarding the current location -- the top stackframe -- is left in as-is, it's just for the stackframes below that we only check for generated sources.)
Anyway, some changes you'll notice I made:
isSourceGenerated
field to the callstack state instacktrace
.stacktrace.current.callstack.preupdated
. This gives the callstack as it actually currently is, rather than with error information still left on top, much like is used instacktrace.current.report
; that bit of code is now factored out into this new selector.stacktrace
.I think that basically covers everything important.
Testing instructions
I didn't add any automated tets for this PR, I just tested in manually.
This issue was uncovered in Sepolia transaction 0x84f82b79b516aa64f38797f204e09a49e821f7f9353723545dfd600e99b60955. Pressing
n
twice would land in the messed-up state described above, where pressings
would yield the bizarre callstack.Now, if you debug this transaction with
-x
and pressn
twice, you will skip over that node, and if you presss
, you will see a callstack without generated sources.