-
Notifications
You must be signed in to change notification settings - Fork 385
Updated gcdecoder from main #5369
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
Conversation
|
Prior to this change doing Same with After this PR with |
|
@mikem8361 to take a closer look from the SOS side @VSadov - SOS needs to work with both new and old versions of .NET and it wasn't clear to me if the current PR handles that? I noticed over on #4795 that discussion around that was already happening so great 👍 |
|
and now the new SPS.dll can also decode gcinfo in a The same |
|
Looks like an unrelated failure: |
|
I think the change has all that was planned. Please take a look. |
| bool GcInfoDecoder::IsInterruptibleSafePoint() | ||
| { | ||
| return IsSafePoint() && AreSafePointsInterruptible(); | ||
| } |
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.
These were helpers for where we can suspend and where we cannot (it all got simpler now).
This does not need to be shimmed as this has no effect on gcinfo dumper.
src/shared/inc/gcinfotypes.h
Outdated
| #define CODE_OFFSETS_NEED_NORMALIZATION 1 | ||
| #define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 2/4 bytes long in Thumb/ARM states, | ||
| #define DENORMALIZE_CODE_OFFSET(x) (x) // but the safe-point offsets are encoded with a -1 adjustment. | ||
| #define NORMALIZE_CODE_OFFSET(x) ((x)>>1) // Instructions are 2/4 bytes long in Thumb/ARM states, |
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.
These macros were changed to methods in dotnet/runtime main. Do you plan to pick up that update separately?
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.
Must have happened recently. I used fairly recent baseline, but perhaps not the latest.
I am thinking to open a parallel PR in runtime with same files content, to make sure we do not need to shuffle changes back and forth.
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 thinking to open a parallel PR in runtime with same files content,
Yes, I think that's a good idea.
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.
Opened dotnet/runtime#113888
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.
Also re-tested in WinDbg if !gcinfo still works after updating from runtime/main, with both 8.0 and 10.0
That does not want to work at the level of GC Info Decoder. I noticed that all the calls to this in at the We do not need .NET FX detection in the Core repo anyways. As long as decoder specialcases V1 and V1 is set somehow for .Net FX, this should work as before. |
|
I am not sure how to test .NET FX support in SOS though. The same thing as I try for Core (i.e. load SOS.dll in WinDbg and examine some methods) does not work for me. Same error happens with the SOS.dll from I do not think this is related to the change. Maybe I am using this incorrectly... (I am not a frequent user of SOS) |
|
Figured why I could not test. .Net FX defaults to 32bit and thus I had a bitness mismatch. Also I found that the change that supposedly "fixes" The reason is that the switch from GC Info v1 --> GC info v2 seems like happened in .Net FX times. The NDP sources that I have (which are by themselves very old) have Since R2R is a CoreClr thing, does this mean that GCInfo v1 is effectively dead - either on .Net FX or on CoreCLR ? |
Sounds like it. |
|
I will revert to the commit that does not handle |
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
|
@mikem8361 The runtime part of this change is ready to go. Please take one more look from diagnostics side. |
|
Thanks!! |
|
Looks like I do not have permissions to merge in this repo, so someone else should merge. |
Fixes: #4795
!gcinfowith bothnet10.0andnet8.0.