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

Native Library Loader API #31612

Closed
annaaniol opened this issue Aug 29, 2018 · 86 comments
Closed

Native Library Loader API #31612

annaaniol opened this issue Aug 29, 2018 · 86 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@annaaniol
Copy link
Contributor

This issue introduces a NativeLibrary abstraction for loading native libraries from managed code in a platform independent way. It provides two key functionalities

  • A facility to register a callback to implement custom native library load logic from managed codeper assembly.
  • A few methods to handle native library manipulations from managed code.

Context

This API is introduced as part of implementing the Dllmap feature. This issue introduces a generic callback strategy, where DllMap is one specific implementation using the API.

Approved API

namespace System.Runtime.InteropServices
{
   /// <summary>
   /// A delegate used to resolve native libraries via callback.
   /// </summary>
   /// <param name="libraryName">The native library to resolve</param>
   /// <param name="assembly">The assembly requesting the resolution</param>
   /// <param name="DllImportSearchPath?">The DllImportSearchPathsAttribute on the PInvoke, if any. 
   ///         Otherwise, the DllImportSearchPathsAttribute on the assembly, if any. Otherwise null.
   /// </param>
  /// <returns>The handle for the loaded native library on success, null on failure</returns>  
   public delegate IntPtr DllImportResolver(string libraryName,
                                             Assembly assembly,
                                             DllImportSearchPath? searchPath);

    public static partial class NativeLibrary
    {
        // Typical or for dependencies which must be there or failure should happen.

        public static IntPtr Load(string libraryPath);
        public static IntPtr Load(string libraryName,
                                  Assembly assembly,
                                  DllImportSearchPath? searchPath);

        // For fast probing scenarios:

        public static bool TryLoad(string libraryPath,
                                   out IntPtr handle);
        public static bool TryLoad(string libraryName,
                                   Assembly assembly,
                                   DllImportSearchPath? searchPath,
                                   out IntPtr handle);

        public static void Free(IntPtr handle);

        public static IntPtr GetExport(IntPtr handle, string name);
        public static bool TryGetExport(IntPtr handle, string name, out IntPtr address);

       /// <summary>
       /// Set a callback for resolving native library imports from an assembly.
       /// This per-assembly callback is the first attempt to resolve native library loads 
       /// initiated by this assembly.
       ///
       /// Only one resolver callback can be registered per assembly. Trying to register a second
       /// callback fails with InvalidOperationException.
       /// </summary>
       /// <param name="assembly">The assembly for which the callback is registered</param>
       /// <param name="resolver">The callback to register</param>
       /// <exception cref="System.ArgumentNullException">If assembly or callback is null</exception>
       /// <exception cref="System.InvalidOperationException">If a callback is already set for this assembly</exception>
        public static void SetDllImportResolver(Assembly assembly, DllImportResolver resolver);
    }
}

Related

Earlier Proposal

namespace System.Runtime.InteropServices
{
    /// <summary>
    ///  NativeLibrary class is an abstraction for loading native libraries
    ///  from managed code in a platform independent way.
    /// </summary>
    public class NativeLibrary
    {
        /// <summary>
        ///  NativeLibrary constructor, given a name and a library handle
        /// </summary>
        /// <param name="libraryName">The library Name</param>
        /// <param name="handle">The library handle obtained from the appropriate system call</param>
        /// <exception cref="System.ArgumentNullException">Thrown when libraryName equals null or handle equals IntPtr.Zero.</exception>
        public NativeLibrary(string libraryName, IntPtr handle)
        {
            Name = libraryName ?? throw new ArgumentNullException("Name of a NativeLibrary can't be set to null.");
            if (handle == IntPtr.Zero)
                throw new ArgumentNullException("Handle of a NativeLibrary can't be set to IntPtr.Zero.");
            Handle = handle;
        }

        /// <summary>
        /// The simple Library loader 
        /// This is a wrapper around OS loader, using "default" flags
        /// This method facilitates platform independent native library load in the simple case
        /// More complex loads will need to PInvoke to the platform-specific system calls.
        /// </summary>
        /// <param name="libraryName">The name of the native library to be loaded</param>
        /// <returns>The NativeLibrary object for the loaded library</returns>  
        /// <exception cref="System.DllNotFoundException ">Thrown if the library can't be found.</exception>
        /// <exception cref="System.BadImageFormatException">Thrown if the library is not valid.</exception>
        public static NativeLibrary Load(string libraryName);

        /// <summary>
        /// Load a native library
        /// This loader API exposes high-level functionality. Given a library name, it searches 
        /// specific paths based on runtime configuration and attributes of the calling module.
        /// This Load() method works at a 'lower-level' than the managed call-backs for native 
        /// library loading. That is, the NativeLibrary.Load() methods do not call back into
        /// * The Per-assembly function registered via NativeLibrary.RegisterNativeLibraryLoadCallback()
        /// * Or AssemblyLoadContext.LoadUnmanagedDll()
        /// </summary>
        /// <param name="libraryName">The name of the native library to be loaded</param>
        /// <param name="dllImportSearchPath">The search path</param>
        /// <param name="assembly">The assembly loading the native library</param>
        /// <returns>The NativeLibrary object for the loaded library</returns>  
        /// <exception cref="System.DllNotFoundException ">Thrown if the library can't be found.</exception>
        /// <exception cref="System.BadImageFormatException">Thrown if the library is not valid.</exception>
        public static NativeLibrary Load(string libraryName, DllImportSearchPath dllImportSearchPath, Assembly assembly);

        /// <summary>
        /// Close a Native library
        /// This function is a simple wrapper around platform-specific OS calls.
        /// </summary>
        /// <returns>True, if the operation succeeds</returns>  
        /// <exception cref="System.InvalidOperationException">If the NativeLibrary object is not backed by an open handle</exception>
        public bool Free();

        /// <summary>
        /// Get the address of a Symbol
        /// This is a simple wrapper around OS calls, and does not perform any name mangling.
        /// </summary>
        /// <returns>The address of the symbol, if it exists in this NativeLibrary</returns>  
        /// <exception cref="System.InvalidOperationException">If the NativeLibrary object is not backed by an open handle</exception>
        /// <exception cref="System.ArgumentException">If the symbol is not found</exception>
        public IntPtr GetProcAddress(string name);

        /// <summary>
        ///  The actual library handle obtained via LoadLibrary() or dlOpen() operations
        /// </summary>
        public IntPtr Handle { get; }

        /// <summary>
        /// The name referenced by the importing assembly
        /// ToString() can return more detailed/debugging information (ex: mapped-name, loaded location, etc)
        /// </summary>
        public string Name { get; } // Will Return the name referenced by the importing assembly, set only for debugging purpose.

        /// <summary>
        /// Method to register a callback to perform NativeLibraryLoads
        /// The callbacks are registered per-assembly, and can implement custom native library load logic
        /// At most one callback can be registered per assembly
        /// Callbacks can be unregistered by passing a null 'callback' parameter
        /// </summary>
        /// <param name="assembly">Assembly with which this callback should be associated </param>
        /// <param name="callback">The callback function</param>
        /// <returns>True, if the callback was updated</returns>  
        /// <exception cref="System.ArgumentNullException">If the assembly argument is null</exception>
        /// <exception cref="System.InvalidOperationException">Thrown when there is already a callback registered for the specified assembly.</exception>
        public static bool RegisterNativeLibraryLoadCallback(Assembly assembly, Func<string, DllImportSearchPath, Assembly, NativeLibrary> callback);
    }
}
@jkotas
Copy link
Member

jkotas commented Aug 29, 2018

A few comments:

  • Should GetEntrypointDelegate take the library handle as an argument; not library name? If it takes libraryName, we do not need LoadNativeLibraryDelegate. All work can be done by GetEntrypointDelegate.
  • I assume NativeLibrary.Name is for diagnostic purposes only. It should be enough to have overriden ToString method for diagnotic purposes. We should not need the Name method for it.
  • The methods to register the callback should not be on Assembly itself. We do not want to add interop-specific methods to Assembly.

For the callback alternative, the existing Marshal or NativeLibrary would be a good place for it and it can be a single method for both, like:

static class Marshal // or NativeLibrary
{
    static void RegisterNativeLibraryResolver(Assembly assembly, LoadNativeLibraryDelegate loadNativeLibrary, GetEntrypointDelegate getEntryPoint = null);
}

For the events alternative, I am not sure.

@AaronRobinsonMSFT
Copy link
Member

@swaroop-sridhar
Copy link
Contributor

I think the title shouldn't say "Dllmap specific events."
While the API is being proposed in the context of implementing DllMap, the APIs are designed to be generically useful for anyone who wants to customize loading of native binaries -- and should be reviewed in that context.

For example, the RegisterGetEntrypointCallBack may be used to locate entry-points that are embedded/linked within a bundled executable. (cc: @sbomer @sdmaclea)

@swaroop-sridhar
Copy link
Contributor

I also think that GetEntrypointDelegate() should take a NativeLibrary handle, since this information is unique enough to identify all other information, if necessary.

@swaroop-sridhar
Copy link
Contributor

We need two pieces of information from NativeLibrary for diagnostic purposes.

  • The Name() or ToString() which refers to the name by which the calling-assembly refers to the imported library
  • the Path() to the actual native library that got loaded (say, after DllMap).

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 29, 2018

  • Can a native library be unloaded?
  • Does the dll/so/dylib have to be located on storage or can they be memory resident only, in which case path wouldn't be distinguishing feature.
  • Even though the Name is for diagnostic purposes it may be wanted at runtime for logging etc, the debugger is not the only way to want to inspect this so I support keeping it.
  • Events vs callbacks doesn't really matter as long as both provide multiple subscribers, AppDomain.AssemblyResolve is an event for historic reasons so I would tend to side with the event mechanism for consistency but any technical merit could outweigh that.

@svick
Copy link
Contributor

svick commented Aug 29, 2018

  • What is the relationship between this proposal and https://github.com/dotnet/corefx/issues/17135, which is marked as approved?
  • For the events version, is it okay that it doesn't follow Framework Design Guidelines by not using EventHandler<T>, EventArgs and having non-void return type?
  • For the callbacks version, why do the Register methods return the delegate back? And is it okay that there doesn't seem to be any way to unregister?

@jkotas
Copy link
Member

jkotas commented Aug 29, 2018

the Path() to the actual native library that got loaded

You cannot always tell the path that got actually loaded.

@AaronRobinsonMSFT
Copy link
Member

What is the relationship between this proposal and #20635, which is marked as approved?

@svick, That is a good point. We need to reconcile these two proposals. cc @danmosemsft and @mellinoe.

@AaronRobinsonMSFT
Copy link
Member

Can a native library be unloaded?

I am inclined to say 'no'. The CLR has a number of caching mechanisms for native library scenarios and I don't know how easy it would be to properly update all of them. There is the native library cache that is for load library and we would also need to look at DllImport and make sure all related native method descs can be removed. There could be more in the CLR and that is not even getting to the scenario where users could cache delegates. I don't see an easy path forward to support unloading from the API itself.

@jeffschwMSFT
Copy link
Member

@jkotas

Should GetEntrypointDelegate take the library handle as an argument; not library name?

The intention of providing the non-modified version of libraryname and entrypoint from the dllmap is to provide the right information to the developer to choose to either return the symbol as declared in code or find a suitable replacement. The NativeLibrary would then be completely managed by the developer and the runtime/fx would not use it as an exchange type.

For the callback alternative, the existing Marshal or NativeLibrary would be a good place for it and it can be a single method for both

We discussed the callback mechanism with the caveat that it would likely use an override semantic and only respect the last callback provided for a given assembly. In that case returning a potenial overriden callback seemed important.

@Wraith2

Events vs callbacks doesn't really matter as long as both provide multiple subscribers

The current design is ambigous on this point, but the initial thought would be that events are multi subscriber but callbacks would be single with the caller having to chain if one is overwritten.

@svick, @AaronRobinsonMSFT

What is the relationship between this proposal and #20635, which is marked as approved?
@svick, That is a good point. We need to reconcile these two proposals. cc @danmosemsft and @mellinoe.

This is the reconcilation, eg. new version. We are drafting off of the approved proposal but there are differences in how complete a solution we are proposing.

@annaaniol
Copy link
Contributor Author

@svick I updated the events version to follow the Framework Design Guidelines:

namespace System.Reflection
{
    public abstract partial class Assembly
    {
        public static event EventHandler<LoadNativeLibraryArgs> LoadNativeLibrary;
        public static event EventHandler<GetEntrypointArgs> GetEntrypoint;
    }

    public class LoadNativeLibraryArgs : EventArgs
    {
        public LoadNativeLibraryArgs(string libraryName, DllImportSearchPath dllImportSearchPath, Assembly callingAssembly)
        {
            LoadedAssembly = loadedAssembly;
            DllImportSearchPath = dllImportSearchPath;
            CallingAssembly = callingAssembly;
        }

        public string LibraryName { get; set; }
        public DllImportSearchPath DllImportSearchPath { get; set; }
        public Assembly CallingAssembly { get; set; }
    }

    public class GetEntrypointArgs : EventArgs
    {
        public GetEntrypointArgs(string libraryName, string entrypointName)
        {
            LoadedAssembly = loadedAssembly;
            EntrypointName = entrypointName;
        }

        public string LibraryName { get; set; }
        public string EntrypointName { get; set; }
    }
}


namespace System
{
    public delegate void LoadNativeLibraryEventHandler(object sender, LoadNativeLibraryArgs args);
    public delegate void GetEntrypointEventHandler(object sender, GetEntrypointArgs args);
}

@jkotas
Copy link
Member

jkotas commented Aug 29, 2018

The intention of providing the non-modified version of libraryname and entrypoint from the dllmap is to provide the right information to the developer to choose

This is not providing complete information to resolve the entrypoint. It would also need to provide the signature of the method and the method attributes to do the right name mangling. The name mangling is actually a pretty hard problem as Levi pointed out.

I am wondering whether we need to have the callback for entrypoint remapping at all. Limiting this to library name remapping would simplify the design a lot.

Do we know how often is the entrypoint remapping done via dllmap today? It may be useful to collect some statistics about it. All real examples of dllmap that I have seen have done library name remapping only. If there are a few rare cases that need entrypoint remapping, they can do it via delegates - it does not need to be built in.

@jkotas
Copy link
Member

jkotas commented Aug 29, 2018

@annaaniol You can edit the post at the top to incorporate the feedback. It makes it easy to see the current version of the proposal in its entirety.

@annaaniol
Copy link
Contributor Author

@jkotas

Do we know how often is the entrypoint remapping done via dllmap today? It may be useful to collect some statistics about it.

How can we collect such statistics?
I'd say such a simplification will strongly limit the functionality which is we don't want to happen.

@jkotas
Copy link
Member

jkotas commented Aug 29, 2018

How can we collect such statistics?

@jeffschwMSFT should be able to help to connect you with the right people. We have data mining experts who should be able to do this easily.

@jeffschwMSFT
Copy link
Member

@jkotas

Do we know how often is the entrypoint remapping done via dllmap today?

I feel that the entrypoint portion is pretty important. Regarding the name mangling, if GetNativeEntryPoint provides the same level of name mangling as the original p/invoke, won't that be sufficient?

@jeffschwMSFT should be able to help to connect you with the right people. We have data mining experts who should be able to do this easily.

A quick search on github for dllmap and dllentry yields hits for both. I agree with Jan that many of them are just for a dll, but there are certainly plenty (that is a scientific measurement) that have name and target.

https://github.com/search?p=2&q=dllentry&type=Code

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 29, 2018

  • No unload is fine with good reasoning.
  • If the callback version is likely to be single subscriber then I'd prefer the event version for safety.
  • If the revised version uses EventHandler<T> the delegates in the System namespace aren't needed anymore I don't think.

/cc @masonwheeler

@masonwheeler
Copy link
Contributor

@Wraith2 Yes?

There doesn't seem to be much of anything here that impacts my reasoning wrt why DllMap is a bad idea that should not be incorporated into .Net Core.

@swaroop-sridhar
Copy link
Contributor

swaroop-sridhar commented Aug 29, 2018

You cannot always tell the path that got actually loaded.

Path is probably a bad name (for assets like memory mapped loads etc) . However, we'll need some string to report the information here (mapped name / path / memory identifier, etc.) for diagnostic purpouses

@swaroop-sridhar
Copy link
Contributor

@jkotas had recommended using Func<> notation instead of delegate typenames. So, the callback-registration version should be something like:

namespace System.Runtime.InteropServices
{
    public sealed partial class NativeLibrary
    {
          ...
         public static Func<string, DllImportSearchPath, Assembly> RegisterNativeLibraryLoadCallBack(Func<string, DllImportSearchPath, Assembly> callBack);
          ...
    }
}

@jkotas
Copy link
Member

jkotas commented Aug 29, 2018

@jeffschwMSFT I have manually looked at first 500 hits in your query. I found 3 distinct cases (there is a lot of duplicated forks of the same project) that have name and target:

  1. mpich/mpi library e.g. https://github.com/UFC-MDCC-HPC/HPC-Shelf-MapReduce/blob/master/br.ufc.mdcc.common.impl.KVPairImpl/br.ufc.mdcc.common.impl.KVPairImpl/bin/Debug/MPI.dll.config
  2. Solaris sockets e.g. https://github.com/ruibarreira/linuxtrail/blob/master/usr/lib/cli/dbus-sharp-2.0/dbus-sharp.dll.config
  3. libMMMLinuxWrapper for gdiplus, e.g. https://github.com/fuadhuseynov/IDReader/blob/master/3MSwipeReaderSDK_1.2.1.2_Ubuntu_16.04/Bin/MMMReaderDotNet40.dll.config

2. and 3. are bogus examples because of both name and target are the same name. Entrypoint remapping is not required to make it work. This leaves us with one library that uses the entrypoint remapping. It is not a plenty for me ... .

My hypothesis of why entrypoint remapping is very rarely used:

We can divide the native libraries into (1) libraries that maintain binary compatibility between versions and (2) libraries that do not care about binary compatibility between versions.

The entrypoint remapping is not necessary to make (1) work.

The entrypoint remapping is not enough to make (2) work. An example of library that does not maintain binary compatibility between versions is Open SSL. https://github.com/dotnet/corefx/pull/32006/files is an example of what kind of stuff you have to do to be compatible with multiple versions of Open SSL.

The entrypoint remapping is only useful to what falls through the cracks in (1) or when you get extremely lucky in (2), either of which can be expected to be very rare.

@jkotas
Copy link
Member

jkotas commented Aug 29, 2018

if GetNativeEntryPoint provides the same level of name mangling as the original p/invoke, won't that be sufficient?

I agree that we can make the entrypoint remapping work if we need to. My question is whether it is useful enough to add it to .NET Core. So far, I am failing to find a set of good examples of what it is actually needed for. It feels like a corner case that it is not worth to have and the few libraries that use it can workaround easily.

@jeffschwMSFT
Copy link
Member

Perhaps I am missing the cost of getting entry point mapping working. We could expose the equivalence of https://github.com/dotnet/coreclr/blob/master/src/vm/dllimport.cpp#L5683 as GetEntryPoint.

Another thought on why entry point may be important is for the DSL case that Miguel pointed out. If you wanted to put your own encoding of library and entrypoint in the dllimport you would need the pair to complete that scenario.

My only concern is that dllmap with only libraries may not be complete enough.

@annaaniol
Copy link
Contributor Author

So we agreed on the following things:

  • a native library can't be unloaded because of the reasons that @AaronRobinsonMSFT has pointed out,
  • we will use LoadNativeLibrary and GetEntrypoint events (using EventHandler),
  • we keep exposing GetNativeEntrypoint,
  • toString() on NativeLibrary will return debugging string that includes source library name, target name that was loaded, and location details.

I updated the description.

@annaaniol
Copy link
Contributor Author

I included your suggestions and updated the Proposed API. Please take a final look and tell me if you have additional comments or questions.

@jkotas
Copy link
Member

jkotas commented Aug 30, 2018

These comments do not appear to be addressed:

@jeffschwMSFT
Copy link
Member

@jkotas

The methods to register the callback should not be on Assembly itself. We do not want to add interop-specific methods to Assembly

Anna and I discussed this. I think the challenge is that to achieve the proper Assembly level isolation we need to associate the callback with an assembly. The simplest approach would be too extend Assembly, but I hear your feedback.

I think we should align with your recommendation, which moves us away from events and back to callbacks. Within that design we felt that we would only allow one callback and then return any potentially overwritten callback. Similar to @swaroop-sridhar suggestion https://github.com/dotnet/corefx/issues/32015#issuecomment-417106670 (two seperate callbacks, using the Func notation and returns a potentially overwritten callback, NativeLibrary would then retain a mapping of Assembly with the callbacks).

This is not providing complete information to resolve the entrypoint. It would also need to provide the signature of the method and the method attributes to do the right name mangling

I thought that based on our discussion that GetEntryPoint would be provided for the simple case. The runtime would then use the method and method attributes provided in the original dllimport to mangle as necessary. In the event that is not sufficient the developer could provide a pre-managed name to the API (we will ensure that pattern works with our implementation). In essese we will expose a p/invoke to GetProcAddress/GetSymbol in a way that does not introduce a cycle with the dllmap hook.

@swaroop-sridhar
Copy link
Contributor

swaroop-sridhar commented Aug 30, 2018

I did some more search for Dll entry point map usage (outside of github), and I did not see widespread use of them. Most of the projects seem to use it for Mono specific mapings. In particular:

<dllmap dll="i:kernel32.dll">
  <dllentry dll="__Internal" name="CopyMemory" target="mono_win32_compat_CopyMemory"/>
  <dllentry dll="__Internal" name="FillMemory" target="mono_win32_compat_FillMemory"/>
  <dllentry dll="__Internal" name="MoveMemory" target="mono_win32_compat_MoveMemory"/>
  <dllentry dll="__Internal" name="ZeroMemory" target="mono_win32_compat_ZeroMemory"/>
</dllmap>

I agree with @jkotas that the reason these mappings are not widely used is that they only work in the situation where we have Dlls on different platforms that differ in name but not in signature.

The place where I can imagine this being useful is: A developer implements the same native code with different naming conventions on different platforms. For example, it is common to name public functions in the style DoSomething() on Windows and doSomething() on Unix – which may be enforced by stylecop like analyzers. But we haven’t seen this usage in publicly searchable code.

If we want to support entrypoint maps, we'll need to support remangling the name. The method-descriptor has pre-computed the mangled name wrt the original name, and we need to compute it based on the mapped name in order to execute GetProcAddress()

We’ll need to do one of:
• Pass signature-info (likely as a method-desc reflection object) object to the GetEntryPoint event handler.
• Get only the name of the new entryPoint from the handler, and compute the mangled name in the native code
• Expose a service to compute the mangled name from managed code

Of these, I think the first option is more generic -- ex: Some other implementation of GetEntryPoint handler may make decisions based on the signature.

@jeffschwMSFT
Copy link
Member

Get only the name of the new entryPoint from the handler, and compute the mangled name in the native code

Do we feel this would not be a viable option? The existing NDirectEntryPoint uses the method descriptor in the p/invoke to compute these values. If the API were to provide a different set of method information would that even work?

@terrajobst
Copy link
Member

terrajobst commented Oct 11, 2018

Here is the API shape we agreed on:

namespace System.Runtime.InteropServices
{
    public static partial class Marshal
    {
        // Typical or for dependencies which must be there or failure should happen.

        public static IntPtr LoadLibrary(string libraryPath);
        public static IntPtr LoadLibrary(string libraryName,
                                         Assembly assembly,
                                         DllImportSearchPath? searchPath);

        // For fast probing scenarios:

        public static bool TryLoadLibrary(string libraryPath,
                                          out IntPtr handle);
        public static bool TryLoadLibrary(string libraryName,
                                          Assembly assembly,
                                          DllImportSearchPath? searchPath,
                                          out IntPtr handle);

        public static void FreeLibrary(IntPtr handle);

        public static IntPtr GetLibraryExport(IntPtr handle, string name);
        public static bool TryGetLibraryExport(IntPtr handle, string name, out IntPtr address);
    }
}

We also concluded that this API needs another meeting:

public static bool RegisterDllImportResolver(
    Assembly assembly,
    Func<string, DllImportSearchPath, Assembly, IntPtr> callback
);

@xoofx
Copy link
Member

xoofx commented Oct 11, 2018

Thanks @terrajobst for the update, it looks much better 👍

I have a few questions:

For LoadLibrary(string libraryPath), I suppose that it will be a simple wrapper around LoadLibrary/dlopen. So kernel32.dll would work, as well as a full path right?

But if we pass only LoadLibrary("kernel32") it would not work, correct?

Is the version LoadLibrary(string libraryName, Assembly assembly, DllImportSearchPath? searchPath); supposed to support this case? (trying to post fix libraryName with platform dependent common for the current platform?).

  • I expect this version to use the simpler LoadLibrary (or TryLoadLibrary(libraryPath)), correct?
  • What's the purpose of the assembly passed?

I'm wondering if it would be valuable to expose a method that returns common library extensions for the current platform (or just a string might be enough, but on some platforms, like macos, I think that .so and .dylib are possible):

        public static string[] GetLibraryExtensions();

@jkotas
Copy link
Member

jkotas commented Oct 11, 2018

For LoadLibrary(string libraryPath), I suppose that it will be a simple wrapper around LoadLibrary/dlopen. So kernel32.dll would work, as well as a full path right?

Right.

But if we pass only LoadLibrary("kernel32") it would not work, correct?

LoadLibrary("kernel32") works fine in C/C++ on Windows, so this would work too. This API is a thin wrapper around LoadLibrary/dlopen. It is not going to try to artifically supress any probing logic that the OS API provides.

Is the version LoadLibrary(string libraryName, Assembly assembly, DllImportSearchPath? searchPath); supposed to support this case?

This API exposes the runtime built-in probing algorithm that is used for DllImport (next to assembly, prepend/append OS specific suffixes, ...).

What's the purpose of the assembly passed?

To get the probing paths for the above.

@xoofx
Copy link
Member

xoofx commented Oct 11, 2018

@jkotas Thanks for all the answers.

LoadLibrary("kernel32") works fine in C/C++ on Windows, so this would work too. This API is a thin wrapper around LoadLibrary/dlopen. It is not going to try to artifically supress any probing logic that the OS API provides.

Yeah, sorry, on Windows it does, but I'm not sure that dlopen does this actually?
Otherwise, that would be LoadLibrary(string libraryName, Assembly assembly, DllImportSearchPath? searchPath) that would handle this as I understand, that's fine.

In the case we would like to have custom dll search path, but still we would like to get the benefit of the auto prefix/postfix and search probe path, wouldn't make sense to pass a list of additional probing paths directly to LoadLibrary instead of an Assembly? (Do we use anything else the assembly.Location? behind the door?) Something like:

        public static IntPtr LoadLibrary(string libraryName,
                                         string[] additionalProbingPaths,
                                         DllImportSearchPath? searchPath);

@jkotas
Copy link
Member

jkotas commented Oct 11, 2018

Yeah, sorry, on Windows it does, but I'm not sure that dlopen does this actually?

Right.

custom dll search path

We know that there are scenarios that may want to supply custom dll search paths of various forms. We are leaving designing APIs for those for the next iteration.

Do we use anything else the assembly.Location

Yes, there is more than just assembly.Location.

@eerhardt
Copy link
Member

  1. In general, why have both Get and TryGet overloads? Why can't IntPtr == 0 signify that it wasn't found? And if it can't, then why not just have the TryGet methods?

  2. Don't you need to pass in the handle to the library in GetLibraryExport?

        public static IntPtr GetLibraryExport(string name);
        public static bool TryGetLibraryExport(string name, out IntPtr address);

@xoofx
Copy link
Member

xoofx commented Oct 11, 2018

In general, why have both Get and TryGet overloads? Why can't IntPtr == 0 signify that it wasn't found? And if it can't, then why not just have the TryGet methods?

It may actually be practical to have LoadLibrary throwing an exception, as on dlopen platforms, we can get the error message via dlerror() right after a failing dlopen (same for dlsym). So, the approach with TryLoad/Load makes sense here. Though, I don't know if dlerror gives relevant details in practice...

Don't you need to pass in the handle to the library in GetLibraryExport?

yep an oversight likely 😉

@AaronRobinsonMSFT
Copy link
Member

Don't you need to pass in the handle to the library in GetLibraryExport?

Updated.

@tannergooding
Copy link
Member

@terrajobst, should this have been marked api-approved, since we reviewed and agreed upon the API shape here: https://github.com/dotnet/corefx/issues/32015#issuecomment-428775858?

@swaroop-sridhar
Copy link
Contributor

There is one more API pending to be discussed: RegisterDllImportResolver

@tannergooding
Copy link
Member

Right, but that shouldn't block the already approved subset, correct?

@swaroop-sridhar
Copy link
Contributor

Yes, correct.

@terrajobst
Copy link
Member

terrajobst commented Dec 18, 2018

Video

This is the shape we agreed on:

  • We centrlized the APIs on a new type NativeLibrary as opposed to Marshal because it helps the developer using these APIs as it groups them better. Marshal already has a ton of APIs.
  • We renamed RegisterDllImportResolver to SetDllImportResolver. Register implies having an unregister and having a chain. Set makes it clear there is a single value and null will clear it.
  • We replaced the func of SetDllImportResolver with a named delegate so that we can name the parameters.
  • Open item: we need to clarify whether the DllImportSearchPath? argument to DllImportResolver is different from any other existing enum value. If not, we should make it non-nullable.
namespace System.Runtime.InteropServices
{
    public delegate IntPtr DllImportResolver(string libraryName,
                                             Assembly assembly,
                                             DllImportSearchPath? searchPath);

    public static partial class NativeLibrary
    {
        // Typical or for dependencies which must be there or failure should happen.

        public static IntPtr Load(string libraryPath);
        public static IntPtr Load(string libraryName,
                                  Assembly assembly,
                                  DllImportSearchPath? searchPath);

        // For fast probing scenarios:

        public static bool TryLoad(string libraryPath,
                                   out IntPtr handle);
        public static bool TryLoad(string libraryName,
                                   Assembly assembly,
                                   DllImportSearchPath? searchPath,
                                   out IntPtr handle);

        public static void Free(IntPtr handle);

        public static IntPtr GetExport(IntPtr handle, string name);
        public static bool TryGetExport(IntPtr handle, string name, out IntPtr address);

        public static bool SetDllImportResolver(Assembly assembly, DllImportResolver resolver);
    }
}

@swaroop-sridhar
Copy link
Contributor

The LoadLibrary spec says:

"The common language runtime handles the call to the LoadLibraryEx function according to the following algorithm:

  1. If the attribute is applied to an individual platform invoke, use the values specified by that instance of the attribute.

  2. Otherwise, if the attribute is applied to the assembly that contains the platform invoke, use the values specified by that instance of the attribute.

  3. Otherwise, search the assembly directory and then call the LoadLibraryEx function with the LOAD_WITH_ALTERED_SEARCH_PATH flag."

Therefore, the difference between default behavior (when no attributes are found) and legacy behavior (when the attribute is present) is that:

  1. Legacy Behavior: Search application directory then call LoadLibraryEx with LOAD_WITH_ALTERED_SEARCH_PATH flag.
  2. Default behavior: Search assembly directory then call LOAD_WITH_ALTERED_SEARCH_PATH flag.

So, I think that we should keep the DllImportResolver parameter nullable on SetDllImportResolver API.

@danmoseley
Copy link
Member

It's unclear to me how the values of DllImportSearchPath map to behavior on Unix - it's defined around behavior of Windows LoadLibrary. I realize that's not introduced in this proposal, but currently i'ts only used in DefaultDllImportSearchPathsAttribute and this makes it more front and center.

