-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Coff linker: Add IMPLIB support #9347
Conversation
Ah it seems this crossed paths with #9251 which looks like it adds support for the ms |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
#9251 can be closed in favor of this if my comments above are addressed. |
3ae6dbb
to
7a7307b
Compare
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. |
Can you elaborate on the caching issue? I don't quite follow. |
Oh sorry it may be due to me swapping between unmodified zig and this patch that causes the caching issue actually. |
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
|
Ok, I see. That actually makes sense. I think we need to handle this similarly to how See: Lines 2396 to 2419 in 7b5d139
|
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, .{});
+ }
},
}
} |
I don't think Zig does this explicitly. Clang internally outputs the $ 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
Here's what I think needs to be done exactly:
|
7a7307b
to
41d1239
Compare
For now I have have zig output into the cache with a fixed name 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 |
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 is a good point, we should definitely do that. I'm not sure whether such a function exists in the stdlib though. |
This line https://github.com/ziglang/zig/blob/master/src/Compilation.zig#L589 which is the source for the cache dir in the linker. |
I asked on the Zig Discord and they suggested that |
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: Lines 1500 to 1504 in 41d1239
|
Next step here is to rebase against master and get the CI tests passing. |
41d1239
to
9a76b33
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/main.zig
Outdated
@@ -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| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inout_implib
optional - if
--out-implib
was not set explicitly, regardless of the targeted ABI, inCompilation.zig
(most likely), we set the default value ofout_implib
to the same path as the final binary; then, inCompilation.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
9a76b33
to
fbbe2f4
Compare
src/main.zig
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/main.zig
Outdated
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry and that .Lib is only set when building shared libs, from line 1510
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not correct. We always discriminate shared objects from static archives using the combo. See here for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/link/Coff.zig
Outdated
@@ -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})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
fbbe2f4
to
20cc7af
Compare
I rebased this and added a few commits on top of it before merging it.
@Kha - mind giving it a shot for your use case and let me know if you run into any other problems? |
@andrewrk note that the really important test here is whether you can get an implib when building with |
|
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.