Skip to content

Conversation

AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Nov 24, 2019

In some cases we may end up in lvaSetClass without a valid ref class handle
from either the IR or the stack. Use the handle for object as a conservative
fallback.

Closes dotnet/coreclr#27923 (aka #13817)

In some cases we may end up in `lvaSetClass` without a valid ref class handle
from either the IR or the stack. Use the handle for `object` as a conservative
fallback.

Closes dotnet/coreclr#27923.
@AndyAyersMS
Copy link
Member Author

cc @dotnet/jit-contrib

@maryamariyan maryamariyan added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 25, 2019
Copy link
Member

@erozenfeld erozenfeld left a comment

Choose a reason for hiding this comment

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

LGTM

@AndyAyersMS AndyAyersMS merged commit ffdb313 into dotnet:master Nov 26, 2019
@AndyAyersMS AndyAyersMS deleted the FixForMissingClassHandle branch November 26, 2019 08:16
AndyAyersMS added a commit to AndyAyersMS/coreclr that referenced this pull request Dec 16, 2019
Fix for #27923

The jit might fail to locate a class handle for a ref class, leading to an
unexpected crash while jitting.

## Customer Impact
Unexpected and hard to diagnose crash/exception

## Regression?
Yes, introduced during the development 3.0 cycle. 2.x behaves correctly.

## Testing
Verified the user's test case now passes; no diffs seen in any existing
framework or test code.

## Risk
**Low**: the jit will now fall back to using the handle for System.Object if no
better option can be found.

cc @BruceForstall

____

In some cases we may end up in lvaSetClass without a valid ref class handle
from either the IR or the stack. Use the handle for object as a conservative
fallback.
Anipik pushed a commit to dotnet/coreclr that referenced this pull request Jan 14, 2020
Fix for #27923

The jit might fail to locate a class handle for a ref class, leading to an
unexpected crash while jitting.

## Customer Impact
Unexpected and hard to diagnose crash/exception

## Regression?
Yes, introduced during the development 3.0 cycle. 2.x behaves correctly.

## Testing
Verified the user's test case now passes; no diffs seen in any existing
framework or test code.

## Risk
**Low**: the jit will now fall back to using the handle for System.Object if no
better option can be found.

cc @BruceForstall

____

In some cases we may end up in lvaSetClass without a valid ref class handle
from either the IR or the stack. Use the handle for object as a conservative
fallback.
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Nov 4, 2020
An interface call with a null `this` needs to throw a `NullReferenceException`. There are two ways to achieve that: either codegen can insert explicit code to trigger a NullRef at the callsite (before calling into the runtime library to do the dispatch), or the runtime library needs to be able to unwind the helper code that tries to resolve the dispatch and blame the callsite. .NET Native did the former, but the latter produces smaller code (no need to insert the checks). The CoreRT runtime doesn't have code to handle the latter. This pull request is adding that.

The fix is relatively straightforward - we mark locations in the helper where AV can occur. If an AV occurs outside managed code, we check whether it's one of the known locations. If so, we remove the helper method frame off the stack and pretend the AV happened in the calling code.

The one-liner fix in CorInfoImpl fixes an unintialized value problem that basically made us roll a dice on whether to do an explicit nullcheck at the callsite.

Fixes dotnet#221.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
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.

3 participants