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

Insert int3 after non-returning calls at the end of basic blocks. #17535

Merged
merged 1 commit into from
Apr 13, 2018

Conversation

erozenfeld
Copy link
Member

This is a follow-up to #17501 that fixed #17398.

#17398 was caused by a break in implicit contract between codegen and
gc pointer reporting in fully-interruptible mode: the latter assumed that
register gc pointer liveness doesn't change across calls while #6103 introduced
codegen where it wasn't true.

#17501 changed gc pointer reporting not to expect that register gc pointer liveness
doesn't change across calls.

This change inserts int3 after non-returning calls at the end of basic blocks
so that gc pointer liveness doesn't change across calls. This is additional
insurance in case any other place in runtime is dependent on that contract.

This is a follow-up to dotnet#17501 that fixed #17398.

gc pointer reporting in fully-interruptible mode: the latter assumed that
register gc pointer liveness doesn't change across calls while dotnet#6103 introduced
codegen where it wasn't true.

doesn't change across calls.

This change inserts int3 after non-returning calls at the end of basic blocks
so that gc pointer liveness doesn't change across calls. This is additional
insurance in case any other place in the runtime is dependent on that contract.
@erozenfeld
Copy link
Member Author

jit-diff --frameworks:

Summary:
(Note: Lower is better)
Total bytes of diff: 4444 (0.02% 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):
        3817 : System.Private.CoreLib.dasm (0.13% of base)
         436 : System.Collections.Immutable.dasm (0.15% of base)
          74 : System.Private.DataContractSerialization.dasm (0.01% of base)
          26 : System.Memory.dasm (0.04% of base)
          26 : System.Net.Http.dasm (0.01% of base)
18 total files with size differences (0 improved, 18 regressed), 112 unchanged.
Top method regessions by size (bytes):
         174 : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.ICollection.CopyTo(ref,int):this (29 methods)
         156 : System.Private.CoreLib.dasm - ReadOnlyCollection`1:System.Collections.ICollection.CopyTo(ref,int):this (26 methods)
         145 : System.Private.CoreLib.dasm - KeyCollection:System.Collections.ICollection.CopyTo(ref,int):this (30 methods)
         145 : System.Private.CoreLib.dasm - ValueCollection:System.Collections.ICollection.CopyTo(ref,int):this (30 methods)
          92 : System.Private.CoreLib.dasm - List`1:FindLastIndex(int,int,ref):int:this (23 methods)
1086 total methods with size differences (0 improved, 1086 regressed), 144430 unchanged.

@erozenfeld
Copy link
Member Author

@dotnet/jit-contrib

@erozenfeld
Copy link
Member Author

The change was suggested by @AndyAyersMS in #17398.

@erozenfeld
Copy link
Member Author

@dotnet-bot test Windows_NT x86 Checked gcstress0xc_zapdisable_jitstress2

@RussKeldorph RussKeldorph added this to the 2.1.0 milestone Apr 13, 2018
Copy link

@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.

LGTM

@AndyAyersMS
Copy link
Member

FWIW I think this bug has been around much longer. The existing int3 padding has compensated for it in many cases, though I'd wager with the right set of IBC data we could have found cases where that padding wouldn't kick in properly and we could have seen the same sort of issue.

GC stress failures look to be cases covered by #17330.

@erozenfeld
Copy link
Member Author

@dotnet-bot test CentOS7.1 x64 Checked Innerloop Build and Test

Copy link

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM

@erozenfeld erozenfeld merged commit ee5ab84 into dotnet:master Apr 13, 2018
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.

x86 jitstress / gcstress failure with methods that do not return
6 participants