[cdac] ExecutionManager contract and RangeSectionMap lookup unit tests #108685
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
the
needs-area-label
lambdageek
removed
the
needs-area-label
|
Tagging subscribers to this area: @tommcdon |
lambdageek
force-pushed
the
cdac-execution-manager
branch
from
0c04ca3
to
af2387f
Compare
This was referenced Oct 9, 2024
* add RangeSectionMap docs
* exhaustively test RangeSectionMap.EffectiveBitsForLevel
* fix lookup in RangeSection.Find
RangeSectionLookupAlgorithm.FindFragment finds the slot containing the range section fragment pointer.
Have to dereference it once to get the actual RangeSectionFragment pointer from the slot.
This "worked" before because RangeSectionFragment.Next is at offset 0, so the first lookup would have a garbage range, so we would follow the "next" field and get to the correct fragment
* make a testable RangeSectionMap.FindFragmentInternal
* brief nibble map summary
* [cdac] Implement NibbleMap lookup and tests
The execution manager uses a nibble map to quickly map program counter
pointers to the beginnings of the native code for the managed method.
Implement the lookup algorithm for a nibble map.
Start adding unit tests for the nibble map
Also for testing in MockMemorySpace simplify ReaderContext, there's nothing special about the descriptor HeapFragments anymore. We can use a uniform reader.
* NibbleMap: fix bug and add test
Previously we incorrectly computed the prior map index when doing the backward linear search
* [testing] display Target values in hex in debugger views
* MockMemorySpace: simplify ReaderContext
there's nothing special about the descriptor HeapFragments anymore. We can use a uniform reader
* refactor ExecutionManager
* ExecutionManager contract
the presence of RangeSection.R2RModule is a discriminator for whether we're looking at EE code or R2R code
* WIP NibbleMap
* WIP JitCodeToMethodInfo
* WIP RangeSectionMap
The EECodeInfo includes the relative offset (given ip - start of method) so it's not ok to share for different code pointers into the same method
lambdageek
force-pushed
the
cdac-execution-manager
branch
from
eb190a2
to
2f26c73
Compare
lambdageek
requested review from
max-charlamb,
davidwrighton,
AaronRobinsonMSFT and
elinor-fung
elinor-fung
reviewed
Oct 15, 2024
| return FindFragmentInternal(target, topRangeSectionMap.TopLevelData, jittedCodeAddress)?.LoadValue(target).Address ?? TargetPointer.Null; | ||
| } | ||
|
|
||
| internal Cursor? FindFragmentInternal(Target target, TargetPointer topMap, TargetCodePointer jittedCodeAddress) |
There was a problem hiding this comment.
Can this just be merged into FindFragment? What is the split into FindFragmentInternal getting us?
There was a problem hiding this comment.
Easier testing. I don't need to mock up a Data.RangeSectionMap value
There was a problem hiding this comment.
Actually it's a bit more than that. We also avoid making the Cursor type visible in callers.
...icrosoft.Diagnostics.DataContractReader.Contracts/ExecutionManagerHelpers/RangeSectionMap.cs
Show resolved
Hide resolved
...icrosoft.Diagnostics.DataContractReader.Contracts/ExecutionManagerHelpers/RangeSectionMap.cs
Outdated
Show resolved
Hide resolved
...rosoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager_1.EEJitManager.cs
Show resolved
Hide resolved
|
@elinor-fung applied all of your suggestions |
This was referenced Oct 16, 2024
elinor-fung
approved these changes
Oct 16, 2024
src/native/managed/cdacreader/tests/ExecutionManagerTestBuilder.cs
Outdated
Show resolved
Hide resolved
|
|
||
| [Theory] | ||
| [ClassData(typeof(MockTarget.StdArch))] | ||
| public void TestLookupOne(MockTarget.Architecture arch) |
There was a problem hiding this comment.
Would it be useful to also cover:
- failing when there are (one or more) ranges in the map
- success with more than one range fragments in the map
|
Last comments were minor around tests and test coverage. Since we already know there will be MethodDesc-related follow-ups (and this already made it through PR builds), I'm entirely fine with this going as-is. |
Co-authored-by: Elinor Fung <[email protected]>
This was referenced Oct 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RangeSectionMap is a data structure for coarse-grained lookups to man native code pointers to managed method descriptors that cover the entire addressable memory.
This PR adds unit tests for the cdac reader RangeSectionMap lookup algorithm and some unit tests for the ExecutionManager contract APIs that use a RangeSectionMap and NibbleMap together to do a full TargetCodePointer->MethodDesc lookup
Contributes to #108553