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

marshalling va_list crossplatform p/invoke binding #9316

Closed
mfkl opened this issue Nov 22, 2017 · 18 comments
Closed

marshalling va_list crossplatform p/invoke binding #9316

mfkl opened this issue Nov 22, 2017 · 18 comments

Comments

@mfkl
Copy link
Contributor

mfkl commented Nov 22, 2017

Hello from VideoLAN!

We're currently implementing a new crossplatform .NET binding over libvlc and have ran into a bit of an issue. Our libvlc log callback prototype looks like this:

typedef void(* libvlc_log_cb) (void *data, int level, const libvlc_log_t *ctx, const char *fmt, va_list args)

We need to marshal this from native to managed in a crossplatform way.

What is the recommended way to perform va_list marshalling with P/Invoke in a crossplatform way (i.e. Xamarin platforms + friends)?

From what we see in clrvarargs.cpp, we assume this is currently unsupported on Linux. What is needed to be done to be able to support this?

Related issues:
https://github.com/dotnet/coreclr/issues/9204
dotnet/standard#20

Thanks for any pointers you may provide.

/cc @jeremyVignelles

@jeremyVignelles
Copy link

Additional info to clear things up:
We need to register a managed callback inside the native library (libvlc).
I am the contributor of an existing Vlc wrapper in .net net, and I have implemented that using a hack:
https://github.com/ZeBobo5/Vlc.DotNet/blob/develop/src/Vlc.DotNet.Core/VlcMediaPlayer/VlcMediaPlayer.Events.Log.cs#L55

va_args are interpreted as an IntPtr and given to the formatting function. This works well on Windows, but did not have any luck on linux.

@jkotas
Copy link
Member

jkotas commented Nov 22, 2017

This works well on Windows, but did not have any luck on linux.

Yes, varargs structure size and internal details are platform specific. IIRC, it is 3 pointer size on Linux x64. You can try to work around this by having Linux x64 specific version of the callback that takes 3 pointer sized structure as argument.

@jeremyVignelles
Copy link

@jkotas : Thanks for your answer! I will try that. Do you have a documentation, a source code or something for that "magic" 3-pointers size?
What about linux x86?
linux ARM?
Is there something specific to the calling convention?

@jkotas
Copy link
Member

jkotas commented Nov 22, 2017

Yes, size of va_list depends on the calling convention unfortunately. You can find the size by running printf("%d\n", sizeof(va_list)); on the target platform.

something for that "magic" 3-pointers size

I think something like this should work: struct VarArg_Linux_x64 { IntPtr x1; IntPtr x2; IntPtr x3 }.

Does your binding have unmanaged part in C/C++? It would be better to deal with the varargs in C/C++.

@jeremyVignelles
Copy link

The question about the calling convention was just for my own understanding and curiosity 🙂

We currently do not have any unmanaged part in our binding, and I'd like to avoid that as much as possible, because that would mean compiling another lib for each platform (say win32/64, linux 32/64, linux rasp, android?, tizen?...) before building our .net interop library. Those native libraries would then be included in a cross platform "libvlcsharp" nuget package, but only one would be really used. This doesn't feel clean, and I'd prefer having everything in .net

I'll come back here once I've done some testing.

@jeremyVignelles
Copy link

Sharing my findings here : This archived pdf https://www.uclibc.org/docs/psABI-x86_64.pdf at page 52 suggests that va_list is a structure with 4 fields (2 unsigned ints and 2 pointers)

This pdf http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf at page 27 has the declaration of va_list for the ARM architecture

@jeremyVignelles
Copy link

Hi,
As promised, I'm coming back here with results of my explorations.
Thanks to your tips, I managed to do interops for linux x86/x64. My work is open source at https://github.com/jeremyVignelles/va-list-interop-demo . If you have time, I'd appreciate that you could tell me if it's totally bullshit or not. It is mostly based on experiments, and I don't know the conditions under which it works.

That being said, and now that I am a little more confident about how these stuff works, would you like me to make some contributions on the clrvarargs.cpp file to support that?
I'm still wondering, what's the purpose of the MarshalToManagedVaList / MarshalToUnmanagedVaList methods? How is that supposed to be used in .net?

@jkotas
Copy link
Member

jkotas commented Dec 3, 2017

