Skip to content

Conversation

davidwrighton
Copy link
Member

Adjust interpmode to automatically work in a few more ways, so that we can run a larger test run with InterpMode=3

- We automatically detected that intrinsics should throw PNSE, but we didn't correctly handle the IL stack in those cases
- StackMark handling relies on comparing the stackmark's address to the stack pointer, but the interpreter stack is seperate.
- Fix this by converting the StackMark pointer to a pointer on the OS stack and then do the compares
- Open virtual delegates have an entrypoint which cannot be processed by NonVirtualEntry2MethodDesc. Handle them specially.
@Copilot Copilot AI review requested due to automatic review settings September 30, 2025 23:57
@davidwrighton
Copy link
Member Author

/azp run runtime-interpreter

@davidwrighton davidwrighton marked this pull request as draft September 30, 2025 23:57
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a 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 adjusts the interpreter mode testing to enable more comprehensive testing with InterpMode=3. The changes modernize the interpreter configuration system and add better support for interpreter compatibility with various system features.

Key changes include:

  • Switching from assembly-specific interpreter configuration to a global InterpMode=3 setting
  • Adding interpreter stack mark conversion functionality for proper stack walking
  • Implementing compatibility adjustments between interpreter mode and hardware intrinsics/ReadyToRun

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/tests/Common/CLRTest.Execute.Batch.targets Replaces assembly-specific interpreter config with global InterpMode=3
src/tests/Common/CLRTest.Execute.Bash.targets Replaces assembly-specific interpreter config with global InterpMode=3
src/coreclr/vm/stackwalk.h Adds function declaration for stack mark conversion
src/coreclr/vm/stackwalk.cpp Implements stack mark conversion for interpreter frames
src/coreclr/vm/reflectioninvocation.cpp Updates SkipStruct to handle interpreter stack marks properly
src/coreclr/vm/jitinterface.cpp Simplifies interpreter loading logic
src/coreclr/vm/jithost.cpp Adds EnableHWIntrinsic configuration support
src/coreclr/vm/interpexec.cpp Improves delegate invocation handling for open virtual dispatch
src/coreclr/vm/eeconfig.h Adds interpreter and hardware intrinsic configuration fields
src/coreclr/vm/eeconfig.cpp Implements interpreter mode configuration logic
src/coreclr/vm/codeman.cpp Adds interpreter mode compatibility with hardware features
src/coreclr/vm/appdomain.cpp Updates CallersDataWithStackMark for interpreter compatibility
src/coreclr/interpreter/eeinterp.cpp Updates interpreter mode documentation comments
src/coreclr/interpreter/compiler.cpp Fixes PlatformNotSupportedException intrinsic handling


pFrame = pFrame->PtrNextFrame();
}

Copy link
Preview

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion message should be more descriptive and include context about what stackMark value was being processed to aid debugging.

Suggested change
LOG((LF_ALWAYS, LL_ALWAYS, "Unable to find InterpMethodContextFrame for stackMark=%p (stackLimit=%p, stackBase=%p)\n", stackMark, pThread->GetCachedStackLimit(), pThread->GetCachedStackBase()));

Copilot uses AI. Check for mistakes.

Comment on lines +696 to +704
SkipStruct(StackCrawlMark* mark, PTR_Thread thread) :
pStackMark(mark)
#ifdef FEATURE_INTERPRETER
// Since the interpreter has its own stack, we need to get a pointer which can be compared on the real
// stack so that IsInCalleesFrames can work correctly.
, stackMarkOnOSStack(ConvertStackMarkToPointerOnOSStack(thread, mark))
#endif
{
}
Copy link
Preview

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The constructor initializer list formatting is inconsistent. The comma should be aligned with other initializers, not placed at the beginning of line 701.

Copilot uses AI. Check for mistakes.

enableHWIntrinsic = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_EnableHWIntrinsic);
if (CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_InterpMode) >= 3)
{
// R2R mode 3 disables all hw intrinsics
Copy link
Preview

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment mentions 'R2R mode 3' but should refer to 'InterpMode 3' for clarity, as this is about interpreter mode, not ReadyToRun mode.

Suggested change
// R2R mode 3 disables all hw intrinsics
// InterpMode 3 disables all hw intrinsics

Copilot uses AI. Check for mistakes.


DELEGATEREF delegateObj = LOCAL_VAR(callArgsOffset, DELEGATEREF);
DELEGATEREF* delegateObj = LOCAL_VAR_ADDR(callArgsOffset, DELEGATEREF);
NULL_CHECK(delegateObj);
Copy link
Preview

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL_CHECK is being called on a pointer to DELEGATEREF rather than the DELEGATEREF itself. This should be NULL_CHECK(*delegateObj) or the logic should be restructured to check the actual delegate reference.

Suggested change
NULL_CHECK(delegateObj);
NULL_CHECK(*delegateObj);

Copilot uses AI. Check for mistakes.

@davidwrighton
Copy link
Member Author

/azp run runtime-interpreter

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

bool allowHWIntrinsic = true;

#if defined(FEATURE_INTERPRETER)
if (maxVectorTBitWidth != 128 && CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_InterpMode) >= 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be GetConfigValue(CLRConfig::EXTERNAL_InterpMode) == 3?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants