[clr-interp] Tweak GC reporting

[davidwrighton]

The interpreter calling convention can result in double reporting of arguments passed to callee's. This works today as all of the arguments will be conservatively reported, but it also adds the problem of excess conservative reporting. This change fixes that most of that problem by only dropping reporting of conservative pointers when they point into a callee's stack space.

Note that this does not fix is the scenario where an object reference is held on the IL stack across a call. In those cases the value shall still be reported conservatively. This PR also tweaks the Collect0 test to disable it under the interpreter as the C# compiler generates that particular problematic pattern for this test case.

[Copilot]

Pull request overview

This PR addresses GC reporting issues in the CoreCLR interpreter by reducing excess conservative reporting of arguments. The main change optimizes GC stack slot reporting by detecting when a stack slot belongs to a callee's frame rather than the caller's frame, avoiding double reporting. Additionally, a test is disabled under the interpreter due to a known issue with object references held on the IL stack across calls.

Key Changes

• Modified GC stack slot reporting to skip conservatively reported slots when they point into a callee's stack space
• Added logic to detect callee frame boundaries using InterpMethodContextFrame
• Disabled Collect0 test under the interpreter due to incompatible C# compiler output patterns

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/tests/GC/API/GC/Collect0.cs	Adds ActiveIssue attribute to disable test under CoreCLR interpreter
src/coreclr/vm/gcinfodecoder.cpp	Implements GC reporting optimization for interpreter frames by detecting and skipping slots in callee frames

[dotnet-policy-service]

Tagging subscribers to this area: @BrzVlad, @janvorli, @kg
See info in area-owners.md if you want to be subscribed.

[BrzVlad]

Why exactly do we report this dummy ref ? Shouldn't we just skip reporting this slot altogether ? It will still end up being reported precisely from the callee's frame, right ?

[davidwrighton]

This is somewhat convoluted.

1. We don't actually report anything from this function. We've already decided we're going to try to report something by the time we get here as the GC info format doesn't have a concept of something that won't be reported (although the scratch slots concept is similar but not ... quite... right for the interpreter calling convention.)
2. There are only 2 functions which can call this function, and I've modified the only one which is used by the interpreter to handle this case specially and just return.

However, using a weird global variable to represent this is ... an odd choice of mine. I should really just return NULL, and use NULL as a sentinel to mean, this stack slot shouldn't be used.

[davidwrighton]

Ok, I've made the change I suggested to use NULL instead. Looks cleaner and more obvious to me.

[BrzVlad]

So the purpose of this change is just to prevent the GC from redundantly pinning some refs. I believe this change is ok for achieving this objective. My comment was mainly related to the fact that the GC shouldn't even get to this place of trying to obtain a ref from this slot. Initially we planned to precisely report refs like this (I think tracked var was the terminology) and, at the moment of a call, during compilation, we would know that the var in question stops being alive, with the responsability of reporting falling to the callee. The gcinfo would store the end liveness for this var so we shouldn't even get to this API during GC.

My understanding was that this wasn't working as expected and we had to switch to conservative reporting. Assuming all I said above is correct, I believe we should look into fixing this at some point and I would at least add a comment here that this is a "temporary" workaround for an issue with the interpreter gcinfo.

[davidwrighton]

So, the problem is that the call instruction may need to execute significant code that might trigger a GC before it actually calls to the target function. (In addition, there are some weird cases like, we may need to change a value on the stack from a regular GC pointer to a byref for the unboxing case, etc. The GC reporting format isn't really designed to deal with individual instructions that actually have different GC states.

The current scheme utilizes conservative gc reporting to avoid these problems, and I'm not at all convinced we should try to "fix" it. It turns out that only reporting these value conservatively reduces the amount of gc info that we need to produce, which improves interpreter compiler performance, and with this change the amount of conservative reporting is kept to a bare minimum, and almost always isn't excess. In addition, if we needed to track reportable gc info precisely, we'd need to ensure that the ip of a frame always moves exactly at the right moment, and I don't actually know how to define what that moment might be. I think the fix there is to end up in a place where the InterpMethodContextFrame needs to have 2 different ips, one for use in reporting when at the top of the stack, and for returning to the function, and also a "gc" ip which indicates which instruction address should be used for GC reporting. I'll need to think on it more. Maybe I can implement exact gc reporting early next year.

[BrzVlad]

I'm wondering whether the basic issue is just that we bump the pFrame->ip to soon, before actually doing the call. If we have places where we do mutate the stack contents in the middle of instructions then that would make it awkward to fix indeed. It is not clear to me how common this scenario is, I would imagine it is fixable.