Enable support DllImport a native assembly whose name contains '.' by luqunl · Pull Request #17505 · dotnet/coreclr

luqunl · 2018-04-10T23:33:55Z

Currently, In Windows, if the name your native dll specified in DllImport contains '.', such as Test.Foo, DllImport will only search for file Test.Foo and it won't search for file Test.Foo.dll

The fix is to append ".dll" if the native dll name specified in DllImport doesn't contain ".dll" .

Fix #17150

jkotas · 2018-04-11T00:36:35Z

Test?

danmoseley · 2018-04-11T02:49:56Z

Ideally a test in Corefx so we can ensure it is fixed in Mono, CoreRT.

jkotas · 2018-04-11T03:11:54Z

CoreCLR has better setup for interop tests. And Mono and CoreRT can run CoreCLR tests too.

mikedn · 2018-04-11T04:50:03Z

The fix is to append ".dll" if the native dll name specified in DllImport doesn't contain ".dll" .

So what happens if the specified name is "foo.exe"? Will it search for "foo.exe.dll"?

ghost · 2018-04-11T14:31:22Z

I totally missed your PR. This approach of using DetermineLibNameVariations for both Windows and Unix is most probably neater than what I was experimenting with https://github.com/kasper3/coreclr/commit/edb45878f6915bf7443d74f7da2d17dd1dae5ac9, except for missing .exe check.
Also, does it account for casing .dll, .DLL etc.?

jkotas · 2018-04-12T18:58:24Z

@luqunl Could you please rebase away the commits not related to your change?

luqunl · 2018-04-12T21:07:11Z

Let me try to rebase my changes

luqunl · 2018-04-12T22:41:00Z

Could you please rebase away the commits not related to your change?

Fixed

Test?

Fixed

So what happens if the specified name is "foo.exe"? Will it search for "foo.exe.dll"?

Thanks for catching this. Fixed

Also, does it account for casing .dll, .DLL etc.?

Thanks for catching this. Fixed

jkotas · 2018-04-12T23:01:12Z

src/vm/dllimport.cpp

            containsSuffix = it == libName.End() || *it == (WCHAR)'.';
        }
+#else
+        if (libName.EndsWithCaseInsensitive(PLATFORM_SHARED_LIB_SUFFIX_W) || libName.EndsWithCaseInsensitive(WINDOWS_SHARED_EXE_SUFFIX_W))


I do not understand why we are special casing .exe suffix here. .exe suffix is non-standard suffix for shared libraries. If I want to, I can use any other non-standard suffix (e.g. .dat) for my .dlls and LoadLibrary will load them too.

The only reason is to avoid to load/search .exe.dll file. if it is possible that someone name his file as a.exe.dll, then we can remove this special case.

I am more worried about that cases that used to work fine before this change, but do not work anymore after this change. Consider:

[DllImport("mydll.dat")] extern static int sum(int a, int b); static void Main() { Console.WriteLine(sum(1,2).ToString()); }

And mydll.dat that is renamed mydll.dll to mydll.dat. It works fine before this change (mydll.dat is passed to LoadLibrary), but it does not work after this change (mydll.dat.dll is passed to LoadLibrary).

For this case, try both name itself and name itself+extension.

' static const char* const SuffixFirst[] =
{
"%.0s%s%s", // name+suffix
#ifdef FEATURE_PAL
"%s%s%s", // prefix+name+suffix
#endif
"%.0s%s", // name
#ifdef FEATURE_PAL
"%s%s%.0s" // prefix+name
#endif
};
'

I can check // name first, then //name + suffix to be more comp with current behavior.

jkotas · 2018-04-12T23:04:23Z

tests/src/Interop/DllImportAttribute/Simple/CMakeLists.txt

+add_library (DllFileProbe SHARED ${SOURCES})
+target_link_libraries(DllFileProbe ${LINK_LIBRARIES_ADDITIONAL}) 
+
+#get_cmake_property(_variableNames VARIABLES)


Do we need these commented out blocks?

I will remove these commented lines..just copy & paste..

Removed these comments

jkotas

To summarize, I think there are 4 interesting cases on Windows:

The name does not contain dot: Try the name only. No other suffixes. LoadLibrary will append .dll suffix internally.
The name ends with a dot: Try the name only. No other suffixes. Maintains the behavior documented in MSDN.
The name ends with .dll (or .exe too?): Try the name only. No point in trying .dll.dll.
Otherwise: Try name + .dll suffix first, and then try name without .dll suffix.

jkotas · 2018-04-13T21:45:36Z

src/vm/dllimport.cpp

@@ -6078,9 +6096,13 @@ static void DetermineLibNameVariations(const char* const** libNameVariations, in
            static const char* const SuffixFirst[] =
            {
                "%.0s%s%s", // name+suffix


For name without any dots, this is going to probe twice now:

First time, we append the .dll

Second time, we pass in name without .dll suffix and LoadLibrary appends it internally.

It would be nice to avoid this duplication (e.g. just let LoadLibrary take care of appending .dll to the simple names).

jkotas · 2018-04-13T21:46:40Z

src/vm/dllimport.cpp

            static const char* const SuffixFirst[] =
            {
                "%.0s%s%s", // name+suffix
+#ifdef FEATURE_PAL


Similar for names that end with a .. We should not do any probing for them on Windows - to maintain the behavior documented for them on MSDN.

jkotas · 2018-04-13T21:49:19Z

src/vm/dllimport.cpp

+#else // !FEATURE_PAL
+#define PLATFORM_SHARED_LIB_SUFFIX_A ".dll"
+#define PLATFORM_SHARED_LIB_PREFIX_A ""
+#define PLATFORM_SHARED_LIB_SUFFIX_W W(".dll")


Nit: I do not think we need to have defines for these since they are used just once.

jkotas · 2018-04-13T23:33:24Z

src/vm/dllimport.cpp

        *numberOfVariations = COUNTOF(NameOnly);
    }
 }
+#else


Nit: It would be nice to add // FEATURE_PAL comment here.

jkotas · 2018-04-13T23:33:33Z

src/vm/dllimport.cpp

+        *numberOfVariations = COUNTOF(SuffixLast);
+    }
+}
+#endif


jkotas · 2018-04-13T23:35:42Z

src/vm/dllimport.cpp

+    bool containsSuffix = libName.EndsWithCaseInsensitive(W(".dll")) || libName.EndsWithCaseInsensitive(W(".exe"));
+    if (endWithDot || notContainDot || containsSuffix || !libNameIsRelativePath)
+    {
+        // file name end with '.'


This comment is not very useful. It is a literal rewrite of the above condition in plain English.

Instead, it would be useful to have comment that describes the reasons for this logic.

Added more comments

jkotas · 2018-04-14T00:49:24Z

Build errors:

src\vm\dllimport.cpp(6110): error C2220: warning treated as error - no 'object' file generated 
src\vm\dllimport.cpp(6110): warning C4800: 'BOOL': forcing value to bool 'true' or 'false' (performance warning)

luqunl · 2018-04-14T03:55:10Z

Fixed build break due to compiler warning about BOOL to bool conversation.

ghost · 2018-04-14T07:21:13Z

tests/src/Interop/DllImportAttribute/FileExtensionProbe.cs is encoded as UTF16LE (file --mime-encoding FileExtensionProbe.cs), and github is showing it as a binary file in diff view. You can fix it by changing it to ASCII using notepad file>saveAs or bash: iconv -f utf-16le -t US_ASCII FileExtensionProbe.cs > FileExtensionProbe.cs_new then delete the old and rename _new one.

jkotas · 2018-04-14T12:32:18Z

src/vm/dllimport.cpp

+    bool containsSuffix = libName.EndsWithCaseInsensitive(W(".dll")) || libName.EndsWithCaseInsensitive(W(".exe"));
+
+    // The purpose of following code is to workaround LoadLibrary limitation: 
+    // LoadLibray won't append extension if filename itself contains '.'. Thus It will break the following scenario:


Typo: It -> it

jkotas · 2018-04-14T12:37:07Z

src/vm/dllimport.cpp

+#else // FEATURE_PAL
+static void DetermineLibNameVariations(const char* const** libNameVariations, int* numberOfVariations, const SString& libName, bool libNameIsRelativePath)
+{
+    bool endWithDot = libName.EndsWith(W(".")) == TRUE;


It is a bad practice to compare BOOL to TRUE. BOOL is actually integer and any non-zero value can mean true. TRUE is defined as 1. So comparing BOOL to TRUE assumes that the non-zero true value is 1 that is not always the case.

Use either !!libName.EndsWith(W(".")) or libName.EndsWith(W(".")) != FALSE.

Thanks for this. Previous I had searched the implementation for SString.EndWith and it always return TRUE or FALSE. I will use !! instead.

luqunl · 2018-04-14T15:50:35Z

tests/src/Interop/DllImportAttribute/FileExtensionProbe.cs is encoded as UTF16LE (file --mime-encoding FileExtensionProbe.cs), and github is showing it as a binary file in diff view. You can fix it by changing it to ASCII using notepad file>saveAs or bash: iconv -f utf-16le -t US_ASCII FileExtensionProbe.cs > FileExtensionProbe.cs_new then delete the old and rename _new one.

Thanks a lot for this info. I saw this issue but didn't realize that this is file encoding issue.

luqunl · 2018-04-14T15:51:12Z

Typo in loadlibrary
Fixed

ghost · 2018-04-14T16:22:36Z

tests/src/Interop/DllImportAttribute/FileExtensionProbe.cs

+
+public class FileExtensionProbe
+{
+    private static int Fails = 0;


nit: s_failures

ghost · 2018-04-14T16:25:30Z

@jkotas, is there a chance to include this in 2.1 release?

jkotas · 2018-04-14T17:24:30Z

@jkotas, is there a chance to include this in 2.1 release?

There would need to be a good justification why this change is important to include in 2.1 release, and @luqunl would have to take this change for review with shiproom (a group of people who are driving the release).

jkotas · 2018-04-14T17:33:23Z

src/vm/dllimport.cpp

+#else // FEATURE_PAL
+static void DetermineLibNameVariations(const char* const** libNameVariations, int* numberOfVariations, const SString& libName, bool libNameIsRelativePath)
+{
+    bool endWithDot = !!libName.EndsWith(W("."));


Now that I am looking at this again ... caching these in bool variables makes this less efficient than it needs to be. If you put these directly into the if statement without caching them in locals like this:

if (!libName.Find(libName.Begin(), W('.')) || libName.EndsWith(W(".")) || libName.EndsWithCaseInsensitive(W(".dll")) || libName.EndsWithCaseInsensitive(W(".exe")) {

It will be more efficient because of it will stop at !libName.Find(libName.Begin(), W('.')) for names without dot; and the remaining two statements won't be executed at all.

It also avoids the !! wart.

jkotas

LGTM modulo the one last nit.

jkotas · 2018-04-14T17:39:52Z

@jkotas, is there a chance to include this in 2.1 release?

@kasper3 Why do you think it is important to include this in 2.1 release?

ghost · 2018-04-14T17:41:23Z

There would need to be a good justification why this change is important to include in 2.1

In 2.0 and older versions, we are using build time conditions:

#if WINNT
[DllImport("shared.library.dll")]
#else
[DllImport("shared.library")]
#endif

With this change, it will be reduced to [DllImport("shared.library")]. The sooner this inconsistency goes away, the better.

jkotas · 2018-04-15T01:38:55Z

src/vm/dllimport.cpp

+        libName.EndsWith(W(".")) || 
+        libName.EndsWithCaseInsensitive(W(".dll")) || 
+        libName.EndsWithCaseInsensitive(W(".exe")) || 
+        !libNameIsRelativePath)


libNameIsRelativePath should be first because of it is the cheapest condition to evaluate?

Also, is the relative path condition really correct here? libNameIsRelativePath is going to be true for a paths like:

XXX\YYY\library

or

\XXX\YYY\library.

On Unix, we will end up trying paths like:

libXXX\YYY\library.so

or

lib\XXX\YYY\library.so.

Move libNameIsRelativePath to the first one.

is the relative path condition really correct here?

This is incorrect. since this is an old issue and I will create another issue to track and solve this problem

jkotas · 2018-04-15T01:56:53Z

src/vm/dllimport.cpp

-    {
-        LPCWSTR currLibNameVariation = wszLibName;
-#endif
+        currLibNameVariation.Printf(prefixSuffixCombinations[i], PLATFORM_SHARED_LIB_PREFIX_A, name, PLATFORM_SHARED_LIB_SUFFIX_A);


We have converted the name from UTF8 to Unicode above once already.

The PrintF is going to convert it again; but I do not think it is going to use UTF8 on Windows. I think this needs to use the converted name to Unicode wszLibName; not the UTF8 name.

Update all of these argument to WCHAR* instead of char*

jkotas · 2018-04-15T01:59:00Z

include this in 2.1 release

There seems to be way too many subtle issues in this change. I do not think we should be considering this for 2.1 release.

jkotas · 2018-04-16T23:28:58Z

LGTM

ghost · 2018-04-17T05:18:58Z

@jkotas, if merge here indicates that we had confidence in implementation, is it now an good candidate for 2.1 if it is sent it to the shiproom for review?

jkotas · 2018-04-17T05:33:05Z

master is open for 2.2 now. Merge means that we believe it is a good change for 2.2.

It would be up to @luqunl to present it to the shiproom to consider it for 2.1. My guess is that it would not meet the bar.

sdmaclea · 2018-04-17T12:47:56Z

This file introduced corrupt line endings https://github.com/dotnet/coreclr/pull/17505/files#diff-23d6cee28d9638de3cc99b9da9fbb3b9

dotnet-bot added the 2 - In Progress label Apr 10, 2018

luqunl added the area-Interop label Apr 10, 2018

luqunl changed the title ~~[WIP]Enable support DllImport a native assembly whose name contains '.'~~ Enable support DllImport a native assembly whose name contains '.' Apr 10, 2018

luqunl removed the 2 - In Progress label Apr 10, 2018

luqunl and others added 4 commits April 12, 2018 14:10

Enable support DllImport a native assembly whose name contains '.'

26373a2

Update according to review comments

64d1d22

update testcase

61a4491

update testcase in linux

79e19a8

luqunl force-pushed the #17150 branch from 7126c01 to 79e19a8 Compare April 12, 2018 22:38

jkotas reviewed Apr 12, 2018

View reviewed changes

jeffschwMSFT added this to the 2.2.0 milestone Apr 13, 2018

luqunl added 2 commits April 13, 2018 10:37

Remove comments in CMakeLists.txt

bcc6b27

Alllow DllImport test.dat in Windows

fd3e436

jkotas suggested changes Apr 13, 2018

View reviewed changes

jkotas reviewed Apr 13, 2018

View reviewed changes

update for jkotas comments

9f32a18

jkotas reviewed Apr 13, 2018

View reviewed changes

update for jkotas comments 2

96396b5

Fix build break and Update code comments

cf0259b

jkotas reviewed Apr 14, 2018

View reviewed changes

Update according to review comments

dfcf86e

ghost reviewed Apr 14, 2018

View reviewed changes

jkotas reviewed Apr 14, 2018

View reviewed changes

jkotas approved these changes Apr 14, 2018

View reviewed changes

update for review comments

c042515

jkotas reviewed Apr 15, 2018

View reviewed changes

Update for comments

df44ee5

jkotas merged commit 9d45401 into dotnet:master Apr 17, 2018

jkotas mentioned this pull request Apr 17, 2018

Enable support DllImport a native assembly whose name contains '.' dotnet/corert#5717

Open

luqunl deleted the #17150 branch April 17, 2018 05:53

sdmaclea mentioned this pull request Apr 17, 2018

Fix line endings #17606

Merged

Apollo3zehn mentioned this pull request Apr 30, 2018

Drop native lib extension OneDAS-Group/OneDAS-Core#163

Closed

jkotas mentioned this pull request Nov 30, 2020

Fix C# native library loading in .NET 5 single-file apps grpc/grpc#24744

Merged

Conversation

luqunl commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Apr 11, 2018

Uh oh!

danmoseley commented Apr 11, 2018

Uh oh!

jkotas commented Apr 11, 2018

Uh oh!

mikedn commented Apr 11, 2018

Uh oh!

ghost commented Apr 11, 2018

Uh oh!

jkotas commented Apr 12, 2018

Uh oh!

luqunl commented Apr 12, 2018

Uh oh!

luqunl commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luqunl Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Apr 14, 2018

Uh oh!

luqunl commented Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

luqunl commented Apr 10, 2018 •

edited

Loading

luqunl commented Apr 12, 2018 •

edited

Loading

jkotas Apr 12, 2018 •

edited

Loading

luqunl Apr 13, 2018 •

edited

Loading

jkotas Apr 13, 2018 •

edited

Loading

luqunl commented Apr 14, 2018 •

edited

Loading

ghost Apr 14, 2018 •

edited by ghost

Loading

jkotas Apr 15, 2018 •

edited

Loading

jkotas Apr 15, 2018 •

edited

Loading

luqunl Apr 16, 2018 •

edited

Loading

jkotas Apr 15, 2018 •

edited

Loading