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] Added support for getting members of static structures. #69542

Merged
merged 13 commits into from
Jul 28, 2022

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented May 19, 2022

Fixes #68539.

Changes in this PR:

  • Members of static valueTypes are now being returned on getProperties, before they were skipped. To do it, flag includeStatic=true was added to the data flow.

  • For static properties, to get values we need to invoke them. Because they are static, we cannot invoke getter using objectId, we have to use typeId instead. This change was made in GetNotAutoExpandableObject where we put different values in containerId, conditioned by property being static. We have to save the information if getter is static in JObject to skip writing ElementType to the buffer in InvokeMethod - for statics runtime does not expect such information.

  • Changed the expected result in CallFunctionOnTests.PropertyGettersTest. Previously properties of static classes were skipped in the reply to getProperties. It was not the planned behavior so in this PR they are added to the reply as well.

  • Changed the expected number of fields for MiscTest.InspectTaskAtLocals - now we expect 4 statics to be there as well:
    image

  • Smaller changes: removed whitespaces in Browsable tests classes to match automatic formatting, renamed Browsable test classes to be more descriptive, added missing test cases for static non-autoproperties in classes and structures.

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture area-Debugger-mono labels May 19, 2022
@ilonatommy ilonatommy self-assigned this May 19, 2022
@ilonatommy ilonatommy requested a review from marek-safar as a code owner May 19, 2022 07:32
@ghost
Copy link

ghost commented May 19, 2022

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

Issue Details

Fixes #68539.

Changes in this PR:

  • Members of static valueTypes are now being returned on getProperties, before they were skipped. To do it, flag includeStatic=true was added to the data flow.
  • Changed the expected result in CallFunctionOnTests.PropertyGettersTest. Previously properties of static classes were skipped in the reply to getProperties. It was not the planned behavior so in this PR they are added to the reply as well.
  • Removed whitespaces in Browsable tests classes to match automatic formatting.
Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-Debugger-mono

Milestone: -

@ilonatommy ilonatommy marked this pull request as draft May 19, 2022 11:46
@ilonatommy ilonatommy marked this pull request as ready for review May 23, 2022 12:18
@ilonatommy ilonatommy marked this pull request as draft May 25, 2022 06:24
@ilonatommy ilonatommy force-pushed the browsable-static-structs-68539 branch from 0689396 to 26f45c5 Compare May 27, 2022 08:53
@ilonatommy ilonatommy requested review from radical and thaystg May 27, 2022 09:07
@ilonatommy
Copy link
Member Author

ilonatommy commented May 31, 2022

/azp run runtime-staging

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ghost ghost closed this Jun 30, 2022
@ghost
Copy link

ghost commented Jun 30, 2022

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ilonatommy ilonatommy reopened this Jul 4, 2022
@ilonatommy ilonatommy marked this pull request as ready for review July 8, 2022 06:19
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lewing
Copy link
Member

lewing commented Jul 25, 2022

@ilonatommy is this waiting on review now?

@lewing
Copy link
Member

lewing commented Jul 25, 2022

cc @radical @thaystg

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

Other than the feedback, it looks good.
In a future PR - static members in base/derived classes .

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jul 26, 2022
@radical
Copy link
Member

radical commented Jul 26, 2022

There is a test failure:

DebuggerTests.GetPropertiesTests.PropertiesSortedByProtectionLevel
...

  Error Message:
   missing or unexpected members found
  Stack Trace:
     at DebuggerTests.GetPropertiesTests.AssertHasOnlyExpectedProperties(String[] expected_names, IEnumerable`1 actual) in /_/src/mono/wasm/debugger/DebuggerTestSuite/GetPropertiesTests.cs:line 445
   at DebuggerTests.GetPropertiesTests.<>c__DisplayClass12_0.<<PropertiesSortedByProtectionLevel>b__0>d.MoveNext() in /_/src/mono/wasm/debugger/DebuggerTestSuite/GetPropertiesTests.cs:line 545
--- End of stack trace from previous location ---
   at DebuggerTests.DebuggerTestBase.CheckInspectLocalsAtBreakpointSite(String type, String method, Int32 line_offset, String bp_function_name, String eval_expression, Func`2 locals_fn, Func`2 wait_for_event_fn, Boolean use_cfo, String assembly, Int32 col) in /_/src/mono/wasm/debugger/DebuggerTestSuite/DebuggerTestBase.cs:line 256
   at DebuggerTests.GetPropertiesTests.PropertiesSortedByProtectionLevel(Dictionary`2 expectedPublic, Dictionary`2 expectedProtInter, Dictionary`2 expectedPriv, String entryMethod) in /_/src/mono/wasm/debugger/DebuggerTestSuite/GetPropertiesTests.cs:line 535
--- End of stack trace from previous location ---
  Standard Output Messages:
 Missing: b

@lewing lewing requested a review from radical July 26, 2022 22:38
@radical
Copy link
Member

radical commented Jul 27, 2022

Looking at the docs:

Private: 1
Assembly: 3
...
FieldAccessMask: 7

So, if it's assembly then HasFlag would return true for Private also, because it will do something like (Attributes & FieldAttributes.Private) == FieldAttributes.Private.

Instead, we should do it like:

public bool IsPrivate => (Attributes & FieldAttributes.FieldAccessMask) == FieldAttributes.Private;

This checks the exact value instead.

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

Only one needed change remaining - #69542 (comment) .

Other than that, LGTM 👍

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy ilonatommy merged commit b0766a9 into dotnet:main Jul 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 27, 2022
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.

[wasm][debugger] Fix member type in static structures
3 participants