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

Convert invalid C# uses of UnmanagedCallersOnly to IL. #42146

Merged
merged 3 commits into from
Sep 13, 2020

Conversation

AaronRobinsonMSFT
Copy link
Member

@am11
Copy link
Member

am11 commented Sep 12, 2020

Thanks! maybe run the outerloop leg (to validate the fix in CI).

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Sep 12, 2020

@am11 That is not needed. I moved this to priority 0 so it now runs in the CI - which is where it should have been when we made the API public.

@@ -371,7 +379,7 @@ .locals init ([0] native int ptr)
il.Emit(OpCodes.Nop);
Copy link
Member

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.

public static Type GetCallbacksType()
{
var asm = Assembly.Load(InvalidCSharpAssemblyName);
return asm.GetType("InvalidCSharp.Callbacks");
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, ProjectReferences do work for build but not to refer to the assembly. The typical issue is the assembly that contains System.Object isn't the same when compiled with ilasm as the one for csc 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:

.assembly extern System.Runtime
{
.publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A ) // .?_....:
.ver 5:0:0:0
}

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.

Copy link
Member

@jkotas jkotas Sep 13, 2020

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

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

error about System.Object from assembly is not known.

This error is typically caused by missing System.Runtime reference, not by having incompatible System.Runtime references.

Copy link
Member Author

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:

error CS0012: The type 'Object' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Runtime, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.

Call site in C#:

InvalidCSharp.Test.Invoke(null, n);

Snippet of IL code being called in C#:

.assembly extern System.Runtime { }
.assembly extern System.Runtime.InteropServices { }

.assembly InvalidCSharp { }

.class public auto ansi beforefieldinit InvalidCSharp.Test
    extends System.Object
{
    .method public hidebysig static 
        void Invoke (
            void* fptr,
            int32 n
        ) cil managed 
    {
        .maxstack 2
        .locals init ([0] native int ptr)

        nop
        ldarg.0
        stloc.0

        ldloc.0
        ldarg.1
        calli int32 *(int32)
        
        ret
    }
}

Copy link
Member

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 ).

Copy link
Member

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 ?

Copy link
Member Author

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 ).

That did it. This is a very confusing issue. But it works now so yay.

Does the C# compiler command line have /r:....\System.Runtime.dll ?

It does.

 /reference:D:\runtime\artifacts\bin\ref\net5.0\System.Runtime.dll

@jkotas
Copy link
Member

jkotas commented Sep 13, 2020

Have you done anything about the test failures before merging?

 You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
  AppDir: /home/helixbot/work/A8DD0933/w/9F1008D3/e/Interop/UnmanagedCallersOnly/UnmanagedCallersOnlyTest/WasmApp/
  TestBinDir: /home/helixbot/work/A8DD0933/w/9F1008D3/e/Interop/UnmanagedCallersOnly/UnmanagedCallersOnlyTest
  ArtifactsBinDir: 
Arguments: --run,UnmanagedCallersOnlyTest.dll,UnmanagedCallersOnlyTest
console.debug: MONO_WASM: Initializing mono runtime
console.debug: MONO_WASM: ICU data archive(s) loaded, disabling invariant mode
console.debug: mono_wasm_runtime_ready fe00e07a-5519-4dfe-b35a-f867dbaf2e28
Initializing.....
Running TestUnmanagedCallersOnlyValid...
Test Failure: System.PlatformNotSupportedException: No native to managed transitions on this platform.
   at System.Delegate.CreateDelegate(Type type, Object firstArgument, MethodInfo method, Boolean throwOnBindFailure, Boolean allowClosed)
   at System.Delegate.CreateDelegate(Type type, Object firstArgument, MethodInfo method, Boolean throwOnBindFailure)
   at System.Delegate.CreateDelegate(Type type, Object firstArgument, MethodInfo method)
   at System.Reflection.Emit.DynamicMethod.CreateDelegate(Type delegateType)
   at Program.TestUnmanagedCallersOnlyValid()
   at Program.Main(String[] args)

ViktorHofer pushed a commit to ViktorHofer/runtime that referenced this pull request Sep 15, 2020
* Convert invalid C# uses of UnmanagedCallersOnly to IL for negative testing.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnmanagedCallersOnly test failing in the outerloop during compilation
3 participants