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] start of RuntimeTypeSystem contract; implement GetMethodTableData SOS method #103444

Merged
merged 72 commits into from
Jul 8, 2024

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Jun 13, 2024

Contributes to #99302

Also updates the existing DAC so that it does not store an EEClass* in DacpMethodTableData::Class, but instead stores a pointer to the canonical method table. Correspondingly GetMethodTableForEEClass now expects a canonical MethodTable.

Also renamed MethodTable::ContainsPointers to ContainsGCPointers, (and corresponding flag and setter method); also updated gcenv for NativeAOT with the same renaming.

Also changed the existing DAC so that DacpMethodTableData::wNumVirtuals and DacpMethodTableData::wNumVtableSlots are always returned as 0. The cDAC Metadata contract does not expose the underlying info

verified that it works for at least one MethodTable by running some manual WinDbgX test

TODO:

  • write the contract in docs/
  • unit tests

Copy link
Contributor

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

@lambdageek
Copy link
Member Author

lambdageek commented Jun 14, 2024

Need to reconcile with #99183 - I was on an older branch


update updated and fixed the merge issues. Validated that GetMethodTableData is still functional

@lambdageek lambdageek marked this pull request as draft June 14, 2024 14:32
lambdageek and others added 6 commits July 2, 2024 15:22
1. rename AttrClass data descriptor field to CorTypeAttr
2. fixup HasComponentSize / RawGetComponentSize comments and code
3. update "system.object" mock methodtable with more field values
4. update "system.string" mock methodtable with more field values
@lambdageek
Copy link
Member Author

I was asked to rename the contract from Metadata to something else because CoreCLR doesn't really use "metadata" to talk about the execution-time type identity of managed objects. The obvious alternative name for the contract is MethodTable, but I'm open to other options.

@jkotas
Copy link
Member

jkotas commented Jul 2, 2024

Naming suggestion: TypeSystem or RuntimeTypeSystem

@AaronRobinsonMSFT
Copy link
Member

Naming suggestion: TypeSystem or RuntimeTypeSystem

I prefer RuntimeTypeSystem.

@lambdageek lambdageek changed the title [cdac] start of Metadata contract; implement GetMethodTableData SOS method [cdac] start of RuntimeTypeSystem contract; implement GetMethodTableData SOS method Jul 3, 2024
private readonly Target _target;
private readonly TargetPointer _freeObjectMethodTablePointer;

// TODO(cdac): we mutate this dictionary - copies of the Metadata_1 struct share this instance.
Copy link
Member

Choose a reason for hiding this comment

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

Support for invalidating our view of memory and re-reading to fill in the world should be a pretty soon now project, but I'm ok with having checking in the current state here, and fixing it later when we add that support.

@lambdageek
Copy link
Member Author

I now have unit tests for MethodTable validation and access for things that look like: System.Object, System.String, a methodTable with a garbage EEClass, a generic instance (and its generic definition), an array of some valuetype. This covers a variety of paths throughValidateMethodTablePointer.

Doing more testing is difficult until we have more query methods and data descriptors to mock something more interesting.

I'd like to finish up with this PR and get into expanding the contract for the next SOS API (most likely GetMethodTableName)

@lambdageek lambdageek merged commit e336326 into dotnet:main Jul 8, 2024
151 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants