Coff linker: Add IMPLIB support #9347

kkartaltepe · 2021-07-10T03:49:06Z

Allow --out-implib as passed by cmake and meson to be correctly passed
through to the linker to generate import libraries.

This allows most of glib to be compiled for --target=x86_64-windows-gnu by just setting the appropriate compiler. Though there are still some compiler/linker issues resulting in missing symbols compared to compile with the mingw toolchain I think they are unrelated.

One note is that cmake/meson pass --subsystem as a linker flag when they detect clang as the compiler but zig appears to support this as a compiler level flag. I'd be happy to add that in another PR as well if it sounds useful.

I am not sure if this the correct way to support the cache, any input is appreciated.

kkartaltepe · 2021-07-10T03:51:39Z

Ah it seems this crossed paths with #9251 which looks like it adds support for the ms link style argument while my change is adding the ld style, maybe they can be combined?

src/main.zig

src/link/Coff.zig

alexrp · 2021-07-10T04:17:19Z

#9251 can be closed in favor of this if my comments above are addressed.

kkartaltepe · 2021-07-10T04:35:54Z

So there is still an issue with the cache only tracking the output dll and not the implib with this change, when rebuilding a project linking will fail as the implib files are not restored when zig attempts to use the cache.

Thanks for the comments, i've added the changes suggested.

alexrp · 2021-07-10T04:42:21Z

Can you elaborate on the caching issue? I don't quite follow.

kkartaltepe · 2021-07-10T04:43:55Z

Oh sorry it may be due to me swapping between unmodified zig and this patch that causes the caching issue actually.

kkartaltepe · 2021-07-10T05:16:39Z

Ah actually no i have replicated, in my case im building glib. And cmake doesnt track the implib correctly so its not cleaned up on ninja clean. But the steps to replicate for me.

delete ~/.cache/zig
build project
delete build dir (or clean and then delete some specific implib)
rebuild project
observe linking to fail with missing implib.

alexrp · 2021-07-10T05:36:02Z

Ok, I see. That actually makes sense.

I think we need to handle this similarly to how .pdb outputs are handled. Basically, save the requested path in out_implib, but output the file to the cache directory. Then, in an update hook, copy the .lib to out_implib.

See:

zig/src/main.zig

Lines 2396 to 2419 in 7b5d139

    
           // If a .pdb file is part of the expected output, we must also copy 
        
           // it into place here. 
        
           const coff_or_pe = switch (comp.bin_file.options.object_format) { 
        
               .coff, .pe => true, 
        
               else => false, 
        
           }; 
        
           const have_pdb = coff_or_pe and !comp.bin_file.options.strip; 
        
           if (have_pdb) { 
        
               // Replace `.out` or `.exe` with `.pdb` on both the source and destination 
        
               const src_bin_ext = fs.path.extension(bin_sub_path); 
        
               const dst_bin_ext = fs.path.extension(full_path); 
        
               const src_pdb_path = try std.fmt.allocPrint(gpa, "{s}.pdb", .{ 
        
                   bin_sub_path[0 .. bin_sub_path.len - src_bin_ext.len], 
        
               }); 
        
               defer gpa.free(src_pdb_path); 
        
               const dst_pdb_path = try std.fmt.allocPrint(gpa, "{s}.pdb", .{ 
        
                   full_path[0 .. full_path.len - dst_bin_ext.len], 
        
               }); 
        
               defer gpa.free(dst_pdb_path); 
        
               _ = try cache_dir.updateFile(src_pdb_path, cwd, dst_pdb_path, .{}); 
        
           }

kkartaltepe · 2021-07-10T06:22:10Z

Ill have to take more look later, but i cannot see where PDBs (my zig is pretty poor) are ever output/copied to cache dir. A trivial implementation fails, presumably because the file is correctly output the build dir and nothing is added to the cache.

--- a/src/main.zig
+++ b/src/main.zig
@@ -2427,6 +2427,10 @@ fn updateModule(gpa: *Allocator, comp: *Compilation, hook: AfterUpdateHook) !voi
 
                 _ = try cache_dir.updateFile(src_pdb_path, cwd, dst_pdb_path, .{});
             }
+
+            if (comp.bin_file.options.out_implib) |out_implib| {
+                _ = try cache_dir.updateFile(out_implib, cwd, out_implib, .{});
+            }
         },
     }
 }

alexrp · 2021-07-10T06:49:22Z

but i cannot see where PDBs (my zig is pretty poor) are ever output/copied to cache dir.

I don't think Zig does this explicitly. Clang internally outputs the .pdb next to the output binary, which happens to be in the cache directory:

$ zig cc main.c -target x86_64-windows-gnu -g
$ tree zig-cache/o
zig-cache/o
├── 6523ca4147cba05665bc073987ed7468
│   └── main.obj
└── 8098b8dc53415f7765e265aa3b4ffb3e
    ├── a.exe
    ├── a.pdb
    └── lld.id -> 24674bd217dde2e17d4a2c84a7261bd8

A trivial implementation fails

Here's what I think needs to be done exactly:

Instead of passing out_implib through the compilation and linking code, just pass a Boolean like emit_implib.
In the COFF linker code, if emit_implib is true, pass -IMPLIB:<path to output binary, but extension changed to .lib>.
In the update hook, if out_implib is non-null, copy the .lib from the cache directory to the output directory in exactly the same way as done for the .pdb.

kkartaltepe · 2021-07-12T06:06:47Z

Here's what I think needs to be done exactly:

* Instead of passing `out_implib` through the compilation and linking code, just pass a Boolean like `emit_implib`.

* In the COFF linker code, if `emit_implib` is `true`, pass `-IMPLIB:<path to output binary, but extension changed to .lib>`.

* In the update hook, if `out_implib` is non-`null`, copy the `.lib` from the cache directory to the output directory in exactly the same way as done for the `.pdb`.

Thanks, I think the issue I have is that to support being called via zig cc and being consistent with other compilers when being called with -Wl,--out-implib,some/path/some.ext zig should ensure that some/path/some.ext is really created, so bool isnt an option to be compatible. Ah I misread.

For now I have have zig output into the cache with a fixed name <bin>.lib and then copy back into the specified location. This working reasonably I think.

I'm not sure how correct this is on two parts. Zig's source suggests you may be able to compile without the cache in which case creating the additional .lib file in the cwd might be wrong? Secondly if you were targeting a different directory than the binary that didnt exist I assume this copy would fail, im not sure if there is a mkdir -p equivalent in zig or if it should be used when we try to do every copy.

alexrp · 2021-07-12T06:20:28Z

Zig's source suggests you may be able to compile without the cache in which case creating the additional .lib file in the cwd might be wrong?

What code are you referring to? You might be right, but I haven't come across a way to actually do that. It seems to me that a lot of functionality in Zig's driver wouldn't work well without the cache (e.g. compiling musl on demand).

Secondly if you were targeting a different directory than the binary that didnt exist I assume this copy would fail, im not sure if there is a mkdir -p equivalent in zig or if it should be used when we try to do every copy.

This is a good point, we should definitely do that. I'm not sure whether such a function exists in the stdlib though.

kkartaltepe · 2021-07-12T06:34:28Z

What code are you referring to? You might be right, but I haven't come across a way to actually do that. It seems to me that a lot of functionality in Zig's driver wouldn't work well without the cache (e.g. compiling musl on demand).

This line https://github.com/ziglang/zig/blob/master/src/Compilation.zig#L589 which is the source for the cache dir in the linker.

alexrp · 2021-07-18T22:51:23Z

im not sure if there is a mkdir -p equivalent in zig or if it should be used when we try to do every copy.

I asked on the Zig Discord and they suggested that std.fs.makePath(std.fs.cwd(), std.fs.path.dirname(implib_path)) should work here.

