Allow creating static or shared native library #4718

tonerdo · 2017-10-13T12:16:58Z

This PR adds support for creating static libraries (.lib, .a) and dynamic libraries (.dll, .dylib, .so) with CoreRT.

Commands

Dynamic library (Windows): dotnet build /t:LinkNative /p:NativeLib=Shared /p:DefFile=<path-to-def-exports-file>
Dynamic library (Unix): dotnet build /t:LinkNative /p:NativeLib=Shared
Static library: dotnet build /t:LinkNative /p:NativeLib=Static

dlopen on Unix and LoadLibrary on Windows are able to call any .NET method (within the generated native shared library) with the NativeCallable attribute applied. Generated static library (.lib, .a) would need to also be linked with the other CoreRT static libraries as well as your C++ code for all links to be resolved.

At this time there are errors when you try build a shared library on Ubuntu. Shared and static libraries can be successfully built on both Windows and macOS, only static libraries on Ubuntu

cc @jkotas @MichalStrehovsky

#1285

MichalStrehovsky

I haven't looked at the build system and bootstrapper changes in detail yet, but I like this!

MichalStrehovsky · 2017-10-13T12:30:59Z

src/ILCompiler.Compiler/src/Compiler/LibraryRootProvider.cs


        public LibraryRootProvider(EcmaModule module)
        {
            _module = module;
        }

+        public LibraryRootProvider(EcmaModule module, IList<MethodDesc> libraryInitializers)


For this we should just create a separate ICompilationRootProvider instead (e.g. a NativeLibraryInitializerRootProvider that looks similar to the existing MainMethodRootProvider).

LibraryRootProvider would go over all the methods and types in the provided module and root them for compilation. What you need here is just a root for the Initialize method (the new root provider will do that) and a root for the NativeCallable methods (existing ExportedMethodsRootProvider does that). You'll get natural tree shaking that way (things that are unreachable don't get compiled).

MichalStrehovsky · 2017-10-13T12:34:32Z

src/ILCompiler.Compiler/src/IL/Stubs/StartupCode/NativeLibraryStartupMethod.cs

+            _libraryInitializers = libraryInitializers;
+        }
+
+        protected override int ClassCode => -304225482;


We put ClassCode and CompareToImpl in separate dot files so that once we stand up a JIT (that doesn't need to sort) we can compile the JIT codebase without support for this. Granted, it's unlikely we would use this particular class in the JIT codebase, but we've been doing it everywhere for consistency.

MichalStrehovsky · 2017-10-13T12:38:01Z

src/ILCompiler.Compiler/src/IL/Stubs/StartupCode/NativeLibraryStartupMethod.cs

+                }
+            }
+
+            // Simply return 0


We could also just make this return void.

I return 0 to signify a success exit code. That way this line: https://github.com/tonerdo/corert/blob/static-shared-library/src/Native/Bootstrap/main.cpp#L388 will return the necessary value for the main function to continue

If this can never return failure exit code, it is not very useful for it to return success exit code...

Also, it would be nice to change this method to take no arguments since they are unused and always null. It should make some of the ifdefs in the boostrapper simpler.

MichalStrehovsky · 2017-10-13T12:40:50Z

src/ILCompiler/src/Program.cs

