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

Wasm: CalliIntrinsics.Call<T> is generating invalid calli #7951

Closed
yowl opened this issue Jan 12, 2020 · 4 comments · Fixed by #7954
Closed

Wasm: CalliIntrinsics.Call<T> is generating invalid calli #7951

yowl opened this issue Jan 12, 2020 · 4 comments · Fixed by #7954

Comments

@yowl
Copy link
Contributor

@yowl yowl commented Jan 12, 2020

CalliIntrinsics.Call<int>(
marshalStub,
((void*)((IntPtr*)unboxedStructPtr + offset)), // safe (need to adjust offset as it could be class)
(void*)ptr // unsafe (no need to adjust as it is always struct)

This is currently a problem for the wasm backend. With the input

    // something to exercise CalliIntrinsics.Call<T>
    struct NonBlittableToMarshal
    {
        internal int a;
        internal object b;
    }

    static void TestMarshalStructToPtr()
    {
        StartTest("Test marshal struct to ptr");
        NonBlittableToMarshal s = new NonBlittableToMarshal();
        s.a = 1;
        s.b = null;
        var pParms = Marshal.AllocHGlobal(Marshal.SizeOf(typeof(NonBlittableToMarshal)));
        Marshal.StructureToPtr(s, pParms, false);
        EndTest(true);
    }

This fails at runtime with an invalid signature message, with ASSERTIONS=2, it produces:

Invalid function pointer 4692 called with signature 'iiii'. Perhaps this is an invalid value (e.g. caused by calling a virtual method on a NULL pointer)? Or calling a function with an incorrect type, which will fail? (it is worth building your source files with -Werror (warnings are errors), as warnings can indicate undefined behavior which can cause this). This pointer might make sense in another type signature: as sig "vi" pointing to function _Internal_CompilerGenerated__Module____ManagedToNative_HelloWasm_Program_NonBlittableToMarshal, 

This is saying that there is a method with a void (int) signature at the right function index, but it does not match the called convention which is int (int,int,int). There is a method produced with a vi signature, although it looks incomplete and throws an exception

define void @"Internal_CompilerGenerated__Module___<ManagedToNative>HelloWasm_Program_NonBlittableToMarshal"(i8*) {
Prolog:
  br label %Block0

Block0:                                           ; preds = %Prolog
  %LoadAddressOfSymbolNode = load i32*, i32** @__Str_Struct__NonBlittableToMarshal__6058104189200C7DE718BE6B8F3608243B48FAFC59CD4893FB0EFFADF26C30EA___SYMBOL
  %1 = getelementptr i8, i8* %0, i32 8
  %Temp0_ = getelementptr i8, i8* %0, i32 8
  %LoadeeType = load i32, i32* bitcast (i32** @__EEType_S_P_CoreLib_System_Exception___SYMBOL to i32*)
  call void @S_P_CoreLib_System_Runtime_RuntimeExports__RhNewObject(i8* %1, i8* %Temp0_, i32 %LoadeeType)
  %2 = getelementptr i8, i8* %0, i32 12
  %Temp0_1 = getelementptr i8, i8* %0, i32 8
  %CastPtrTemp0_ = bitcast i8* %Temp0_1 to i8**
  %LdTemp0_ = load i8*, i8** %CastPtrTemp0_
  %3 = getelementptr i8, i8* %2, i32 0
  %CastPtrTypedStore = bitcast i8* %3 to i8**
  store i8* %LdTemp0_, i8** %CastPtrTypedStore
  %CastPtr = bitcast i32* %LoadAddressOfSymbolNode to i8*
  %4 = getelementptr i8, i8* %2, i32 4
  %CastPtrTypedStore2 = bitcast i8* %4 to i8**
  store i8* %CastPtr, i8** %CastPtrTypedStore2
  call void @S_P_CoreLib_System_Exception___ctor_0(i8* %2)
  call void @llvm.trap()
  unreachable
}

And the iiii signature method would match the calli inside

                        CalliIntrinsics.Call<int>(
                            marshalStub,
                            ((void*)((IntPtr*)unboxedStructPtr + offset)),  // safe (need to adjust offset as it could be class)
                            (void*)ptr                                      // unsafe (no need to adjust as it is always struct)
                        );

With an the extra i for the shadow stack. I guess the question starts with how does the marshal code get generated because it shouldn't terminate in a throw I imagine?

@MichalStrehovsky

This comment has been minimized.

Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Jan 13, 2020

The internal object b; field takes us down the COM marshalling path. I think this is just the compiler going down the path where it failed in the middle of generating marshalling code.

CoreRT compiler can't do COM. If you just need something that is not blittable, use e.g. a string.

@yowl

This comment has been minimized.

Copy link
Contributor Author

@yowl yowl commented Jan 13, 2020

Thanks, moving on Wasm is confused by the different arguments to the generated method for the managed to native conversion, in this case:

.method void ManagedToNative__NonBlittableToMarshal(valuetype [HelloWasm]Program/NonBlittableToMarshal&, valuetype Internal.CompilerGenerated.__NativeType__NonBlittableToMarshal&) cil managed
{

And the call from the CalliIntrinsics:

  IL_0003:  calli       int32(void*, void*)

The first are ByRefType and the second PointerType, which normally would be no issue, both pointers. But in Wasm it makes a decision about whether a variable needs to placed on the shadow stack for conservative GC purposes using:

        private static bool CanStoreTypeOnStack(TypeDesc type)
        {
            if (type is DefType defType)
            {
                if (!defType.IsGCPointer && !defType.ContainsGCPointers)
                {
                    return true;
                }
            }
            else if (type is PointerType)
            {
                return true;
            }

            return false;
        }

And each case returns a different value, ByRefType are on the shadow stack, and PointerType are passed normally. I assume that this logic is ok, and ByRefTypes do indeed need to be scanned when looking for roots so maybe I need so special case by identifying somehow the generated method (ManagedToNative__NonBlittableToMarshal) and using some logic, like, "I know this is only going to be called from CalliIntrinsics and put its arguments on the llvm stack". This would of course mean that the parameters are not scanned for GC reporting, which might be ok for this scenario - if there's nothing else referencing the managed struct then its ok to collect.

This accounts for 2 of the "missing" parameters on the call, the other is the return type, the generated method is void but CalliIntrinsics.Call<int> is int32. Is it the case that some generated methods do return something and in cases like this one the return value is never used so its not an issue?

MichalStrehovsky added a commit to MichalStrehovsky/corert that referenced this issue Jan 14, 2020
* The called methods accept a byref, not a pointer.
* The called methods don't return anything.

This is also a slight perf improvement (no longer need to allocate the lambda for `PinObjectAndCall`).

Fixes dotnet#7951.
@MichalStrehovsky

This comment has been minimized.

Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Jan 14, 2020

Yeah, the calli is illegal. Per the ECMA-335 spec, the signature needs to match the destination. I've submitted #7954 to hopefully unblock you. The sig doesn't match exactly either (callsite is ref byte, but the actual method is ref X where X is the struct type), but it should be good enough for the WASM backend.

I think the existing shape was just legacy wart (Project N at some point only supported calli intrinsic that return a value, and doing pointer math with refs used to be impossible with C#).

@yowl

This comment has been minimized.

Copy link
Contributor Author

@yowl yowl commented Jan 14, 2020

Thanks a lot.

@jkotas jkotas closed this in #7954 Jan 14, 2020
jkotas added a commit that referenced this issue Jan 14, 2020
* The called methods accept a byref, not a pointer.
* The called methods don't return anything.

This is also a slight perf improvement (no longer need to allocate the lambda for `PinObjectAndCall`).

Fixes #7951.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.