Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

JIT: retype some return expressions - #21217

Merged
AndyAyersMS merged 2 commits into
dotnet:masterfrom
AndyAyersMS:Fix21051
Nov 28, 2018
Merged

AndyAyersMS merged 2 commits into
dotnet:masterfrom
AndyAyersMS:Fix21051

Conversation

@AndyAyersMS

Copy link
Copy Markdown
Member

The jit is fairly tolerant of byref/native int mismatches for inline arguments
and return values. And some of the new unsafe helpers do this kind of
reinterpretation across call boundaries as well. This sometimes leads the jit
to lose track of byrefs.

A general fix for this is likely somewhat involved. For now we simply detect if
we're about to lose track of a byref when substituting a particular kind of
return expression, and retype the expression to a byref.

Fixes #21051.

The jit is fairly tolerant of byref/native int mismatches for inline arguments
and return values. And some of the new unsafe helpers do this kind of
reinterpretation across call boundaries as well. This sometimes leads the jit
to lose track of byrefs.

A general fix for this is likely somewhat involved. For now we simply detect if
we're about to lose track of a byref when substituting a particular kind of
return expression, and retype the expression to a byref.

Fixes #21051.
@AndyAyersMS

Copy link
Copy Markdown
Member Author

@briansull PTAL
cc @dotnet/jit-contrib

Note this is a targeted workaround for this specific issue.

No diffs (w/ same corelib)

@@ -297,7 +296,6 @@ public static UnicodeCategory GetUnicodeCategory(int codePoint)
////////////////////////////////////////////////////////////////////////

// Workaround for https://github.com/dotnet/coreclr/issues/21051

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also delete the "Workaround" comments?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. Thanks for spotting that.

@briansull briansull left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks Good

@AndyAyersMS

Copy link
Copy Markdown
Member Author

CI got restarted...

@dotnet-bot test this please

@CarolEidt CarolEidt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@AndyAyersMS

Copy link
Copy Markdown
Member Author

Might have restarted a bit too soon.

@dotnet-bot test this please

@AndyAyersMS

Copy link
Copy Markdown
Member Author

CentOS download failure:

/mnt/resource/j/workspace/dotnet_coreclr/master/x64_checked_centos7.1_innerloop_prtest/Tools/dotnetcli/sdk/2.1.301/NuGet.targets(114,5): 
error : Failed to download package 'System.Reflection.Extensions.4.0.0' from 'https://dotnetfeed.blob.core.windows.net/dotnet-core/flatcontainer/system.reflection.extensions/4.0.0/system.reflection.extensions.4.0.0.nupkg'. 

CoreFX tests also seems to have run into a download failure:

23:56:09 Error downloading tests from: https://cloudcijobs.blob.core.windows.net/coreclrci/CoreFXArchives/TestList.json
23:56:09 An error occurred while sending the request.
23:56:09 RUNTEST: CoreFX tests finished with failures. Refer to the following:

OSX disconnected while building tests:

00:57:17 FATAL: command execution failed
00:57:17 java.nio.channels.ClosedChannelException
00:57:17 	at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer.onReadClosed(ChannelApplicationLayer.java:208)

@AndyAyersMS

Copy link
Copy Markdown
Member Author

@dotnet-bot test Windows_NT x64 Release CoreFX Tests
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test CentOS7.1 x64 Checked Innerloop Build and Test

@AndyAyersMS
AndyAyersMS merged commit bd299c4 into dotnet:master Nov 28, 2018
@AndyAyersMS
AndyAyersMS deleted the Fix21051 branch November 28, 2018 20:51
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
JIT: retype some return expressions

Commit migrated from dotnet/coreclr@bd299c4
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AF in in System.Globalization.CharUnicodeInfo:InternalGetNumericValue

4 participants