Add Per-assembly Load Native Library callbacks #21555

swaroop-sridhar · 2018-12-15T02:18:38Z

This Change implements the Native Library resolution
Call-backs proposed in dotnet/corefx#32015

pentp · 2018-12-15T03:07:14Z

src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs

@@ -32,6 +32,11 @@ public static partial class Marshal
        internal static Guid IID_IUnknown = new Guid("00000000-0000-0000-C000-000000000046");
 #endif //FEATURE_COMINTEROP

+        static Marshal()


An explicit static constructor is bad for performance and in this case also causes an infinite loop during startup. Maybe nativeDllResolveMap should be lazy-initialized on first use?

jkotas · 2018-12-15T08:58:05Z

This API is not yet approved, and the API contracts in CoreFX will not be added until the API approval is complete

That's not the usual process...

jkotas · 2018-12-15T09:00:39Z

src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs

+        /// Therefore, this table uses weak assembly pointers to indirectly achieve 
+        /// similar behavior.
+        /// </summary>
+        public static ConditionalWeakTable<Assembly, Func<string, Assembly, DllImportSearchPath?, IntPtr>> nativeDllResolveMap;


From https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md: prefix static fields with s_

jkotas · 2018-12-15T09:01:53Z

src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs

+        internal static IntPtr LoadLibraryCallbackStub(string libraryName, Assembly assembly, 
+                                                       bool hasDllImportSearchPathFlags, uint dllImportSearchPathFlags)
+        {
+            Func<string, Assembly, DllImportSearchPath?, IntPtr> callBack;


callback is one word. B should be lowercase.

(multiple places)

jkotas · 2018-12-15T09:04:54Z

src/vm/dllimport.cpp

@@ -6280,23 +6280,32 @@ INT_PTR NDirect::GetNativeLibraryExport(NATIVE_LIBRARY_HANDLE handle, LPCWSTR sy
    return address;
 }

+#ifndef PLATFORM_UNIX
+BOOL IsWindowsAPI(PCWSTR wszLibName)


This does not return whether the name is Windows API. It returns whether the name is Windows API set. Change the name to IsWindowsAPISet ?

jkotas · 2018-12-15T09:05:36Z

src/vm/dllimport.cpp

+NATIVE_LIBRARY_HANDLE NDirect::LoadLibraryModuleViaCallBack(NDirectMethodDesc * pMD, LPCWSTR wszLibName)
+{
+#ifndef PLATFORM_UNIX
+    if (IsWindowsAPI(wszLibName))


What is the rationale behind blocking this?

My only concern with not blocking this is the scenario where we deploy Windows API sets on downlevel Windows SKUs (i.e. Windows 7). The issue here would be why are we looking in the system if they are app local? This is largely a hedge about not trusting Windows to deploy API sets that are guaranteed backwards compatible. I think that is a fair assumption the API sets will always be backwards compat, but note that the behavior will be different on different platforms if we rely on a system API set in some cases and our applocal version of the API sets in others.

When it comes to Windows 7, API sets are incompatible mess. Note that there are no officially shipped API sets for Windows 7, except for the few that support universal CRT. We learned this hard-way during the .NET Core 1.0 days when we were using API sets everywhere, only to find out that there is no way to make things work well with Windows 7 if you do that.

If somebody wants to resolve API sets on their own to make stuff work, we should not stand in the way.

jkotas · 2018-12-15T09:06:47Z

src/vm/callhelpers.h

@@ -566,6 +566,7 @@ enum DispatchCallSimpleFlags
 #define STRINGREF_TO_ARGHOLDER(x) (LPVOID)STRINGREFToObject(x)
 #define PTR_TO_ARGHOLDER(x) (LPVOID)x
 #define DWORD_TO_ARGHOLDER(x)   (LPVOID)(SIZE_T)x
+#define BOOL_TO_ARGHOLDER(x) DWORD_TO_ARGHOLDER(x)   


This is not quite right. Assuming the managed argument is going to be bool, this value should be normalized, e.g. using !!

jkotas · 2018-12-15T09:09:11Z

src/vm/dllimport.cpp

+
+    Assembly* pAssembly = pMD->GetMethodTable()->GetAssembly();
+    protect.libNameRef = StringObject::NewString(wszLibName);
+    protect.assemblyRef = pAssembly->GetExposedObject();


This has GC hole. You are assigning object references without them being protected.

This exact situation is described in "Your first GC hole" in https://github.com/dotnet/coreclr/blob/master/Documentation/coding-guidelines/clr-code-guide.md#2.1.2.

jkotas · 2018-12-15T09:12:10Z

src/vm/dllimport.cpp

@@ -6362,6 +6371,55 @@ NATIVE_LIBRARY_HANDLE NDirect::LoadLibraryModuleViaHost(NDirectMethodDesc * pMD,
    return (NATIVE_LIBRARY_HANDLE)hmod;
 }

+NATIVE_LIBRARY_HANDLE NDirect::LoadLibraryModuleViaCallBack(NDirectMethodDesc * pMD, LPCWSTR wszLibName)
+{


Add contracts to all new methods.

jkotas · 2018-12-15T09:15:58Z

src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs

+                return false;
+            }
+
+            nativeDllResolveMap.Add(assembly, callBack);


This has race condition: if the method is called from multiple threads on same assembly, we may end up throwing here.

jkotas · 2018-12-15T09:16:20Z

src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs

+        /// and returns
+        ///   - The handle to the loaded native library (on success) or null (on failure)
+        /// The parameters on this callback are such that they can be directly passed to 
+        /// Marhall.LoadLibrary(libraryName, assembly, dllImportSearchPath) to approximately achieve 


Suggested change

/// Marhall.LoadLibrary(libraryName, assembly, dllImportSearchPath) to approximately achieve

/// Marhal.LoadLibrary(libraryName, assembly, dllImportSearchPath) to approximately achieve

Marshal ?

tests/src/Interop/MarshalAPI/DllImportUtil/DllImportUtil.cs

swaroop-sridhar · 2019-01-15T23:32:25Z

@jkotas @AaronRobinsonMSFT @pentp I have addressed your feedback in the new commit.
NativeLibrary APIs are approved. The API now lives in the NativeLibrary class instead of Marshal

src/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs

swaroop-sridhar · 2019-01-17T00:47:58Z

@dotnet-bot test Windows_NT x64 Release CoreFX Tests
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

swaroop-sridhar · 2019-01-17T03:24:40Z

The "OSX10.12 x64 Checked Innerloop Build and Test" failure seems unrelated. It is failing on all runs.

src/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs

src/System.Private.CoreLib/Resources/Strings.resx

This Change implements the Native Library resolution Call-backs proposed in https://github.com/dotnet/corefx/issues/32015

jkotas · 2019-01-17T22:50:42Z

Looks good to me, but it can use a second pair of eyes. I have looked at this too many times...

AaronRobinsonMSFT · 2019-01-17T22:59:19Z

@jkotas I will take another look.

src/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs

src/vm/dllimport.h

src/vm/dllimport.cpp

src/vm/interoputil.cpp

swaroop-sridhar · 2019-01-18T01:52:55Z

Thanks for the feedback @AaronRobinsonMSFT.
I've addressed it in a306c61 -- made the changes you've suggested, except as noted in the comments.

swaroop-sridhar · 2019-01-18T07:26:26Z

@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

Add Per-assembly Load Native Library callbacks This Change implements the Native Library resolution Call-backs proposed in https://github.com/dotnet/corefx/issues/32015 Commit migrated from dotnet/coreclr@732f892

swaroop-sridhar force-pushed the cb branch from 9e26a86 to ea09225 Compare December 15, 2018 02:20

pentp reviewed Dec 15, 2018

View reviewed changes

jkotas suggested changes Dec 15, 2018

View reviewed changes

AaronRobinsonMSFT reviewed Dec 15, 2018

View reviewed changes

tests/src/Interop/MarshalAPI/DllImportUtil/DllImportUtil.cs Outdated Show resolved Hide resolved

pentp mentioned this pull request Dec 30, 2018

Remove parsing from GUID constants #21717

Merged

swaroop-sridhar force-pushed the cb branch 2 times, most recently from 82c8738 to 7751a17 Compare January 15, 2019 23:28

jkotas reviewed Jan 15, 2019

View reviewed changes

swaroop-sridhar force-pushed the cb branch from 7751a17 to 9462e00 Compare January 15, 2019 23:39

pentp reviewed Jan 16, 2019

View reviewed changes

src/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs Outdated Show resolved Hide resolved

swaroop-sridhar force-pushed the cb branch 5 times, most recently from 886fc14 to f84249e Compare January 16, 2019 23:41

jkotas suggested changes Jan 17, 2019

View reviewed changes

src/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs Outdated Show resolved Hide resolved

swaroop-sridhar force-pushed the cb branch from f84249e to 7e5c573 Compare January 17, 2019 17:26

jkotas reviewed Jan 17, 2019

View reviewed changes

src/System.Private.CoreLib/Resources/Strings.resx Outdated Show resolved Hide resolved

swaroop-sridhar force-pushed the cb branch from 7e5c573 to f414408 Compare January 17, 2019 18:04

Add Per-assembly Load Native Library callbacks

0606292

This Change implements the Native Library resolution Call-backs proposed in https://github.com/dotnet/corefx/issues/32015

swaroop-sridhar force-pushed the cb branch from f414408 to 0606292 Compare January 17, 2019 20:51

jkotas approved these changes Jan 17, 2019

View reviewed changes

AaronRobinsonMSFT suggested changes Jan 17, 2019

View reviewed changes

Address Aaron's feedback

c9a3c60

swaroop-sridhar force-pushed the cb branch from a306c61 to c9a3c60 Compare January 18, 2019 02:15

AaronRobinsonMSFT approved these changes Jan 18, 2019

View reviewed changes

swaroop-sridhar merged commit 732f892 into dotnet:master Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Per-assembly Load Native Library callbacks #21555

Add Per-assembly Load Native Library callbacks #21555

swaroop-sridhar commented Dec 15, 2018 •

edited

Loading

pentp Dec 15, 2018

jkotas Dec 15, 2018

jkotas commented Dec 15, 2018

jkotas Dec 15, 2018

jkotas Dec 15, 2018

jkotas Dec 15, 2018

jkotas Dec 15, 2018

jkotas Dec 15, 2018

AaronRobinsonMSFT Dec 30, 2018

jkotas Dec 31, 2018

jkotas Dec 15, 2018

jkotas Dec 15, 2018

jkotas Dec 15, 2018

jkotas Dec 15, 2018

jkotas Dec 15, 2018

AaronRobinsonMSFT Dec 15, 2018

swaroop-sridhar commented Jan 15, 2019

swaroop-sridhar commented Jan 17, 2019

swaroop-sridhar commented Jan 17, 2019

jkotas commented Jan 17, 2019

AaronRobinsonMSFT commented Jan 17, 2019

swaroop-sridhar commented Jan 18, 2019

swaroop-sridhar commented Jan 18, 2019

	/// Marhall.LoadLibrary(libraryName, assembly, dllImportSearchPath) to approximately achieve
	/// Marhal.LoadLibrary(libraryName, assembly, dllImportSearchPath) to approximately achieve

Add Per-assembly Load Native Library callbacks #21555

Add Per-assembly Load Native Library callbacks #21555

Conversation

swaroop-sridhar commented Dec 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Dec 15, 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

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

swaroop-sridhar commented Jan 15, 2019

swaroop-sridhar commented Jan 17, 2019

swaroop-sridhar commented Jan 17, 2019

jkotas commented Jan 17, 2019

AaronRobinsonMSFT commented Jan 17, 2019

swaroop-sridhar commented Jan 18, 2019

swaroop-sridhar commented Jan 18, 2019

swaroop-sridhar commented Dec 15, 2018 •

edited

Loading