Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Convert invalid C# uses of UnmanagedCallersOnly to IL. #42146
Convert invalid C# uses of UnmanagedCallersOnly to IL. #42146
Changes from 1 commit
3036986
23e6b2f
fa02c18
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ProjectReferences seems to be working fine for test projects. For example: https://github.com/dotnet/runtime/blob/master/src/tests/baseservices/callconvs/TestCallingConventions.csproj#L11
Project reference would be even better than
Assembly.Load
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
ProjectReference
s do work for build but not to refer to the assembly. The typical issue is the assembly that containsSystem.Object
isn't the same when compiled withilasm
as the one forcsc
and during assembly reference an error occurs. The only way I have found to circumvent this is to load during runtime.The reason the referenced test works is because of the following:
runtime/src/tests/baseservices/callconvs/CallFunctionPointers.il
Lines 4 to 8 in c867337
This is fine for now, but when we version to .NET 6, this will break. The current approach is the only way I have found that continue to work through time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think this should be a problem. What is the csc error that you are seeing? I know that csc was picky about situations like these in the past, but these checks were removed a while ago. They did not make sense for .NET Core nor for .NET Framework with binding redirects.
It is fairly normal to have multiple different System.Runtime references in .NET Core world. For example, it is fully supported for project targeting net6 (that has System.Runtime 6.0) to reference project targeting net5 (that has System.Runtime 5.0.).
If you do not like hardcoding versions, you can just omit them. For example: https://github.com/jkotas/runtime/blob/5095f1f75f86c2aedaedf9e35457cb6635a3c989/src/tests/Regressions/coreclr/16355/boring.il#L4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hitting the error about
System.Object
from assembly is not known. However, when I try it now I don't see that... this is unsettling because I did try that first. I'm not a fan of not understanding why I hit that error for 20 minutes changed to the runtime load but now it doesn't repro.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that works just fine throughout. Any idea how I could have made that error occur? That is frustrating beyond belief. I will push up a new PR with the IL updates as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is typically caused by missing System.Runtime reference, not by having incompatible System.Runtime references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is what I am seeing now.
Failure during build of C# test application:
Call site in C#:
Snippet of IL code being called in C#:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe C# compiler needs the public key tokens to match? You can try adding
.publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A )
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the C# compiler command line have
/r:....\System.Runtime.dll
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That did it. This is a very confusing issue. But it works now so yay.
It does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dynamic IL generation can be moved to the IL project too. It would improve the test coverage for AOT.
It can be done as a separate PR.