Skip to content

[wasm][debugger] Fix DebuggerTypeProxy for structs and for classes with multiple constructors #77039

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
ilonatommy merged 10 commits into from
Nov 8, 2022
Merged

[wasm][debugger] Fix DebuggerTypeProxy for structs and for classes with multiple constructors #77039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1e34ad2
Adding any new constructor breaks UsingDebuggerTypeProxy test.
ilonatommy Oct 21, 2022
95ff171
Fix: now objects won't get identified as valueTypes by a mistake.
ilonatommy Oct 21, 2022
78e9072
Test logic fix: rely on comparison with fixed entities only.
ilonatommy Oct 21, 2022
bf51e21
Fix: choosing proxy's matching constructor works.
ilonatommy Oct 21, 2022
8e02c8d
Merge branch 'main' into fix-root-hidden-tests
ilonatommy Oct 21, 2022
29488c8
Support DebuggerProxy in valueTypes.
ilonatommy Nov 2, 2022
f6a4028
Fix test to not rely on `testPropertiesNone`.
ilonatommy Nov 4, 2022
a883b9e
Fix tests.
ilonatommy Nov 4, 2022
a25f90b
Fix tests.
ilonatommy Nov 4, 2022
6b72197
Edit and disable rootHidden test.
ilonatommy Nov 7, 2022
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
7 changes: 4 additions & 3 deletions src/mono/wasm/debugger/BrowserDebugProxy/MemberObjectsExplorer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,10 @@ private static async Task<JArray> GetRootHiddenChildren(
{
// a collection - expose elements to be of array scheme
var memberNamedItems = members
.Where(m => m["name"]?.Value<string>() == "Items" || m["name"]?.Value<string>() == "_items")
.Where(m => m["name"]?.Value<string>() == "Items")
Copy link
Member

@radical radical Nov 2, 2022

Choose a reason for hiding this comment

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

In what cases(example code) would we be executing this block?

Copy link
Member Author

@ilonatommy ilonatommy Nov 3, 2022

Choose a reason for hiding this comment

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

All cases in DebuggerTests.EvaluateOnCallFrameTests.EvaluateBrowsableRootHidden because

Error Message:
   [] Could not find variable 'listRootHidden[0]'

This PR fixes only the problem with Items vs _items (now it's always Items). But it does not fix the fact that they need to get expanded manually - rootHidden info does not exist on them and cannot be easily extracted. The issue where we address it is this one: #76876.

Copy link
Member

@radical radical Nov 3, 2022

Choose a reason for hiding this comment

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

Is my understanding here correct?:

  1. List<T> has [DebuggerTypeProxy(typeof(ICollectionDebugView<>))], so we look at the proxy
  2. the proxy has Items property
  3. Items has [DebuggerBrowsable(DebuggerBrowsableState.RootHidden)], which isn't working yet ([wasm][debugger] RootHidden is not working correctly in DebuggerTypeProxy #76876)
  4. So, we are expanding Items here to work specifically for List<T>?

If so, then is this only to get EvaluateBrowsableRootHidden passing?
Also, does #76876 still happen after the DebuggerTypeProxy fix here?

Copy link
Member

@radical radical Nov 4, 2022

Choose a reason for hiding this comment

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

I looked at it, and (4) is true. The test for listRootHidden should be disabled for now, and a comment mentioning the issue. And we can remove this incorrect hack.
And in a follow up PR, the root hidden issue can be fixed.

Copy link
Member Author

@ilonatommy ilonatommy Nov 4, 2022

Choose a reason for hiding this comment

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

I would say it's a misunderstanding caused by me not deleting the dependency on non-fixed values in the tests properly. Manual expansion was not done on an object decorated with RootHidden but on testPropertiesNone. The else block is not to make the tests pass, it is necessary till RootHidden on Items will be received correctly by Proxy. It was the test that was not 100% logical.
I think now it will make more sense after the last commit.

Yes, the issue is still here. Proxy is not receiving any DebuggerAttributes assigned to Items, so it does not know it's of rootHidden type.

Copy link
Member Author

@ilonatommy ilonatommy Nov 4, 2022

Choose a reason for hiding this comment

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

I looked at it, and (4) is true. The test for listRootHidden should be disabled for now, and a comment mentioning the issue. And we can remove this incorrect hack. And in a follow up PR, the root hidden issue can be fixed.

Sorry, this comment did not display for me when I was investigating. I don't agree. This will break mechanism for all Collections that are in rootHidden objects. The else block can be removed when we have the fix already, so that we won't be changing the behavior from working to not working.

Copy link
Member

@radical radical Nov 5, 2022

Choose a reason for hiding this comment

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

runtime/src/mono/wasm/debugger/BrowserDebugProxy/MemberObjectsExplorer.cs

Lines 149 to 161 in 8064bfe

else
{
// a collection - expose elements to be of array scheme
var memberNamedItems = members
.Where(m => m["name"]?.Value<string>() == "Items" || m["name"]?.Value<string>() == "_items")
.FirstOrDefault();
if (memberNamedItems is not null &&
(DotnetObjectId.TryParse(memberNamedItems["value"]?["objectId"]?.Value<string>(), out DotnetObjectId itemsObjectId)) &&
itemsObjectId.Scheme == "array")
{
rootObjectId = itemsObjectId;
}
}

This code is very explicitly looking for a Items member which is an array. So, the else arm is affecting only the very specific types that have such a member, which is the case for List<T>. The proxy, or the debugger agent aren't populating it as an extra thing.

I agree that we shouldn't break the behavior in this PR. But it is incorrect. We want to avoid depending on magic like that, especially when proper mechanisms are available to get the information - DebuggerTypeProxy in this case.

The rootHidden issue is in addition to this, and that's fine to fix separately.

.FirstOrDefault();
if (memberNamedItems is not null &&
(DotnetObjectId.TryParse(memberNamedItems["value"]?["objectId"]?.Value<string>(), out DotnetObjectId itemsObjectId)) &&
DotnetObjectId.TryParse(memberNamedItems["value"]?["objectId"]?.Value<string>(), out DotnetObjectId itemsObjectId) &&
itemsObjectId.Scheme == "array")
{
rootObjectId = itemsObjectId;
Expand Down Expand Up @@ -550,7 +550,8 @@ public static async Task<GetMembersResult> GetObjectMemberValues(
// 2
if (!getCommandType.HasFlag(GetObjectCommandOptions.ForDebuggerDisplayAttribute))
{
GetMembersResult debuggerProxy = await sdbHelper.GetValuesFromDebuggerProxyAttribute(objectId, typeIdsIncludingParents[0], token);
GetMembersResult debuggerProxy = await sdbHelper.GetValuesFromDebuggerProxyAttributeForObject(
objectId, typeIdsIncludingParents[0], token);
if (debuggerProxy != null)
return debuggerProxy;
}
Expand Down
162 changes: 100 additions & 62 deletions src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2105,8 +2105,7 @@ public async Task<int> GetTypeByName(string typeToSearch, CancellationToken toke
return retDebuggerCmdReader.ReadInt32();
}

// FIXME: support valuetypes
public async Task<GetMembersResult> GetValuesFromDebuggerProxyAttribute(int objectId, int typeId, CancellationToken token)
public async Task<GetMembersResult> GetValuesFromDebuggerProxyAttributeForObject(int objectId, int typeId, CancellationToken token)
{
try
{
Expand All @@ -2116,25 +2115,11 @@ public async Task<GetMembersResult> GetValuesFromDebuggerProxyAttribute(int obje

using var ctorArgsWriter = new MonoBinaryWriter();
ctorArgsWriter.Write((byte)ValueTypeId.Null);

// FIXME: move method invocation to valueTypeclass?
if (ValueCreator.TryGetValueTypeById(objectId, out var valueType))
{
//FIXME: Issue #68390
//ctorArgsWriter.Write((byte)0); //not used but needed
//ctorArgsWriter.Write(0); //not used but needed
//ctorArgsWriter.Write((int)1); // num args
//ctorArgsWriter.Write(valueType.Buffer);
return null;
}
else
{
ctorArgsWriter.Write((byte)0); //not used
ctorArgsWriter.Write(0); //not used
ctorArgsWriter.Write((int)1); // num args
ctorArgsWriter.Write((byte)ElementType.Object);
ctorArgsWriter.Write(objectId);
}
ctorArgsWriter.Write((byte)0); //not used
ctorArgsWriter.Write(0); //not used
ctorArgsWriter.Write((int)1); // num args
ctorArgsWriter.Write((byte)ElementType.Object);
ctorArgsWriter.Write(objectId);

var retMethod = await InvokeMethod(ctorArgsWriter.GetParameterBuffer(), methodId, token);
if (!DotnetObjectId.TryParse(retMethod?["value"]?["objectId"]?.Value<string>(), out DotnetObjectId dotnetObjectId))
Expand All @@ -2154,6 +2139,39 @@ public async Task<GetMembersResult> GetValuesFromDebuggerProxyAttribute(int obje
return null;
}

public async Task<GetMembersResult> GetValuesFromDebuggerProxyAttributeForValueTypes(int valueTypeId, int typeId, CancellationToken token)
{
try
{
var typeName = await GetTypeName(typeId, token);
int methodId = await FindDebuggerProxyConstructorIdFor(typeId, token);
if (methodId == -1)
return null;

using var ctorArgsWriter = new MonoBinaryWriter();
ctorArgsWriter.Write((byte)ValueTypeId.Null);

if (!ValueCreator.TryGetValueTypeById(valueTypeId, out var valueType))
return null;
ctorArgsWriter.Write((byte)0); //not used but needed
ctorArgsWriter.Write(0); //not used but needed
ctorArgsWriter.Write((int)1); // num args
ctorArgsWriter.Write(valueType.Buffer);
var retMethod = await InvokeMethod(ctorArgsWriter.GetParameterBuffer(), methodId, token);
if (!DotnetObjectId.TryParse(retMethod?["value"]?["objectId"]?.Value<string>(), out DotnetObjectId dotnetObjectId))
throw new Exception($"Invoking .ctor ({methodId}) for DebuggerTypeProxy on type {typeId} returned {retMethod}");
GetMembersResult members = await GetTypeMemberValues(dotnetObjectId,
GetObjectCommandOptions.WithProperties | GetObjectCommandOptions.ForDebuggerProxyAttribute,
token);
return members;
}
catch (Exception e)
{
logger.LogDebug($"Could not evaluate DebuggerTypeProxyAttribute of type {await GetTypeName(typeId, token)} - {e}");
return null;
}
}

private async Task<int> FindDebuggerProxyConstructorIdFor(int typeId, CancellationToken token)
{
try
Expand All @@ -2162,59 +2180,79 @@ private async Task<int> FindDebuggerProxyConstructorIdFor(int typeId, Cancellati
if (getCAttrsRetReader == null)
return -1;

var methodId = -1;
var parmCount = getCAttrsRetReader.ReadInt32();
for (int j = 0; j < parmCount; j++)
if (parmCount != 1)
throw new InternalErrorException($"Expected to find custom attribute with only one argument, but it has {parmCount} parameters.");

byte monoParamTypeId = getCAttrsRetReader.ReadByte();
// FIXME: DebuggerTypeProxyAttribute(string) - not supported
if ((ValueTypeId)monoParamTypeId != ValueTypeId.Type)
{
var monoTypeId = getCAttrsRetReader.ReadByte();
// FIXME: DebuggerTypeProxyAttribute(string) - not supported
if ((ValueTypeId)monoTypeId != ValueTypeId.Type)
continue;
logger.LogDebug($"DebuggerTypeProxy attribute is only supported with a System.Type parameter type. Got {(ValueTypeId)monoParamTypeId}");
return -1;
}

var typeProxyTypeId = getCAttrsRetReader.ReadInt32();

using var commandParamsWriter = new MonoBinaryWriter();
commandParamsWriter.Write(typeProxyTypeId);
var originalClassName = await GetTypeNameOriginal(typeProxyTypeId, token);

var cAttrTypeId = getCAttrsRetReader.ReadInt32();
using var commandParamsWriter = new MonoBinaryWriter();
commandParamsWriter.Write(cAttrTypeId);
var className = await GetTypeNameOriginal(cAttrTypeId, token);
if (className.IndexOf('[') > 0)
if (originalClassName.IndexOf('[') > 0)
{
string className = originalClassName;
className = className.Remove(className.IndexOf('['));
var assemblyId = await GetAssemblyIdFromType(typeProxyTypeId, token);
var assemblyName = await GetFullAssemblyName(assemblyId, token);

StringBuilder typeToSearch = new(className);
typeToSearch.Append('[');
List<int> genericTypeArgs = await GetTypeParamsOrArgsForGenericType(typeId, token);
for (int k = 0; k < genericTypeArgs.Count; k++)
{
className = className.Remove(className.IndexOf('['));
var assemblyId = await GetAssemblyIdFromType(cAttrTypeId, token);
var assemblyName = await GetFullAssemblyName(assemblyId, token);
var typeToSearch = className;
typeToSearch += "[["; //System.Collections.Generic.List`1[[System.Int32,mscorlib,Version=4.0.0.0,Culture=neutral,PublicKeyToken=b77a5c561934e089]],mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089
List<int> genericTypeArgs = await GetTypeParamsOrArgsForGenericType(typeId, token);
for (int k = 0; k < genericTypeArgs.Count; k++)
{
var assemblyIdArg = await GetAssemblyIdFromType(genericTypeArgs[k], token);
var assemblyNameArg = await GetFullAssemblyName(assemblyIdArg, token);
var classNameArg = await GetTypeNameOriginal(genericTypeArgs[k], token);
typeToSearch += classNameArg + ", " + assemblyNameArg;
if (k + 1 < genericTypeArgs.Count)
typeToSearch += "], [";
else
typeToSearch += "]";
}
typeToSearch += "]";
typeToSearch += ", " + assemblyName;
var genericTypeId = await GetTypeByName(typeToSearch, token);
if (genericTypeId < 0)
break;
cAttrTypeId = genericTypeId;
// typeToSearch += '[';
var assemblyIdArg = await GetAssemblyIdFromType(genericTypeArgs[k], token);
var assemblyNameArg = await GetFullAssemblyName(assemblyIdArg, token);
var classNameArg = await GetTypeNameOriginal(genericTypeArgs[k], token);
typeToSearch.Append($"{(k == 0 ? "" : ",")}[{classNameArg}, {assemblyNameArg}]");
}
int[] methodIds = await GetMethodIdsByName(cAttrTypeId, ".ctor", BindingFlags.Default, token);
if (methodIds != null)
methodId = methodIds[0];
break;
typeToSearch.Append($"], {assemblyName}");
var genericTypeId = await GetTypeByName(typeToSearch.ToString(), token);
if (genericTypeId < 0)
{
logger.LogDebug($"Could not find instantiated generic type id for {typeToSearch}.");
return -1;
}
typeProxyTypeId = genericTypeId;
}
int[] constructorIds = await GetMethodIdsByName(typeProxyTypeId, ".ctor", BindingFlags.DeclaredOnly, token);
if (constructorIds is null)
throw new InternalErrorException($"Could not find any constructor for DebuggerProxy type: {originalClassName}");

if (constructorIds.Length == 1)
return constructorIds[0];

return methodId;
string expectedConstructorParamType = await GetTypeName(typeId, token);
foreach (var methodId in constructorIds)
{
var methodInfoFromRuntime = await GetMethodInfo(methodId, token);
// avoid calling to runtime if possible
var ps = methodInfoFromRuntime.Info.GetParametersInfo();
if (ps.Length != 1)
continue;
string parameters = await GetParameters(methodId, token);
if (string.IsNullOrEmpty(parameters))
throw new InternalErrorException($"Could not get method's parameter types. MethodId = {methodId}.");
if (parameters == $"({expectedConstructorParamType})")
return methodId;
}
throw new InternalErrorException($"Could not find a matching constructor for DebuggerProxy type: {originalClassName}");
}
catch (Exception e)
{
logger.LogDebug($"Could not evaluate DebuggerTypeProxyAttribute of type {await GetTypeName(typeId, token)} - {e}");
return -1;
}

return -1;
}

public ValueTypeClass GetValueTypeClass(int valueTypeId)
Expand Down
4 changes: 1 addition & 3 deletions src/mono/wasm/debugger/BrowserDebugProxy/ValueTypeClass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,7 @@ public async Task<GetMembersResult> GetMemberValues(
if (!getObjectOptions.HasFlag(GetObjectCommandOptions.ForDebuggerDisplayAttribute))
{
// FIXME: cache?
result = await sdbHelper.GetValuesFromDebuggerProxyAttribute(Id.Value, TypeId, token);
if (result != null)
Console.WriteLine($"Investigate GetValuesFromDebuggerProxyAttribute\n{result}. There was a change of logic from loop to one iteration");
result = await sdbHelper.GetValuesFromDebuggerProxyAttributeForValueTypes(Id.Value, TypeId, token);
}

if (result == null && getObjectOptions.HasFlag(GetObjectCommandOptions.AccessorPropertiesOnly))
Expand Down
8 changes: 6 additions & 2 deletions src/mono/wasm/debugger/DebuggerTestSuite/CustomViewTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public async Task UsingDebuggerDisplay()
[ConditionalFact(nameof(RunningOnChrome))]
public async Task UsingDebuggerTypeProxy()
{
var bp = await SetBreakpointInMethod("debugger-test.dll", "DebuggerTests.DebuggerCustomViewTest", "run", 15);
var bp = await SetBreakpointInMethod("debugger-test.dll", "DebuggerTests.DebuggerCustomViewTest", "run", 16);
var pause_location = await EvaluateAndCheck(
"window.setTimeout(function() { invoke_static_method ('[debugger-test] DebuggerTests.DebuggerCustomViewTest:run'); }, 1);",
"dotnet://debugger-test.dll/debugger-custom-view-test.cs",
Expand All @@ -61,6 +61,10 @@ public async Task UsingDebuggerTypeProxy()
props = await GetObjectOnFrame(frame, "b");
await CheckString(props, "Val2", "one");

await CheckValueType(locals, "bs", "DebuggerTests.WithProxyStruct", description:"DebuggerTests.WithProxyStruct");
props = await GetObjectOnFrame(frame, "bs");
await CheckString(props, "Val2", "one struct");

await CheckObject(locals, "openWith", "System.Collections.Generic.Dictionary<string, string>", description: "Count = 3");
props = await GetObjectOnFrame(frame, "openWith");
Assert.Equal(1, props.Count());
Expand Down Expand Up @@ -106,7 +110,7 @@ async Task<bool> CheckProperties(JObject pause_location)
Assert.True(task.Result);
}
}

[ConditionalFact(nameof(RunningOnChrome))]
public async Task InspectObjectOfTypeWithToStringOverriden()
{
Expand Down
12 changes: 10 additions & 2 deletions src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,9 +772,17 @@ internal async Task CheckProps(JToken actual, object exp_o, string label, int nu
var exp_i = exp_v_arr[i];
var act_i = actual_arr[i];

AssertEqual(i.ToString(), act_i["name"]?.Value<string>(), $"{label}-[{i}].name");
string exp_name = exp_i["name"]?.Value<string>();
if (string.IsNullOrEmpty(exp_name))
exp_name = i.ToString();

AssertEqual(exp_name, act_i["name"]?.Value<string>(), $"{label}-[{i}].name");
if (exp_i != null)
await CheckValue(act_i["value"], exp_i, $"{label}-{i}th value");
{
await CheckValue(act_i["value"],
((JObject)exp_i).GetValue("value")?.HasValues == true ? exp_i["value"] : exp_i,
$"{label}-{i}th value");
}
}
return;
}
Expand Down
Loading