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

Fix poisoning for very large structs #61521

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

jakobbotsch
Copy link
Member

For very large structs poisoning could end up generating instructions
requiring larger local var offsets than we can handle which would hit an
IMPL_LIMIT that throws InvalidProgramException. Switch to using rep
stosd (x86/x64)/memset helper (other platforms) when a local needs more
than a certain number of mov instructions to poison.

Fix #60852

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 12, 2021
@ghost
Copy link

ghost commented Nov 12, 2021

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

Issue Details

For very large structs poisoning could end up generating instructions
requiring larger local var offsets than we can handle which would hit an
IMPL_LIMIT that throws InvalidProgramException. Switch to using rep
stosd (x86/x64)/memset helper (other platforms) when a local needs more
than a certain number of mov instructions to poison.

Fix #60852

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@@ -3291,7 +3299,7 @@ void LinearScan::BuildStoreLocDef(GenTreeLclVarCommon* storeLoc,
defCandidates = allRegs(type);
}
#else
defCandidates = allRegs(type);
defCandidates = allRegs(type);
Copy link
Member Author

Choose a reason for hiding this comment

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

No idea why clang-tidy wants this changed all of a sudden.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

LGTM.

We should consider backporting this one.

src/coreclr/jit/lsrabuild.cpp Outdated Show resolved Hide resolved
@jakobbotsch
Copy link
Member Author

We should consider backporting this one.

Agree. Luckily it only affects code compiled in debug without .zeroinit, but the impact may still be quite large.

@jakobbotsch jakobbotsch reopened this Nov 13, 2021
jakobbotsch added a commit to jakobbotsch/runtime that referenced this pull request Nov 15, 2021
For very large structs (> 64K in size) poisoning could end up generating
instructions requiring larger local var offsets than we can handle. This
hits IMPL_LIMIT that throws InvalidProgramException. Turn off poisoning
for larger structs that require more than 16 movs to also avoid the
significant code bloat by the singular movs.

This is a less risky version of dotnet#61521 for backporting to .NET 6.

Fix dotnet#60852
jakobbotsch added a commit that referenced this pull request Nov 15, 2021
For very large structs (> 64K in size) poisoning could end up generating
instructions requiring larger local var offsets than we can handle. This
hits IMPL_LIMIT that throws InvalidProgramException. Turn off poisoning
for larger structs that require more than 16 movs to also avoid the
significant code bloat by the singular movs.

This is a less risky version of #61521 for backporting to .NET 6.

Fix #60852
github-actions bot pushed a commit that referenced this pull request Nov 15, 2021
For very large structs (> 64K in size) poisoning could end up generating
instructions requiring larger local var offsets than we can handle. This
hits IMPL_LIMIT that throws InvalidProgramException. Turn off poisoning
for larger structs that require more than 16 movs to also avoid the
significant code bloat by the singular movs.

This is a less risky version of #61521 for backporting to .NET 6.

Fix #60852
For very large structs poisoning could end up generating instructions
requiring larger local var offsets than we can handle which would hit an
IMPL_LIMIT that throws InvalidProgramException. Switch to using rep
stosd (x86/x64)/memset helper (other platforms) when a local needs more
than a certain number of mov instructions to poison.

Also includes a register allocator change to mark killed registers as
modified in addRefsForPhysRegMask instead of by the (usually) calling
function, since this function is used directly in the change.

Fix dotnet#60852
@jakobbotsch jakobbotsch merged commit 9a9a4f3 into dotnet:main Nov 16, 2021
@jakobbotsch jakobbotsch deleted the fix-poisoning-large-structs branch November 20, 2021 12:00
safern pushed a commit that referenced this pull request Dec 15, 2021
* Disable poisoning for large structs

For very large structs (> 64K in size) poisoning could end up generating
instructions requiring larger local var offsets than we can handle. This
hits IMPL_LIMIT that throws InvalidProgramException. Turn off poisoning
for larger structs that require more than 16 movs to also avoid the
significant code bloat by the singular movs.

This is a less risky version of #61521 for backporting to .NET 6.

Fix #60852

* Run jit-format

* Add regression test

* Update src/coreclr/jit/codegencommon.cpp

Co-authored-by: Andy Ayers <andya@microsoft.com>

* Don't check poisoning for large struct in test

Since it's disabled, there is nothing to check.

Co-authored-by: Jakob Botsch Nielsen <jakob.botsch.nielsen@gmail.com>
Co-authored-by: Andy Ayers <andya@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Dec 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.NET 6 C++/CLI runtime generates InvalidProgramException when large stack-allocated variables are used
2 participants