Add NativeLibrary Resolve Event #21929

swaroop-sridhar · 2019-01-10T10:10:45Z

This change adds the Native library resolving event, to be raised as the last attempt to resolve a native DLL in an AssemblyLoadContext.

With this change, the DllImport resolution sequence is as follows (stopping at any step with successful resolution):

If the invoking-assembly is not in the default load context, call AssemblyLoadContext.LoadUnmanagedDll()
Run the default load logic, try loading from:
- AppDomain cache
- NATIVE_DLL_SEARCH_DIRECTORIES
- Invoking-assembly directory, System32, etc. based on DllImportSearchPaths
Raise the ResolvingUnmanagedDll event

API Review: https://github.com/dotnet/corefx/issues/32850

src/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs

jkotas · 2019-01-10T17:29:31Z

src/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs

+
+            if (dllResolveHandler != null)
+            {
+                // Loop through the event subscribers and return the first non-null Assembly instance


return the first non-null Assembly instance

Copy&paste. This is not returning Assembly instances...

jkotas · 2019-01-10T17:30:14Z

src/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs

@@ -509,7 +540,22 @@ internal static void OnProcessExit()
            }
        }

+        /// Event handler for resolving native libraries.


If you want to use /// comments, you have to make them a valid XML.

src/vm/dllimport.cpp

src/vm/dllimport.h

tests/src/Interop/CMakeLists.txt

tests/src/Interop/ResolveEvent/ResolveEventTests.cs

AaronRobinsonMSFT · 2019-01-10T17:48:23Z

I do have a quick questions about the ordering here. What is the expectation of there being an app local native library that is the wrong version, but the user wants a different version with the same name. In the current algorithm, the app local version would be found first and the defined callback wouldn't be found. Is this what we want? In the below directory layout, would the application ever get the chance to load v1/foo.dll?

App/
  foo.dll
  v1/
    foo.dll

jkotas · 2019-01-10T17:59:30Z

he expectation of there being an app local native library that is the wrong version, but the user wants a different version with the same name

This fallback does not help you with that.

You have to use other ways to solve this - the NativeLibrary APIs, do not deploy bad applocal library, etc. No different from how would you solve the same situation for C/C++ code.

AaronRobinsonMSFT · 2019-01-10T18:05:47Z

No different from how would you solve the same situation for C/C++ code.

Other than the fact that if the user defined a P/Invoke that is now no longer valid and it isn't obvious that the wrong library was loaded until the extension library or app developer gets a very unexpected bug.

Not arguing if this is or isn't the wrong approach, just wanted to clarify this is the intent. So our expectation here is that the events are for P/Invokes with a well structured app layout that is known to app and potential plugin developers. In the event there is a situation where non-versioned libraries can pollute the loader paths, using the NativeLibrary API directly is our recommendation right?

swaroop-sridhar · 2019-01-10T21:29:54Z

So our expectation here is that the events are for P/Invokes with a well structured app layout that is known to app and potential plugin developers.

Yes the primary motivation for the native library resolve event is resolution for plugins. The app and it components have to agree on native-library versions to avoid problems with native libraries in the app-global scope.

src/vm/dllimport.cpp

tests/src/Interop/NativeLibraryResolveEvent/ResolveEventTests.cs

swaroop-sridhar · 2019-01-11T00:34:11Z

Thanks @AaronRobinsonMSFT I've made the additional changes.

swaroop-sridhar · 2019-01-11T06:31:35Z

@dotnet-bot test Windows_NT arm Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test

swaroop-sridhar · 2019-01-11T23:54:32Z

@adityamandaleeka and I Iooked at the failure on ARM64. The ResolveEventTests triggered a pre-existing bug in the exception handling code. So, I filed #21964 for it.

Until this is fixed, I'll disable the test on ARM64 Windows.

adityamandaleeka · 2019-01-12T00:01:24Z

@swaroop-sridhar The assertion is familiar, but of course it could be getting triggered in this case for a completely different reason than previous occurrences. I'll investigate what's going on with EH in this situation. Thanks for reporting it!

jkotas · 2019-01-12T00:30:12Z

src/vm/dllimport.cpp

@@ -6656,27 +6736,37 @@ HINSTANCE NDirect::LoadLibraryModule(NDirectMethodDesc * pMD, LoadLibErrorTracke
    if (SString::_wcsicmp(wszLibName, MAIN_CLR_MODULE_NAME_W) == 0)
    {
        hmod = GetCLRModule();
+        if (hmod == NULL)


Should this condition rather be if (hmod != NULL) ?

Also, do we need the check for NULL at all? This should never be NULL.

I am surprised that this is not causing things to fail. It looks like that this special path that checks for MAIN_CLR_MODULE_NAME_W is probably just dead code and can be just deleted.

Yes, sorry, fixed it.

Could you please rather just delete it?

I've fixed the comparison for now. I'll remove the check along with the PAL registration cleanup in another upcoming PR.

@janvorli can you please confirm if this check is now defunct? Thanks.
https://github.com/dotnet/coreclr/blob/master/src/vm/dllimport.cpp#L6657-L6667

Yes, this is defunct. The few places that PInvoke into coreclr in CoreLib use the proper name (libcoreclr) and I expect we will get rid of those few places too at some point.

Thanks for the explanation, I deleted the code.

swaroop-sridhar · 2019-01-12T08:58:37Z

@dotnet-bot test Windows_NT arm Cross Checked Innerloop Build and Test

This change adds the Native library resolving event, to be raised as the last attempt to resolve a native DLL in an AssemblyLoadContext. With this change, the DllImport resolution sequence is as follows (stopping at any step with successful resolution): * If the invoking-assembly is not in the default load context, call AssemblyLoadContext.LoadUnmanagedDll() * Run the default load logic, try loading from: * AppDomain cache * NATIVE_DLL_SEARCH_DIRECTORIES * Invoking-assembly directory, System32, etc. based on DllImportSearchPaths * Raise the ResolvingUnmanagedDll event API Review: https://github.com/dotnet/corefx/issues/32850 The ResolveEventTests triggered a pre-existing bug in the exception handling code (#21964). Disabling the test on ARM64 Windows until the issue is fixed.

swaroop-sridhar · 2019-01-13T06:24:32Z

The "Tizen armel Cross Checked Innerloop Build and Test" leg is failing in every build, and looks unrelated. So will merge the change.

This change adds the Native library resolving event, to be raised as the last attempt to resolve a native DLL in an AssemblyLoadContext. With this change, the DllImport resolution sequence is as follows (stopping at any step with successful resolution): * If the invoking-assembly is not in the default load context, call AssemblyLoadContext.LoadUnmanagedDll() * Run the default load logic, try loading from: * AppDomain cache * NATIVE_DLL_SEARCH_DIRECTORIES * Invoking-assembly directory, System32, etc. based on DllImportSearchPaths * Raise the ResolvingUnmanagedDll event API Review: https://github.com/dotnet/corefx/issues/32850 The ResolveEventTests triggered a pre-existing bug in the exception handling code (dotnet/coreclr#21964). Disabling the test on ARM64 Windows until the issue is fixed. Commit migrated from dotnet/coreclr@8b7d300

swaroop-sridhar requested review from jkotas and AaronRobinsonMSFT January 10, 2019 10:10

AaronRobinsonMSFT reviewed Jan 10, 2019

View reviewed changes

src/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs Show resolved Hide resolved

jkotas reviewed Jan 10, 2019

View reviewed changes

AaronRobinsonMSFT suggested changes Jan 10, 2019

View reviewed changes

AaronRobinsonMSFT reviewed Jan 10, 2019

View reviewed changes

src/vm/dllimport.cpp Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Jan 10, 2019

View reviewed changes

tests/src/Interop/NativeLibraryResolveEvent/ResolveEventTests.cs Outdated Show resolved Hide resolved

AaronRobinsonMSFT approved these changes Jan 11, 2019

View reviewed changes

jkotas reviewed Jan 12, 2019

View reviewed changes

swaroop-sridhar force-pushed the Event branch 2 times, most recently from e78519f to 678c6dd Compare January 12, 2019 00:48

swaroop-sridhar force-pushed the Event branch from 678c6dd to 5ceff61 Compare January 12, 2019 09:02

swaroop-sridhar merged commit 8b7d300 into dotnet:master Jan 13, 2019

swaroop-sridhar mentioned this pull request Jan 31, 2020

Arm64: RegDisplay Missing Context while throwing exceptions dotnet/runtime#11801

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NativeLibrary Resolve Event #21929

Add NativeLibrary Resolve Event #21929

swaroop-sridhar commented Jan 10, 2019

jkotas Jan 10, 2019

jkotas Jan 10, 2019

AaronRobinsonMSFT commented Jan 10, 2019

jkotas commented Jan 10, 2019 •

edited

Loading

AaronRobinsonMSFT commented Jan 10, 2019

swaroop-sridhar commented Jan 10, 2019

swaroop-sridhar commented Jan 11, 2019

swaroop-sridhar commented Jan 11, 2019

swaroop-sridhar commented Jan 11, 2019 •

edited

Loading

adityamandaleeka commented Jan 12, 2019 •

edited

Loading

jkotas Jan 12, 2019

jkotas Jan 12, 2019

jkotas Jan 12, 2019

swaroop-sridhar Jan 12, 2019

jkotas Jan 12, 2019

swaroop-sridhar Jan 12, 2019

swaroop-sridhar Jan 12, 2019

jkotas Jan 12, 2019

swaroop-sridhar Jan 12, 2019

swaroop-sridhar commented Jan 12, 2019

swaroop-sridhar commented Jan 13, 2019

Add NativeLibrary Resolve Event #21929

Add NativeLibrary Resolve Event #21929

Conversation

swaroop-sridhar commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronRobinsonMSFT commented Jan 10, 2019

jkotas commented Jan 10, 2019 • edited Loading

AaronRobinsonMSFT commented Jan 10, 2019

swaroop-sridhar commented Jan 10, 2019

swaroop-sridhar commented Jan 11, 2019

swaroop-sridhar commented Jan 11, 2019

swaroop-sridhar commented Jan 11, 2019 • edited Loading

adityamandaleeka commented Jan 12, 2019 • 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

swaroop-sridhar commented Jan 12, 2019

swaroop-sridhar commented Jan 13, 2019

jkotas commented Jan 10, 2019 •

edited

Loading

swaroop-sridhar commented Jan 11, 2019 •

edited

Loading

adityamandaleeka commented Jan 12, 2019 •

edited

Loading