-
Notifications
You must be signed in to change notification settings - Fork 5k
Interpreter GC info stage 2: Generate empty GC info and add baseline implementation of EnumGcRefs #113948
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
kg
commented
Mar 26, 2025
•
edited
Loading
edited
- Modify GcInfoEncoder and its sibling files so that they can be successfully linked into the interpreter module; added new gcinfohelpers.h file with support code and macros
- There are a couple load-bearing asserts inside the encoder that need to be evaluated for safemath to not explode in debug builds of crossgen2.
- Add new InterpreterGcInfoEncoder and InterpreterGcInfoDecoder
- Link gcinfo into the interpreter so the encoder and decoder are available
- At the end of interpreter method compilation, use InterpreterGcInfoEncoder to generate gc info for the method. EnumGcRefs will fail if we don't do this. We need to specifically do this at the end after the code header has been allocated, we can't do it earlier.
- Add placeholder implementation of EnumGcRefs for the interpreter, modeled on the existing one for the JIT.
- Add placeholder interp opcode that triggers a GC, so that we can easily test gcinfo and stackwalking in various interpreter scenarios.
Tagging subscribers to this area: @mangod9 |
01ff252
to
861cb52
Compare
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.
Pull Request Overview
This PR introduces baseline implementations for GC info generation and decoding in the interpreter, laying the groundwork for EnumGcRefs and improved GC info encoder/decoder functionality. Key changes include disabling some logging/assertions temporarily in GC info encoding, adding new InterpreterGcInfoEncoder/Decoder, and triggering a GC in the interpreter tests for stackwalking validation.
Files not reviewed (16)
- src/coreclr/gcinfo/arraylist.cpp: Language not supported
- src/coreclr/gcinfo/gcinfodumper.cpp: Language not supported
- src/coreclr/gcinfo/gcinfohelpers.h: Language not supported
- src/coreclr/inc/bitposition.h: Language not supported
- src/coreclr/inc/gcinfodecoder.h: Language not supported
- src/coreclr/inc/gcinfoencoder.h: Language not supported
- src/coreclr/inc/gcinfotypes.h: Language not supported
- src/coreclr/interpreter/CMakeLists.txt: Language not supported
- src/coreclr/interpreter/compiler.cpp: Language not supported
- src/coreclr/interpreter/compiler.h: Language not supported
- src/coreclr/interpreter/eeinterp.cpp: Language not supported
- src/coreclr/interpreter/intops.def: Language not supported
- src/coreclr/vm/codeman.cpp: Language not supported
- src/coreclr/vm/eetwain.cpp: Language not supported
- src/coreclr/vm/gcinfodecoder.cpp: Language not supported
- src/coreclr/vm/interpexec.cpp: Language not supported
There are some Interpreter-FIXMEs/Interpreter-TODOs left in here that I still haven't figured out what to do with, so thoughts on those are appreciated. In particular I don't love how I ended up solving various assertion-related problems here. I've manually verified that when the interpreter test performs a GC collection, the stackwalker visits each interpreted frame and uses the gc info decoder to see that there are no gc refs on the stack. |
5bddd7f
to
679f0b3
Compare
36fc0bf
to
25431c6
Compare
|
The checked lanes that get through build are failing on the interp tests, I'm not really sure I understand the error though:
Any ideas? It doesn't fail locally for me in a debug build, I'm assuming something is different about Checked |
This looks like the InterpreterFrame was skipped for some reason. The check that asserts is not valid for the InterpreterFrame, but I am not sure why it is declared as skipped. |
I test on win11 with debug builds. I assumed debug builds would have all the possible checks enabled, do I need to test in checked builds instead? It works both from the command line and in the debugger. |
Yes, that's correct for Windows |
a6296f4
to
212c3bb
Compare
Is there an intended way to disable our weird "return" instrumentation? I'm running into it again, this time it breaks
MSVC doesn't seem to mind, but it consistently breaks the linux build. |
I can verify that the interpreter tests fail locally in Checked configuration and not Debug. So it's something specific to the Debug configuration that makes it work, and isn't happening in Checked builds. I'll poke around but I'm not really sure what's up there. EDIT: Even more interesting, it's sufficient to run a checked version of the test using a debug corerun. |
It looks like for some reason we're building the interpreter module for x86, even though it won't work there, and it's failing to link as a result:
My guess is this is because I feature flag guarded the interpreter gc info decoder/encoder. Should I not feature guard them? |
@jkoritzinsky has been looking into deleting it. |
My guess is that the problem is caused by gcdumpx86 being pulled into clrinterpreter. Nothing in clrinterpreter should need the dumper of the special x86 gc info. I would not bother with x86. I do not think we will spend time on making interpreter work on x86. |
The CLR_Tools_Tests failure looks like an existing problem and not me:
|
Yes, it has been failing on every PR since yesterday. I have created an issue for it, but the CI build analysis somehow cannot see it. |
59b313f
to
72c2433
Compare
Thanks, I think I see what you mean by this and am making the relevant changes. Sorry for missing the nuance earlier when you described how to fix this :) |
I noticed I forgot to initialize gInterpJitInfoTls; I'll push a commit that fixes that once I see what happens with this latest CI run. |
dd18187
to
4d71c55
Compare
Looks like someone modified gcinfoencoder.cpp so I have a complex manual merge ahead of me. |
There weren't that many changes, mostly deletes. So I guess taking your changes and applying the changes from that commit manually might be the least painful way. |
Yeah I think that's the right approach in this case, I just need to be careful not to lose any of the changes. |
Forward System.GC.Collect calls in interpreter to a dedicated opcode that invokes the GC so we can test interpreter stack walking Repair merge damage Checkpoint Do horrible things to make it possible to link against gcinfo from the interpreter Fix release build Generate empty GcInfo for interpreter methods when they are compiled so we don't crash when trying to EnumGcRefs Change GCINFO_LOG signature to make it more feasible to implement it Make GCINFO_ASSERT call _DbgBreak on assertion failure in debug builds (better than nothing) Cleanup/restore some disabled assertions Define GCINFOENCODER_ASSERT for assertions within TGcInfoEncoder that will use ICorJitInfo::doAssert on assertion failure to flow the assertion failure info through Move the GC.Collect Undo unnecessary changes Comment cleanup Build fixes Different approach to nomem noreturn since what I tried didn't cut it Fix GcInfoEncoder assertion due to zero code length Remove _DbgBreak dependency in gcinfohelpers.h Remove assert(false) from Interp_NOMEM Define a mempool new operator for InterpCompiler like the one in the JIT and use it to allocate the GcInfoEncoder Throw std::bad_alloc from Interp_NOMEM instead of trapping Disable debugreturn.h in compiler.cpp to fix linux build Disable debugreturn further up to fix linux build (I'm not sure why this is suddenly necessary) Attempt to disable linking against gcinfo on x86 Try just disabling the interpreter folder if the feature flag isn't set Assertion macro changes to address PR feedback Repair refactoring damage Address PR feedback GCINFOENCODER_ASSERT -> _ASSERTE Revert "Address PR feedback" This reverts commit 8727000. Address PR feedback - centralized _ASSERTE for gcinfo and interp that relies on assertAbort Add assertAbort implementations in dac and interpreter Remove gcinfo dependency from coreclr and add it to apphost instead Fix release build Try to fix cdecl build issues on CI Address PR feedback Actually initialize gInterpJitInfoTls Add missing cdecl for x86 No longer make assertAbort __cdecl Address PR feedback: partial debugmacros revert Revert "Address PR feedback: partial debugmacros revert" This reverts commit dae3b11.
9786c47
to
1eab486
Compare
Does anyone have objections to landing this so part 3 can leave draft and get reviewed? Happy to make more changes but I think I've addressed everything. |
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.
LGTM, thank you!
/ba-g known test issue |