alexrp · 2021-07-18T23:15:17Z

What code are you referring to? You might be right, but I haven't come across a way to actually do that. It seems to me that a lot of functionality in Zig's driver wouldn't work well without the cache (e.g. compiling musl on demand).

This line https://github.com/ziglang/zig/blob/master/src/Compilation.zig#L589 which is the source for the cache dir in the linker.

Someone might be able to offer more insight here, but as far as I can tell, we don't have to worry about the non-caching case:

zig/src/main.zig

Lines 1500 to 1504 in 41d1239

    
           .link => { 
        
               output_mode = if (is_shared_lib) .Lib else .Exe; 
        
               emit_bin = if (out_path) |p| .{ .yes = p } else EmitBin.yes_a_out; 
        
               enable_cache = true; 
        
           },

Note the enable_cache assignment.
There's no way to explicitly disable the cache from the command line.
We only care about the C case; -Wl,-implib (etc) can't be passed when building Zig code.

src/link/Coff.zig

andrewrk · 2021-07-23T02:54:09Z

Next step here is to rebase against master and get the CI tests passing.

kkartaltepe · 2021-07-23T05:11:20Z

I have pushed the PR with the linting error about the closure fixed and with the steps to mkdir when there is a directory as part of the implib path.

kubkon

Thanks for the PR! Here's my suggestion for handling this and hopefully making zig cc -shared behave the same way by default as zig build-lib -dynamic.

From my brief investigation, I believe that by default link-lld generates a matching importlib side-by-side with the output PE binary if we target msvc ABI. However, for mingw (or gnu ABI), we are required to explicitly request generating a matching importlib by providing an -IMPLIB flag as you've done in this PR. Now, since zig build-lib -dynamic always generates a matching importlib regardless of the targeted ABI, my suggestion would be to replicate the same behaviour with zig cc -shared and append -IMPLIB always with the default output path if the user didn't override the path, or the path if they did. Does that make sense?

src/Compilation.zig

src/main.zig

kubkon · 2021-07-27T17:37:49Z

src/main.zig

@@ -2418,6 +2428,15 @@ fn updateModule(gpa: *Allocator, comp: *Compilation, hook: AfterUpdateHook) !voi

                _ = try cache_dir.updateFile(src_pdb_path, cwd, dst_pdb_path, .{});
            }
+
+            if (comp.bin_file.options.out_implib) |out_implib| {


I believe this doesn't cover the case when targeting msvc ABI for which link-lld by default does generate an importlib and therefore setting out_implib is not required. In this case, might I suggest incorporating an else-prong or equivalent similar to the following diff:

diff --git a/src/main.zig b/src/main.zig index cbe57eb84..cfa1374e5 100644 --- a/src/main.zig +++ b/src/main.zig @@ -2448,6 +2448,35 @@ fn updateModule(gpa: *Allocator, comp: *Compilation, hook: AfterUpdateHook) !voi _ = try cache_dir.updateFile(src_pdb_path, cwd, dst_pdb_path, .{}); } + + // If a .lib file is part of the expected output, we must also copy + // it into place here. + // Note that when targeting GNU (mingw), importlib will only be generated + // by the linker if explicitly asked for it, unlike on msvc which is always + // generated side-by-side with the dll. Therefore, we try to update .lib + // from source to destination output paths, but don't trip up when the file + // is not found. + const is_dyn_lib = comp.bin_file.options.output_mode == .Lib and comp.bin_file.options.link_mode == .Dynamic; + const have_importlib = is_coff and is_dyn_lib; + if (have_importlib) { + const src_bin_ext = fs.path.extension(bin_sub_path); + const dst_bin_ext = fs.path.extension(full_path); + + const src_lib_path = try std.fmt.allocPrint(gpa, "{s}.lib", .{ + bin_sub_path[0 .. bin_sub_path.len - src_bin_ext.len], + }); + defer gpa.free(src_lib_path); + + const dst_lib_path = try std.fmt.allocPrint(gpa, "{s}.lib", .{ + full_path[0 .. full_path.len - dst_bin_ext.len], + }); + defer gpa.free(dst_lib_path); + + _ = cache_dir.updateFile(src_lib_path, cwd, dst_lib_path, .{}) catch |err| switch (err) { + error.FileNotFound => {}, // just fall-through + else => |e| return e, + }; + } }, } }

Feel free to reuse the diff and no attribution is needed.

Can we tell if we are targeting msvc linkers at the time that we are parsing flags? If so it seems it would be cleaner to just always set the out_implib option on those targets and make that functionality explicit rather than rely on the implicit behavior of those linkers?

That's exactly what I'd like to see. So to sum up, here are the behavior characteristics:

the user may explicitly specify path/name of the importlib via --out-implib flag which will override any default set in out_implib optional

if --out-implib was not set explicitly, regardless of the targeted ABI, in Compilation.zig (most likely), we set the default value of out_implib to the same path as the final binary; then, in Compilation.updateModule we copy the *.lib from cache into the final destination dir.

Does that make sense?

You can check for ABI with

some_target: std.Target = ...; some_target.abi == .msvc;

In particular, in Compilation.updateModule(comp) you could do comp.bin_file.options.target.abi == .msvc or similar.

I have implemented what I tried to describe, let me know if this works for you or you would prefer to do this after the linker has been invoked like you described.

--edit
Or push it into Compilation.Zig if you think that makes sense.

kubkon · 2021-07-28T06:01:29Z

src/main.zig

@@ -2005,6 +2014,16 @@ fn buildOutputType(
        else => false,
    };

+    // Always output import libraries (.lib) when building for msvc to replicate
+    // `link` behavior. lld does not always output import libraries so on the


Actually, we always output import lib regardless of the ABI, so for mingw too. Hence why I suggested always setting the default for out_implib. This way we would unify interfaces between build-lib and cc, which I believe we should aim to do.

kubkon · 2021-07-28T06:06:49Z

src/main.zig