+                else if (_nativeLib)
+                {
+                    EcmaModule module = null;
+                    // Get first (and only) module


Is this limitation necessary? We could go over all of them and create a ExportedMethodsRootProvider for each.

MichalStrehovsky · 2017-10-13T12:46:26Z

Also, could you sign the Contribution License Agreement at https://cla2.dotnetfoundation.org? I know you contributed in the past (with CLA signed), but something in the system thinks you didn't sign it yet.

tonerdo · 2017-10-13T13:19:21Z

@MichalStrehovsky I think it's because I changed my GitHub username. Signed it now

jkotas · 2017-10-13T13:39:39Z

src/Native/Runtime/thread.cpp

 // Standard calling convention variant of RhpReversePInvoke
 COOP_PINVOKE_HELPER(void, RhpReversePInvoke2, (ReversePInvokeFrame * pFrame))
 {
+    // Entrypoint for reverse PInvoke
+    // ensure runtime is initialized
+    InitializeRuntime();


This is a hot fast path. The runtime initialization should to be done in RhpReversePInvokeAttachOrTrapThread2 - similar to how the thread initialization is done.

jkotas · 2017-10-13T13:41:01Z

src/Native/Bootstrap/main.cpp

+
+extern "C" int InitializeRuntime()
+{
+    if (RUNTIME_INITIALIZED)


This should be thread safe. If one thread is initializing the runtime, the other thread should wait for it. I think the logic to do that should be in the Runtime, not in the bootstrapper.

jkotas · 2017-10-13T13:46:59Z

src/Native/Runtime/thread.cpp

@@ -1295,9 +1295,15 @@ EXTERN_C NOINLINE void FASTCALL RhpReversePInvokeAttachOrTrapThread2(ReversePInv
 // PInvoke
 //

+extern "C" int InitializeRuntime();


This creates a static dependency from Runtime to bootstrapper. These dependencies are problematic. (E.g. for .NET Native on Windows UWP, we build runtime as separate .dll that does not include bootstrapper.)

Can InitializeRuntime here be a function pointer that the bootstrapper sets - e.g. in a static constructor?

@jkotas sounds reasonable. Is there a part in the code where something similar is done? I might be able to get some ideas from there

I do not think there is anything like that in CoreRT today. There are number of examples out there on how to achieve it: https://www.bing.com/search?q=__attribute__%20((constructor))

jkotas · 2017-10-13T13:48:18Z

I'm unable to get a CoreRT to build on Ubuntu

What's your Ubuntu version - what are the errors you are getting?

tonerdo · 2017-10-13T21:12:32Z

@jkotas I'm using Ubuntu 14.04 in a docker container running on macOS. Very little helpful error message, just says that clang exited with a non-zero code

tonerdo · 2017-10-16T13:24:16Z

@jkotas my bad I've now been able to build CoreRT on Ubuntu. However, trying to build a shared library fails (static library works). Will address comments once I fix that

jkotas · 2017-10-24T10:00:15Z

src/Native/Runtime/thread.cpp

 // Standard calling convention variant and actual implementation for RhpReversePInvokeAttachOrTrapThread
 EXTERN_C NOINLINE void FASTCALL RhpReversePInvokeAttachOrTrapThread2(ReversePInvokeFrame * pFrame)
 {
+    // Entrypoint for reverse PInvoke
+    // ensure runtime is initialized
+    InitializeRuntime();


I would push it even lower into Thread::ReversePInvokeAttachOrTrapThread to the path that is initializing the thread.

jkotas · 2017-11-08T13:46:42Z

src/Native/gc/env/gcenv.base.h

@@ -39,6 +39,8 @@

 typedef uint32_t BOOL;
 typedef uint32_t DWORD;
+typedef HANDLE HINSTANCE;


This should not be added here.

jkotas · 2017-11-08T14:15:24Z

src/Native/Bootstrap/main.cpp

+
+#ifdef CORERT_DLL
+#if defined(_WIN32)
+extern "C" BOOL WINAPI DllMain(


Whoever is linking the static library may want to have custom DllMain too. This will make it impossible.

Have you tried using static constructor for the initialization?

static struct InitializeRuntimePointerHelper { InitializeRuntimePointerHelper() { ... } } initializeRuntimePointerHelper;

@jkotas won't this mean that there'll still be a static dependency on the bootstrapper (e.g. with .NET Native on Windows UWP)? Would this static construct be run when this library is loaded?

The dependencies from runtime to the bootstrapper are the problematic ones. If you add the static constructor to the boostrapper, I do not see any problems with dependencies.

)

* Include nativelib switch for compiler * suppress checking for entrypoint when -nativelib

dotnet#1285)

* Fixes windows build error

tonerdo · 2017-11-11T23:55:06Z

All comments addressed. I'm still looking into the build failure for shared libraries on Ubuntu, although at this point it could be an issue on its own.

cc @jkotas @MichalStrehovsky

jkotas · 2017-11-12T00:01:18Z

src/Native/Runtime/thread.cpp

@@ -1118,7 +1121,15 @@ FORCEINLINE bool Thread::InlineTryFastReversePInvoke(ReversePInvokeFrame * pFram
 void Thread::ReversePInvokeAttachOrTrapThread(ReversePInvokeFrame * pFrame)
 {
    if (!IsStateSet(TSF_Attached))
+    {
+        if (*InitializeRuntimePtr != NULL && RuntimeInitializingThread != this)


This is not thread safe. If two threads happen to be first to call a managed method at the same time, both of them will start initializing the runtime and bad things will happen. There needs to be logic to wait for the initialization to finish that will kick in for the second thread. A spin wait loop is probably the easiest way to do that.

There is problem even without the race actually. The RuntimeInitializingThread != this condition will be false for the second thread calling here and the initialization will be done second time.

jkotas · 2017-11-12T00:07:10Z

src/Native/Bootstrap/main.cpp

 }
+
+#ifdef CORERT_DLL
+static struct InitializeRuntimePointerHelper


Does this work with optimizations on? I would think there needs to be something to keep the static constructor alive, e.g. linker directive on Windows - pragma(comment(linker,"/include... mentioned in one of the articles I linked earlier.

jkotas · 2017-11-12T00:23:07Z

src/Native/Runtime/thread.cpp

@@ -33,6 +33,9 @@
 EXTERN_C REDHAWK_API void* REDHAWK_CALLCONV RhpHandleAlloc(void* pObject, int type);
 EXTERN_C REDHAWK_API void REDHAWK_CALLCONV RhHandleFree(void*);

+EXTERN_C int (*InitializeRuntimePtr)() = NULL;


It would look better - more consistent with the overall style of the CoreRT runtime - to keep the pointer static without EXTERN_C, and expose the setter as a method - like RhSetRuntimeInitializationCallback - that is called from the bootstrapper.

jkotas · 2017-11-12T00:24:15Z

src/Native/Runtime/thread.cpp

@@ -33,6 +33,9 @@
 EXTERN_C REDHAWK_API void* REDHAWK_CALLCONV RhpHandleAlloc(void* pObject, int type);
 EXTERN_C REDHAWK_API void REDHAWK_CALLCONV RhHandleFree(void*);

+EXTERN_C int (*InitializeRuntimePtr)() = NULL;
+static Thread* RuntimeInitializingThread;


Nit: g_ prefix maybe nice (I know that we are not 100% consistent).

jkotas · 2017-11-12T00:25:55Z

src/Native/Runtime/thread.cpp

+        if (*InitializeRuntimePtr != NULL && RuntimeInitializingThread != this)
+        {
+            RuntimeInitializingThread = this;
+            (*InitializeRuntimePtr)();


Set the RuntimeInitializingThread to NULL once you are done?

jkotas · 2017-11-12T00:28:50Z

src/Native/Bootstrap/main.cpp

@@ -293,15 +293,58 @@ static const pfn c_classlibFunctions[] = {
 #endif // !CPPCODEGEN

 extern "C" void InitializeModules(void* osModule, void ** modules, int count, void ** pClasslibFunctions, int nClasslibFunctions);
+extern "C" int InitializeRuntime();


You can move the implementation of InitializeRuntime up to avoid the forward declaration.

MichalStrehovsky · 2017-11-13T10:42:09Z

src/Native/Bootstrap/main.cpp

    int retval;
 #ifdef CPPCODEGEN
    try
 #endif
    {
+#ifndef CORERT_DLL


This is in #ifndef CORERT_DLL already

True, thanks!

jkotas · 2017-11-13T14:07:58Z

src/Native/Runtime/thread.cpp

@@ -33,6 +33,9 @@
 EXTERN_C REDHAWK_API void* REDHAWK_CALLCONV RhpHandleAlloc(void* pObject, int type);
 EXTERN_C REDHAWK_API void REDHAWK_CALLCONV RhHandleFree(void*);

+static int (*InitializeRuntimePtr)();


Nit: g_RuntimeInitializationCallback

tonerdo · 2017-11-13T14:50:01Z

Does this work with optimizations on? I would think there needs to be something to keep the static constructor alive, e.g. linker directive on Windows - pragma(comment(linker,"/include... mentioned in one of the articles I linked earlier.

A normal build on Windows works just as well as on Nix. How do I turn on optimizations?

jkotas · 2017-11-13T15:09:04Z

How do I turn on optimizations?

Use Release build of the runtime: "build.cmd Release"

Fabi · 2017-11-17T20:01:28Z

Is there any special Method called from the DllMain method in the native dll on Windows? Like idk a Main equivalent method in the managed code which is called when DllMain is called?

jkotas · 2017-11-17T20:22:03Z

Is there any special Method called from the DllMain method in the native dll on Windows?

There is not, and we do not plan to add one. It is not a good idea to execute managed code during DllMain - it leads to deadlocks with Windows loader lock.

tonerdo · 2017-11-20T14:36:39Z

Does this work with optimizations on? I would think there needs to be something to keep the static constructor alive, e.g. linker directive on Windows - pragma(comment(linker,"/include... mentioned in one of the articles I linked earlier.

I can confirm that this works with optimizations on (release build of CoreRT)

Fabi · 2017-11-20T17:42:36Z

Is generating exports working right now? I built a simple dll file with
[NativeCallable] public static int TestFunction(int first, int second) { return first+ second; }

However there is no export created for that single functions.

MichalStrehovsky · 2017-11-20T17:56:50Z

I built a simple dll file with

@Fabi You'll want to set the entrypoint name. Otherwise you get a function that is native callable (you can take a function pointer to it and give it to native code and it will work), but doesn't have a symbolic name.

Don't forget to also pass a def file, if you're on Windows.

jkotas · 2017-11-21T00:19:34Z

src/Native/Runtime/thread.cpp

 void Thread::ReversePInvokeAttachOrTrapThread(ReversePInvokeFrame * pFrame)
 {
    if (!IsStateSet(TSF_Attached))
+    {
+        if (*InitializeRuntimePtr != NULL && g_RuntimeInitializingThread != this)


To make this thread-safe, you can use something like this:

... if (g_RuntimeInitializationCallback != NULL && g_RuntimeInitializingThread != this) { EnsureRuntimeInitialized(); } ... void Thread::EnsureRuntimeInitialized() { while (PalInterlockedCompareExchangePointer(&g_RuntimeInitializingThread, this, NULL) != NULL) { PalSleep(1); } if (g_RuntimeInitializationCallback != NULL) { g_RuntimeInitializationCallback(); g_RuntimeInitializationCallback = NULL; } PalInterlockedExchangePointer(&g_RuntimeInitializingThread, NULL); }

jkotas · 2017-11-21T14:25:10Z

src/Native/Bootstrap/main.cpp

+#ifdef CORERT_DLL
+static struct InitializeRuntimePointerHelper
+{
+	InitializeRuntimePointerHelper()


The indentation is off here. Tabs vs. spaces?

Spaces 😄

jkotas · 2017-11-21T14:27:11Z

src/Native/Bootstrap/main.cpp

+extern "C" void __managed__Startup();
+#endif // !CORERT_DLL
+
+int InitializeRuntime()


static int InitializeRuntime()

jkotas · 2017-11-21T14:29:47Z

src/Native/Runtime/thread.cpp

+
+    if (g_RuntimeInitializationCallback != NULL)
+    {
+        g_RuntimeInitializationCallback();


The callback returns int, but the return value is not checked. Change the return value of the callback to void?

Or keep it as int, check the return and fail fast it is non-zero.

Going with the second option, makes sense to act when runtime initialization fails

jkotas · 2017-11-21T14:33:18Z

src/Native/Bootstrap/main.cpp

@@ -310,7 +315,11 @@ int main(int argc, char* argv[])
 #endif // CPPCODEGEN

 #ifndef CPPCODEGEN
+#ifndef CORERT_DLL
    void * osModule = PalGetModuleHandleFromPointer((void*)&__managed__Main);


#define CORERT_ENTRYPOINT __managed__Main / #define CORERT_ENTRYPOINT __managed__Startup in the ifdef above, so that we do not need to have ifdef here?

jkotas · 2017-11-21T14:37:59Z

Windows build is failing with error C2664: 'void *PalInterlockedCompareExchangePointer(void *volatile *,void *,void *)': cannot convert argument 1 from 'Thread **' to 'void *volatile *'. Need to cast the first argument to make it happy.

jkotas · 2017-11-21T14:39:21Z

This looks good to me, modulo the last set of comments. Thank you for pushing on this!

jkotas · 2017-11-21T15:17:28Z

@MichalStrehovsky Could you please take a final look at this as well?

MichalStrehovsky

Looks great, apart from the nits.

We'll also want to do something about the library initializers (we probably don't need to inject System.Private.DeveloperExperience.Console into a non-console app), but that can be fixed in subsequent pull requests.

MichalStrehovsky · 2017-11-21T15:23:40Z

src/ILCompiler.Compiler/src/IL/Stubs/StartupCode/NativeLibraryStartupMethod.Sorting.cs

+
+using Internal.TypeSystem;
+
+using AssemblyName = System.Reflection.AssemblyName;


Nit: unneeded using

MichalStrehovsky · 2017-11-21T15:23:49Z

src/ILCompiler.Compiler/src/IL/Stubs/StartupCode/NativeLibraryStartupMethod.cs

+
+using Internal.TypeSystem;
+
+using AssemblyName = System.Reflection.AssemblyName;


Nit: unneeded using

tonerdo · 2017-11-21T15:58:36Z

All done!!!

dnfclas added the cla-required label Oct 13, 2017

MichalStrehovsky reviewed Oct 13, 2017

View reviewed changes

dnfclas added cla-signed and removed cla-required labels Oct 13, 2017

jkotas reviewed Oct 13, 2017

View reviewed changes

tonerdo force-pushed the static-shared-library branch 2 times, most recently from aa4a40e to 20b1280 Compare October 23, 2017 11:54

jkotas reviewed Oct 24, 2017

View reviewed changes

jongalloway removed the cla-signed label Oct 31, 2017

dotnet deleted a comment from dnfclas Nov 1, 2017

MichalStrehovsky mentioned this pull request Nov 5, 2017

Support for dotnet publish #4870

Merged

jkotas reviewed Nov 8, 2017

View reviewed changes

tonerdo added 9 commits November 11, 2017 18:46

add bootstrapper for native libraries (dotnet#1285)

b7a5b0f

overwrite main method when building library (dotnet#1285)

e0752ec

update buildscripts to build shared library for non-windows (dotnet#1285

a915ad9

)

add stub for native library startup method (dotnet#1285)

155a9ca

* Include nativelib switch for compiler * suppress checking for entrypoint when -nativelib

initialize runtime at start of reverse PInvoke, unix dlopen successful (

65b2b25

dotnet#1285)

don't run library initializers for Multi module build (dotnet#1285)

80d721b

* Fixes windows build error

update Windows buildscripts, LoadLibrary successful (dotnet#1285)

79d3322

tidy up native binary extension conditions (dotnet#1285)

27ef3aa

update buildscript to build unix static library (dotnet#1285)

f5f71f4

jkotas reviewed Nov 12, 2017

View reviewed changes

update code to match overall style of the CoreRT runtime (dotnet#1285)

0c3b863

MichalStrehovsky reviewed Nov 13, 2017

View reviewed changes

jkotas reviewed Nov 13, 2017

View reviewed changes

jkotas reviewed Nov 21, 2017

View reviewed changes

tonerdo added 2 commits November 21, 2017 12:40

update code style (dotnet#1285)

474dd62

handle race conditions when initializing runtime (dotnet#1285)

426693e

jkotas reviewed Nov 21, 2017

View reviewed changes

tonerdo added 3 commits November 21, 2017 15:49

fix Windows build (dotnet#1285)

928f04d

some code improvements (dotnet#1285)

727bcd1

explicitly use spaces (dotnet#1285)

7a2d8a0

jkotas approved these changes Nov 21, 2017

View reviewed changes

MichalStrehovsky approved these changes Nov 21, 2017

View reviewed changes

remove unneeded usings (dotnet#1285)

2c3b68c

jkotas merged commit 8d0f5ed into dotnet:master Nov 21, 2017


		using Internal.TypeSystem;

		using AssemblyName = System.Reflection.AssemblyName;

Allow creating static or shared native library #4718

Allow creating static or shared native library #4718

Conversation

tonerdo commented Oct 13, 2017 • edited Loading

Commands

MichalStrehovsky 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

tonerdo Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky commented Oct 13, 2017

tonerdo commented Oct 13, 2017

Choose a reason for hiding this comment

jkotas Oct 13, 2017 • 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

jkotas commented Oct 13, 2017

tonerdo commented Oct 13, 2017

tonerdo commented Oct 16, 2017 • 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

tonerdo commented Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

jkotas Nov 12, 2017 • 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

tonerdo commented Nov 13, 2017

jkotas commented Nov 13, 2017

Fabi commented Nov 17, 2017

jkotas commented Nov 17, 2017

tonerdo commented Nov 20, 2017

Fabi commented Nov 20, 2017

MichalStrehovsky commented Nov 20, 2017

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

jkotas commented Nov 21, 2017

jkotas commented Nov 21, 2017

jkotas commented Nov 21, 2017

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonerdo commented Nov 21, 2017

tonerdo commented Oct 13, 2017 •

edited

Loading

tonerdo Oct 23, 2017 •

edited

Loading

jkotas Oct 13, 2017 •

edited

Loading

tonerdo commented Oct 16, 2017 •

edited

Loading

tonerdo commented Nov 11, 2017 •

edited

Loading

jkotas Nov 12, 2017 •

edited

Loading

jkotas Nov 12, 2017 •

edited

Loading