Skip to content

[wasm][debugger] Fixing step over in an async method#42639

Merged
thaystg merged 2 commits intodotnet:masterfrom
thaystg:thays_fix_42424_3
Sep 24, 2020
Merged

[wasm][debugger] Fixing step over in an async method#42639
thaystg merged 2 commits intodotnet:masterfrom
thaystg:thays_fix_42424_3

Conversation

@thaystg
Copy link
Copy Markdown
Member

@thaystg thaystg commented Sep 23, 2020

We don't have ctx available on wasm, but we don't need it to calculate the frames, the ifdef on debugger-engine.h fix the #42424.

@ghost
Copy link
Copy Markdown

ghost commented Sep 23, 2020

Tagging subscribers to this area: @thaystg
See info in area-owners.md if you want to be subscribed.

Copy link
Copy Markdown
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context is unused in the wasm case so this looks fine. I'll double check the tests then approve.

Comment thread src/mono/mono/mini/debugger-engine.c
if (ss_args->ctx && !frames) {

#else
if (!frames) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this causing the issue with the test? Why did it specifically break the async test?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because once it doesn't execute ss_calculate_framecount because ss_args->ctx == NULL, nframes variable would be 0, and wouldn't enter in this if: if (asyncMethod && asyncMethod->num_awaits && nframes && rt_callbacks.ensure_jit (frames [0])) { and wouldn't call ss_bp_add_one

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. nframes ==0 would also cause the loop starting line 1341 to get skipped, IIUC. Is there any case where that loop will get incorrectly skipped?catch`(not the last one, IIUC) in an async method? Or catch in non-async methods?

Just trying to understand with silly/naive questions 😬

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this loop would be skipped wrongly without the fix. Now the value of nframes and frames variables will be correct for both loops, line 1341 and line 1358.

@lewing
Copy link
Copy Markdown
Member

lewing commented Sep 23, 2020

I'm seeing some failures on the locally. @thaystg can you verify?

@lewing
Copy link
Copy Markdown
Member

lewing commented Sep 23, 2020

Looks like the issue is that #42486 fails to handle null strings in on of the versions of
js_string_to_mono_string. I'll open a separate pr for it because that change is only on master.

@lewing
Copy link
Copy Markdown
Member

lewing commented Sep 23, 2020

debugger test regressions fixed in #42658

@lewing
Copy link
Copy Markdown
Member

lewing commented Sep 24, 2020

@steveisok still getting cancellations we can't clear

@thaystg thaystg merged commit fa8fb1d into dotnet:master Sep 24, 2020
@thaystg
Copy link
Copy Markdown
Member Author

thaystg commented Sep 24, 2020

/backport to release/5.0-rc2

@github-actions
Copy link
Copy Markdown
Contributor

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/271123075

@github-actions
Copy link
Copy Markdown
Contributor

@thaystg backporting to release/5.0-rc2 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fixing step over in an async method.
error: sha1 information is lacking or useless (src/mono/mono/mini/debugger-engine.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fixing step over in an async method.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants