Automatic generation of method export file for shared libraries #5154

tonerdo · 2017-12-26T06:04:52Z

This PR adds automatic generation of .def, .exports and .map passed with linker options /DEF, exported_symbols_list and version-script for building shared libraries on Windows, macOS and Linux respectively.

Building shared libraries on all platforms is now a simple:

dotnet build /t:LinkNative /p:NativeLib=Shared

A good side effect of this change is that on non-Windows platforms it shaves off about 2MB from the produced binary because internally used NativeCallable methods are no longer exported by default

Fixes #4986

jkotas · 2017-12-26T08:20:42Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ExportedMethodsWriter.cs

+
+        private string GetExportsFileExtenstion()
+        {
+            if (_targetDetails.OperatingSystem == TargetOS.Windows)


The file name should be an argument on the command line.

Won't this be an extra cognitive load for the developer? I was thinking that just having everything done automatically might be better, experience wise

I meant it should be argument on ilc.exe command and passed in by the msbuild script. The msbuild script is computing the name already and passing it to the linker, so it can pass it to ilc.exe as well.

Most developers won't invoke ilc.exe directly, so they won't really notice this argument.

jkotas · 2017-12-26T08:25:26Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/ExportedMethodsWriter.cs

+            if (!factory.NodeAliases.Values.Contains("__managed__Startup"))
+                return;
+
+            var nativeCallables = factory.NodeAliases.Where(n => n.Key is IMethodNode)


We should get this list in ExportedMethodsRootProvider as it is rooting the native exports instead of trying to reverse engineer it here.

Will an internal static field on ExportedMethodsRootProvider suffice?

It should not be static field. It should be instance field.

Maybe the function to produce the exports file can be moved to ExportedMethodsRootProvider - it does not need to be in separate type.

tonerdo · 2017-12-26T21:56:54Z

I now also pass exported_symbols_list on Linux, clang-3.9 doesn't support --version-script, and we do not use ld directly. That goes without saying that until #4988 is fixed this can't be tested on Linux.

jkotas · 2017-12-26T21:43:44Z

src/BuildIntegration/Microsoft.NETCore.Native.Unix.props

@@ -70,7 +70,8 @@ See the LICENSE file in the project root for more information.
      <LinkerArg Include="-luuid" Condition="'$(TargetOS)' != 'OSX'" />
      <LinkerArg Include="-lrt" Condition="'$(TargetOS)' != 'OSX'" />
      <LinkerArg Include="-licucore" Condition="'$(TargetOS)' == 'OSX'" />
-      <LinkerArg Include="-shared" Condition="'$(NativeLib)' == 'Shared'" />
+      <LinkerArg Include="-dynamiclib" Condition="'$(TargetOS)' == 'OSX' and '$(NativeLib)' == 'Shared'" />


I though that -shared works on OSX too. Just for my education - why did you change this to -dynamiclib?

Oh yeah I know it works. I guess I was just trying to be as platform accurate as possible, most Apple docs use -dynamiclib

Ok, that's fine.

jkotas · 2017-12-26T21:45:38Z

src/BuildIntegration/Microsoft.NETCore.Native.targets

@@ -151,7 +156,8 @@ See the LICENSE file in the project root for more information.
      <IlcArg Condition="$(DebugSymbols) == 'true'" Include="-g" />
      <IlcArg Condition="$(IlcGenerateMapFile) == 'true'" Include="--map:$(NativeIntermediateOutputPath)%(ManagedBinary.Filename).map.xml" />
      <IlcArg Condition="$(RdXmlFile) != ''" Include="--rdxml:$(RdXmlFile)" />
-      <IlcArg Condition="$(OutputType) ==  'Library' and $(NativeLib) != ''" Include="--nativelib" />
+      <IlcArg Condition="$(OutputType) == 'Library' and $(NativeLib) != ''" Include="--nativelib" />
+      <IlcArg Condition="$(OutputType) == 'Library' and $(NativeLib) == 'Shared'" Include="--exportsfile:$(ExportsFile)" />


I would change the condition to $(ExportsFile) != '' to make it simpler and more flexible

jkotas · 2017-12-26T21:45:46Z

src/BuildIntegration/Microsoft.NETCore.Native.targets

@@ -193,6 +199,8 @@ See the LICENSE file in the project root for more information.
      <CustomLinkerArg Include="$(NativeObject)" />
      <CustomLinkerArg Include="-o $(NativeBinary)" Condition="'$(OS)' != 'Windows_NT'" />
      <CustomLinkerArg Include="/OUT:$(NativeBinary)" Condition="'$(OS)' == 'Windows_NT'" />
+      <CustomLinkerArg Include="/DEF:$(ExportsFile)" Condition="'$(OS)' == 'Windows_NT' and '$(NativeLib)' == 'Shared'" />
+      <CustomLinkerArg Include="-exported_symbols_list $(ExportsFile)" Condition="'$(TargetOS)' != 'Windows_NT' and '$(NativeLib)' == 'Shared'" />


jkotas · 2017-12-26T21:46:14Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

+
+            // Only emit exported methods for user supplied assemblies
+            // _exportsFile is null when we're not doing a shared library build
+            if (_module != _module.Context.SystemModule && _exportsFile != null)


_module != _module.Context.SystemModule part of the condition should not be required (once the other part of the feedback is addressed).

jkotas · 2017-12-26T21:47:54Z

src/ILCompiler/src/Program.cs

@@ -356,7 +358,7 @@ private int Run(string[] args)
                    // TODO: Wasm fails to compile some of the xported methods due to missing opcodes
                    if (!_isWasmCodegen)
                    {
-                        compilationRoots.Add(new ExportedMethodsRootProvider((EcmaModule)typeSystemContext.SystemModule));
+                        compilationRoots.Add(new ExportedMethodsRootProvider((EcmaModule)typeSystemContext.SystemModule, _exportsFile));


Do not pass in _exportsFile argument here - pass in null, or make null to be default value of the argument.

jkotas · 2017-12-26T21:51:15Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

+            // Only emit exported methods for user supplied assemblies
+            // _exportsFile is null when we're not doing a shared library build
+            if (_module != _module.Context.SystemModule && _exportsFile != null)
+                EmitExportedMethods();


@MichalStrehovsky What do you think about this call here? Do you think it would be better to make it an explicit call from the compiler driver?

jkotas · 2017-12-26T21:59:28Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

+        private void EmitExportedMethods()
+        {
+            string moduleName = Path.GetFileNameWithoutExtension(_exportsFile);
+            StringBuilder stringBuilder = new StringBuilder();


Nit: It is better to write to the stream directly as you go instead of creating the whole string in memory and the writing it at once. (It does not matter a lot here since the file will be relatively small.)

using (var writer = new StreamWriter(fileName)) { writer.Write(...); writer.WriteLine(...); }

jkotas · 2017-12-26T22:00:48Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

+            if (_module.Context.Target.IsWindows)
+            {
+                stringBuilder.Append("LIBRARY   ");
+                stringBuilder.AppendLine(moduleName.ToUpper());


This should be ToUpperInvariant so that it does not depend on the current OS culture.

jkotas · 2017-12-26T22:01:59Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

+
+                stringBuilder.AppendLine("EXPORTS");
+                foreach (var method in _methods)
+                    stringBuilder.AppendLine("   " + method.GetNativeCallableExportName());


Nit: You can consider using interpolated strings for all these - it looks better:

AppendLine($" {method.GetNativeCallableExportName()}");

jkotas · 2017-12-26T22:04:08Z

src/BuildIntegration/Microsoft.NETCore.Native.targets

    <NativeObject>$(NativeIntermediateOutputPath)$(TargetName)$(NativeObjectExt)</NativeObject>
    <NativeBinary>$(NativeOutputPath)$(TargetName)$(NativeBinaryExt)</NativeBinary>
+    <ExportsFile>$(NativeIntermediateOutputPath)$(TargetName)$(ExportsFiletExt)</ExportsFile>


Only set this if it is empty and we are producing SharedLib. (It allows users to supply custom exports file.)

jkotas · 2017-12-27T04:02:47Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

+            {
+                if (_module.Context.Target.IsWindows)
+                {
+                    streamWriter.Write("LIBRARY   ");


It the LIBRARY line actually needed? It seem unnecessary.

I created it as specified in the DEF file documentation. I'll do a test and get back to you

Number of the .def files in CoreCLR do not have the LIBRARY line. e.g. https://github.com/dotnet/coreclr/blob/master/src/dlls/dbgshim/dbgshim.ntdef

Ah, that's fine then

jkotas · 2017-12-27T04:05:45Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

+        private void EmitExportedMethods()
+        {
+            string moduleName = Path.GetFileNameWithoutExtension(_exportsFile);
+            FileStream fileStream = new FileStream(_exportsFile, FileMode.OpenOrCreate);


This should be FileMode.Create. We want to overwrite the file if it exists already.

MichalStrehovsky · 2017-12-27T14:18:56Z

src/ILCompiler/src/Program.cs

@@ -305,7 +307,7 @@ private int Run(string[] args)
                    // TODO: Wasm fails to compile some of the exported methods due to missing opcodes
                    if (!_isWasmCodegen)
                    {
-                        compilationRoots.Add(new ExportedMethodsRootProvider(module));
+                        compilationRoots.Add(new ExportedMethodsRootProvider(module, _exportsFile));


If there are multiple assemblies specified in InputFilePaths that have NativeCallable methods in them, they're going to stomp over each other's writes into the exports file.

I would prefer a scheme where ExportedMethodsRootProvider has a public property that returns an IEnumerable<string> of all the exported symbols.

Once we're done adding to the compilationRoots list here, we pass the list of compilation roots to a helper method that does a foreach over the list of root provider, finds the list of exported methods from each ExportedMethodsRootProvider, and writes it to a file.

Ah thanks for this, totally overlooked the multiple input file paths scenario. On what type do you suggest the helper method should be?

On what type do you suggest the helper method should be?

I would probably just add a DefFileWriter.cs or something like that to the ILCompiler project (it probably doesn't even need to be in the ILCompiler.Compiler project). I also wouldn't mind much if it was in Program.cs, but that file is starting to be a dumping ground for lots of short methods and maybe we should stop doing that.

…ipts (dotnet#4986)

…otnet#4986)

…ild (dotnet#4986)

MichalStrehovsky · 2017-12-28T16:36:39Z

src/ILCompiler/src/Program.cs

@@ -302,7 +304,11 @@ private int Run(string[] args)
                        entrypointModule = module;
                    }

-                    compilationRoots.Add(new ExportedMethodsRootProvider(module));
+                    // TODO: Wasm fails to compile some of the exported methods due to missing opcodes
+                    if (!_isWasmCodegen)


This coming back looks like a bad merge.

Thanks for this. Been trying to fix the Windows build error

MichalStrehovsky · 2017-12-28T16:38:32Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

+        {
+            get
+            {
+                return _methods;


Could you factor this class in a way that we can't get different results depending on when ExportedMethods is accessed? It's not a problem right now, but I can see someone getting burned by this at some point.

MichalStrehovsky · 2017-12-28T16:40:37Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

                        if (nativeCallableExportName != null)
+                        {
+                            if (ecmaMethod.Module != _module.Context.SystemModule)


Skipping exports from the system module is a policy decision. This class should ideally be policy-free (some other component might want this list too and a filtered list might not be what they intended). Could we filter in DefFileWriter.cs?

jkotas · 2017-12-28T16:51:24Z

src/ILCompiler.Compiler/src/Compiler/DefFileWriter.cs

+
+namespace ILCompiler
+{
+    public class DefFileWriter


It is called ExportsFile everywhere else in this change. This should be called ExportsFileWriter to match.

jkotas · 2017-12-28T17:02:23Z

src/ILCompiler/src/Program.cs

+                DefFileWriter defFileWriter = new DefFileWriter(typeSystemContext, _exportsFile);
+                foreach (var compilationRoot in compilationRoots)
+                {
+                    if (compilationRoot is ExportedMethodsRootProvider)


Nit: You can use new C# features to avoid the cast.

if (compilationRoot is ExportedMethodsRootProvider provider) defFileWriter.AddExportedMethod(provider);

jkotas · 2017-12-28T17:02:53Z

src/ILCompiler/src/Program.cs

@@ -446,6 +456,17 @@ private int Run(string[] args)
            ObjectDumper dumper = _mapFileName != null ? new ObjectDumper(_mapFileName) : null;

            CompilationResults compilationResults = compilation.Compile(_outputFilePath, dumper);
+            if (_nativeLib)


This condition should be _exportsFile != null.

tonerdo · 2017-12-31T05:25:33Z

@jkotas @MichalStrehovsky I can't for the life of me figure out why my latest change causes the WASM tests to fail. It's just a refactoring that doesn't change the end result of the AddCompilationRoots method.

jkotas · 2017-12-31T06:43:06Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

+                if (_methods != null)
+                    return _methods;
+
+                _methods = new List<EcmaMethod>();


Nit: It is a better pattern to create the list in a local, and only assign it to the field once it is done. (Slightly better performance and reliability.)

jkotas · 2017-12-31T06:44:37Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

                }
            }
+
+            foreach (var ecmaMethod in ExportedMethods)


I do not think we need to be differentiating between NativeCallable and RuntimeExports here. They can be treated the same.

jkotas · 2017-12-31T06:47:43Z

I can't for the life of me figure out why my latest change causes the WASM tests to fail.

I do not see anything obvious either. I have kicked a fresh CI run on #5171 to see whether it is a problem that is affecting all PRs.

tonerdo · 2017-12-31T07:31:05Z

whether it is a problem that is affecting all PRs.

The issue only happens with my inclusion of the ExportedMethods property, it passes fine when I revert it to how it was. It's just really weird

jkotas · 2017-12-31T16:08:06Z

src/ILCompiler.Compiler/src/Compiler/ExportedMethodsRootProvider.cs

-                foreach (var method in type.GetMethods())
-                {
-                    EcmaMethod ecmaMethod = (EcmaMethod)method;
+                var methods = new List<EcmaMethod>();


Building the List of methods made sense when it was cached. Without cache, it would be better to yield return the methods.

Either keep the cache, or replace List with yield return.

tonerdo · 2017-12-31T16:54:39Z

I think the WASM tests failed because there were two different loops for RuntimeExport and NativeCallable, don't know why but it works fine when a single loop is used instead.

tonerdo · 2018-01-02T18:50:00Z

Anything else required of me on this?

jkotas · 2018-01-02T18:53:55Z

@MichalStrehovsky Does this look good to you?

MichalStrehovsky

Looks good minus the nit.

MichalStrehovsky · 2018-01-02T19:50:52Z

src/BuildIntegration/Microsoft.NETCore.Native.targets

@@ -48,8 +49,12 @@ See the LICENSE file in the project root for more information.
    <NativeBinaryExt Condition="'$(OutputType)' !=  'Exe' and '$(TargetOS)' != 'Windows_NT' and $(NativeLib) == 'Static'">.a</NativeBinaryExt>
    <NativeBinaryExt Condition="'$(NativeCodeGen)' == 'wasm'">.html</NativeBinaryExt>

+    <ExportsFiletExt Condition="'$(OutputType)' !=  'Exe' and '$(TargetOS)' == 'Windows_NT' and '$(NativeLib)' == 'Shared'">.def</ExportsFiletExt>


Nit: typo: Filet -> File

jkotas · 2018-01-02T21:28:17Z

@tonerdo Thank you!

jkotas reviewed Dec 26, 2017

View reviewed changes

jkotas reviewed Dec 27, 2017

View reviewed changes

MichalStrehovsky reviewed Dec 27, 2017

View reviewed changes

tonerdo added 17 commits December 27, 2017 14:16

add ExportedDefinitionsWriter (dotnet#4986)

9f615d8

add automatic location of auto-generated definition file to build scr…

0d17711

…ipts (dotnet#4986)

add support for generating OSX symbol export files (dotnet#4986)

655be26

rename ExportedDefinitionsWriter to ExportsWriter (dotnet#4986)

ea9e84c

update buildscripts for OSX exports file location (dotnet#4986)

cb11f1f

rename ExportsWriter to ExportedMethodsWriter (dotnet#4986)

7443e76

restructure ExportedMethodsWriter class (dotnet#4986)

f8106e1

remove exporting of internal nativecallable methods (dotnet#4986)

0fad626

only generate exports file during native library build (dotnet#4986)

996bea2

add generation and location of version-script on linux (dotnet#4986)

b5050e6

make linux version script generation the default case (dotnet#4986)

99435e1

move exported methods emission to ExportedMethodsRootProvider class (d…

2dbde95

…otnet#4986)

set ExportsFile variable if not user defined and it's a shared lib bu…

50b0686

…ild (dotnet#4986)

make optimizations to exports file emission code (dotnet#4986)

7fc2d0d

remove redundant info from exported definition files (dotnet#4986)

3bad6d1

ensure definition file is overwritten (dotnet#4986)

0c00f1a

add DefFileWriter class (dotnet#4986)

4f80359

tonerdo force-pushed the auto-generate-def branch from 64606e2 to 4f80359 Compare December 27, 2017 21:24

MichalStrehovsky reviewed Dec 28, 2017

View reviewed changes

jkotas reviewed Dec 28, 2017

View reviewed changes

fix wasm build error caused by merge (dotnet#4986)

b46e2c0

tonerdo added 2 commits December 28, 2017 11:19

rename DefFileWriter to ExportsFileWriter (dotnet#4986)

4b7424b

restructure ExportedMethodsRootProvider class (dotnet#4986)

7f7e1d7

jkotas reviewed Dec 31, 2017

View reviewed changes

refactor ExportedMethodsRootProvider class (dotnet#4986)

9d581df

jkotas reviewed Dec 31, 2017

View reviewed changes

improve performance of ExportedMethods property (dotnet#4986)

a062443

jkotas approved these changes Dec 31, 2017

View reviewed changes

MichalStrehovsky approved these changes Jan 2, 2018

View reviewed changes

fix typo (dotnet#4986)

f085583

jkotas merged commit d5a0166 into dotnet:master Jan 2, 2018

tonerdo mentioned this pull request Apr 27, 2018

How to compile a class library project #5751

Closed

rickge mentioned this pull request Aug 7, 2018

NativeCallable Attribute could not be found #6187

Closed

hc4 mentioned this pull request Apr 10, 2019

export function in generated lib #7045

Closed

Automatic generation of method export file for shared libraries #5154

Automatic generation of method export file for shared libraries #5154

Conversation

tonerdo commented Dec 26, 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

tonerdo commented Dec 26, 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

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 Dec 27, 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

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 Dec 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Dec 31, 2017

tonerdo commented Dec 31, 2017

Choose a reason for hiding this comment

tonerdo commented Dec 31, 2017

tonerdo commented Jan 2, 2018

jkotas commented Jan 2, 2018

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jan 2, 2018

tonerdo Dec 27, 2017 •

edited

Loading