[clr-interp] Fix conservative reporting data in gc enumeration

[davidwrighton]

I found that if the conservative reference pointed into the GC it caused problems in the relocation logic in the GC. For all forms of pinned references, the relocation phase isn't necessary, so add a check for it and avoid the problem.

[Copilot]

Pull Request Overview

This PR fixes a bug in the garbage collection (GC) enumeration for the CLR interpreter by preventing inappropriate promotion of conservative roots during the relocate phase.

• Adds conditional logic to skip promotion for pinned objects during relocate phase
• Restructures the existing interior pointer handling into clearer conditional blocks
• Prevents conservative reporting from incorrectly reporting roots during GC relocate phase

[davidwrighton]

/azp run runtime-interpreter

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[dotnet-policy-service]

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

[kg]

LGTM

[davidwrighton]

@jkotas could you provide an opinion here? I think that this is correct, but I'd be interested to see if you think we shouldn't have such a change outside of the FEATURE_INTERPRETER path. Notably, this is NOT localized to the interpreter codebase. It will change all pinned pointers to not be reported during the relocate phase of gc scanning.

[jkotas]

It looks correct to me.

Alternatively, it can be checked on the GC side as the first thing in Relocate callback. It would save us from checking the bit during promotion. The stackroot reporting is relatively expensive - it is unlikely to make a material difference.

If we keep it here, we may want to make the same change in NAOT clone of this method for consistency:

runtime/src/coreclr/nativeaot/Runtime/GcEnum.cpp

Lines 68 to 82 in 6246c4d

	static void GcEnumObject(PTR_PTR_Object ppObj, uint32_t flags, ScanFunc* fnGcEnumRef, ScanContext* pSc)
	{
	//
	// Sanity check that the flags contain only these values
	//
	assert((flags & ~(GC_CALL_INTERIOR | GC_CALL_PINNED)) == 0);
	// for interior pointers, we optimize the case in which
	// it points into the current threads stack area
	//
	if (flags & GC_CALL_INTERIOR)
	PromoteCarefully(ppObj, flags, fnGcEnumRef, pSc);
	else
	fnGcEnumRef(ppObj, pSc, flags);
	}

[davidwrighton]

@jkotas, I decided against moving the check to GCHeap::Relocate, since that function is potentially called on somewhat more expensive constructs like promoting the background roots and such. Again, not something that is likely a problem, but stack root marking is definitely expensive enough that a single check isn't going to break the world.