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

JIT: Fix bug in finally cloning caused by unsound callfinally reordering #18348

Merged
merged 2 commits into from
Jun 8, 2018

Conversation

AndyAyersMS
Copy link
Member

We need to make sure that if we reorder a callfinally during finally cloning
that the callfinally is actually the one being targeted by the last block in
the try range.

Closes #18332. Linked issue has some more detailed notes.

We need to make sure that if we reorder a callfinally during finally cloning
that the callfinally is actually the one being targeted by the last block in
the try range.

Closes #18332. Linked issue has some more detailed notes.
@AndyAyersMS
Copy link
Member Author

@BruceForstall PTAL
cc @dotnet/jit-contrib

Don't expect any FX diffs from this. Will verify.

@AndyAyersMS
Copy link
Member Author

Well, there are FX diffs.... possibly requires exceptions to see the impact of this bug, so maybe not caught in any testing.

Completed Crossgen Diffs for System.Private.CoreLib.dll, framework assemblies in 273.73s
Analyzing diffs...
Found files with textual diffs.
Summary:
(Note: Lower is better)
Total bytes of diff: 169 (0.00% of base)
    diff is a regression.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
         123 : Microsoft.CodeAnalysis.dasm (0.02% of base)
          30 : System.Net.WebSockets.dasm (0.07% of base)
          13 : System.Net.Http.dasm (0.00% of base)
           6 : System.Net.HttpListener.dasm (0.00% of base)
           5 : NuGet.Protocol.Core.v3.dasm (0.00% of base)
Top file improvements by size (bytes):
          -6 : System.Net.Requests.dasm (-0.01% of base)
          -3 : System.Runtime.Extensions.dasm (0.00% of base)
          -2 : System.Security.AccessControl.dasm (0.00% of base)
          -1 : System.Collections.Concurrent.dasm (0.00% of base)
          -1 : System.Private.CoreLib.dasm (0.00% of base)
12 total files with size differences (5 improved, 7 regressed), 118 unchanged.
Top method regessions by size (bytes):
         123 : Microsoft.CodeAnalysis.dasm - CommonCompiler:RunCore(ref,ref,struct):int:this
          30 : System.Net.WebSockets.dasm - <ReceiveAsyncPrivate>d__65`2:MoveNext():this (3 methods)
           5 : System.Net.Http.dasm - <ConnectAsync>d__2:MoveNext():this
           4 : System.Net.HttpListener.dasm - HttpListener:RemovePrefix(ref):bool:this
           3 : NuGet.Protocol.Core.v3.dasm - <StartWithTimeout>d__0`1:MoveNext():this (3 methods)
Top method improvements by size (bytes):
          -7 : System.Net.Requests.dasm - FtpWebRequest:GetResponse():ref:this
          -3 : System.Runtime.Extensions.dasm - Environment:get_WorkingSet():long
          -1 : System.Collections.Concurrent.dasm - InternalPartitionEnumerable:GrabChunk_Buffered(ref,int,byref):bool:this
          -1 : System.Private.CoreLib.dasm - NativeOrStaticEventRegistrationImpl:RemoveAllEventHandlers(ref)
          -1 : System.Security.AccessControl.dasm - Win32:GetSecurityInfo(int,ref,ref,int,byref):int
21 total methods with size differences (6 improved, 15 regressed), 142199 unchanged.
Completed analysis in 112.09s

Am also trying out PMI based diffs on this across all tests, but amhitting some issues with (I think) marker file collision. So need to tweak that some...

@AndyAyersMS
Copy link
Member Author

Here's an all-up PMI diff for this change using the still somewhat-in-flight PMI diffs I've been working on over in jitutils.

Not surprisingly the new test case shows up.

