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

Workaround unavoidable marshaling on function pointers #99

Closed
PathogenDavid opened this issue Nov 25, 2020 · 3 comments
Closed

Workaround unavoidable marshaling on function pointers #99

PathogenDavid opened this issue Nov 25, 2020 · 3 comments
Labels
Area-OutputGeneration Issues concerning the process of generating output from Biohazrd Bug Concept-Correctness Issues concerning the correctness of the translation from an ABI perspective Concept-OutputUsability Issues concerning whether or not the output from Biohazrd is actually usable Language-C# Issues specific to C# Workaround

Comments

@PathogenDavid
Copy link
Member

PathogenDavid commented Nov 25, 2020

Due to its heritage as a Windows technology, the CLR really wants to pretend sizeof(bool) == 4 for the purposes of marshaling. Typically you can avoid this by using MarshalAs(I1). The bool will still be marshaled to eliminate invalid boolean values (non-0/1 bools), but it'll be 1 byte at least. Unfortunately, it turns out the calli instruction (which is what function pointer calls are under the hood) perform marshaling too. (For some reason I was under the impression they didn't until I had a a "fun" time debugging bad marshaling in a case where a function returned a bool via al when the CLR used eax. This does make sense though since calli was used heavily for C++/CLI and is expected: dotnet/runtime#40908)

In the case of char, the situation is normally better because if you mark it as being marshaled as U2, U1, or default marshaling on a Unicode PInvoke/struct it will become blittable. (Source) You can enforce this behavior across the entire module using DefaultCharSetAttribute. Unfortunately, again we can't use MarshalAs or similar on function pointers and DefaultCharSetAttribute does not apply to function pointers. (Whether this should be considered a bug is somewhat debatable since it'd make two modules with different default charsets incompatible for function pointer passing.)

In the long term, we'd like to see an escape hatch added to the CLR to allow these types to be safely used in unmanaged function pointers, and I'm working on a duo (now triplet) of proposals to improve the situation. Unfortunately we need a fix today.

As a (hopefully temporary) workaround, Biohazrd will synthesize two tiny wrapper structs to avoid marshaling.

One for booleans: (SharpLab with JIT comparisons)

[StructLayout(LayoutKind.Sequential)]
public readonly struct NativeBoolean
{
    private readonly byte Value;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static implicit operator bool(NativeBoolean b)
        => Unsafe.As<NativeBoolean, bool>(ref b);

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static implicit operator NativeBoolean(bool b)
        => Unsafe.As<bool, NativeBoolean>(ref b);

    // ...
}

And another for chars: (SharpLab with JIT comparisons)

[StructLayout(LayoutKind.Sequential)]
public readonly struct NativeChar
{
    // For the purposes of sign extension the CLR treats char as unsigned.
    private readonly ushort Value;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static implicit operator char(NativeChar c)
        => Unsafe.As<NativeChar, char>(ref c);

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static implicit operator NativeChar(char c)
        => Unsafe.As<char, NativeChar>(ref c);

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool operator ==(NativeChar a, NativeChar b)
        => a.Value == b.Value;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool operator !=(NativeChar a, NativeChar b)
        => a.Value == b.Value;
    
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool operator ==(char a, NativeChar b)
        => a == b.Value;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool operator !=(char a, NativeChar b)
        => a == b.Value;
    
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool operator ==(NativeChar a, char b)
        => a.Value == b;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool operator !=(NativeChar a, char b)
        => a.Value == b;
        
    // Equals, etc...
}

One thing to note from the JIT disassembly is that for whatever reason the JIT does not enregister these structs for some reason. (It does enregister similar structs that wrap int, so I assume this is an issue of their size.) This probably causes a small perf hit with these structs, but it will pass them on the stack and in registers for P/Invokes. It seems like dotnet/runtime#37745 (which fixed dotnet/runtime#8016) should've caused these to be enregistered, but I need to look into it more.

It is unclear how valuable implementing the various instance methods on System.Boolean and System.Char on these types will be. In general native consumers should not use these types directly. but they can be desirable for making a native function calls where the return value is immediately consumed feel more natural. Initially I won't provide these, but they will likely be added in later.

Ideally we should not expose these types anywhere on the generated API surface except where absolutely necessary. Eventually we should support a few generation modes:

  • Always use native wrappers
  • Always use native wrappers at interop boundaries
  • Only use when MarshalAs can't be used
  • Always use if MarshalAs would've been required (IE: Everywhere that isn't a pointer to bool or char)
  • Assume fictional/future modified runtime that doesn't marshal bool or char ever.
@PathogenDavid PathogenDavid added Concept-Correctness Issues concerning the correctness of the translation from an ABI perspective Bug Area-OutputGeneration Issues concerning the process of generating output from Biohazrd Language-C# Issues specific to C# Concept-OutputUsability Issues concerning whether or not the output from Biohazrd is actually usable labels Nov 25, 2020
@PathogenDavid
Copy link
Member Author

PathogenDavid commented Nov 25, 2020

Since we can make use char for NativeChar.Value and still have it be blittable by marking the struct as Unicode, I decided to go with that instead. In general it results in the same codegen (sometimes marginally better or worse*), but makes the implementation clearer. (SharpLab)

*The worse scenario is this one:

    public static NativeChar Test(char c)
        => c;

For some reasonthe JIT emits an unnecessary movzx when we use char instead of ushort:

Tests.Test(Char)
    L0000: movzx eax, cx
    L0003: movzx eax, ax
    L0006: ret

@PathogenDavid
Copy link
Member Author

One problem that came up while implementing this: Using these types prevents using default parameter values.

Furthermore, it revealed a bug in the constant emit where it can emit a constant when it isn't valid to do so:

public unsafe IIteratorLayer* addIterator(ITensor* tensor, int axis = 0, NativeBoolean reverse = 0)

PathogenDavid added a commit that referenced this issue Nov 25, 2020
… use in function pointers where marshaling can't be customized.

See #99 for details.
@PathogenDavid
Copy link
Member Author

PathogenDavid commented Nov 25, 2020

Workaround implemented in 120b8de

Ideally I would like to make this behavior optional whenever the runtime or language supports treating bool and char as blittable on function pointers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-OutputGeneration Issues concerning the process of generating output from Biohazrd Bug Concept-Correctness Issues concerning the correctness of the translation from an ABI perspective Concept-OutputUsability Issues concerning whether or not the output from Biohazrd is actually usable Language-C# Issues specific to C# Workaround
Projects
None yet
Development

No branches or pull requests

1 participant