Skip to content

Fix the stackoverflow test output checks - #35914

Merged
janvorli merged 2 commits into
dotnet:masterfrom
janvorli:fix-stackoverflow-test
May 7, 2020
Merged

Fix the stackoverflow test output checks#35914
janvorli merged 2 commits into
dotnet:masterfrom
janvorli:fix-stackoverflow-test

Conversation

@janvorli

@janvorli janvorli commented May 6, 2020

Copy link
Copy Markdown
Member

The System.Threading.ThreadHelper.ThreadStart can tail call the
System.Threading.ExecutionContext.RunInternal (and some other methods in
the runtime as well) and thus it would not be visible on the stack
trace. So the fix is to not to look at the System.Threading.ThreadHelper.ThreadStart
in the stack trace and use a method in the test itself instead. Since
the test is compiled with optimizations disabled, JIT should not do any
"interesting" things.

Close #35878

The System.Threading.ThreadHelper.ThreadStart can tail call the
System.Threading.ExecutionContext.RunInternal (and some other methods in
the runtime as well) and thus it would not be visible on the stack
trace. So the fix is to not to look at the System.Threading.ThreadHelper.ThreadStart
in the stack trace and use a method in the test itself instead. Since
the test is compiled with optimizations disabled, JIT should not do any
"interesting" things.
@janvorli janvorli added the area-ExceptionHandling-coreclr only use for closed issues label May 6, 2020
@janvorli janvorli added this to the 5.0 milestone May 6, 2020
@janvorli
janvorli requested a review from BruceForstall May 6, 2020 19:16
@janvorli janvorli self-assigned this May 6, 2020

@BruceForstall BruceForstall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

Would it be worthwhile adding a comment to the SecondaryThreadStart function about the requirement that it appear on the stack trace, and thus we require/expect the JIT not to optimize it away due to the fact the test is build without optimization? Should you annotate it NoOptimization as well?

@janvorli

janvorli commented May 6, 2020

Copy link
Copy Markdown
Member Author

I can annotate all the methods involved with the NoOptimization. I was thinking that disabling optimizations on the project level is sufficient.

@BruceForstall

Copy link
Copy Markdown
Contributor

You're right that disabling on the project level is sufficient. I was thinking that annotating the specific functions makes it more explicit and possibly protects the test against changes to the build process (and maybe allows it to run if built optimized?)

@janvorli

janvorli commented May 6, 2020

Copy link
Copy Markdown
Member Author

It seems that marking all of the methods that should show up on the call stack with NoInlining should be sufficient, right? Or is the NoOptimization a better option?

@BruceForstall

Copy link
Copy Markdown
Contributor

To be honest, I never remember the answer to that. @AndyAyersMS Can you help?

@AndyAyersMS

Copy link
Copy Markdown
Member

NoInlining should be sufficient.

@janvorli
janvorli merged commit 6985ce6 into dotnet:master May 7, 2020
@janvorli
janvorli deleted the fix-stackoverflow-test branch May 7, 2020 17:26
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclr only use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failures: stackoverflowtester

3 participants