Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use resumable leaf frames in CET hijack and in GC stress. #104198

Merged
merged 5 commits into from
Jul 1, 2024

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Jun 29, 2024

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@VSadov
Copy link
Member Author

VSadov commented Jun 29, 2024

/azp run runtime-coreclr gcstress0x3-gcstress0xc

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VSadov VSadov marked this pull request as ready for review June 29, 2024 17:08
@VSadov VSadov requested review from jkotas and janvorli June 29, 2024 17:10
Comment on lines 1822 to 1823
DWORD_PTR retValRegs[1] = { 0 };
UINT numberOfRegs = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DWORD_PTR retValRegs[1] = { 0 };
UINT numberOfRegs = 0;
DWORD_PTR retValReg = 0;

This can simplified since we are only protecting a single value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think with runtime async the need to protect two returns will be back fairly soon, so i did not simplify to strictly one return.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@dotnet dotnet deleted a comment from azure-pipelines bot Jun 29, 2024
@VSadov
Copy link
Member Author

VSadov commented Jun 29, 2024

The stress failures on OSX arm64 look all preexisting.
A bug in GC reporting of returns generally causes crashes even in a regular run, and under GC stress would break very quickly, so it looks we are clean on that part.

src/coreclr/vm/gccover.cpp Outdated Show resolved Hide resolved
// The legacy X86 GC encoder does not encode the state of return registers at
// call sites, so we must add an extra frame to protect returns.
#ifdef TARGET_X86
DWORD_PTR retValReg = 0;

if (afterCallProtect[0])
Copy link
Member

Choose a reason for hiding this comment

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

Given that we only need afterCallProtect for x86 now, we can also delete the whole bunch of arch-specific code above to compute it for non-x86 platforms.

Copy link
Member Author

@VSadov VSadov Jun 30, 2024

Choose a reason for hiding this comment

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

There is a lot of cleanup possible since we’ve incremented r2r version.

I’d like to do that in a separate follow up change as that could be larger, but mostly mechanical and unlikely to break anything change.

Copy link
Member Author

Choose a reason for hiding this comment

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

We would not need disassemble anything other than x86.
Perhaps x64 will merge with RISC and only x86 will be special.

Copy link
Member Author

Choose a reason for hiding this comment

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

For non-x86 it will be just iterating over interruptible locations (partial or full interruptible - just different iterators) and stick HLT there.

Copy link
Member Author

@VSadov VSadov Jun 30, 2024

Choose a reason for hiding this comment

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

Although that may still require disassembling - to ensure that hlt is on instruction edges.

@VSadov
Copy link
Member Author

VSadov commented Jul 1, 2024

Thanks!!

@VSadov VSadov merged commit 104e88d into dotnet:main Jul 1, 2024
89 checks passed
@VSadov VSadov deleted the afterR2Rver01 branch July 1, 2024 22:01
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 2024
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.

2 participants