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

JIT: refine x86 gc reg kill set for CORINFO_HELP_INIT_PINVOKE_FRAME#17421

Merged
AndyAyersMS merged 1 commit into
dotnet:masterfrom
AndyAyersMS:PinvokeFrameSetupGcKill
Apr 5, 2018
Merged

JIT: refine x86 gc reg kill set for CORINFO_HELP_INIT_PINVOKE_FRAME#17421
AndyAyersMS merged 1 commit into
dotnet:masterfrom
AndyAyersMS:PinvokeFrameSetupGcKill

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

This helper only kills EAX/ESI on x86, so make sure that is reflected in
the gc kill set.

Resolves #17404.

This helper only kills EAX/ESI on x86, so make sure that is reflected in
the gc kill set.

Resolves #17404.
@AndyAyersMS
Copy link
Copy Markdown
Member Author

@BruceForstall PTAL
cc @dotnet/jit-contrib

@dotnet-bot test Windows_NT x86 Checked gcstress0xc_jitstress1

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Might as well throw a few other stress legs on there.. picking ones with some of the hard to repro issues from #17330.

@dotnet-bot test Windows_NT x86 Checked gcstress0xc
@dotnet-bot test test Windows_NT x86 Checked gcstress0xc_zapdisable_heapverify1

Copy link
Copy Markdown

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM for x86. It certainly warrants more investigation whether this change should be all-platform, not just x86. As is, we're hoping/assuming RBM_CALLEE_TRASH_NOGC is correct for other platforms.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Opened #17425 for a broader review of compNoGCHelperCallKillSet.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Peeked in at the stress legs. So far two of them hit this error which #17330 purports to fix:

Assert failure(PID 10944 [0x00002ac0], Thread: 15948 [0x3e4c]): 
Consistency check failed: hit privileged instruction!
FAILED: !ExecutionManager::IsManagedCode(GetIP(pContext))

Also there is something amiss in the CI request parsing, as I asked for 3 gc stress legs to run, but somehow ended up with 4. No big deal but given how resource-hungry these stress runs are we may want to figure out why this happens.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Also after getting through tests, the GC stress legs continue on to do OSX & Linux builds. This seems unnecessary? I think we had this in normal builds so we could have assets to forward to other legs but I doubt we do that with stress legs (and nothing is built differently here, it's just tested differently).

@BruceForstall maybe something you could look at someday?

@BruceForstall
Copy link
Copy Markdown

Also there is something amiss in the CI request parsing, as I asked for 3 gc stress legs to run, but somehow ended up with 4.

The problem (I think) is that "Windows_NT x86 Checked gcstress0xc_zapdisable_heapverify1" has prefix substrings that are also legal trigger phrases. We've seen this before. E.g., if you trigger "jitstressregs0x1000" jobs, you'll also get "jitstressregs0x10" jobs (which are a prefix).

Also after getting through tests, the GC stress legs continue on to do OSX & Linux builds. This seems unnecessary?

it's just the mscorlib build, right? That's unnecessary, but also not costly, right?

@BruceForstall maybe something you could look at someday?

I could.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

You're right, it's not costly, takes about 3 minutes.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

x86 gcstress 0xC jitstress 1 now passing cleanly.

x86 gcstress 0xC jittress 1 heapverify 1 also hitting the dbgu/relu test failures seen in #17330, which I've been unable to repro locally. But no new failures there yet.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

All the stress failures seen here and here are known issues, tracked by #17330 and #17428.

So am going to merge.

@RussKeldorph this can potentially show up as a failure in non-gc stress runs for x86, so I recommend we try and get this into 2.1 preview 2.

@AndyAyersMS AndyAyersMS merged commit e206e83 into dotnet:master Apr 5, 2018
@AndyAyersMS AndyAyersMS deleted the PinvokeFrameSetupGcKill branch April 5, 2018 00:54
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.

2 participants