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

JIT: fix overly aggressive type propagation from returns#21316

Merged
AndyAyersMS merged 1 commit into
dotnet:masterfrom
AndyAyersMS:Fix21295
Dec 3, 2018
Merged

JIT: fix overly aggressive type propagation from returns#21316
AndyAyersMS merged 1 commit into
dotnet:masterfrom
AndyAyersMS:Fix21295

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

For quite a while now the jit has been propagating return types from
callees to the return spill temp. However this is only safe when the
callee has a single return site (or all return sites return the same
type).

Because return spill temps often end up getting assigned to still more
temps we haven't seen this overly aggressive type propgagation lead to
bugs, but now that we're tracking single def temps and doing more type
propagation during the late devirtualization callback, the fact that
these types are wrong has been exposed and can lead to incorrect
devirtualization.

The fix is to only consider the return spill temp as single def if the
callee has a single return site, and to check that the return spill temp
is single def before trying to propagate the type.

Fixes #21295.

For quite a while now the jit has been propagating return types from
callees to the return spill temp. However this is only safe when the
callee has a single return site (or all return sites return the same
type).

Because return spill temps often end up getting assigned to still more
temps we haven't seen this overly aggressive type propgagation lead to
bugs, but now that we're tracking single def temps and doing more type
propagation during the late devirtualization callback, the fact that
these types are wrong has been exposed and can lead to incorrect
devirtualization.

The fix is to only consider the return spill temp as single def if the
callee has a single return site, and to check that the return spill temp
is single def before trying to propagate the type.

Fixes #21295.
@AndyAyersMS
Copy link
Copy Markdown
Member Author

@briansull PTAL
cc @dotnet/jit-contrib

A handful diffs from PMI... these may not have lead to observable bugs; it depends on which type was actually returned by the callee.

This bug was exposed in JitStress=2 via random inlining. Methods with multiple returns tend not to get inlined by default, but the random inliner (intentionally) doesn't play by the normal rules.

PMI Diffs for System.Private.CoreLib.dll, framework assemblies for x64 default jit
Summary:
(Lower is better)
Total bytes of diff: 61 (0.00% of base)
    diff is a regression.
Top file regressions by size (bytes):
          32 : System.Net.Sockets.dasm (0.02% of base)
          25 : System.Private.CoreLib.dasm (0.00% of base)
           4 : System.Net.Security.dasm (0.00% of base)
3 total files with size differences (0 improved, 3 regressed), 126 unchanged.
Top method regressions by size (bytes):
          32 ( 9.94% of base) : System.Net.Sockets.dasm - Socket:SendFileInternal(ref,ref,ref,int):this
          18 ( 7.76% of base) : System.Private.CoreLib.dasm - RuntimeConstructorInfo:get_InvocationFlags():int:this
           4 ( 0.61% of base) : System.Net.Security.dasm - SecureChannel:NextMessage(ref,int,int):ref:this
           3 ( 0.90% of base) : System.Private.CoreLib.dasm - RuntimePropertyInfo:SetValue(ref,ref,int,ref,ref,ref):this
           3 ( 4.92% of base) : System.Private.CoreLib.dasm - ConstructorBuilder:get_CallingConvention():int:this
Top method improvements by size (bytes):
          -1 (-0.53% of base) : System.Private.CoreLib.dasm - RuntimePropertyInfo:GetValue(ref,int,ref,ref,ref):ref:this
Top method regressions by size (percentage):
          32 ( 9.94% of base) : System.Net.Sockets.dasm - Socket:SendFileInternal(ref,ref,ref,int):this
          18 ( 7.76% of base) : System.Private.CoreLib.dasm - RuntimeConstructorInfo:get_InvocationFlags():int:this
           3 ( 4.92% of base) : System.Private.CoreLib.dasm - ConstructorBuilder:get_CallingConvention():int:this
           2 ( 2.86% of base) : System.Private.CoreLib.dasm - RuntimeConstructorInfo:get_ContainsGenericParameters():bool:this
           3 ( 0.90% of base) : System.Private.CoreLib.dasm - RuntimePropertyInfo:SetValue(ref,ref,int,ref,ref,ref):this
Top method improvements by size (percentage):
          -1 (-0.53% of base) : System.Private.CoreLib.dasm - RuntimePropertyInfo:GetValue(ref,int,ref,ref,ref):ref:this
7 total methods with size differences (1 improved, 6 regressed), 191116 unchanged.

@dotnet-bot test Windows_NT x64 Cross Checked corefx_jitstress2 Build and Test
@dotnet-bot test Windows_NT x64_arm64_altjit Checked corefx_jitstress2

@AndyAyersMS
Copy link
Copy Markdown
Member Author

@dotnet-bot test Windows_NT x86_arm_altjit Checked corefx_jitstress2

@AaronRobinsonMSFT
Copy link
Copy Markdown
Member

@AndyAyersMS This is failing because of a new test. I am checking in something that should fx the issue, but if it doesn;t I will simply disable the test.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Ok, thanks...

@AndyAyersMS
Copy link
Copy Markdown
Member Author

@dotnet-bot test Windows_NT x64 Checked corefx_jitstress2

@AaronRobinsonMSFT
Copy link
Copy Markdown
Member

See #21315

@dotnet-bot test Windows_NT arm Cross Debug Innerloop Build please
@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness please
@dotnet-bot test Windows_NT x86 Checked Innerloop Build and Test please
@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness please

@AndyAyersMS
Copy link
Copy Markdown
Member Author

I can't repro the trace listener failures in the x64 checked corefx jitstress 2 and other runs. Wonder if there is some artifact where if the test run crosses over a month-end boundary they might fail. Errors are of the form

Assert.Equal() Failure\r\n ↓ (pos 1)\r\nExpected: 12-DD-YY\r\nActual: 11-DD-YY\r\n ↑ (pos 1)

So am going to retry these.

@dotnet-bot test Windows_NT x64 Checked corefx_jitstress2
@dotnet-bot test Windows_NT x64_arm64_altjit Checked corefx_jitstress2
@dotnet-bot test Windows_NT x86_arm_altjit Checked corefx_jitstress2

There also appear to be some baseline failures in the CoreFX jitstress2 runs, but only a handful.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Think the remaining jitstress 2 failures are unrelated.

Copy link
Copy Markdown

@briansull briansull left a comment

Choose a reason for hiding this comment

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

Looks Good

@AndyAyersMS AndyAyersMS merged commit 562ae44 into dotnet:master Dec 3, 2018
@AndyAyersMS AndyAyersMS deleted the Fix21295 branch December 3, 2018 18:46
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…clr#21316)

For quite a while now the jit has been propagating return types from
callees to the return spill temp. However this is only safe when the
callee has a single return site (or all return sites return the same
type).

Because return spill temps often end up getting assigned to still more
temps we haven't seen this overly aggressive type propgagation lead to
bugs, but now that we're tracking single def temps and doing more type
propagation during the late devirtualization callback, the fact that
these types are wrong has been exposed and can lead to incorrect
devirtualization.

The fix is to only consider the return spill temp as single def if the
callee has a single return site, and to check that the return spill temp
is single def before trying to propagate the type.

Fixes dotnet/coreclr#21295.

Commit migrated from dotnet/coreclr@562ae44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thousands of tests failed with System.NullReferenceException in x86_arm_altjit_checked_windows_nt_corefx_jitstress2

3 participants