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

Added "delayed" generation of delegate signatures (follow-up) #81

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xanathar
Copy link
Contributor

This is a reopening of #53, rebased on top of the latest main branch.

I apologize for not replying to the request of an example, sorry.

I'll show an example in this PR; I'm not 100% sure this is the best way to fix the issue, but we can start from this for a discussion.


Description

Just as before, with this patch if a delegate is found during the construction of a method, its declaration is still emitted, after all the other types have been.

This is implemented through an HashSet of forward declarations that is used as now for delegates, but can be expanded to include other item types in the future.


Example

Take this example Rust source:

#[no_mangle]
pub extern "C" fn set_callback(alloc_callback: extern "C" fn(size: usize, data: *const u16) -> *mut u8) {
    println!("Your callback is {alloc_callback:p}");
}

This CORRECTLY generates the following C# code:

namespace ExampleNamespace
{
    internal static unsafe partial class NativeMethods
    {
        const string __DllName = "example.dll";

        [UnmanagedFunctionPointer(CallingConvention.Cdecl)]
        public delegate byte* set_callback_alloc_callback_delegate(System.UIntPtr size, ushort* data);

        [DllImport(__DllName, EntryPoint = "set_callback", CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
        public static extern void set_callback(set_callback_alloc_callback_delegate alloc_callback);
    }
}

So far so good. Now, that Rust declaration is nasty and would probably get refactored as:

type MyCallback = extern "C" fn(size: usize, data: *const u16) -> *mut u8;

#[no_mangle]
pub extern "C" fn set_callback(alloc_callback: MyCallback) {
    println!("Your callback is {alloc_callback:p}");
}

which produces

namespace ExampleNamespace
{
    internal static unsafe partial class NativeMethods
    {
        const string __DllName = "example.dll";

        [DllImport(__DllName, EntryPoint = "set_callback", CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
        public static extern void set_callback(set_callback_alloc_callback_delegate alloc_callback);
    }
}

which is obviously bad because set_callback_alloc_callback_delegate's definition is nowhere to be found.

The proposed fix produces this version instead:

namespace ExampleNamespace
{
    internal static unsafe partial class NativeMethods
    {
        const string __DllName = "example.dll";

        [DllImport(__DllName, EntryPoint = "set_callback", CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
        public static extern void set_callback(set_callback_alloc_callback_delegate alloc_callback);

        [UnmanagedFunctionPointer(CallingConvention.Cdecl)]
        public delegate byte* set_callback_alloc_callback_delegate(System.UIntPtr size, ushort* data);
    }
}

Differences from the version in PR #53

The code is verbatim the same with these small differences:

  • in Added "delayed" generation of delegate signatures. #53 the delegate declaration was being generated at the namespace level (as it does for other types like structs and enums), now I moved it to be at the class level to follow what it would do for the first Rust code in the example above
  • added public visibility modifier, given that the declaration is now inside the class and would default to private instead of internal
  • added the [UnmanagedFunctionPointer(CallingConvention.Cdecl)] boilerplate

@xanathar xanathar changed the title Added "delayed" generation of delegate signatures. Added "delayed" generation of delegate signatures (follow-up) May 29, 2024
@neuecc
Copy link
Member

neuecc commented Jul 8, 2024

sorry for delayed response and thank you for clarification.
Certainly, it seems useful.
I have a CONFLICT due to a merge of another branch, can you please fix it?
If so, we would like to check it out and incorporate it into the release process as soon as possible.

	With this patch if a delegate is found during the construction of a method,
        its declaration is still emitted, after all the other types have been.

        This is implemented through an HashSet of forward declarations that is
        used as now for delegates, but can be expanded to include other item types
        in the future.
@xanathar xanathar force-pushed the pr/delayed-delegate-generation branch from 5475a75 to 9cafa3d Compare July 9, 2024 22:50
@xanathar
Copy link
Contributor Author

xanathar commented Jul 9, 2024

Rebased, should have no conflicts.

@neuecc
Copy link
Member

neuecc commented Jul 10, 2024

Hmm, I tried it with the current main branch, but

type MyCallback = extern "C" fn(size: usize, data: *const u16) -> *mut u8;

#[no_mangle]
pub extern "C" fn set_callback(alloc_callback: MyCallback) {
    println!("Your callback is {alloc_callback:p}");
}

This seems to be expanded to:

[DllImport(__DllName, EntryPoint = "set_callback", CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
internal static extern void set_callback(delegate* unmanaged[Cdecl]<nuint, ushort*, byte*> alloc_callback);

@xanathar
Copy link
Contributor Author

xanathar commented Jul 14, 2024

That's not how it expanded in my tests.
I wonder if it could be configuration or some other difference.

I will recheck, but I'm currently away from "decent computing devices" for a couple of weeks.

@xanathar
Copy link
Contributor Author

xanathar commented Aug 1, 2024

I wonder if it could be configuration or some other difference.

It is configuration.

Namely, it works with csharp_use_function_pointer(true) but not with csharp_use_function_pointer(false). I'll check if it can be made to work in the second case without all this infrastructure.

Copy link
Contributor

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 30 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants