Skip to content
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] Support method calls #55458

Merged
merged 8 commits into from
Jul 14, 2021

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Jul 10, 2021

Support method calls:

  • without parameter
  • with integer parameter
  • with bool parameter
  • with string parameter
  • with object parameter
  • with const char parameter

@ghost
Copy link

ghost commented Jul 10, 2021

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

Issue Details

Support method calls:

  • without parameter
  • with integer parameter
  • with bool parameter
  • with string parameter
Author: thaystg
Assignees: -
Labels:

area-Debugger-mono

Milestone: -

@thaystg thaystg linked an issue Jul 10, 2021 that may be closed by this pull request
@thaystg thaystg requested a review from lewing July 10, 2021 14:42
@thaystg thaystg assigned radical and unassigned radical Jul 10, 2021
@thaystg thaystg requested review from radical and removed request for marek-safar July 10, 2021 14:42
Support object.
@lewing lewing added the arch-wasm WebAssembly architecture label Jul 12, 2021
@ghost
Copy link

ghost commented Jul 12, 2021

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

Issue Details

Support method calls:

  • without parameter
  • with integer parameter
  • with bool parameter
  • with string parameter
  • with object parameter
  • with const char parameter
Author: thaystg
Assignees: -
Labels:

arch-wasm, area-Debugger-mono

Milestone: -

@pavelsavara
Copy link
Member

pavelsavara commented Jul 12, 2021

Few test cases could be added.

  • - chained expression this.That.Method()
  • also handle error if That was null
  • also handle error if That thrown error
  • - property setter is a method actually this.set_Name("hello"), this.set_Age(5)
  • - static method Foo.Bar(11)
  • - static method of generic class Fiz<Buzz>.Bar(11)
  • - static generic method Fiz.Bar<Buzz>(11)
  • - static method in nested class Fiz.Buzz.Bar(12)
  • - methods with same name and multiple overloads different by type of parameters this.Add(1) and this.Add("infinity")
  • - parameter casting ? this.Add((uint)-1)
  • - parameter format ? this.Add(0xCAFE) and this.Add(1_000_000)
  • - expression chaining Math.Sqrt(Math.Sqrt(256))
  • - invoke this.OnClick() where OnClick is property of type Action
  • - invoke delegate this.OnClick() where OnClick is property of type delegate
  • - invoking method with side effects to member fields, test that field updates are pushed to UI

@pavelsavara
Copy link
Member

pavelsavara commented Jul 12, 2021

Negative test cases

  • - wrong method name (and it's not found propagation to user)
  • - wrong namespace
  • - wrong class name
  • - parameter: integer out of range
  • - wrong parameter type
  • - wrong parameter count
  • - invoking method that throws (and it's propagation to user)
  • - invoking method that never returns (like await Task.Result on something never completed)
  • - test that cancelation token works

@pavelsavara
Copy link
Member

pavelsavara commented Jul 12, 2021

  • - unicode non-ascii class and method names Žluťoučký.Kůň()
  • - unicode non-ascii parameter string value "žluťoučký kůň"
  • - pass object from another method chained call this.Eat(this.Cook())
  • - pass object from another method chained call with cast this.Eat( (ITasty) this.Cook())

@thaystg
Copy link
Member Author

thaystg commented Jul 13, 2021

@pavelsavara I added all the tests that makes sense for this PR. Thanks for the review!!!
All the other tests are useful but not supported yet.

AssertEqual("Object reference not set to an instance of an object.", res.Error["message"]?.Value<string>(), "wrong error message");

(_, res) = await EvaluateOnCallFrame(id, "this.ParmToTestObjException.MyMethod()", expect_ok: false );
AssertEqual("Object reference not set to an instance of an object.", res.Error["message"]?.Value<string>(), "wrong error message");
Copy link
Member

Choose a reason for hiding this comment

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

Should that really be NRE ? I would rather want to know what was the exception message

@@ -612,6 +696,18 @@ internal async Task<MonoBinaryReader> SendDebuggerAgentCommandWithParmsInternal(
return ret_debugger_cmd_reader;
}

public async Task<int> CreateString(SessionId sessionId, string value, CancellationToken token)
{
var command_params = new MemoryStream();
Copy link
Member

Choose a reason for hiding this comment

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

using keyword and camelCase in this C# method.

if (rootObject != null)
{
DotnetObjectId.TryParse(rootObject?["objectId"]?.Value<string>(), out DotnetObjectId objectId);
var typeId = await proxy.sdbHelper.GetTypeIdFromObject(sessionId, int.Parse(objectId.Value), true, token);
Copy link
Member

Choose a reason for hiding this comment

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

proxy.sdbHelper could be local variable sdbHelper here for readability. Also I wonder if that's private field since it has lowercase name.

var typeName = await proxy.sdbHelper.GetTypeName(sessionId, typeId[0], token);
throw new Exception($"Method '{methodName}' not found in type '{typeName}'");
}
var command_params_obj = new MemoryStream();
Copy link
Member

Choose a reason for hiding this comment

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

using pattern

@thaystg thaystg merged commit 599b7ac into dotnet:main Jul 14, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 14, 2021
…debugger_custom_views

* 'main' of github.com:thaystg/runtime: (125 commits)
  [wasm] [debugger] Support method calls  (dotnet#55458)
  [debugger] Fix debugging after hot reloading (dotnet#55599)
  Inliner: Extend IL limit for profiled call-sites, allow inlining for switches. (dotnet#55478)
  DiagnosticSourceEventSource supports base class properties (dotnet#55613)
  [mono] Fix race during mono_image_storage_open (dotnet#55201)
  [mono] Add wrapper info for native func wrappers. (dotnet#55602)
  H/3 and Quic AppContext switch (dotnet#55332)
  Compression.ZipFile support for Unix Permissions (dotnet#55531)
  [mono] Fix skipping of static methods during IMT table construction. (dotnet#55610)
  Combine System.Private.Xml TrimmingTests projects (dotnet#55606)
  fix name conflict with Configuration class (dotnet#55597)
  Finish migrating RSAOpenSsl from RSA* to EVP_PKEY*
  Disable generic math (dotnet#55540)
  Obsolete CryptoConfig.EncodeOID (dotnet#55592)
  Address System.Net.Http.WinHttpHandler's nullable warnings targeting .NETCoreApp (dotnet#54995)
  Enable Http2_MultipleConnectionsEnabled_ConnectionLimitNotReached_ConcurrentRequestsSuccessfullyHandled (dotnet#55572)
  Fix Task.WhenAny failure mode when passed ICollection of zero tasks (dotnet#55580)
  Consume DistributedContextPropagator in DiagnosticsHandler (dotnet#55392)
  Add property ordering feature (dotnet#55586)
  Reduce subtest count in Reflection (dotnet#55537)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Debugger-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to evaluate method calls in watch window
4 participants