Completed PMI Diffs for System.Private.CoreLib.dll, framework assemblies, assemblies in D:\repos\coreclr\bin\tests\Windows_NT.x64.Release in 1262.86s
Analyzing diffs...
Found files with textual diffs.
Summary:
(Note: Lower is better)
Total bytes of diff: 136 (0.00% of base)
    diff is a regression.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
          77 : Microsoft.CodeAnalysis.dasm (0.01% of base)
          31 : JIT\Regression\JitBlue\GitHub_18332\GitHub_18332\GitHub_18332.dasm (6.22% of base)
          14 : NuGet.Protocol.Core.v3.dasm (0.01% of base)
          12 : System.Collections.Concurrent.dasm (0.01% of base)
           6 : System.Net.HttpListener.dasm (0.00% of base)
Top file improvements by size (bytes):
          -6 : System.Net.Requests.dasm (-0.01% of base)
          -3 : System.Runtime.Extensions.dasm (0.00% of base)
          -1 : JIT\HardwareIntrinsics\X86\Avx\CompareScalar_ro\CompareScalar_ro.dasm (-0.02% of base)
11 total files with size differences (3 improved, 8 regressed), 2802 unchanged.
Top method regessions by size (bytes):
          77 : Microsoft.CodeAnalysis.dasm - CommonCompiler:RunCore(ref,ref,struct):int:this
          31 : JIT\Regression\JitBlue\GitHub_18332\GitHub_18332\GitHub_18332.dasm - GitHub_18332:Aargh():ref
          12 : NuGet.Protocol.Core.v3.dasm - <StartWithTimeout>d__0`1:MoveNext():this (4 methods)
          12 : System.Collections.Concurrent.dasm - InternalPartitionEnumerable:GrabChunk_Buffered(ref,int,byref):bool:this (4 methods)
           4 : System.Net.HttpListener.dasm - HttpListener:RemovePrefix(ref):bool:this
Top method improvements by size (bytes):
          -3 : System.Net.Requests.dasm - FtpWebRequest:GetResponse():ref:this
          -3 : System.Net.Requests.dasm - FtpWebRequest:BeginGetResponse(ref,ref):ref:this
          -3 : System.Runtime.Extensions.dasm - Environment:get_WorkingSet():long
          -2 : System.Net.Http.dasm - <DrainAsync>d__15:MoveNext():this
          -1 : System.Net.Http.dasm - <ConnectAsync>d__2:MoveNext():this
19 total methods with size differences (7 improved, 12 regressed), 342431 unchanged.
Completed analysis in 286.99s

Copy link

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

LGTM

@AndyAyersMS
Copy link
Member Author

OSX failure is some kind of infrastructure/networking hiccup. Will retry

@dotnet-bot retest OSX10.12 x64 Checked Innerloop Build and Test

@AndyAyersMS AndyAyersMS merged commit 311322b into dotnet:master Jun 8, 2018
@AndyAyersMS AndyAyersMS deleted the Investigate18332 branch June 8, 2018 06:17
@AndyAyersMS
Copy link
Member Author

@RussKeldorph this is a candidate for 2.1 servicing.

AndyAyersMS added a commit to AndyAyersMS/coreclr that referenced this pull request Jun 8, 2018
…ing (dotnet#18348)

We need to make sure that if we reorder a callfinally during finally cloning
that the callfinally is actually the one being targeted by the last block in
the try range.

Closes #18332. Linked issue has some more detailed notes.
AndyAyersMS added a commit that referenced this pull request Jun 27, 2018
Port of #18348 to release/2.1

We need to make sure that if we reorder a callfinally during finally cloning
that the callfinally is actually the one being targeted by the last block in
the try range.

Closes #18332. Linked issue has some more detailed notes.
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…ing (dotnet/coreclr#18348)

We need to make sure that if we reorder a callfinally during finally cloning
that the callfinally is actually the one being targeted by the last block in
the try range.

Closes dotnet/coreclr#18332. Linked issue has some more detailed notes.


Commit migrated from dotnet/coreclr@311322b
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.

3 participants