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

[cdac] add v2 ExecutionManager contract for NibbleMap change #109654

Merged
merged 12 commits into from
Nov 11, 2024

Conversation

max-charlamb
Copy link
Contributor

@max-charlamb max-charlamb commented Nov 8, 2024

Required for the contract change in #108939

Changes

  • Bumps ExecutionManager contract version to 2
  • Refactors ExecutionManager related files into folder.
  • Factors out nearly all of ExecutionManager_1 to ExecutionManagerBase
  • Adds ExecutionManager_2
  • Runs tests for both versions of contract.

Testing

Verified ExecutionManager_2 using WinDbg to debug a managed application and debugged that with VS to check that !sos clrstack has the correct stack (function names) and that the new nibblemap is invoked.

The nibblemap lookup is easily invoked using !sos ip2md <ip>

See previous comments from: max-charlamb#1

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 8, 2024
@max-charlamb max-charlamb added area-Diagnostics-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 8, 2024
Copy link
Contributor

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

eeInfo = em.GetCodeBlockHandle(methodStart + 0x450 - 1);
Assert.NotNull(eeInfo);
Assert.Equal(expectedMethodDescAddress, actualMethodDesc);
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add tests for the negative case of trying to get a code block for an address out of the range?

Copy link
Contributor Author

@max-charlamb max-charlamb Nov 11, 2024

Choose a reason for hiding this comment

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

The difficulty here is that the behavior changes depending on the version of the nibblemap. I talked to @davidwrighton about nibblemap behavior when the IP is outside of a managed method, and we agreed that it could be undefined.


The linear algorithm scans until it finds the previous function. If there are no previous functions in the code range it will return null.

In the new constant algorithm, methods may be "extended" between a few and a couple hundred bytes if there is no immediately following method. Outside of that range it will return null.


I see two ways to test what happens when trying to get a code block for a value not in a managed function.

  1. Pick a value that will neither nibblemap algorithm will return a value for (ie. a code range without any functions).
  2. Mock out nibblemap from the ExecutionManager tests. This is probably the preferred approach but would take more refactoring to get right.

For now, I would be in favor of option 1 and maybe switch to option 2 once we have mocking infrastructure set up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a new test which doesn't have any allocated methods and asserted we return null.

Copy link
Member

Choose a reason for hiding this comment

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

For now, I would be in favor of option 1 and maybe switch to option 2 once we have mocking infrastructure set up.

Agreed.


namespace Microsoft.Diagnostics.DataContractReader.Contracts;

internal sealed class ExecutionManager_2 : ExecutionManagerBase<NibbleMapConstantLookup>
Copy link
Member

Choose a reason for hiding this comment

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

I like the move to composability here. I think it is also worth considering (perhaps as a separate change) how this could help with out unit testing. Are we now at a place where we can have a mock nibble map for testing the execution manager? For example, we could have the nibble map tests have the extensive coverage of the nibble map itself and then the execution manager tests could expect the nibble map to be correct based on that coverage and mock out INibbleMap.FindMethodCode to return what is needed instead of having to build out the memory.

docs/design/datacontracts/ExecutionManager.md Outdated Show resolved Hide resolved
docs/design/datacontracts/ExecutionManager.md Outdated Show resolved Hide resolved
docs/design/datacontracts/ExecutionManager.md Outdated Show resolved Hide resolved
docs/design/datacontracts/ExecutionManager.md Outdated Show resolved Hide resolved
@max-charlamb max-charlamb merged commit 470afc6 into dotnet:main Nov 11, 2024
148 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants