LoadLibrary refactoring #20841

swaroop-sridhar · 2018-11-06T19:37:55Z

This change refactors the code in DllImport in preparation
for implementing the new NativeLibrary API here:
dotnet/corefx#32015

In particular, it introduces a change in the semantics of the
internal LoadLibrary helper functions.

When a native library is loaded, there are two categories of callers
expecting different return values:

External callers like AssemblyNative::InternalLoadUnmanagedDllFromPath()
and the upcoming System.Runtime.Interop.Marshall.LoadLibrary()
need the raw system handle
Internal callers like LoadLibraryModule() need the PAL registered handle

This change modifies the internal LoadLibraryModule* methods to work
in terms of native system handles, so that external callers can obrain
them directly. Methods requiring PAL-handles can register them explicitly.

[PS: NDirect::LoadLibraryFromPath was already written to return the
system handle instead of the PAL handle. This change extends
the behavior to other private members.]

There is no change in external signature of DllImport class, or the
native Dll cache in AppDomain class.

AaronRobinsonMSFT · 2018-11-07T02:04:24Z

src/vm/dllimport.cpp

-//  Local helper function for the LoadLibraryModule function below
+// Local helper function to load a library.
+// Load the library directly, and don't register it yet with PAL. Returns the native system libray handle.
+// * External callers likeAssemblyNative::InternalLoadUnmanagedDllFromPath() and the upcoming 


"like AssemblyNative::InternalLoadUnmanagedDllFromPath()"

AaronRobinsonMSFT · 2018-11-07T02:04:34Z

src/vm/dllimport.cpp

+// Load the library directly, and don't register it yet with PAL. Returns the native system libray handle.
+// * External callers likeAssemblyNative::InternalLoadUnmanagedDllFromPath() and the upcoming 
+//   System.Runtime.Interop.Marshall.LoadLibrary() need the raw system handle
+// * Internal callers like LoadLibraryModule() can obrain it via PAL_RegisterLibraryDirect()


AaronRobinsonMSFT · 2018-11-07T02:19:37Z

src/vm/dllimport.cpp

-            *szComma = '\0';
-            while (COMCharacter::nativeIsWhiteSpace(*(++szComma)));
+        *szComma = '\0';
+        while (COMCharacter::nativeIsWhiteSpace(*(++szComma)));


Please move the ; to the next line or comment that we are trimming here.

janvorli · 2018-11-07T15:12:49Z

@swaroop-sridhar I am sorry for the delay, I was focusing on other stuff. I am planning to review this later today.

janvorli

One thing to consider. You have added comments describing when the HMODULE is actually the native handle, which is great. But I wonder if it would be even better to change the return type of these functions to VOID*.

janvorli · 2018-11-07T21:27:07Z

src/vm/dllimport.cpp

@@ -6069,7 +6069,13 @@ class LoadLibErrorTracker
    SString  m_message;
 };  // class LoadLibErrorTracker

-//  Local helper function for the LoadLibraryModule function below
+// Local helper function to load a library.
+// Load the library directly, and don't register it yet with PAL. Returns the native system libray handle.


A nit - maybe update to "don't register it yet with PAL on Unix" as there is no PAL on Windows. There is another place with a similar comment.

janvorli · 2018-11-07T22:56:44Z

src/vm/dllimport.cpp


-            if (pMD->HasDefaultDllImportSearchPathsAttribute())
+            if(pModule->HasDefaultDllImportSearchPathsAttribute())


A nit - missing space after if at multiple places. I know it didn't come from your change, but it would be nice to fix when you are editing the code.

janvorli · 2018-11-07T23:25:01Z

src/vm/dllimport.cpp

+//   System.Runtime.Interop.Marshall.LoadLibrary() need the raw system handle
+// * Internal callers like LoadLibraryModule() can obrain it via PAL_RegisterLibraryDirect()
+//
+// This method returns native system libray handle not registered with the PAL


A nit: libray -> library (multiple places)

janvorli · 2018-11-07T23:41:52Z

src/vm/dllimport.cpp

+        // First checks if the method has DefaultDllImportSearchPathsAttribute. If method has the attribute
+        // then dllImportSearchPathFlag is set to its value.
+        // Otherwise checks if the assembly has the attribute. 
+        // If assembly has the attribute then flag ise set to its value.


A nit: ise -> is

jkotas · 2018-11-08T00:09:10Z

added comments describing when the HMODULE

Or even better: define NATIVE_LIBRARY_HANDLE to make this self-documenting.

swaroop-sridhar · 2018-11-08T20:06:12Z

Or even better: define NATIVE_LIBRARY_HANDLE to make this self-documenting.

I thought about this, but held back because the types defined in pal_mstypes.h were a subset of
https://docs.microsoft.com/en-us/windows/desktop/winprog/windows-data-types

I'll make the change now.

swaroop-sridhar · 2018-11-08T20:53:48Z

Thanks for the feedback @janvorli @jkotas @AaronRobinsonMSFT
I've addressed the feedback in 73012b5

janvorli · 2018-11-08T21:01:12Z

src/pal/inc/pal.h

+// In Windows, NATIVE_LIBRARY_HANDLE is the same as HMODULE.
+// On Unix systems, NATIVE_LIBRARY_HANDLE type represents a library handle not registered with the PAL.
+// To get a HMODULE on Unix, call PAL_RegisterLibraryDirect() on a NATIVE_LIBRARY_HANDLE.
+typedef HANDLE NATIVE_LIBRARY_HANDLE;


A nit - the dlopen returns void*, so it would be cleaner to define it that way.

Also, I cannot see the type definition for Windows. The pal.h is included for Unix only.

I added a definition in common.h, for the !FEATURE_PAL case Thanks

inc/palclr.h would be a better place for it. It is where all similar definitions are.

Thanks, I changed the Windows definition to be in palclr.h via palclr_win.h

out

swaroop-sridhar · 2018-11-08T23:05:15Z

@dotnet-bot test dotnet-coreclr

swaroop-sridhar · 2018-11-08T23:41:42Z

@dotnet-bot test Ubuntu x64 Checked CoreFX Tests

This change refactors the code in DllImport in preparation for implementing the new NativeLibrary API here: dotnet/corefx#32015 The two main changes are: 1) A change in the semantics of the internal LoadLibrary helper functions. When a native library is loaded, there are two categories of callers expecting different return values: External callers like AssemblyNative::InternalLoadUnmanagedDllFromPath() and the upcoming System.Runtime.Interop.Marshall.LoadLibrary() need the raw system handle Internal callers like LoadLibraryModule() need the PAL registered handle This change modifies the internal LoadLibraryModule* methods to work in terms of native system handles, so that external callers can obrain them directly. Methods requiring PAL-handles can register them explicitly. There is no change in external signature of DllImport class, or the native Dll cache in AppDomain class. 2) Differentiate HMODULE and NATIVE_LIBRARY_HANDLE This change defines NATIVE_LIBRARY_HANDLE type to represent raw system handles to native libraries that are not registered with the PAL (On Unix systems). The types on PAL and DlImport methods are adjusted to make this semantic distinction explicit.

swaroop-sridhar · 2018-11-09T19:04:29Z

@dotnet-bot test this please

swaroop-sridhar · 2018-11-12T16:25:44Z

@janvorli, I needed to make another small change in PAL code for test pass. Please review this commit: 48289a5

* Refactor LoadLibrary Methods This change refactors the code in DllImport in preparation for implementing the new NativeLibrary API here: dotnet/corefxdotnet/coreclr#32015 The two main changes are: 1) A change in the semantics of the internal LoadLibrary helper functions. When a native library is loaded, there are two categories of callers expecting different return values: External callers like AssemblyNative::InternalLoadUnmanagedDllFromPath() and the upcoming System.Runtime.Interop.Marshall.LoadLibrary() need the raw system handle Internal callers like LoadLibraryModule() need the PAL registered handle This change modifies the internal LoadLibraryModule* methods to work in terms of native system handles, so that external callers can obrain them directly. Methods requiring PAL-handles can register them explicitly. There is no change in external signature of DllImport class, or the native Dll cache in AppDomain class. 2) Differentiate HMODULE and NATIVE_LIBRARY_HANDLE This change defines NATIVE_LIBRARY_HANDLE type to represent raw system handles to native libraries that are not registered with the PAL (On Unix systems). The types on PAL and DlImport methods are adjusted to make this semantic distinction explicit. * Fix loading LibC via PAL_LoadLibraryDirect() Commit migrated from dotnet/coreclr@214c3b6

swaroop-sridhar force-pushed the pal branch from 4989d99 to fb048e3 Compare November 7, 2018 01:22

swaroop-sridhar changed the title ~~WIP Test Load Library refactoring~~ Test Load Library refactoring Nov 7, 2018

swaroop-sridhar requested review from janvorli and AaronRobinsonMSFT November 7, 2018 01:23

AaronRobinsonMSFT added area-PAL area-Interop area-AssemblyLoader labels Nov 7, 2018

AaronRobinsonMSFT added this to the 3.0 milestone Nov 7, 2018

AaronRobinsonMSFT reviewed Nov 7, 2018

View reviewed changes

swaroop-sridhar changed the title ~~Test Load Library refactoring~~ LoadLibrary refactoring Nov 7, 2018

swaroop-sridhar force-pushed the pal branch from fb048e3 to ca59138 Compare November 7, 2018 03:29

janvorli approved these changes Nov 7, 2018

View reviewed changes

swaroop-sridhar force-pushed the pal branch from ca59138 to 73012b5 Compare November 8, 2018 20:52

janvorli reviewed Nov 8, 2018

View reviewed changes

out Outdated Show resolved Hide resolved

swaroop-sridhar force-pushed the pal branch 3 times, most recently from 7b14435 to 220797a Compare November 8, 2018 22:01

swaroop-sridhar force-pushed the pal branch 2 times, most recently from ec861ec to 0a0077c Compare November 9, 2018 09:01

swaroop-sridhar force-pushed the pal branch from 0a0077c to 17ce8cf Compare November 12, 2018 09:09

Fix loading LibC via PAL_LoadLibraryDirect()

48289a5

swaroop-sridhar force-pushed the pal branch from 17ce8cf to 48289a5 Compare November 12, 2018 10:31

janvorli merged commit 214c3b6 into dotnet:master Nov 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LoadLibrary refactoring #20841

LoadLibrary refactoring #20841

swaroop-sridhar commented Nov 6, 2018 •

edited

Loading

AaronRobinsonMSFT Nov 7, 2018

AaronRobinsonMSFT Nov 7, 2018

AaronRobinsonMSFT Nov 7, 2018

janvorli commented Nov 7, 2018

janvorli left a comment

janvorli Nov 7, 2018

janvorli Nov 7, 2018

janvorli Nov 7, 2018

janvorli Nov 7, 2018

jkotas commented Nov 8, 2018

swaroop-sridhar commented Nov 8, 2018

swaroop-sridhar commented Nov 8, 2018

janvorli Nov 8, 2018

janvorli Nov 8, 2018

swaroop-sridhar Nov 8, 2018

jkotas Nov 8, 2018

swaroop-sridhar Nov 8, 2018

swaroop-sridhar commented Nov 8, 2018

swaroop-sridhar commented Nov 8, 2018

swaroop-sridhar commented Nov 9, 2018

swaroop-sridhar commented Nov 12, 2018


		if (pMD->HasDefaultDllImportSearchPathsAttribute())
		if(pModule->HasDefaultDllImportSearchPathsAttribute())

LoadLibrary refactoring #20841

LoadLibrary refactoring #20841

Conversation

swaroop-sridhar commented Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janvorli commented Nov 7, 2018

janvorli left a 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

jkotas commented Nov 8, 2018

swaroop-sridhar commented Nov 8, 2018

swaroop-sridhar commented Nov 8, 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

swaroop-sridhar commented Nov 8, 2018

swaroop-sridhar commented Nov 8, 2018

swaroop-sridhar commented Nov 9, 2018

swaroop-sridhar commented Nov 12, 2018

swaroop-sridhar commented Nov 6, 2018 •

edited

Loading