Skip to content

[wasm][debugger] Avoiding assert on mono runtime - #62601

Merged
thaystg merged 3 commits into
dotnet:mainfrom
thaystg:thays_fix_runtime_assert
Dec 14, 2021
Merged

[wasm][debugger] Avoiding assert on mono runtime#62601
thaystg merged 3 commits into
dotnet:mainfrom
thaystg:thays_fix_runtime_assert

Conversation

@thaystg

@thaystgthaystg commented Dec 9, 2021

Copy link
Copy Markdown
Member

It was passing id = 0 and asserting on mono runtime when trying to evaluate something in a Class that has the assembly loaded, but doesn't have any breakpoint added.

Fix https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1427671?src=WorkItemMention&src-action=artifact_link

@ghost

ghost commented Dec 9, 2021

Copy link
Copy Markdown

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

Issue Details

It was passing id = 0 and asserting on mono runtime when trying to evaluate something in a Class that has the assembly loaded, but doesn't have any breakpoint added.

Fix https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1427671?src=WorkItemMention&src-action=artifact_link

Author:thaystg
Assignees:-
Labels:

area-Debugger-mono

Milestone:-

@radicalradical added the arch-wasm WebAssembly architecture label Dec 9, 2021
@ghost

ghost commented Dec 9, 2021

Copy link
Copy Markdown

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

It was passing id = 0 and asserting on mono runtime when trying to evaluate something in a Class that has the assembly loaded, but doesn't have any breakpoint added.

Fix https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1427671?src=WorkItemMention&src-action=artifact_link

Author:thaystg
Assignees:-
Labels:

arch-wasm, area-Debugger-mono

Milestone:-

Comment threadsrc/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs Outdated
{
if (debugId != -1)
return debugId;
debugId = await sdbAgent.GetAssemblyId(Name, token);

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.

would it make sense to throw here if debugId <= 0?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Do you mean after GetAssemblyId?

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.

yeah

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I will do it, but in my backport version of this PR I will not add the throw to avoid any side effect on net6, does it make sense?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Okay, I changed my mind, I don't think we will get any side effect on net 6, because this is called from FindStaticTypeId and this has a try catch in the caller.


public void SetDebugId(int id)
{
debugId = id;

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.

Should this throw if debugId != -1 && debugId != id to avoid reseting the id by mistake?

@thaystgthaystgDec 10, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don't like the idea to throw an exception but I added an if to set only if (debugId <= 0 && debugId != id)

});

[Fact]
public async Task EvaluateStaticAttributeInAssemblyNotRelatedButLoaded() => await CheckInspectLocalsAtBreakpointSite(

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.

ideas: Maybe add another test that tries to test the same thing, but by enumerating members of a local, or a field?
And maybe one where a previous frame is in that other assembly, and then check members on that frame?

thaystgand others added 2 commits December 10, 2021 10:08
Co-authored-by: Ankit Jain <radical@gmail.com>
Removing unused fields in another test case.
@thaystg
thaystg requested a review from radicalDecember 13, 2021 12:05
@thaystg
thaystg merged commit 2658a0b into dotnet:mainDec 14, 2021
@thaystg

Copy link
Copy Markdown
MemberAuthor

/backport to release/6.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1579681367

@github-actions

Copy link
Copy Markdown
Contributor

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

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Avoiding assert on mono runtime
Using index info to reconstruct a base tree...
M	src/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs
M	src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
M	src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs
M	src/mono/wasm/debugger/DebuggerTestSuite/EvaluateOnCallFrameTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/mono/wasm/debugger/DebuggerTestSuite/EvaluateOnCallFrameTests.cs
Auto-merging src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs
Auto-merging src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
CONFLICT (content): Merge conflict in src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
Auto-merging src/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs
CONFLICT (content): Merge conflict in src/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Avoiding assert on mono runtime
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!

@ghostghost locked as resolved and limited conversation to collaborators Jan 14, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-Debugger-mono

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@thaystg@radical