Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Do not fast tail call with callee stack space. - #27926

Closed
jashook wants to merge 1 commit into
dotnet:release/3.1from
jashook:workaround_12468
Closed

jashook wants to merge 1 commit into
dotnet:release/3.1from
jashook:workaround_12468

Conversation

@jashook

@jashook jashook commented Nov 22, 2019

Copy link
Copy Markdown

This change is pessimistic, but it will guarentee correctness.
We specifically know that LowerFastTailCall fundamentally is broken.
This has been fixed in #26255.
This change exists only to make sure that we do not have SBC in older releases.

Note that this changes behavoir, there are valid fast tail calls which could have avoided StackOverFlow exceptions.

These tail calls must have not had overlapping slot locations, for a flawed definition of slot location. Which simply means they got lucky when generating code.

This change is pessimistic, but it will guarentee correctness.
We specifically know that LowerFastTailCall fundamentally is broken.
This has been fixed in dotnet#26255.
This change exists only to make sure that we do not have SBC in older releases.

Note that this changes behavoir, there are valid fast tail calls which could have avoided StackOverFlow exceptions.

These tail calls must have not had overlapping slot locations, for a flawed definition of slot location. Which simply means they got lucky when generating code.
@BruceForstall

Copy link
Copy Markdown

Can you run Linux/x64 diffs for framework assemblies + tests and see what kind of diffs there are?

@BruceForstall

Copy link
Copy Markdown

@BruceForstall

Copy link
Copy Markdown

I presume you have verified that it does indeed fix https://github.com/dotnet/coreclr/issues/27904

@jashook

jashook commented Nov 22, 2019 •

Copy link
Copy Markdown
Author

#26255 fixes #27904 for 5.0, this change is a targeted fix that will keep #26255 from reproing in release/3.1.

@jashook

jashook commented Nov 22, 2019

Copy link
Copy Markdown
Author

As mentioned in person, I will run diffs. Although I expect there will be a non-trivial amount of diffs.

@jashook

jashook commented Dec 6, 2019

Copy link
Copy Markdown
Author

Closing this change, as the opportunity to regress valid scenarios is too high.

@jashook jashook closed this Dec 6, 2019
@jashook
jashook deleted the workaround_12468 branch December 6, 2019 18:34
@janvorli

janvorli commented Dec 9, 2019

Copy link
Copy Markdown
Member

@jashook I would like to understand how that change would regress valid scenarios. When someone doesn't use the explicit tail. prefix for a call, the fast tail call that jit makes is just an optimization and I believe noone can rely on the fact that JIT will emit a tail call there. Am I wrong?

@jashook

jashook commented Dec 9, 2019

Copy link
Copy Markdown
Author

You are not wrong, it would be undefined behavior to expect the optimization. However that being said, this removes the optimization for a large set of methods and can lead to behavior differences between versions of release 3.1. Which I do not believe is desirable.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants