Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT\Directed\tailcall fails at runtime with jitstress=2 #1752

Closed
AndyAyersMS opened this issue Jan 15, 2020 · 3 comments · Fixed by #1771
Closed

JIT\Directed\tailcall fails at runtime with jitstress=2 #1752

AndyAyersMS opened this issue Jan 15, 2020 · 3 comments · Fixed by #1771
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@AndyAyersMS
Copy link
Member

Stack overflow.
Return code: 1
BEGIN EXECUTION
 "C:\h\w\A59B0955\p\corerun.exe" tailcall.dll 
Test Start
Expected: 100
Actual: -1073741571
@AndyAyersMS AndyAyersMS added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 15, 2020
@AndyAyersMS AndyAyersMS added this to the 5.0 milestone Jan 15, 2020
@AndyAyersMS AndyAyersMS self-assigned this Jan 15, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jan 15, 2020
@AndyAyersMS AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label Jan 15, 2020
@AndyAyersMS
Copy link
Member Author

See also dotnet/coreclr#27658

This looks like a stress mode imposed issue.

We have a method Recurse4 with an explicit tail call. But under STRESS_UNSAFE_BUFFER_CHECKS or STRESS_GENERIC_VARN this tail call is inhibited (via gs checks and gc checks, respectively). And this is likely what leads to stack overflow.

This test has passed jitstress in the past, eg it passes on my 3.0 snapshot, and the same inhibitions were in place there. But stress modes are a function of method hash, and it looks like the method hash has changed since then and new stress modes have been enabled for this method.

Timing of the first reported stress failure in dotnet/coreclr#27658 on Nov 3rd lines up nicely with dotnet/coreclr#27147 which went in on Oct 31 and altered method hash computations.

Suggest we mark this test as optimization sensitive and exclude it from jitstress.

@jakobbotsch
Copy link
Member

Suggest we mark this test as optimization sensitive and exclude it from jitstress.

You can also explicitly disable this test for that stress mode, eg.
https://github.com/dotnet/runtime/blob/4f9ae42d861fcb4be2fcd5d3d55d5f227d30e723/src/coreclr/tests/src/JIT/Directed/tailcall/more_tailcalls.ilproj

However it would probably make more sense to disable this stress mode for functions containing explicit tail calls. By the way, #341 relaxes this and allows explicit tail calls out of functions with stack cookies.

@AndyAyersMS
Copy link
Member Author

Thanks @jakobbotsch.

Looks like STRESS_UNSAFE_BUFFER_CHECKS could be handled internally (for now) by disabling it if there are explicit tail calls. But if that limitation is about to be relaxed, I will update the proj file insteaed.

STRESS_GENERIC_VARN sets compGcChecks early before we've imported anything, but it looks like we could selectively disable the check (added in impReturnInstruction) for explicit tail calls.

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 15, 2020
Make STRESS_GENERIC_VARN more compatible with methods that make explicit tail
calls. Don't add gc checks for explicit tail calls, and remove the code in
morph that blocks tail calls if gc checks are active.

Update the tailcall test to to disable STRESS_UNSAFE_BUFFER_CHECKS. This can
be reverted when dotnet#314 is merged.

Fixes dotnet#1752.
AndyAyersMS added a commit that referenced this issue Jan 15, 2020
Make STRESS_GENERIC_VARN more compatible with methods that make explicit tail
calls. Don't add gc checks for explicit tail calls, and remove the code in
morph that blocks tail calls if gc checks are active.

Update the tailcall test to to disable STRESS_UNSAFE_BUFFER_CHECKS. This can
be reverted when #341 is merged.

Fixes #1752.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants