System.Linq.Expressions.Tests.CompilerTests.CompileDeepTree_NoStackOverflowFast overflows stack with Checked CoreCLR build #20788
Comments
|
@JonHanna since you last modified this test. |
|
The purpose of that test is to test some functionality that pre-empts overflow by probing with It failing rather suggests the test is doing its job, and the functionality it is not working in this case. |
|
@JonHanna The Checked build is similar to a Debug build (assertions enabled, etc.) with C++ and C# compiler optimizations enabled. It is the most useful build mode for testing the CLR JIT, since it's faster (C++ optimizations), it's testing JIT optimizations (C# optimizations), and assertions are enabled to validate the internal state of the runtime and JIT. I will edit my comment above with repro steps if you would like to try it. We assumed the test didn't have stack probing functionality and that the additional stack requirements of a Checked build were simply causing the overflow. If probing is enabled, perhaps the issue is more subtle. @AndyAyersMS @dotnet/jit-contrib |
|
It's the functionality itself that has stack-probing, but if that's not going to work then the test has done its job in alerting us to that. Expressions can get itself into some very deep recursion in cases that are relatively rare but common enough that they've caused users problem, and the probing allowed that to be resolved. If that test overflows the stack, then likely so would that real-world code that was considered fixed. |
|
cc: @bartdesmet who wrote the first version of the stack-probing code. |
|
TryEnsureSufficientExecutionStack has some hardcoded limits in it "to allow a typical non-recursive call chain to execute": |
|
Are the outerloop tests being run here? There's a slower outerloop test that hits the same functionality with a normal stack size. If that passes then I think it would be reasonable to just disable the faster test in this context, if there was a good way of signalling that it should be skipped. |
|
Outerloop tests aren't being run currently, though they are planned once innerloop are green. This is (until something else breaks) the last failure in the innerloop tests with Checked coreclr. I can kick off outerloop now and see how they look. |
|
It'd be worth knowing if this test's outerloop sibling breaks or not, I think. |
|
I kicked off an -Outerloop test run and another run with a CoreCLR with higher _DEBUG min stack size by changing the lines pointed out by @stephentoub. Will report back. |
|
In this particular test the SO hits during classloading when the runtime is trying to cons up a descriptive string for the class being loaded. This string consing is a diagnostic aid that only happens in debug/check runtimes. I like @stephentoub's suggestion since this behavior is coupled to how the runtime is built, we could easily just increase the min stack size for debug/check builds (not sure just how much is needed though). On the other hand I dislike tests whose behavior varies depending on how the runtime is built.... |
|
Update: The Outerloop version (CompilerTests.CompileDeepTree_NoStackOverflow) passes, and increasing the constants @stephentoub pointed out has no effect. If I understand the code correctly, the constants are not limiting the min stack size but are instead controlling a mechanism where code can query to ensure "at least 128k (or 64k for 32-bit) bytes of stack remain." The test is only providing 64k of stack total, so this TryEnsureSufficientExecutionStack thing is largely irrelevant since it can never return true for this test. It looks like Checked CoreCLR is simply blowing 64k of stack. The three biggest frames on the stack are about 4k each, which is bad but not outrageous. It could be that CoreCLR is simply too big for a 64k stack. I suppose CoreCLR could elsewhere enforce a minimum stack size parameter on thread creation, but I could also believe we want to leave people to get what they deserve, in which case we need to either disable or modify this test case to accomodate non-Release builds of the runtime. I agree with @AndyAyersMS that we shouldn't make the test somehow detect the flavor of the runtime. Either the runtime should accommodate or we should simply make this test leg not run it. Stack trace FYI (sorry, how do I combine |
|
The purpose of the small stack is simply to reach the point where it would overflow where it not for the guard against that, in a timely manner. A larger stack that works but is still reasonably prompt would be just as good (though it takes some fiddling to figure out the size of query large enough to hit the threshold but no so large as to take forever doing so). But if the outerloop tests will be run, we've a canary about issues specific to this build type, and if the fast version is still run in the tests people run while hacking on expressions, we've a prompt signal if they break things, so just disabling this test for this build would be fine IMO. |
|
If I understand the code correctly (not necessarily safe to assume), then I don't think this test is "tuned" based on your description. The test is providing for a 64k stack, but At any rate, if you think it's best to disable the test for our runs, can you suggest how to do that? I see some examples of "Traits" and "Categories" that are used to disable tests, but I'm not sure what's appropriate here. It's fine to require us to pass a switch (e.g. |
|
So maybe around 192k would be enough to let it pass for this case? |
|
I tried changing stack size here to 192k. The test now passes with a Checked runtime, and this line is hit multiple times during the test, which I think indicates the code is taking the intended path. If I increase the stack size to 256k, the "false" path is no longer taken, so that would be too much stack. This is all with a Checked runtime. I haven't tried a Release runtime. Changing the stack size seems like a more promising path than disabling the test to me. |
|
That sounds perfect. |
|
@JonHanna Just to be clear: are you planning to make this change, or do you need me to? I'm blocked, so I don't mind, but I'd just as soon you do it. :) |
|
I'll open a PR shortly. |
Should allow it to run with checked coreclr builds, but still test the stack-overflow guards without running for a long time. Fixes #17550
Should allow it to run with checked coreclr builds, but still test the stack-overflow guards without running for a long time. Fixes #17550
Should allow it to run with checked coreclr builds, but still test the stack-overflow guards without running for a long time. Fixes #17550
Should allow it to run with checked coreclr builds, but still test the stack-overflow guards without running for a long time. Fixes #17550
ghost
locked as resolved and limited conversation to collaborators
CompilerTests.CompileDeepTree_NoStackOverflowFast overflows the stack with Checked (and presumably Debug) builds of CoreCLR. See comment.
This makes it problematic for us to use CoreFx tests to validate CoreCLR. Is it reasonable to reduce the complexity of the test so that it fits on the stack regardless of runtime build type? If not, can you recommend an attribute to tag this test so we can disable it in CoreCLR runs?
Repro
(Windows, from dotnet/coreclr repo!)
(There are likely simpler repro steps that can be distilled from the operation of
run-corefx-tests.py, but those are left as an exercise for the reader.)The text was updated successfully, but these errors were encountered: