Linking native libraries into superhost #38684

VSadov · 2020-07-01T21:43:33Z

Related to: #37119

This change results in singlefilehost to embed the following native libraries on Linux:

libSystem.Native
libSystem.IO.Compression.Native
libSystem.Net.Security.Native
libSystem.Security.Cryptography.Native.OpenSsl

jkotas · 2020-07-02T02:31:13Z

delete exports file

Is this going to export everything - is it going to expose us to problems with colliding symbols like #38189 ?

VSadov · 2020-07-02T15:04:20Z

Yes, removing export file, in theory would export what the libraries would be exporting by themselves.
It could be possible to get duplicates if more than one library export same named methods - like "Initialize()". I hope we do not have those.

I want to see first if the whole thing works at all. If there are other reasons why this may not work - for example on OSX or MUSL, it is better to know that earlier.

jkotas · 2020-07-02T15:10:54Z

in theory

It is not a theoretical risk. We had number of crashing bugs around this.

src/installer/corehost/cli/apphost/static/CMakeLists.txt

src/installer/corehost/build.sh

AaronRobinsonMSFT · 2020-07-14T20:31:03Z

src/coreclr/src/pal/src/loader/module.cpp

+    for (int i = 0; i < count; ++i)
+    {
+        const char* match = toRedirect[i];
+        int matchLength = strlen(match);


These lengths should be compile time constants and have no need to be computed at runtime. This should be simple to handle above.

Right. Originally this was supposed to be more dynamic. Now that only Linux is supported, this can be simplified a bit.

Just call strcmp ?

I am not sure how fast we need to be with this check.
I can as well switch on a strlen against 4 known values and strcmp accordingly, but it might be that just strcmp is good enough. These are relatively short strings.

Actually, it looks like we try to support some/path/System.Native.so as well, then knowing the length is needed.
In C# this would be EndsWith.

it looks like we try to support some/path/System.Native.so

Is it a good idea to support this? If somebody specifies a path, I would expect that we will load the stuff from that path and do not do any single-file redirects.

Another thought: The Unix PAL may be a wrong places for these redirects. Doing the redirects in PAL would not work on Windows - there is no Unix PAL on Windows. Would it be better to do these redirects in LoadNativeLibraryBySearch ?

not supporting path (just using strcmp) works. I am not sure what was the intent to support path as well. Would we ever get a path for these libraries?

LoadNativeLibraryBySearch seems like a better place to put the shim. Especially when supporting Windows.
I did not know that this entry point existed. I wonder if there are caveats with placing shim there. Maybe we should do that separately.

A lot of this came from quick&dirty prototype that was put together without much thinking. The point of this work is to do it right (tm).

Moved the library load redirection shim to LoadNativeLibraryBySearch

src/installer/corehost/cli/apphost/static/CMakeLists.txt

janvorli · 2020-07-14T20:49:22Z

src/installer/corehost/cli/apphost/static/CMakeLists.txt

-    generate_exports_file(${DEF_SOURCES} ${EXPORTS_FILE})
+    if(CLR_CMAKE_TARGET_OSX)
+        LIST(APPEND NATIVE_LIBS
+          ${NATIVE_LIBS_LOCATION}/libSystem.Security.Cryptography.Native.Apple.a


Why are we adding this?

We would be adding this on OSX, since OSX has one more native library.

But that should not be part of this PR as we don't add any other static libraries for OSX, right? That's what I was asking about.

I had two options - remove the non-linux code completely or keep it somehow, but not run. There was a suggestion above to comment it out and put explanation why (we kind of have an explanation already).
Would that be better?

Also, this cmake code path is now Linux only.

I will comment out the non-Linux code. It seems indeed confusing to leave it as this.

I would prefer just completely removing this whole if. It is very confusing and after we enable OSX, it would not be correct either (the libSystem.Security.Cryptography.Native.OpenSsl.a is not for OSX).

Ok. I will remove this part.

Pushed a change. Please take a look if it is what you expected.

AaronRobinsonMSFT · 2020-07-15T19:23:36Z

src/coreclr/src/vm/dllimport.cpp

+                W("System.Security.Cryptography.Native.OpenSsl.so")
+            };
+
+            int count = sizeof(toRedirect) / sizeof(toRedirect[0]);


Nit: ~~sizeof(toRedirect) / sizeof(toRedirect[0])~~ -> lengthof(toRedirect)

Feel free to defer this change until post P8.

AaronRobinsonMSFT · 2020-07-15T19:24:06Z

src/coreclr/src/pal/src/loader/module.cpp

@@ -592,6 +592,11 @@ PALAPI
 PAL_LoadLibraryDirect(
    IN LPCWSTR lpLibFileName)
 {
+    if (!lpLibFileName && g_running_in_exe)


I think this warrants a comment.

There is a helper that is self-explanatory ShouldRedirectToCurrentLibrary. Just made the code call that.

jkotas · 2020-07-15T20:22:28Z

src/coreclr/src/pal/src/loader/module.cpp

@@ -592,6 +604,11 @@ PALAPI
 PAL_LoadLibraryDirect(
    IN LPCWSTR lpLibFileName)
 {
+    if (ShouldRedirectToCurrentLibrary(lpLibFileName))


This should not need to check g_running_in_exe at all. We should be able to return dlopen(NULL...) for lpLibFileName == NULL uncoditionally.

There is code following this treating NULL and "" as error conditions. I thought there is some value to fail on unexpected NULL.

So should the g_running_in_exe be removed in the ShouldRedirectToCurrentLibrary helper?

If the check is unnecessary, I guess it is unnecessary in either case.

Actually - I think the g_running_in_exe check could be an assert.

jkotas · 2020-07-15T20:23:09Z

src/coreclr/src/pal/src/loader/module.cpp

@@ -592,6 +604,11 @@ PALAPI
 PAL_LoadLibraryDirect(
    IN LPCWSTR lpLibFileName)
 {
+    if (ShouldRedirectToCurrentLibrary(lpLibFileName))
+    {


Nit: This should be after the ENTRY macro to follow PAL conventions

Good to know.

Also should not return without LOGEXIT and PERF_EXIT . Fixed that too.

…eher to redirect load to current module

… an assert

jkotas · 2020-07-16T18:01:07Z

src/coreclr/src/pal/src/loader/module.cpp

@@ -603,6 +615,12 @@ PAL_LoadLibraryDirect(
          lpLibFileName ? lpLibFileName : W16_NULLSTRING,
          lpLibFileName ? lpLibFileName : W16_NULLSTRING);

+    if (ShouldRedirectToCurrentLibrary(lpLibFileName))


Nit: I do not think that the assert has much value. I would make this just libraryNameOrPath == NULL.

I had doubts that we may be using NULL on this codepath for something else, perhaps transiently, especially when I saw failures on Linux arm/arm64 related to "kernel32.dll not found".
There was a thought - maybe NULL name is involved in that scenario. However, having this assert I knew that it cannot be so. It turned out something unrelated to this change and passed reliably after rebasing.

I guess at this point we know fairly well that NULL is not used for anything else, so assert can be removed.

…Library will be always considered self-referring.

janvorli · 2020-07-16T19:30:45Z

src/coreclr/src/vm/dllimport.cpp

@@ -51,6 +51,9 @@
 #include "clr/fs/path.h"
 using namespace clr::fs;

+// Specifies whether coreclr is embedded or standalone
+extern bool g_coreclr_embedded;


Where are we setting this variable? I cannot see it in this PR.

the variable was introduced when we did coreclr embedding (previous PR of this series). It relies on coreclr mergeable knowing this fact at compile time.

runtime/src/coreclr/src/vm/ceemain.cpp

Line 242 in 371ae3c

// g_coreclr_embedded indicates that coreclr is linked directly into the program

Ah, thanks for the details, I've missed this one, I somehow thought we had just the g_running_in_exe.

g_running_in_exe is the PAL mirror of g_coreclr_embedded, since it can't see g_coreclr_embedded directly.

VSadov · 2020-07-16T23:02:15Z

Assert failure(PID 22 [0x00000016], Thread: 122 [0x007a]): Assertion failed '!foundDiff' in 'OpenSsl:GetSslError(Microsoft.Win32.SafeHandles.SafeSslHandle,int,byref):int' during 'Linear scan register alloc' (IL size 99)

File: /__w/1/s/src/coreclr/src/jit/lsra.cpp Line: 2262

This is in an ordinary (not singlefile) Library test on Linux musl arm64
It does not look like it could be related to the change

VSadov · 2020-07-16T23:11:52Z

Same assert failed in #39464 as well

VSadov · 2020-07-16T23:27:12Z

Logged a bug about the LSRA assert - #39484

VSadov · 2020-07-16T23:28:00Z

I think this is ok for merging. Thanks everyone for reviewing!!!!

* native libs location * pass artifacts locations as CMake variables. * Link native libs * use LibrariesConfiguration to discover libraries artifacts * redirect to current exe * remove bundled binaries from the netcoreapp/pkg * delete exports file * no bundling of native libs on OSX * only link native libs on Linux * Support linking of coreclr on Linux only * PR feedback * move the shim to dllimport.cpp * Use library names vs. file names * removed ShouldRedirectToCurrentLibrary. NULL library name in PAL_LoadLibrary will be always considered self-referring.

VSadov added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Jul 1, 2020

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 1, 2020

VSadov force-pushed the nativeBundle branch from cdf00c1 to 42a1102 Compare July 2, 2020 19:32

jkoritzinsky reviewed Jul 2, 2020

View reviewed changes

src/installer/corehost/cli/apphost/static/CMakeLists.txt Outdated Show resolved Hide resolved

jkoritzinsky reviewed Jul 2, 2020

View reviewed changes

src/installer/corehost/build.sh Show resolved Hide resolved

VSadov force-pushed the nativeBundle branch 2 times, most recently from c94ea93 to 77bcb18 Compare July 7, 2020 01:54

VSadov force-pushed the nativeBundle branch 3 times, most recently from 4ee534c to 8cd0844 Compare July 13, 2020 17:06

VSadov removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Jul 13, 2020

VSadov force-pushed the nativeBundle branch 3 times, most recently from 81a5cae to dc532a0 Compare July 14, 2020 18:02

VSadov marked this pull request as ready for review July 14, 2020 18:04

VSadov requested review from swaroop-sridhar, janvorli, jkotas and vitek-karas July 14, 2020 18:05

VSadov changed the title ~~[WIP]Linking native libraries into superhost~~ Linking native libraries into superhost Jul 14, 2020

AaronRobinsonMSFT reviewed Jul 14, 2020

View reviewed changes

janvorli reviewed Jul 14, 2020

View reviewed changes

AaronRobinsonMSFT reviewed Jul 15, 2020

View reviewed changes

jkotas reviewed Jul 15, 2020

View reviewed changes

VSadov added 14 commits July 16, 2020 07:14

redirect to current exe

ce95b7f

remove bundled binaries from the netcoreapp/pkg

3c5bba1

add some exports. (this all should be generated)

490d16f

delete exports file

a0210b6

no bundling of native libs on OSX

f8b9078

only link native libs on Linux

a9c52d8

Support linking of coreclr on Linux only

8f64699

PR feedback

ba35d2e

just strcmp

a018ace

move the shim to dllimport.cpp

ae2ab51

Use library names vs. file names

0f27413

call self-documenting ShouldRedirectToCurrentLibrary when deciding wh…

ef8ab47

…eher to redirect load to current module

move code after ENTRY per PAL convention, make g_running_in_exe check…

4b4f018

… an assert

a few nits

b7fa391

VSadov force-pushed the nativeBundle branch from 9223398 to b7fa391 Compare July 16, 2020 14:16

jkotas reviewed Jul 16, 2020

View reviewed changes

removed ShouldRedirectToCurrentLibrary. NULL library name in PAL_Load…

37237af

…Library will be always considered self-referring.

janvorli reviewed Jul 16, 2020

View reviewed changes

janvorli approved these changes Jul 16, 2020

View reviewed changes

VSadov merged commit ef6c035 into dotnet:master Jul 16, 2020

VSadov deleted the nativeBundle branch July 16, 2020 23:29

VSadov mentioned this pull request Jul 17, 2020

[preview8] Linking native libraries into superhost (#38684) #39490

Merged

agocke mentioned this pull request Jul 20, 2020

SingleFile: Statically link core native libraries to Linux SingleFileHost #38304

Closed

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Linking native libraries into superhost #38684

Linking native libraries into superhost #38684

Conversation

VSadov commented Jul 1, 2020 • edited Loading

jkotas commented Jul 2, 2020

VSadov commented Jul 2, 2020 • edited Loading

jkotas commented Jul 2, 2020

Choose a reason for hiding this comment

VSadov Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jul 14, 2020 • 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

VSadov Jul 14, 2020 • 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

VSadov Jul 16, 2020 • 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

VSadov commented Jul 16, 2020

VSadov commented Jul 16, 2020

VSadov commented Jul 16, 2020

VSadov commented Jul 16, 2020

VSadov commented Jul 1, 2020 •

edited

Loading

VSadov commented Jul 2, 2020 •

edited

Loading

VSadov Jul 14, 2020 •

edited

Loading

VSadov Jul 14, 2020 •

edited

Loading

jkotas Jul 14, 2020 •

edited

Loading

VSadov Jul 14, 2020 •

edited

Loading

VSadov Jul 16, 2020 •

edited

Loading