The MarshalToManagedVaList / MarshalToUnmanagedVaList methods are left overs from .NET Framework managed C++ support (#659). I do not think they are really used in .NET Core.

Thank you for your offer to contribute to CoreCLR. Could you please clarify what you would like to add support for exactly? A good way to clarify it would be to show a C# code snippet that will demonstrate what one will be able to do in .NET Core with your change, that is not possible today.

@jeremyVignelles
Copy link

Ok, thanks for the clarification.
Just thinking out loud, I'd like something like this to exist, in a wonderful world:
void LibvlcCallback(IntPtr data, int level, IntPtr logContext, string format, VaList args)
I don't know if that VaList would be usable in .net, but the idea is to pass it to another native function like

        [DllImport("msvcrt.dll", CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)]
        public static extern int vsprintf(
            IntPtr buffer,
            string format,
            VaList args);

for windows.
I don't know if that is even possible, but the upstream code of native-managed bindings like libvlc would be simplified if the va_list scenario could be handled easily.

@jkotas
Copy link
Member

jkotas commented Dec 3, 2017

VaList would be a new .NET Core public API, and so it would need to go through https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md.

I think that we would just want to add back the .NET Framework ArgIterator for this, and make it usable for more than just the very narrow case of getting valist from one PInvoke and passing it to another PInvoke - it is non-trivial feature though. We have decided to drop the ArgIterator from .NET Core because of it is rarely used, it does not have first class support in C#, but it is expensive to implement for every new platform.

@jeremyVignelles
Copy link

I'm not quite familiar with the ArgIterator class, but from what I understood, it's for methods with ... arguments, which is a different things than va_list.
Currently, I do not have any clear idea of the ideal API that could come out of this discussion. Is anything even possible?

Let's sum up

  • va_list, va_arg, va_start, va_end and va_copy are things that are universally available, but their implementation varies a lot.
  • When you need to call this kind of methods (vsprintf for example) from managed code, you will have a bad time, because you will need to know how those things are built.
  • When you need to be called from native methods, and be given a va_list, it is also hard to read things from that, because you need to know how things are stored in memory.
  • I think that most of the times, va_list and ... in native code are used for printf-like formatting (please tell me if you see any other use case), and when you have a callback like this, you likely want to call the native printf function family as well, but you rarely need to have a callback on his own, or have a vsprintf call alone (why not using the .net formatting capabilities?)
  • Having a va_list callback in code is mainly designed to let the callback decide if he wants to format the string or not (otherwise, it would have taken juste a const char* as an argument). This implies that the solution chosen should be able to let the .net code to make a decision about whether the string has to be formatted (and not just "combine arguments" into a string)
  • I have currently no idea on how to make it the best way possible, and as generic as possible. Here are a few options on my mind
    • Don't change anything and let clients do things in managed code, like this : https://github.com/jeremyVignelles/va-list-interop-demo. This requires a lot of research work on any supported platform and feels hackish.
    • Don't change anything, but encourage clients to do that kind of things in native code. I'm not really satisfied by this solution because it requires to build another project (if you don't have access to the native library) on each of your targets platform. On the other hand, you have full control of the platforms you're deploying on and the native code is not that complex.
    • Integrate the ability to do that in core CLR for all possible use cases and all platforms. This still need to be defined
    • Integrate the ability to do only printf-like scenarios on all platforms. This, in my opinion, covers most of the use cases.

Still thinking, but what do you think of this kind of things?

[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
public delegate void Callback(string format, IntPtr args);

where args is a va_list
becomes

[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
public delegate void Callback(VPrintfFormattedString string);
public class VPrintfFormattedString {
   public string Format();
}

This still needs reflexion. In any case, thanks for your consideration, Merry Christmas and Happy New Year :)

@jkotas
Copy link
Member

jkotas commented Dec 24, 2017

ArgIterator is equivalent of va_list .NET Framework. The undocumented C# keyword __arglist is equivalent of .... The following works in .NET Framework:

using System;
using System.Runtime.InteropServices;

class Program
{
    [DllImport("msvcrt")]
    extern static void vprintf(string format, ArgIterator va_list);

    static void Log(string s, __arglist)
    {
        vprintf(s, new ArgIterator(__arglist));
    }

    static void Main(string[] args)
    {
        Log("%d %d", __arglist(1, 2));
    }
}

It does not work in .NET Core. ArgIterator does not exist in .NET Core today.

@option-blackfire
Copy link

@jkotas __arglist doesn't work for a callback that needs to be supplied from the managed world to the unmanaged world. ArgIterator doesn't work as far as I can tell. I already tried to make it work but couldn't. Every method throws an exception.

@jkotas
Copy link
Member

jkotas commented Jan 8, 2018

__arglist doesn't work for a callback

Yes, that's correct, for both .NET Core and .NET Framework in C#. I think it worked in .NET Framework in managed C++. The story for vargs interop was always far from perfect.

@philcarbone
Copy link

Wondering if anyone could provide guidance on this:
https://github.com/dotnet/coreclr/issues/659#issuecomment-380813183

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@simplexidev
Copy link

Have there been any updates on this issue? I seem to have ran into this issue as well while looking into writing a binding for raylib.

@jeremyVignelles
Copy link

Hello.
We didn't manage to propose a correct API shape, so at VideoLAN, we came up with a manual solution of using the libraries available on the platform :

https://github.com/videolan/libvlcsharp/blob/ff246a39fb9e35541357ffcd90062a8c0cc431f6/src/LibVLCSharp/Shared/LibVLC.cs#L729

https://github.com/videolan/libvlcsharp/blob/ff246a39fb9e35541357ffcd90062a8c0cc431f6/src/LibVLCSharp/Shared/Helpers/MarshalUtils.cs#L66

We're not satisfied with the solution, but it seems to work. Still feels hackish.

We have also explored the possibility to change the API, but we faced strong opposition from the libvlc developpers.

Do you also need that for logs?
Feel free to come and chat with us on the LibVLCSharp gitter 🙂

@AaronRobinsonMSFT
Copy link
Member

Closing here and tracking in #48796.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants