Skip to content

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

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.

Sign up for GitHub

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
thaystg merged 3 commits into from
Dec 14, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da02371
Avoiding assert on mono runtime
thaystg Dec 9, 2021
f128459
Update src/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs
thaystg Dec 10, 2021
84684c6
Addressing @radical comments.
thaystg Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ internal class AssemblyInfo
internal MetadataReader asmMetadataReader { get; }
internal MetadataReader pdbMetadataReader { get; set; }
internal List<MetadataReader> enCMetadataReader = new List<MetadataReader>();
public int DebugId { get; set; }
private int debugId;
internal int PdbAge { get; }
internal System.Guid PdbGuid { get; }
internal string PdbName { get; }
Expand All @@ -539,6 +539,7 @@ internal class AssemblyInfo

public unsafe AssemblyInfo(MonoProxy monoProxy, SessionId sessionId, string url, byte[] assembly, byte[] pdb, CancellationToken token)
{
debugId = -1;
this.id = Interlocked.Increment(ref next_id);
using var asmStream = new MemoryStream(assembly);
peReader = new PEReader(asmStream);
Expand Down Expand Up @@ -579,6 +580,19 @@ public unsafe AssemblyInfo(MonoProxy monoProxy, SessionId sessionId, string url,
Populate();
}

public async Task<int> DebugId(MonoSDBHelper sdbAgent, CancellationToken token)
{
if (debugId != -1)
return debugId;
debugId = await sdbAgent.GetAssemblyId(Name, token);
Copy link
Member

@radical radical Dec 9, 2021

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
Member Author

@thaystg thaystg Dec 10, 2021

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
Member

@radical radical Dec 10, 2021

Choose a reason for hiding this comment

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

yeah

Copy link
Member Author

@thaystg thaystg Dec 10, 2021

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
Member Author

@thaystg thaystg Dec 10, 2021

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.

return debugId;
}

public void SetDebugId(int id)
{
debugId = id;
Copy link
Member

@radical radical Dec 10, 2021

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?

Copy link
Member Author

@thaystg thaystg Dec 10, 2021 •
edited
Loading

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)

}

public bool EnC(byte[] meta, byte[] pdb)
{
var asmStream = new MemoryStream(meta);
Expand Down
2 changes: 1 addition & 1 deletion src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ async Task<int> FindStaticTypeId(string typeName)
if (type == null)
continue;

int id = await context.SdbAgent.GetTypeIdFromToken(asm.DebugId, type.Token, token);
int id = await context.SdbAgent.GetTypeIdFromToken(await asm.DebugId(context.SdbAgent, token), type.Token, token);
if (id != -1)
return id;
}
Expand Down
2 changes: 1 addition & 1 deletion src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,7 @@ public async Task<AssemblyInfo> GetAssemblyInfo(int assemblyId, CancellationToke
return null;
}
}
asm.DebugId = assemblyId;
asm.SetDebugId(assemblyId);
assemblies[assemblyId] = asm;
return asm;
}
Expand Down
10 changes: 10 additions & 0 deletions src/mono/wasm/debugger/DebuggerTestSuite/EvaluateOnCallFrameTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,6 +855,16 @@ await RuntimeEvaluateAndCheck(
("\"15\"", TString("15")));
});

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

@radical radical Dec 10, 2021

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?

"DebuggerTests.EvaluateTestsClass", "EvaluateLocals", 9, "EvaluateLocals",
"window.setTimeout(function() { invoke_static_method ('[debugger-test] DebuggerTests.EvaluateTestsClass:EvaluateLocals'); })",
wait_for_event_fn: async (pause_location) =>
{
await RuntimeEvaluateAndCheck(
("ClassToBreak.valueToCheck", TNumber(10)));
});

}

}
1 change: 1 addition & 0 deletions src/mono/wasm/debugger/tests/debugger-test-with-source-link/test.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ public static int TestBreakpoint()
{
return 50;
}
public static int valueToCheck = 10;
}
}