Interpreter GC info stage 3: Report locals to the GC #114469
Open
+434
−41
Conversation
This file contains hidden or 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
|
Tagging subscribers to this area: @mangod9 |
filipnavara
reviewed
Apr 12, 2025
BrzVlad
reviewed
Apr 14, 2025
BrzVlad
reviewed
Apr 14, 2025
|
Rebased and updated to report byrefs. Tweaked the test to explicitly use a ref local. |
This was referenced Apr 19, 2025
Open
BrzVlad
reviewed
Apr 19, 2025
|
Initial pass at handling structs containing refs. Will need to refactor it to not rely on lambdas. |
This was referenced Apr 24, 2025
jkotas
reviewed
Apr 28, 2025
| eeinterp.cpp) | ||
| eeinterp.cpp | ||
| stackmap.cpp | ||
| ../../native/containers/dn-simdhash.c |
There was a problem hiding this comment.
As discussed in offline conversation, it would be better to start reusing code from the regular JIT. There appears to be a quite a bit of code that can be shared between regular JIT and the interpreter JIT.
There was a problem hiding this comment.
I'll create a 'jitshared' and move JitHashTable into it and use that, then.
There was a problem hiding this comment.
Moving JitHashTable is looking very time intensive, could I do it in a follow-up PR?
BrzVlad
reviewed
May 2, 2025
BrzVlad
reviewed
May 2, 2025
BrzVlad
reviewed
May 2, 2025
BrzVlad
reviewed
May 2, 2025
|
Yeah, I originally thought I needed to recursively build the map for nested
types. I'll clean it up, good catch.
…-kg
On Fri, May 2, 2025, 03:08 Vlad Brezae ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/coreclr/interpreter/stackmap.cpp
<#114469 (comment)>:
> + }
+ return result;
+}
+
+void InterpreterStackMap::PopulateStackMap(ICorJitInfo* jitInfo, CORINFO_CLASS_HANDLE classHandle, uint32_t offset)
+{
+ unsigned size = jitInfo->getClassSize(classHandle);
+ unsigned maxGcPtrs = (size + sizeof(void *) - 1) / sizeof(void *);
+ if (maxGcPtrs < 1)
+ return;
+
+ uint8_t *gcPtrs = (uint8_t *)alloca(maxGcPtrs);
+ unsigned numGcPtrs = jitInfo->getClassGClayout(classHandle, gcPtrs),
+ newCapacity = m_slotCount + numGcPtrs;
+
+ m_slots = (InterpreterStackMapSlot *)realloc(m_slots, sizeof(InterpreterStackMapSlot) * newCapacity);
Seems to me that for every class we create a stack map that is populated
here and m_slots is initialized only once. I don't understand why we are
using realloc ? Could we use malloc just to avoid confusion ?
—
Reply to this email directly, view it on GitHub
<#114469 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABQL4X2QJGOIB5QK2VS7RT24M73DAVCNFSM6AAAAAB22RCDLSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMJRGY2DONRZGQ>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
janvorli
reviewed
May 2, 2025
kg
force-pushed
the
interp-gcinfo-3
branch
2 times, most recently
from
68b0a27 to
4f56fef
Compare
jkotas
reviewed
May 2, 2025
…ss ranges Use ConvertOffset and live range helpers to record actual byte offsets Fix globals not being reported Specialize GetStackSlot for the interpreter because we stash the actual stack location in the frame register Doing aggressive blocking GC produces earlier, more consistent failures Calculate stack slot addresses correctly, at least on x64 Disable problematic GC in the test for now Better debug spew and adjust test Fix nativeAOT build on CI Implement additional architectures in GetStackSlot based on CONTEXTGetFp Maybe fix CI build by using TARGET instead of HOST Updated guesses at fp register / more architectures Report and explicitly test byrefs Refactoring Maintain separate slot tables for byref and non-byref locals Handle allocation failure in simdhash Checkpoint support for byref and ref fields inside VT interpreter locals Add test coverage for structs containing refs Match getClassGClayout's size calculation Remove use of lambdas Fix release build Fix linux build Fix linux build more Use GetFP to get the frame pointer in interpreter's GcInfoDecoder Formatting cleanup Patch initlocals once we have allocated offsets so that we zero our vars in addition to IL vars (GC safety) Preserve effect of not initializing IL locals even when we expand the initlocals Introduce globalVarsStackTop, the high water mark for global vars that might contain refs or interior pointers Use globalVarsStackTop to reduce how much stack we zero on method entry Formatting cleanups Repair damage to simdhash files
This was referenced May 6, 2025
BrzVlad
reviewed
May 6, 2025
| { | ||
| int32_t endOfVar = pVar->offset + pVar->size; | ||
| if (endOfVar > globalVarsStackTop) | ||
| globalVarsStackTop = endOfVar; |
There was a problem hiding this comment.
This name is a bit misleading, since it is the top of the global vars that might contain refs, and not the end of all global vars. Maybe a more explicit name, like globalVarsWithRefsStackTop or some other name that you think is good.
BrzVlad
reviewed
May 6, 2025
| pVar->global && ( | ||
| (pVar->interpType == InterpTypeO) || | ||
| (pVar->interpType == InterpTypeByRef) || | ||
| (pVar->interpType == InterpTypeVT) |
There was a problem hiding this comment.
If you want you could improve the check here for valuetypes with m_compHnd->getClassAttribs(pVar->clsHnd) & CORINFO_FLG_CONTAINS_GC_PTR
BrzVlad
reviewed
May 6, 2025
| { | ||
| // We may have global vars containing managed pointers or interior pointers, so we need | ||
| // to zero the region of the stack containing global vars, not just IL locals. | ||
| // data[0] is either the start of IL locals or the end of IL locals depending on whether local |
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.
This updates the interpreter compiler to use
GcInfoEncoderto allocate slot IDs for interp vars and then report their liveness ranges (unless they're globals).I template-specialized the implementation of
TGcInfoDecoder<InterpreterGcInfoEncoding>::GetStackSlotso that it knows how to find the interpreter stack and look up slots in it, since the default implementation isn't compatible with the way we store the interpreter SP.I've verified that when a GC happens inside TestFields, the object reference(s) on the interpreter stack are reported to the GC, and the GC is able to successfully move them and update the stack.
I had to modify the way we do INITLOCALS to ensure that any global vars containing pointers are zeroed at method entry, because we don't have accurate liveness information for them to report, otherwise the GC might see garbage data where it expects a managed pointer.
This PR also adds some basic test coverage for structs containing object references.