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

JIT: Fix too wide loads on arm64 for small structs #76341

Merged
merged 18 commits into from
Nov 17, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Sep 28, 2022

Fixes #76194
using @SingleAccretion's snippet since mine was uglier.

We've had similar issues in the past e.g. #64683 (reproduces even on .NET Framework x64)
it's just that normally it's an extremely rare case, perhaps, still worth backporting to .NET 7.0

@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 Sep 28, 2022
@ghost ghost assigned EgorBo Sep 28, 2022
@ghost
Copy link

ghost commented Sep 28, 2022

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

Issue Details

Fixes #76194
using @SingleAccretion's snippet since mine was uglier.

We've had similar issues in the past e.g. #64683 (reproduces even on .NET Framework)
it's just that normally it's an extremely rare case, perhaps, still worth backporting to .NET 7.0

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo marked this pull request as draft September 29, 2022 02:48
@EgorBo EgorBo marked this pull request as ready for review September 30, 2022 02:14
@EgorBo
Copy link
Member Author

EgorBo commented Sep 30, 2022

PTAL @jakobbotsch @SingleAccretion

Comment on lines 3797 to 3804
LIR::Use retValUse(BlockRange(), &ret->gtOp1, ret);
unsigned tmpNum = BAD_VAR_NUM;
if (structCls != NO_CLASS_HANDLE)
{
tmpNum = comp->lvaGrabTemp(true DEBUGARG("mis-sized struct return"));
comp->genReturnLocal = tmpNum;
comp->lvaSetStruct(tmpNum, structCls, true);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LIR::Use retValUse(BlockRange(), &ret->gtOp1, ret);
unsigned tmpNum = BAD_VAR_NUM;
if (structCls != NO_CLASS_HANDLE)
{
tmpNum = comp->lvaGrabTemp(true DEBUGARG("mis-sized struct return"));
comp->genReturnLocal = tmpNum;
comp->lvaSetStruct(tmpNum, structCls, true);
}
LIR::Use retValUse(BlockRange(), &ret->gtOp1, ret);
unsigned tmpNum = comp->lvaGrabTemp(true DEBUGARG("mis-sized struct return"));
comp->lvaSetStruct(tmpNum, structCls, false);
  1. genReturnLocal has a very specific purpose, no reason to set it here.
  2. No unsafe value class checks needed (in principle, in practice it doesn't matter) since the address of this local won't escape.

Copy link
Member Author

Choose a reason for hiding this comment

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

genReturnLocal has a very specific purpose, no reason to set it here.

Tests assert after this patch: Assertion failed 'tmp == genReturnLocal'

Copy link
Member Author

@EgorBo EgorBo Nov 13, 2022

Choose a reason for hiding this comment

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

I reverted some suggestions if you don't mind because we hit the path for non-structs as well (e.g. small type primitives) so now it's cleaner that we have a special case for that rare case with IND<struct>.

Copy link
Contributor

@SingleAccretion SingleAccretion Nov 13, 2022

Choose a reason for hiding this comment

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

We still need to handle the case with handle-less BLK nodes though.

Reproduction:

struct Sx3
{
    public byte A, B, C;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static Sx3 Problem(void* s)
{
    Sx3 a;
    Unsafe.CopyBlock(&a, s, 3);
    return a;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, reverted the revert. @SingleAccretion does it look good to you? I assume it's just your change written with my hands 😄

Copy link
Contributor

@SingleAccretion SingleAccretion left a comment

Choose a reason for hiding this comment

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

Yep, looks good!

@EgorBo
Copy link
Member Author

EgorBo commented Nov 14, 2022

@jakobbotsch could you please take a look and sign it off?

CI failure is known: #78290

if (realSize == 0)
{
// TODO-ADDR: delete once "IND<struct>" nodes are no more
realSize = comp->info.compCompHnd->getClassSize(structCls);
Copy link
Member

Choose a reason for hiding this comment

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

I assume it is guaranteed that structCls != NO_CLASS_HANDLE here or we would have hit some BADCODE previously? (e.g. returning struct in method declared to return int)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not that the current behavior tries to be careful in such cases, I've just tried and got an AccessViolationException in Main for it when I changed return type to int while was trying to return a small struct.

@EgorBo EgorBo merged commit 57b286f into dotnet:main Nov 17, 2022
@EgorBo
Copy link
Member Author

EgorBo commented Nov 17, 2022

LLVMAOT failure was known and already fixed in Main

_ptr = mmap(null, 2 * PageSize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
if (_ptr != null && mprotect(_ptr + PageSize, PageSize, PROT_NONE) != 0)
{
munmap(_ptr, 2 * PageSize);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it will result in a double free

Copy link
Member

Choose a reason for hiding this comment

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

(But I doubt that's the cause of #78758)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see the reason now:

On success, mmap() returns a pointer to the mapped area.  On
       error, the value MAP_FAILED (that is, (void *) -1) is returned,
       and [errno](https://man7.org/linux/man-pages/man3/errno.3.html) is set to indicate the error.

So this does not correctly check that we failed to allocate the memory.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2022
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.

Unsafe.ReadUnaligned causes System.AccessViolationException on Windows ARM64
3 participants