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

Fix for potential bad tailcall code generation #1298

Merged
merged 1 commit into from Jul 29, 2015

Conversation

Projects
None yet
@mmitche
Member

mmitche commented Jul 27, 2015

The JIT can potentially pass an incorrect parameter to a callee when optimizing a tail call. While updating a GT_LCL* node with lcl var num of temp passed to the tail call, we also need to change its opererand to GT_LCL_VAR.

Fixes #1296

@masonwheeler

This comment has been minimized.

Show comment
Hide comment
@masonwheeler

masonwheeler Jul 27, 2015

Nice work tracking this down! :)

masonwheeler commented Jul 27, 2015

Nice work tracking this down! :)

@akoeplinger

This comment has been minimized.

Show comment
Hide comment
@akoeplinger

akoeplinger Jul 27, 2015

Member

Seems this is the same as in #1289 from TFS that was later rolled back?

dotnet-bot@838b043
dotnet-bot@0aef784

Member

akoeplinger commented Jul 27, 2015

Seems this is the same as in #1289 from TFS that was later rolled back?

dotnet-bot@838b043
dotnet-bot@0aef784

@masonwheeler

This comment has been minimized.

Show comment
Hide comment
@masonwheeler

masonwheeler Jul 27, 2015

Yeah, looks like it. Why did it get rolled back?

masonwheeler commented Jul 27, 2015

Yeah, looks like it. Why did it get rolled back?

@mmitche

This comment has been minimized.

Show comment
Hide comment
@mmitche

mmitche Jul 27, 2015

Member

@masonwheeler Not quite sure, but here it is again :)

Member

mmitche commented Jul 27, 2015

@masonwheeler Not quite sure, but here it is again :)

@hmemcpy

This comment has been minimized.

Show comment
Hide comment
@hmemcpy

hmemcpy Jul 27, 2015

Maybe we could ask @sivarv? :)

hmemcpy commented Jul 27, 2015

Maybe we could ask @sivarv? :)

@mmdriley

This comment has been minimized.

Show comment
Hide comment
@mmdriley

mmdriley Jul 27, 2015

Should we include the regression test here or in a followup PR?

mmdriley commented Jul 27, 2015

Should we include the regression test here or in a followup PR?

@mmitche

This comment has been minimized.

Show comment
Hide comment
@mmitche

mmitche Jul 28, 2015

Member

@mmdriley I've managed to narrow down a regression test, I'll have that included in the merge tomorrow morning :)

Member

mmitche commented Jul 28, 2015

@mmdriley I've managed to narrow down a regression test, I'll have that included in the merge tomorrow morning :)

@danielgindi

This comment has been minimized.

Show comment
Hide comment
@danielgindi

danielgindi Jul 28, 2015

Wow you guys still haven't figured out why the bug is there, and why the fix was rolled back previously...?
This is really worrying.

danielgindi commented Jul 28, 2015

Wow you guys still haven't figured out why the bug is there, and why the fix was rolled back previously...?
This is really worrying.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 28, 2015

@danielgindi it looks like @sivarv wanted to fix Microsoft/visualfsharp#536 (comment), but it wasn't the correct fix for that so I assume he rolled it back.

forki commented Jul 28, 2015

@danielgindi it looks like @sivarv wanted to fix Microsoft/visualfsharp#536 (comment), but it wasn't the correct fix for that so I assume he rolled it back.

@danielgindi

This comment has been minimized.

Show comment
Hide comment
@danielgindi

danielgindi Jul 28, 2015

@forki Thanks for your response :-)

Btw, I love it that Microsoft has moved to Git!

danielgindi commented Jul 28, 2015

@forki Thanks for your response :-)

Btw, I love it that Microsoft has moved to Git!

@PeterMurdoch

This comment has been minimized.

Show comment
Hide comment
@PeterMurdoch

PeterMurdoch commented Jul 28, 2015

Thanks!

@wickedsheep

This comment has been minimized.

Show comment
Hide comment
@wickedsheep

wickedsheep Jul 28, 2015

I was just thinking, how come this didn't show up in the tests? It seems like this would blow up everywhere.

wickedsheep commented Jul 28, 2015

I was just thinking, how come this didn't show up in the tests? It seems like this would blow up everywhere.

@danielgindi

This comment has been minimized.

Show comment
Hide comment
@danielgindi

danielgindi Jul 28, 2015

Nope. This blows up in production environment only, and only where there are function call which are the last statement in a function, and only when the optimizer decides to...

danielgindi commented Jul 28, 2015

Nope. This blows up in production environment only, and only where there are function call which are the last statement in a function, and only when the optimizer decides to...

@wickedsheep

This comment has been minimized.

Show comment
Hide comment
@wickedsheep

wickedsheep Jul 28, 2015

This would imply the tests are either not run in "production environment" (meaning "Release" mode with "Optimize code" enabled) or there is no such test that would trigger the tail call opt.

wickedsheep commented Jul 28, 2015

This would imply the tests are either not run in "production environment" (meaning "Release" mode with "Optimize code" enabled) or there is no such test that would trigger the tail call opt.

@Rutix

This comment has been minimized.

Show comment
Hide comment
@Rutix

Rutix Jul 28, 2015

Or this case is such an edge case that one of the test just doesn't trigger it. They probably have added a test case now to their internal tests that hits this edge case. (Atleast I hope so ;))

Rutix commented Jul 28, 2015

Or this case is such an edge case that one of the test just doesn't trigger it. They probably have added a test case now to their internal tests that hits this edge case. (Atleast I hope so ;))

Fix for potential bad tailcall code generation
The JIT can potentially pass an incorrect parameter to a callee when optimizing a tail call.  While updating a GT_LCL* node with lcl var num of temp passed to the tail call, we also need to change its opererand to GT_LCL_VAR.
@mmitche

This comment has been minimized.

Show comment
Hide comment
@mmitche

mmitche Jul 28, 2015

Member

Hey all, sorry for the delay in response. I have added the test case along with the fix.

The reason that the original fix was backed was for a few reasons:

  1. We wanted to check the fix directly into github rather than having it go across the mirror for visibility reasons
  2. We wanted to evaluate the fix further to see what kind of security impact it might have.

@richlander has posted a nice blog about the issue over at the MSDN dotnet blog: http://blogs.msdn.com/b/dotnet/archive/2015/07/28/ryujit-bug-advisory-in-the-net-framework-4-6.aspx

Member

mmitche commented Jul 28, 2015

Hey all, sorry for the delay in response. I have added the test case along with the fix.

The reason that the original fix was backed was for a few reasons:

  1. We wanted to check the fix directly into github rather than having it go across the mirror for visibility reasons
  2. We wanted to evaluate the fix further to see what kind of security impact it might have.

@richlander has posted a nice blog about the issue over at the MSDN dotnet blog: http://blogs.msdn.com/b/dotnet/archive/2015/07/28/ryujit-bug-advisory-in-the-net-framework-4-6.aspx

@wickedsheep

This comment has been minimized.

Show comment
Hide comment
@wickedsheep

wickedsheep commented Jul 29, 2015

Thank you @mmitche.

mmitche added a commit that referenced this pull request Jul 29, 2015

Merge pull request #1298 from mmitche/tail-call-fix
Fix for potential bad tailcall code generation

@mmitche mmitche merged commit f579474 into dotnet:master Jul 29, 2015

1 check passed

default Build finished. No test results found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment