Add NativeLibrary class #16409

GrabYourPitchforks · 2018-02-15T23:39:23Z

This is the prototype for the NativeLibrary type based on the latest approved API design. Addresses https://github.com/dotnet/corefx/issues/17135.

The behavior of each public API should be clearly described via the devdoc, but please let me know if something's unclear and we can change it or clarify the comments.

/cc @ianhays @bartonjs @morganbr

GrabYourPitchforks · 2018-02-15T23:40:43Z

src/mscorlib/src/System/Runtime/InteropServices/Marshal.cs

@@ -864,6 +864,14 @@ public static void PrelinkAll(Type c)
            }
        }

+        internal static int NumParamBytes(RuntimeMethodInfo m, bool isForStdCallDelegate)


This is based somewhat on the full framework public API (https://msdn.microsoft.com/en-us/library/system.runtime.interopservices.marshal.numparambytes(v=vs.110).aspx).

GrabYourPitchforks · 2018-02-15T23:41:36Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.Unix.cs

+
+        // This is a *partial* copy of include/pal/module.h so that we can extract the dl_handle.
+        [StructLayout(LayoutKind.Sequential)]
+        private struct _MODSTRUCT


I'm not really a fan of this pattern, but this seemed safer than trying to have NativeLibrary.cpp reach deep into the PAL-specific header files. Thoughts?

We could expose it as a PAL function, e.g. PAL_GetDlHandle(HMODULE hMod) and pinvoke into it here (or expose a wrapper that calls this function as a QCALL) to get rid of this exposure of PAL internals.

It actually seems that moving the IsValidModuleHandle to the native part of the implementation and exposing it as a QCALL would be better.

the IsValidModuleHandle to the native part of the implementation

There is a lot of chattiness between the managed and native parts - there are 5 new FCalls/QCalls already. I think it would be better to have bulk of this implementation in C++, and not have this chattiness.

Agree.. I would either all(or mostly) implements in native code or all implements in managed code. but not in middle. It will help interop team maintain these codes in long term..

When we were developing this I chatted with quite a few people who have a vested interest in this type's implementation, and the recommendations eventually came down to: (a) reuse as much code as practical; and (b) if new code must be created keep as much of it managed as possible, even at the expense of performance. This would make the code easier to understand and would benefit maintenance in the long run.

In practice the only real implementation logic that exists in C# is a managed equivalent to the C++ FindEntryPoint method. Tweaking and reusing the FindEntryPoint method directly was dismissed due to how much refactoring it would need. All other C# code is parameter validation / error handling / etc., with p/invokes back into the runtime to do the real work.

If you feel strongly about moving the managed FindEntryPoint logic back into C++ let me know and I'll defer to your judgment. Be aware that it will result in larger, more complex, less readable code, with minimal visible effects for consumers of this type. It also risks slipping this type from 2.1 to vNext.

GrabYourPitchforks · 2018-02-15T23:44:52Z

src/vm/nativelibrary.cpp

+        pAssembly = callingAssembly->GetAssembly();
+    }
+
+    hmod = NDirect::LoadLibraryModuleForNativeLibrary(moduleName, pAssembly, searchAssemblyDirectory, searchPaths);


NDirect has traditionally only been used for [DllImport], not for Marshal.GetDelegateForFunctionPointer. This still seems like an ok dependency though because we're basically trying to mimic [DllImport] logic. Thoughts?

it is a OK dependency. I would prefer move into dllimport.cpp, so it will remove this dependency

GrabYourPitchforks · 2018-02-15T23:45:47Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.Unix.cs

+
+namespace System.Runtime.InteropServices
+{
+    // Contains UNIX-specific logic for NativeLibrary class.


Here, "UNIX" also includes OSX.

GrabYourPitchforks · 2018-02-15T23:49:06Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.cs

+        /// <summary>
+        /// Equivalent to <see cref="TryGetDelegate{TDelegate}(string, bool, out TDelegate)"/> where <em>exactSpelling</em> is <see langword="false"/>.
+        /// </summary>
+        public bool TryGetDelegate<TDelegate>(string name, out TDelegate result) where TDelegate : class


Ideally we'd use where TDelegate : delegate, but that language feature doesn't exist :(. This is the next best constraint.

where TDelegate : System.Delegate is coming in C# 7.3 (as is : System.Enum, you just can't use the keywords, delegate and enum, as of the current iteration)

See dotnet/roslyn#24443

Yeah, the PR to support the constraints looks about as good as done (and consuming the delegate/enum constraints has always been supported anyway).

Do you know when that version of the compiler will be made available to the coreclr and corefx projects? If it's dropping imminently then we can probably get the change in time for 2.1.

I believe @VSadov would know. It requires, at the very least, for the features/constraints branch to be merged back into master before it is available.

jkotas · 2018-02-16T11:25:16Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.cs

+                retVal = CreateDelegateForSymbolByName<TDelegate>(name + "W") ?? retVal;
+            }
+
+#if (BIT32 && !ARM)


if this is X86 specific, it should be under #if X86 and not this.

jkotas · 2018-02-16T11:27:47Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.cs

+            charSet = default;
+            callingConvention = default;
+
+            var attr = delegateType.GetCustomAttribute<UnmanagedFunctionPointerAttribute>(inherit: false);


I do not think we should be duplicating the UnmanagedFunctionPointerAttribute parsing logic here. We should just use the one we have in unmanaged runtime.

(Same for other pieces of logic that are duplicated in the unmanaged runtime.)

I can remove ParseCharsetAndCallingConvention by reusing existing PInvokeStaticSigInfo logic in dllimport.h. However, I don't think I can reuse existing unmanaged FindEntryPoint logic without introducing a significant refactoring, and others I spoke with offline offered that the current proposal of rewriting the logic in managed code would be much less risky.

I does not have to reusing the code if it is unnatural. But I think having a method with same structure as NDirectMethodDesc::FindEntryPoint right next to NDirectMethodDesc::FindEntryPoint to handle this case would be better.

bartonjs · 2018-02-16T15:38:15Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.cs

+            // the value immediately following the '#' can fit into a WORD), then process the
+            // ordinal and skip all other logic in this routine.
+
+            if (AllowLocatingFunctionsByOrdinal && name.Length >= 1 && name[0] == '#')


Is this functionality we need to support? It's unfortunate that a new API whose purpose seems targeted at being cross-platform would have platform-specific behavioral differences designed in.

Exporting by ordinal is still actively recommended to customers as a way to reduce DLL size on disk. See https://docs.microsoft.com/en-us/cpp/build/determining-which-exporting-method-to-use and https://docs.microsoft.com/en-us/cpp/build/exporting-functions-from-a-dll-by-ordinal-rather-than-by-name.

bartonjs · 2018-02-16T15:44:06Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.cs

+                // TODO: What should the exception message say?
+                throw new Exception("Must specify DllImportSearchPaths if assembly not specified.");
+            }
+


In addition to the low byte being reserved, I think it's important to check that the enum value being passed in is one we understand. Unmapped values will lead to differing behaviors on Windows and non-Windows systems, and we should endeavor to prevent that from happening in new API.

I don't know how we could do that without interfering with the scenario. The only values understood by non-Windows platforms are AssemblyDirectory and LegacyBehavior. We can't eagerly fail on other values being passed in because it would block the scenario below.

NativeLibrary lib; if (NativeLibrary.TryLoad("foo.dll", null, System32, out lib)) { // We're on Windows, use OS-provided foo.dll. } else if (NativeLibrary.TryLoad("foo.so", typeof(me).Assembly, AssemblyDirectory, out lib)) { // We're on non-Windows, use our local equivalent. }

We don't want the call to TryLoad("foo.dll", ...) to throw since it's really nothing more than a glorified OS check.

We spoke offline about this. We can throw as long as the caller has the ability to conditionally guard the call. For example, the guarded call site could look like this.

NativeLibrary lib; if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { if (!NativeLibrary.TryLoad("foo.dll", ..., System32, out lib)) { throw new Exception("Expected this DLL to be found."); } } else { if (!NativeLibrary.TryLoad("bar.so", ..., Assembly, out lib)) { throw new Exception("Expected this DLL to be found."); } }

This pattern would also be more resilient to DLL poisoning attacks because the OS version check is performed up front, allowing the caller to pass the search paths appropriate for the OS.

- Forbid invalid flags on TryLoad - Define 'X86' constant - Use existing PInvokeStaticSigInfo logic to crack open the attribute - Fix compilation error on non-Windows

luqunl · 2018-02-20T22:30:21Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.Unix.cs

+
+        // Per PInvokeStaticSigInfo::InitCallConv and GetDefaultCallConv, the default calling convention
+        // on Unix is cdecl.
+        private const CallingConvention FallbackCallingConvention = CallingConvention.Cdecl;


This may require msdn doc update, since in MSDN(https://msdn.microsoft.com/en-us/library/system.runtime.interopservices.callingconvention(v=vs.110).aspx),

It states that "The callee cleans the stack. This is the default convention for calling unmanaged functions with platform invoke."

luqunl · 2018-02-20T23:23:25Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.cs

+            RuntimeMethodHandleInternal invokeMethodHandle = new RuntimeMethodHandleInternal(StubHelpers.StubHelpers.GetDelegateInvokeMethodFromDelegateType(delegateType));
+            RuntimeMethodInfo invokeMethodInfo = (RuntimeMethodInfo)RuntimeType.GetMethodBase(delegateType, invokeMethodHandle);
+
+            // Generic types are disallowed


why generic delegates is n't allowed? Func or Action should be okay.

I assume that the check here was supposed to be for delegateType.ContainsGenericParameters, aka delegateType.IsOpenType (that property doesn't actually exist).

Using ContainsGenericParameters allows typeof(Func<bool>), but not typeof(Func<>).

It's an existing limitation of Marshal.GetDelegateForFunctionPointer. Many of these checks are duplicates of the checks performed by that method; I'm just performing the checks upfront so that we don't go too far down the "find the entry point" logic if we know we're going to fail immediately after.

Huh. Weird. Well, that's sufficient for me. (Though now I wonder why GetDelegateForFunctionPointer has that restriction. But, it already exists, so OK.)

If I had to guess (and this is only a guess), I'd venture that the internal marshaling system has trouble specializing generics for reference types. For example, consider Func<string, int> and Func<SafeHandle, int>. They're both represented under the covers as a single Func<__Canon, int> type, but string and SafeHandle have very different requirements for marshaling. This would seemingly greatly complicate the marshaling stub lookup. I suppose it would be possible to support this but due to lack of interest nobody ever took the time to allow it.

luqunl · 2018-02-20T23:36:03Z

src/vm/nativelibrary.cpp

+// LoadLibrary
+// Loads the specified library, returning its handles, or nullptr if library cannot be found
+HINSTANCE QCALLTYPE NativeLibrary::LoadLibrary(LPCUTF8 moduleName, QCall::AssemblyHandle callingAssembly, BOOL searchAssemblyDirectory, DWORD searchPaths)
+{


is it possible that expose these functions(overload is fine) into dllimport.cpp, so that all Pinvoke related functions are implemented in dllimport.cpp?

All of these functions are implemented in dllimport.cpp adjacent to the existing code. This is just the QCALL entry point with parameter validation, similar to how comdelegate.cpp and related files already serve as the QCALL entry points into existing dllimport.cpp code.

bartonjs · 2018-02-20T23:36:28Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.Unix.cs

+    {
+        // The allowed mask values for the DllImportSearchPath.
+        // Non-Windows sytems only allow AssemblyDirectory and LegacyBehavior.
+        private const uint AllowedDllImportSearchPathsMask = (uint)DllImportSearchPath.AssemblyDirectory;


Might be worth a comment that LegacyBehavior is 0, so it's implicitly in this list. (So it doesn't look like a mismatch to the current comment)

luqunl · 2018-02-20T23:39:29Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.cs

+        /// <remarks>
+        /// Functions can also be queried by ordinal export by using a "#" prefix (e.g., "#1") for <paramref name="name"/>.
+        /// </remarks>
+        public bool TryGetDelegate<TDelegate>(string name, bool exactSpelling, out TDelegate result) where TDelegate : class


this is a little concern for me. since it is very easy for customer to forgot to add CallConvention attribute for this Delegate Type. Maybe we should add a MSDN page to show how to use this API correctly.

The Marshal.GetDelegateForFunctionPointer API has a fallback behavior if the [UnmanagedFunctionPointer] attribute isn't present. This API tries to honor that same logic to the best of its ability. I agree that documentation will be necessary for correct usage of this API.

luqunl · 2018-02-22T21:03:37Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.cs

+        /// <remarks>
+        /// Functions can also be queried by ordinal export by using a "#" prefix (e.g., "#1") for <paramref name="name"/>.
+        /// </remarks>
+        public bool TryGetDelegate<TDelegate>(string name, bool exactSpelling, out TDelegate result) where TDelegate : class


Do you try to add support for SetLastError? also others fields in DllImportAttribute

The existing marshaling code handles that automatically. We get it for free.

For reference, this comes from the existing Marshal.GetDelegateForFunctionPointer behavior. It respects any [UnmanagedFunctionPointer] attribute specified on the delegate type, including the SetLastError field if specified. Since we're really just a glorified wrapper around Marshal.GetDelegateForFunctionPointer we get it for free.

This need to add a doc(or sample code) to show customer how to do it correctly.

GrabYourPitchforks · 2018-02-23T02:13:58Z

Removing "WIP" marker as the implementation is now code complete. Unit tests and reference APIs will be added in dotnet/corefx#27258. Have performed manual verification on Windows and Ubuntu independent of the unit tests.

jkotas · 2018-02-23T16:13:00Z

src/vm/marshalnative.cpp

@@ -54,7 +54,7 @@
 //
 // NumParamBytes
 // Counts # of parameter bytes
-INT32 QCALLTYPE MarshalNative::NumParamBytes(MethodDesc * pMD)
+INT32 QCALLTYPE MarshalNative::NumParamBytes(MethodDesc * pMD, CLR_BOOL fForStdCallDelegate)


CLR_BOOL is not correct here.

Good catch, will fix.

I do see QCALLTYPE and CLR_BOOL used together in a few places, such as

coreclr/src/vm/comthreadpool.h

Line 38 in fae8cc7

static void QCALLTYPE InitializeVMTp(CLR_BOOL* pEnableWorkerTracking);

. Is it worth opening an issue for these or should we just let existing code slide?

jkotas · 2018-02-23T16:14:19Z

src/mscorlib/src/System/Runtime/InteropServices/NativeLibrary.cs

+
+                if (paths == DllImportSearchPath.LegacyBehavior)
+                {
+                    var attr = caller.GetCustomAttribute<DefaultDllImportSearchPathsAttribute>();


The unmanaged runtime has cache for this value to avoid reflection call every time.

I'm going for simplicity here rather than performance. The action of having the OS load a module into the current process is by far going to dominate the run time of this method.

Will be reintroduced later when the implementation is cleaner

jkotas · 2018-02-23T21:01:34Z

Remove public Handle property

Why?

GrabYourPitchforks · 2018-02-23T21:13:30Z

I'm not convinced I can perform the requested cleanup to move the Handle validation / unwrapping logic into native code under the deadline. This allows us to add it in vNext using the preferred pattern.

GrabYourPitchforks · 2018-03-12T16:15:07Z

Punted to Future. Will send new PR at later date.

Initial NativeLibrary design and prototype

7ece113

GrabYourPitchforks commented Feb 15, 2018

View reviewed changes

jkotas added the area-Interop label Feb 16, 2018

jkotas requested review from luqunl and jeffschwMSFT February 16, 2018 11:20

jkotas reviewed Feb 16, 2018

View reviewed changes

bartonjs reviewed Feb 16, 2018

View reviewed changes

Respond to CR feedback

1870ec8

- Forbid invalid flags on TryLoad - Define 'X86' constant - Use existing PInvokeStaticSigInfo logic to crack open the attribute - Fix compilation error on non-Windows

GrabYourPitchforks mentioned this pull request Feb 19, 2018

[WIP] Add NativeLibrary reference assemblies and tests dotnet/corefx#27258

Closed

luqunl reviewed Feb 20, 2018

View reviewed changes

bartonjs reviewed Feb 20, 2018

View reviewed changes

luqunl reviewed Feb 20, 2018

View reviewed changes

GrabYourPitchforks added 3 commits February 21, 2018 16:45

CR feedback; fix Unix build break

09d1d06

Replace _X86_ with _TARGET_X86_, which seems to be more popular

93debde

Fix build breaks

f95e29b

luqunl reviewed Feb 22, 2018

View reviewed changes

GrabYourPitchforks added 2 commits February 22, 2018 16:50

Fix parameter validation in TryLoad

4c56c0c

Add proper resource strings for error messages

67900c5

GrabYourPitchforks changed the title ~~[WIP] Add NativeLibrary class~~ Add NativeLibrary class Feb 23, 2018

jkotas reviewed Feb 23, 2018

View reviewed changes

Remove public Handle property

096cab3

Will be reintroduced later when the implementation is cleaner

GrabYourPitchforks closed this Mar 12, 2018

svick mentioned this pull request Mar 14, 2018

Dynamic DLL Import - Runtime PInvoke dotnet/core#1349

Closed

migueldeicaza mentioned this pull request Jan 31, 2020

Lightweight and dynamic driving of P/Invoke dotnet/runtime#10759

Closed

This was referenced Jan 31, 2020

Native Library Loader API dotnet/runtime#27267

Closed

Native Library Loader API dotnet/runtime#31612

Closed

Add NativeLibrary class #16409

Add NativeLibrary class #16409

Conversation

GrabYourPitchforks commented Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrabYourPitchforks Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrabYourPitchforks Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

GrabYourPitchforks Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

It states that "The callee cleans the stack. This is the default convention for calling unmanaged functions with platform invoke."

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrabYourPitchforks Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrabYourPitchforks Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrabYourPitchforks commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Feb 23, 2018

GrabYourPitchforks commented Feb 23, 2018

GrabYourPitchforks commented Mar 12, 2018

GrabYourPitchforks commented Feb 15, 2018 •

edited

Loading

GrabYourPitchforks Feb 23, 2018 •

edited

Loading

GrabYourPitchforks Feb 15, 2018 •

edited

Loading

GrabYourPitchforks Feb 15, 2018 •

edited

Loading

GrabYourPitchforks Feb 22, 2018 •

edited

Loading

GrabYourPitchforks Feb 23, 2018 •

edited

Loading