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
erozenfeld merged 1 commit intodotnet:masterfrom
erozenfeld:NonReturningCallsInt3
Apr 13, 2018
Merged

Insert int3 after non-returning calls at the end of basic blocks.#17535
erozenfeld merged 1 commit intodotnet:masterfrom
erozenfeld:NonReturningCallsInt3

Conversation

@erozenfeld
Copy link
Member

@erozenfeld erozenfeld commented Apr 13, 2018

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.

@erozenfeld
Insert int3 after non-returning calls at the end of basic blocks.
9fdb2d1 
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

erozenfeld commented Apr 13, 2018

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

erozenfeld commented Apr 13, 2018

@dotnet/jit-contrib

@erozenfeld
Copy link
Member Author

erozenfeld commented Apr 13, 2018

The change was suggested by @AndyAyersMS in #17398.

@erozenfeld
Copy link
Member Author

erozenfeld commented Apr 13, 2018

@dotnet-bot test Windows_NT x86 Checked gcstress0xc_zapdisable_jitstress2

@RussKeldorph RussKeldorph added this to the 2.1.0 milestone Apr 13, 2018
briansull
briansull approved these changes Apr 13, 2018
View reviewed changes
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

AndyAyersMS commented Apr 13, 2018

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

erozenfeld commented Apr 13, 2018

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

CarolEidt
CarolEidt approved these changes Apr 13, 2018
View reviewed changes
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.

Reviewers

2 more reviewers

@CarolEidt CarolEidt CarolEidt approved these changes

@briansull briansull briansull approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-CodeGen

Projects

None yet

Milestone

2.1.0

Development

Successfully merging this pull request may close these issues.

x86 jitstress / gcstress failure with methods that do not return

6 participants

@erozenfeld @AndyAyersMS @CarolEidt @briansull @dotnet-bot @RussKeldorph