@swaroop-sridhar
Copy link
Contributor

Yes DefaultDllImportSearchPath is currently Windows-specific, except for the SearchAssemblyDirectory value. I filed https://github.com/dotnet/coreclr/issues/21584 to track this.

@jkotas
Copy link
Member

jkotas commented Dec 18, 2018

bool SetDllImportResolver

What do we expect the callers to do when this returns false?

@weltkante
Copy link
Contributor

weltkante commented Dec 19, 2018

We renamed RegisterDllImportResolver to SetDllImportResolver. Register implies having an unregister and having a chain. Set makes it clear there is a single value and null will clear it.

Having consumers to design around a single global resource sounds to me like a recipe for frustration. How can we prevent 3rd party libraries from calling it? If only a single delegate can be registered it should be reserved to the main application, otherwise libraries are gonna try and take ownership of the resolver for themselves.

If you insist on supporting only a single resolver then this should be designed in a way that makes it only work in the main application (the Main entry point or the STAThread attribute are examples). Forcing library authors to do the right thing and expose their resolver (having the main application coordinate it) is the only sensible design I can think of.

@AaronRobinsonMSFT
Copy link
Member

@weltkante This callback is associated only with the passed in Assembly, it is not global. The idea here is that the Assembly author should be defining their own deployment requirements and as such should be able to handle all native library loads for their Assembly.

@swaroop-sridhar
Copy link
Contributor

What do we expect the callers to do when this returns false?
There are a few possibilities:

  1. Handling depends on the app -- likely indicates a bug in the app?
  2. Unset the resolver by passing nullptr handle. Then they can pass a new resolver. This makes the overwrite explicit, but may be confusing to use.

@jkotas, Since the API is now called SetDllImportResolver I think it is more intuitive to always overwrite the resolver per-assembly (or unset by setting nullptr). We could add GetDllImportResolver or HasDllImportResolver properties if deemed necessary.

@jkotas
Copy link
Member

jkotas commented Dec 19, 2018

Handling depends on the app -- likely indicates a bug in the app?

Right. We just throw exceptions for patterns that are bugs in the app. We do not return bool.

Unset the resolver by passing nullptr handle.

Why would anybody want to do that? It would be just a factory for race conditions and other hard to diagnose problems.

@swaroop-sridhar
Copy link
Contributor

We just throw exceptions for patterns that are bugs in the app.
@jkotas. OK I've updated the proposal to throw InvalidOperationException on registering a second handler.

Daniel15 referenced this issue in Daniel15/gpgme-sharp Jan 31, 2019
This adds support for .NET Standard 2.0 / .NET Core 2.2.

The trickiest part of this was the handling of the library file name. The file name differs on Windows (`libgpgme-11.dll`) vs on Linux (`libgpgme.so.11`). Mono has a feature called "dllmap" which makes this remapping quite easy to perform:
```
<dllmap dll="libgpgme-11.dll" target="libgpgme.so.11" />
```

However, this same functionality is not available in .NET Core. Work is currently underway to have some sort of native library loader functionality that would support this (see https://github.com/dotnet/corefx/issues/32015), but as of now, it's not available.

This means I had to hack around it. My initial approach was going to be to have two classes (one for Windows, one for Linux) that implement a common interface:
```
interface INativeMethods {
    IntPtr gpgme_check_version(IntPtr req_version);
}

class LinuxNativeMethods : INativeMethods {
    [DllImport("libgpgme.so.11", CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)]
    internal extern IntPtr gpgme_check_version([In] IntPtr req_version);
}

class WindowsNativeMethods : INativeMethods {
    [DllImport("libgpgme-11.dll", CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)]
    internal extern IntPtr gpgme_check_version([In] IntPtr req_version);
}
```

However, `DllImport` methods **must** be static, and C# doesn't allow static methods on an interface.

Because of this, my approach ended up being quite hacky: I have one class with the Linux methods, one class with the Windows methods, and a wrapper class that has properties for each of them (see `NativeMethodsWrapper.cs`).

There was another issue: I wanted to avoid copying-and-pasting the P/Invoke code, however .NET doesn't let you have the same file twice in the same assembly, with two different sets of preprocessor values. So, the Windows and Linux classes needed to be in two separate assemblies.

I updated the build to use ILRepack to merge all the assemblies together, so users shouldn't actually notice any of this hackiness. Ideally this will all be cleaned up once https://github.com/dotnet/corefx/issues/32015 is completed.
@jeffschwMSFT
Copy link
Member

Closed with dotnet/corefx#34686

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Projects
None yet
Development

No branches or pull requests