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.
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
[cdac] start of RuntimeTypeSystem contract; implement GetMethodTableData SOS method #103444
Changes from 36 commits
b98afa8
890f9c6
41ba95c
29214f0
8beced0
5213286
622e01a
e112416
ae1eac9
79ea0d4
5c696da
6eaf80f
3a7808d
66e5476
95914b8
2b8fda3
5c7d2ac
7be5c60
30b7b26
187bcbe
f0d1fcb
c53db36
b70bb1d
a65fd50
6f844bf
76e0384
cdb7543
78830ea
fbbd45b
32665b2
acf8436
f246c86
8409f3e
08d069a
674655f
2ae4625
e18a2be
846e779
383af83
0f8c7f1
7a337c1
a7c8158
f4a3493
10624c7
65cc531
d526087
0a4112e
1071ca4
6c5235c
95b728a
6573e14
8596892
6eabf42
4d3200d
2e66740
8533148
77cf405
f9bce4c
3721992
1af7c80
993ae1d
f04d880
76859d1
a12a407
9cf4c5a
1814848
89f98a3
a0989fa
617bf62
815ff0d
1ab4f08
ee3a362
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would having a singe API for getting slot counts (or maybe also method/field counts) be reasonable, or do you think keeping everything separate makes more sense? I'm wondering if there are any logical groupings that work for all the
Get
/Is
APIs on here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would question the diagnostic tools need for these numbers. These numbers go into the MethodTable data layout details. Are the number of slots actually needed by anything else except some SOS command printing them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this a little bit in our standup. The explanation was that some of these fields, while they're only used for SOS command output are important for a number of reasons:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry I was not able to make it to the standup today.
I do not see a problem with that. The screen scrapping done by 3rd party tools should not prevent us from making the SOS better.
The main value of SOS dump commands - for both platform bring up and general debugging - is in displaying information that is hard (labor intensive) to get manually via regular windbg/lldb commands. Type name is an example of such information.
NumVtableSlots is easy to get using regular windbg/lldb commands, only needed to interpret data layout of the methodtable, and rarely used for low-level debugging. It does not need to be part of Dump... output. I would even say that Dump... would be better without it.. We should be free to delete it to make SOS better. dotnet/coreclr#16560 or dotnet/diagnostics#4751 are examples of changes like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this in the long term and how we should frame the ultimate outcome of the cDAC workstream. For these initial phases though I tend to consider them more proving this can be done in a seamless and transparent manner. This implies ensuring we maintain the illusion of the static nature should be a priority until we can announce changes in this space and start to innovate. Basically, for now I think we should match precise output unless there is a compelling or hard-blocking reason to deviate from that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an initial goal, I would've said "yes, we should stick to avoiding breaking changes in this space". However, it seems like that ship has already sailed. I want to innovate in this space a great deal so as an end goal I agree. My concern was and is the burden this can place on automation that relies on scapping. We end up encouraging odd solutions as people try to anticipate or adapt to our "infight work". Since it seems that isn't a current goal, I agree then that we should remove as much cruft as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps there should be an SOS query that can help scrapping tooling know what they can expect? Is a new SOS command along the lines of
!sos.version
appropriate in this case? I truly don't know how onerous this is so I could be concerned about something that has limited impact. Perhaps @leculver can shed some light on a version command option, if one doesn't already exist.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the current PR to always set
DacpMethodTableData::wNumVirtuals
andDacpMethodTableData::wNumVtableSlots
to 0 in both the cDAC and the Legacy DAC. Updated the Metadata contract to exclude getters for this info.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related change in dotnet/diagnostics to not print the counts if they're 0 dotnet/diagnostics#4760
I checked and https://github.com/Microsoft/clrmd doesn't use these fields. They only look at
wNumMethods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this ping.
My guiding principle is: When possible, and when it doesn't cause too much work, don't regress or take back features that affect debugging. That includes .Net Core and Desktop Framework (since we can debug 4.5+ desktop framework with current SOS).
That's my only real request.
I don't think we need that. The output of SOS does not constitute an interface. I've been saying this for 15 years and will die on this hill. :)
We shouldn't bend over backwards to accommodate screen scraping. Literally the original reason ClrMD was built was to replace screen-scraping SOS. We don't have all functionality built into ClrMD, but the SOS source is open-source and folks can reimplement functionality from there if they need to.
I haven't fully dug into everything here, but I'm fine with these changes. I'd honestly be fine if folks wanted to randomize the output of SOS to intentionally break screen scraping. (Hyperbole. Obviously, we would never actually do that.) I owned SOS 15 years ago and built a lot of its functionality. Screen scraping was the bane of my existence because we couldn't do anything to SOS in the presence of screen scraping. Just use ClrMD if you need that programmatic access! No screen scraping!