-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
[wasm][debugger] Improvements in debugging in async methods #78651
Conversation
Tagging subscribers to this area: @thaystg Issue DetailsFixing 2 variables with same names in different scopes in an async method. Before this PR it was always showing the value of the last variable declared. Fixes #77031
|
…de, and also fixing 2 variables with same name in differente scopes, and also fixing method name for async methods in VB.
@@ -804,7 +804,7 @@ internal sealed class MonoSDBHelper | |||
private SessionId sessionId; | |||
|
|||
internal readonly ILogger logger; | |||
private static readonly Regex regexForAsyncLocals = new (@"\<([^)]*)\>", RegexOptions.Singleline); | |||
private static readonly Regex regexForAsyncLocals = new (@"\<([^)]*)\>([^)]*)([_][_])([0-9]*)", RegexOptions.Singleline); |
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.
Can you add some example strings that we are trying to match?
@@ -320,6 +320,8 @@ public override bool Equals(object obj) | |||
public static bool operator !=(SourceId a, SourceId b) => !a.Equals(b); | |||
} | |||
|
|||
internal sealed record Scope (int Id, int StartOffset, int EndOffset); |
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.
internal sealed record Scope (int Id, int StartOffset, int EndOffset); | |
internal sealed record Scope(int Id, int StartOffset, int EndOffset); |
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.
Rename to AsyncScope
, or AsyncScopeDebugInformation
. And it can be a private nested type of MethodInfo
.
I pushed the changes that I suggested to https://github.com/radical/runtime/commits/aj-thays_fix_77031 . |
else if (fieldName.StartsWith("$")) | ||
{ | ||
continue; | ||
} | ||
else | ||
{ | ||
asyncLocalsFull.Add(asyncLocal); |
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.
Should this be unconditional? It seems to be common, for the cases we don't continue
earlier.
var match = regexForVBAsyncMethodName.Match(klassName); | ||
ret = ret.Insert(0, match.Groups[2].Value); |
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.
Check match.Success
.
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.
Other than the comments, LGTM 👍
/backport to release/7.0 |
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3676987035 |
@thaystg backporting to release/7.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fixing 2 variables with same names in different scopes in a async method.
Using index info to reconstruct a base tree...
M src/mono/wasm/debugger/BrowserDebugProxy/DebugStore.cs
M src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
M src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs
Falling back to patching base and 3-way merge...
Auto-merging src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs
CONFLICT (content): Merge conflict in src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs
Auto-merging src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
Auto-merging 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 Fixing 2 variables with same names in different scopes in a 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! |
@thaystg an error occurred while backporting to release/7.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
Fixes #77031
Fixes #77481