Skip to content

Conversation

@VSadov
Copy link
Member

@VSadov VSadov commented Jun 29, 2024

@dotnet-policy-service
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

@azure-pipelines
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 janvorli and jkotas 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.

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