Skip to content

Conversation

kg
Copy link
Member

@kg kg commented Aug 19, 2025

When I run JIT\directed\tailcall\more_tailcalls in the debugger it crashes in this helper because pCallStub is NULL. I'm not sure why it's only broken in the debugger. I think @kotlarmilos has hit this problem before, this is not my first time seeing it. It manifests as an AV reading the address 0x10.

EDIT: This PR disables tiered compilation if DOTNET_Interpreter or DOTNET_InterpMode are in use.

Copy link
Contributor

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

@kotlarmilos
Copy link
Member

When I run JIT\directed\tailcall\more_tailcalls in the debugger it crashes in this helper because pCallStub is NULL. I'm not sure why it's only broken in the debugger. I think @kotlarmilos has hit this problem before, this is not my first time seeing it. It manifests as an AV reading the address 0x10.

I encountered this issue only in the Checked configuration. It occurs when pCallStub is null and the routines offset is 0x10:

ldr x9, [x9, #OFFSETOF__InterpMethod__pCallStub]
add x10, x9, #OFFSETOF__CallStubHeader__Routines

@kg
Copy link
Member Author

kg commented Aug 20, 2025

RE: Comments so far:

  • I don't see any other thread activity involved in the creation/non-creation of this pCallStub
  • No other threads are running when it fails, so I don't think a lock would help
  • Call stub creation is already done atomically in a way that looks race safe
  • I checked and we didn't lose a race here, pCallStub is still NULL when I check its value in the watch window

It seems like we're somehow getting an InterpMethod without a pCallStub when we need it to have one. And it sounds like it's not okay to generate a callstub in advance for every method (jkotas's reason for this makes sense). I think this means I need to make the InterpreterStub lazily instantiate the pCallStub as needed. Any ideas for other solutions?

@kg
Copy link
Member Author

kg commented Aug 20, 2025

It occurred to me to try setting DOTNET_TieredCompilation=0 and that fixes it.

EDIT: Evidence via tracepoints:

27352: Compiling 0x0000024d007c3d30 "Instance:CalcInstance(int,int)"
27352: Compiling 0x0000024d00857c00 "Instance:CalcInstanceOther(int,S32,int)"
34908: Compiling 0x0000024d00857e80 "Program+<>c__DisplayClass7_1`1[int]:<Main>b__31()"
The thread '.NET Tiered Compilation Worker' (34908) has exited with code 0 (0x0).
27352: Compiling 0x0000024d00858480 "Instance:CalcInstanceRetbuf(int,int)"
27352: Compiling 0x0000024d00858480 "Instance:CalcInstanceRetbufOther(int,S32,int)"

@jkotas
Copy link
Member

jkotas commented Aug 20, 2025

Does it repro reliably with DOTNET_TieredCompilation=1? (You can try to use

RETAIL_CONFIG_DWORD_INFO(EXTERNAL_TC_AggressiveTiering, W("TC_AggressiveTiering"), 0, "Transition through tiers aggressively.")
RETAIL_CONFIG_DWORD_INFO(EXTERNAL_TC_CallCountThreshold, W("TC_CallCountThreshold"), TC_CallCountThreshold, "Number of times a method must be called in tier 0 after which it is promoted to the next tier.")
RETAIL_CONFIG_DWORD_INFO(EXTERNAL_TC_CallCountingDelayMs, W("TC_CallCountingDelayMs"), TC_CallCountingDelayMs, "A perpetual delay in milliseconds that is applied to call counting in tier 0 and jitting at higher tiers, while there is startup-like activity.")
to tweak the tiered compilation timing to make it more reliable repro.)

I think this means I need to make the InterpreterStub lazily instantiate the pCallStub as needed.

I do not think this should be needed. The invariant should be that we go through prestub before we need the pCallStub. It sounds like that background tiered compilation breaks this invariant. I would rather look at fixing the tiered compilation so that it does not break this invariant.

@kg
Copy link
Member Author

kg commented Aug 20, 2025

Further investigation shows that tiered compilation is recompiling the interpreted method, so we're replacing an existing InterpMethod that has a pCallStub with a new one that doesn't have a pCallStub. I don't think tiered compilation should do this, so I'm going to investigate whether I can make it not overwrite an existing interpreted method.

EDIT:

Does it repro reliably with DOTNET_TieredCompilation=1

Yes

@kg kg force-pushed the interp-pcallstub branch from 51152d5 to a69298e Compare August 20, 2025 17:18
@kg kg changed the title Fix InterpreterStub invoke on InterpMethod that does not have a pCallStub [interp] Disable tiered compilation for methods that have interpreter code Aug 20, 2025
@kg kg marked this pull request as ready for review August 20, 2025 17:20
@Copilot Copilot AI review requested due to automatic review settings August 20, 2025 17:20
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 fixes a crash in the interpreter when tiered compilation overwrites existing interpreter code with new code that lacks a call stub. The fix disables tiered compilation for methods that have interpreter code, preventing the tiered compilation thread from interfering with interpreter execution.

Key changes:

  • Disable tiered compilation eligibility for methods with interpreter code
  • Disable call counting for methods with interpreter code to prevent tiered compilation triggers

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/coreclr/vm/method.inl Adds check to exclude methods with interpreter code from tiered compilation eligibility
src/coreclr/vm/method.hpp Prevents call counting for methods with interpreter code to avoid tiered compilation triggers

@kg kg force-pushed the interp-pcallstub branch from 731baba to 23b343d Compare August 20, 2025 18:10
@kg kg changed the title [interp] Disable tiered compilation for methods that have interpreter code [interp] Disable tiered compilation if the interpreter is enabled Aug 20, 2025
@kg
Copy link
Member Author

kg commented Aug 20, 2025

Open to thoughts on whether the diagnostic message in here is useful or not. I'm leaning towards removing it, it was just added for testing. But maybe it's useful for the user to know when they enable the interpreter that it's turning off tiered compilation?

@jkotas
Copy link
Member

jkotas commented Aug 20, 2025

the diagnostic message in here is useful or not. I'm leaning towards removing

I agree. If you set environment variables, you have to know what you are doing. We do not have diagnostic message like this for any other environment variables.

@kg kg merged commit 03bc5a8 into dotnet:main Aug 21, 2025
96 of 98 checks passed
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.

4 participants