+    // Always output import libraries (.lib) when building for msvc to replicate
+    // `link` behavior. lld does not always output import libraries so on the
+    // gnu abi users must set out_implib.
+    if (output_mode == .Lib and emit_bin == .yes and target_info.target.abi == .msvc and out_implib == null) {


If emit_bin is set when building static archives, this check will let that through, so you should also check for dynamic linking comp.bin_file.options.link_mode == .Dynamic.

My understand is the import library is only needed when building dynamic libraries. When building static libraries the import parts are built into the archive in all platforms, and this is why they share the extension in windows.

Since i dont have windows handy to test this is simply my assumption and some googling suggests its probably not wrong.

Your understanding is correct and the way we signal a dynamic library in the linker is with a combination comp.bin_file.options.output_mode.Lib and comp.bin_file.options.link_mode.Dynamic, and I'm pointing at the fact that we should check for this combo and not only if it's a .Lib.

Sorry and that .Lib is only set when building shared libs, from line 1510

That's not correct. We always discriminate shared objects from static archives using the combo. See here for instance.

I see, I think the issue is I am adding this outside of the .cc/.cpp args check. Meaning it will affect other build modes. Maybe this is what you intend with your comment on aligning build-lib? I can add the dynamic check as well but should we restrict this to cc/cpp modes as well until the option can be added as a regular zig flag instead of strictly a linker flag?

I think we should be fine with other build modes if we set the default for the flag in further down the stack in Compilation rather than here. Anyhow, my suggestion would be to try it out and if things break along the way, then we can figure out what to fix and where. If you don't feel like wanting to pursue this, I'm happy to take over from here. Otherwise, I'm happy to work with you to get this merged.

Sure, by all means if you have time to work on this id be happy if you could work on it as Im mostly just muddling my way through zig to get something that works. Of course feel free to use anything in this pr as you see fit.

kubkon · 2021-07-28T06:09:01Z

src/link/Coff.zig

@@ -1095,6 +1099,10 @@ fn linkWithLLD(self: *Coff, comp: *Compilation) !void {
            try argv.append(p);
        }

+        if (self.base.options.out_implib != null) {
+            try argv.append(try allocPrint(arena, "-IMPLIB:{s}.lib", .{full_out_path}));


Should this not set -IMPLIB to the value in out_implib rather than full_out_path? What if the user sets --out-implib to something different than location of the final binary?

full_out_path here is the path within the cache and not the final destination. The final destination is handled is the path stored in out_implib that is used during the UpdateModule step, similar to pdbs. We just choose a path here that is unlikely to collide with other linker outputs.

Gotcha, thanks.

I guess this is fine to leave as-is for now but this will probably need to be cleaned up and put as logic in the linker proper rather than Compilation.updateModule. After all, we also need to update a dSYM bundle in MachO which should not be of any concern to the Compilation module. cc @andrewrk

Kha · 2021-09-15T11:04:50Z

Fwiw, I can confirm that this PR works for building and linking against a non-trivial import library, getting me one step closer to shipping Zig with Lean.

Allow --out-implib and -implib as passed by cmake and meson to be correctly passed through to the linker to generate import libraries.

Previously these were added to the hash but not actually appended to the linker arguments.

This mechanism for sending arbitrary linker args to LLD has no place in the Zig frontend, because our goal is for the frontend to understand all the arguments and not treat linker args like a black box. For example we have self-hosted linking in addition to LLD, so we want to have the options make sense to both linking codepaths, not just the LLD one. Passing -O linker args will now result in a warning that the arg does nothing.

* Improve the logic for determining whether emitting an import lib is eligible, and improve the error message when the user provides contradictory arguments. * Integrate with the EmitLoc / Emit system that already exists, and use the `-femit-implib[=path]`/`-fno-emit-implib` convention that already exists. * Proper integration with the caching system. * CLI: fix bug in error reporting for resolving EmitLoc values for other parameters.

In 7e23b32 I made -O flags to the linker emit a warning that the argument does nothing. That was not correct however; LLD does have some logic that does different things depending on -O0, -O1, and -O2. It defaults to -O1, and it does less optimizations with -O0 and more with -O2. With this commit, e.g. `-Wl,-O1` is supported by the `zig cc` frontend, and by default we pass `-O0` to LLD in debug mode, and `-O3` in release modes. I also fixed a bug in the LLD ELF linker line which was incorrectly passing `-O` flags instead of `--lto-O` flags for LTO.

andrewrk · 2021-11-25T01:48:50Z

I rebased this and added a few commits on top of it before merging it.

$ ls blah*
blah.zig

$ cat blah.zig 
export fn foo() void {}

$ ./zig build-lib -dynamic blah.zig -target x86_64-windows

$ ls blah*
blah.dll  blah.lib  blah.pdb  blah.zig

$ file blah.lib 
blah.lib: current ar archive

@Kha - mind giving it a shot for your use case and let me know if you run into any other problems?

alexrp · 2021-11-25T04:38:33Z

@andrewrk note that the really important test here is whether you can get an implib when building with zig cc -shared. AFAIK, zig build-lib -dynamic already produced implibs correctly before this PR.

andrewrk · 2021-11-25T05:35:08Z

andy@ark ~/tmp> cat foo.c
void foo(void) {}
andy@ark ~/tmp> ls foo*
foo.c
andy@ark ~/tmp> zig cc -o foo.dll -shared foo.c -target x86_64-windows
andy@ark ~/tmp> ls foo*
foo.c  foo.dll  foo.lib

This comment has been minimized.

Sign in to view

alexrp reviewed Jul 10, 2021

View reviewed changes

src/main.zig Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

alexrp reviewed Jul 10, 2021

View reviewed changes

src/link/Coff.zig Outdated Show resolved Hide resolved

kkartaltepe force-pushed the implib-support branch from 3ae6dbb to 7a7307b Compare July 10, 2021 04:35

alexrp mentioned this pull request Jul 10, 2021

link: Recognize -Wl,-implib,foo.lib and pass it to LLD as /implib:foo.lib. #9251

Closed

kkartaltepe force-pushed the implib-support branch from 7a7307b to 41d1239 Compare July 12, 2021 05:57

alexrp reviewed Jul 18, 2021

View reviewed changes

src/link/Coff.zig Outdated Show resolved Hide resolved

alexrp mentioned this pull request Jul 20, 2021

Project and package reference support vezel-dev/zig-sdk#8

Open

kkartaltepe force-pushed the implib-support branch from 41d1239 to 9a76b33 Compare July 23, 2021 05:10

kubkon linked an issue Jul 27, 2021 that may be closed by this pull request

Creating a DLL on Windows with zig cc does not produce an import library (.lib) #9210

Closed

kubkon removed a link to an issue Jul 27, 2021

Creating a DLL on Windows with zig cc does not produce an import library (.lib) #9210

Closed

kubkon requested changes Jul 27, 2021

View reviewed changes

kubkon linked an issue Jul 27, 2021 that may be closed by this pull request

Creating a DLL on Windows with zig cc does not produce an import library (.lib) #9210

Closed

kkartaltepe force-pushed the implib-support branch from 9a76b33 to fbbe2f4 Compare July 28, 2021 03:26

kubkon requested changes Jul 28, 2021

View reviewed changes

zhaozg mentioned this pull request Jul 29, 2021

ability to produce DLL import .lib files as build artifacts #6646

Open

andrewrk self-assigned this Jul 29, 2021

Kha mentioned this pull request Sep 20, 2021

Build & distribute release builds with Zig as stand-alone C compiler leanprover/lean4#659

Closed

kkartaltepe and others added 5 commits November 24, 2021 17:14

Coff linker: Add IMPLIB support

a950cb4

Allow --out-implib and -implib as passed by cmake and meson to be correctly passed through to the linker to generate import libraries.

Coff Linker: Pass extra lld args

fd369bc

Previously these were added to the hash but not actually appended to the linker arguments.

andrewrk force-pushed the implib-support branch from fbbe2f4 to 20cc7af Compare November 25, 2021 01:47

andrewrk merged commit 0581756 into ziglang:master Nov 25, 2021

Coff linker: Add IMPLIB support #9347

Coff linker: Add IMPLIB support #9347

Conversation

kkartaltepe commented Jul 10, 2021

kkartaltepe commented Jul 10, 2021

This comment has been minimized.

This comment has been minimized.

alexrp commented Jul 10, 2021

kkartaltepe commented Jul 10, 2021

alexrp commented Jul 10, 2021

kkartaltepe commented Jul 10, 2021

kkartaltepe commented Jul 10, 2021 • edited Loading

alexrp commented Jul 10, 2021

kkartaltepe commented Jul 10, 2021

alexrp commented Jul 10, 2021 • edited Loading

kkartaltepe commented Jul 12, 2021 • edited Loading

alexrp commented Jul 12, 2021

kkartaltepe commented Jul 12, 2021

alexrp commented Jul 18, 2021 • edited Loading

alexrp commented Jul 18, 2021

andrewrk commented Jul 23, 2021

kkartaltepe commented Jul 23, 2021

kubkon 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

kkartaltepe Jul 28, 2021 • 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

Kha commented Sep 15, 2021

andrewrk commented Nov 25, 2021

alexrp commented Nov 25, 2021

andrewrk commented Nov 25, 2021

kkartaltepe commented Jul 10, 2021 •

edited

Loading

alexrp commented Jul 10, 2021 •

edited

Loading

kkartaltepe commented Jul 12, 2021 •

edited

Loading

alexrp commented Jul 18, 2021 •

edited

Loading

kkartaltepe Jul 28, 2021 •

edited

Loading