compiler: resolve library paths in the frontend #16058
Conversation
andrewrk
force-pushed
the
frontend-lib-paths
branch
from
bbcd48b to
889ad14
Compare
|
This needs another commit to fix handling of system DLLs on Windows such as kernel32, but it's ready to go for other targets. |
src/link/MachO.zig
Outdated
| for (search_dirs) |dir| { | ||
| if (try resolveLib(arena, dir, "System", ".tbd")) |full_path| { | ||
| try out_libs.put(full_path, .{ .needed = true }); | ||
| try out_libs.put(full_path, .{ |
There was a problem hiding this comment.
Perhaps not in this PR (I can follow up with a subsequent PR) but we could move this logic to main.zig too? This way we would have all path handling done in the frontend.
There was a problem hiding this comment.
I ran into an issue with this right away when testing with one of my projects linking system libraries using std.Build.linkSystemLibrary() which invokes pkg-config to determine the required linker flags. pkg-config assumes the flags will be passed to the system linker and omits flags that would be redundant with the system linker's defaults. For example, pkg-config --libs xkbcommon on my system prints only -lxkbcommon since /usr/lib is a default search path of my system linker.
With this patch we no longer rely on the library searching of the system linker however so libxkbcommon is not found as the code in main.zig is not aware of the /usr/lib system default.
As for how best to fix this change in behavior for my use-case it appears adding --keep-system-libs to the pkg-config invocation in std.Build.Compile is sufficient.
Unfortunately this affects zig cc as well however and users will expect zig cc's linker to continue to have the traditional default search paths. I guess we can probably look at lld's source to determine exactly what these paths are and add the necessary logic to main.zig to match the behavior there.
To recap, this works perfectly for my use-cases with the following patch!
diff --git a/lib/std/Build/Step/Compile.zig b/lib/std/Build/Step/Compile.zig
index 092fdf7e6..51ab882db 100644
--- a/lib/std/Build/Step/Compile.zig
+++ b/lib/std/Build/Step/Compile.zig
@@ -842,6 +842,7 @@ fn runPkgConfig(self: *Compile, lib_name: []const u8) ![]const []const u8 {
pkg_name,
"--cflags",
"--libs",
+ "--keep-system-libs",
}, &code, .Ignore)) |stdout| stdout else |err| switch (err) {
error.ProcessTerminated => return error.PkgConfigCrashed,
error.ExecNotSupported => return error.PkgConfigFailed,
This looks like an incorrect assumption of the system linker (hello GNU ld). Having said that, zig doesn't use system linker on linux - it uses ld.lld so I am surprised how this issue arises. Anyhow, library search paths, IMHO, should always be passed explicitly to the linker to avoid confusion. There are also a few env vars we could read such as |
|
I mentioned zld and mold as neither assumes any default search paths and hence their output differs from that of gold. |
|
@kubkon It looks like I was wrong about where these defaults that It appears zig 0.10 discovered and passed several library paths to lld that aren't currently being searched for libraries on this branch. For this 0.10 zig build-exe invocationThis is the lld invocation, notably with -L paths not passed to zig build-exe |
src/main.zig
Outdated
| }); | ||
| } | ||
| } | ||
| try lib_dirs.appendSlice(paths.lib_dirs.items); |
There was a problem hiding this comment.
I think doing this before searching for libraries would fix the issue I describe.
The order of adding native paths to lib_dirs and searching for libraries has effectively changed with respect to master branch, which I believe is the root cause of the behavior change I see.
There was a problem hiding this comment.
I've confirmed that the following patch changing the ordering of these steps fixes this regression for my use-case:
patch
diff --git a/src/main.zig b/src/main.zig
index 56f305e6c..deac5b904 100644
--- a/src/main.zig
+++ b/src/main.zig
@@ -2541,6 +2541,54 @@ fn buildOutputType(
}
lib_dir_args = undefined; // From here we use lib_dirs instead.
+ if (comptime builtin.target.isDarwin()) {
+ // If we want to link against frameworks, we need system headers.
+ if (framework_dirs.items.len > 0 or frameworks.count() > 0)
+ want_native_include_dirs = true;
+ }
+
+ if (sysroot == null and cross_target.isNativeOs() and
+ (system_libs.count() != 0 or want_native_include_dirs))
+ {
+ const paths = std.zig.system.NativePaths.detect(arena, target_info) catch |err| {
+ fatal("unable to detect native system paths: {s}", .{@errorName(err)});
+ };
+ for (paths.warnings.items) |warning| {
+ warn("{s}", .{warning});
+ }
+
+ const has_sysroot = if (comptime builtin.target.isDarwin()) outer: {
+ if (std.zig.system.darwin.isDarwinSDKInstalled(arena)) {
+ const sdk = std.zig.system.darwin.getDarwinSDK(arena, target_info.target) orelse
+ break :outer false;
+ native_darwin_sdk = sdk;
+ try clang_argv.ensureUnusedCapacity(2);
+ clang_argv.appendAssumeCapacity("-isysroot");
+ clang_argv.appendAssumeCapacity(sdk.path);
+ break :outer true;
+ } else break :outer false;
+ } else false;
+
+ try clang_argv.ensureUnusedCapacity(paths.include_dirs.items.len * 2);
+ const isystem_flag = if (has_sysroot) "-iwithsysroot" else "-isystem";
+ for (paths.include_dirs.items) |include_dir| {
+ clang_argv.appendAssumeCapacity(isystem_flag);
+ clang_argv.appendAssumeCapacity(include_dir);
+ }
+
+ try clang_argv.ensureUnusedCapacity(paths.framework_dirs.items.len * 2);
+ try framework_dirs.ensureUnusedCapacity(paths.framework_dirs.items.len);
+ const iframework_flag = if (has_sysroot) "-iframeworkwithsysroot" else "-iframework";
+ for (paths.framework_dirs.items) |framework_dir| {
+ clang_argv.appendAssumeCapacity(iframework_flag);
+ clang_argv.appendAssumeCapacity(framework_dir);
+ framework_dirs.appendAssumeCapacity(framework_dir);
+ }
+
+ try lib_dirs.appendSlice(paths.lib_dirs.items);
+ try rpath_list.appendSlice(paths.rpaths.items);
+ }
+
// Now that we have target info, we can find out if any of the system libraries
// are part of libc or libc++. We remove them from the list and communicate their
// existence via flags instead.
@@ -2756,54 +2804,6 @@ fn buildOutputType(
}
}
- if (comptime builtin.target.isDarwin()) {
- // If we want to link against frameworks, we need system headers.
- if (framework_dirs.items.len > 0 or frameworks.count() > 0)
- want_native_include_dirs = true;
- }
-
- if (sysroot == null and cross_target.isNativeOs() and
- (resolved_system_libs.len != 0 or want_native_include_dirs))
- {
- const paths = std.zig.system.NativePaths.detect(arena, target_info) catch |err| {
- fatal("unable to detect native system paths: {s}", .{@errorName(err)});
- };
- for (paths.warnings.items) |warning| {
- warn("{s}", .{warning});
- }
-
- const has_sysroot = if (comptime builtin.target.isDarwin()) outer: {
- if (std.zig.system.darwin.isDarwinSDKInstalled(arena)) {
- const sdk = std.zig.system.darwin.getDarwinSDK(arena, target_info.target) orelse
- break :outer false;
- native_darwin_sdk = sdk;
- try clang_argv.ensureUnusedCapacity(2);
- clang_argv.appendAssumeCapacity("-isysroot");
- clang_argv.appendAssumeCapacity(sdk.path);
- break :outer true;
- } else break :outer false;
- } else false;
-
- try clang_argv.ensureUnusedCapacity(paths.include_dirs.items.len * 2);
- const isystem_flag = if (has_sysroot) "-iwithsysroot" else "-isystem";
- for (paths.include_dirs.items) |include_dir| {
- clang_argv.appendAssumeCapacity(isystem_flag);
- clang_argv.appendAssumeCapacity(include_dir);
- }
-
- try clang_argv.ensureUnusedCapacity(paths.framework_dirs.items.len * 2);
- try framework_dirs.ensureUnusedCapacity(paths.framework_dirs.items.len);
- const iframework_flag = if (has_sysroot) "-iframeworkwithsysroot" else "-iframework";
- for (paths.framework_dirs.items) |framework_dir| {
- clang_argv.appendAssumeCapacity(iframework_flag);
- clang_argv.appendAssumeCapacity(framework_dir);
- framework_dirs.appendAssumeCapacity(framework_dir);
- }
-
- try lib_dirs.appendSlice(paths.lib_dirs.items);
- try rpath_list.appendSlice(paths.rpaths.items);
- }
-
const object_format = target_info.target.ofmt;
if (output_mode == .Obj and (object_format == .coff or object_format == .macho)) {There was a problem hiding this comment.
Hmm, I believe that patch is not entirely correct however as it changes the behavior when the only libs in system_libs are part of libc for example.
There was a problem hiding this comment.
Ok, I think this patch should have fully correct behavior:
diff --git a/src/main.zig b/src/main.zig
index 56f305e6c..be2f5e47f 100644
--- a/src/main.zig
+++ b/src/main.zig
@@ -2544,44 +2544,31 @@ fn buildOutputType(
// Now that we have target info, we can find out if any of the system libraries
// are part of libc or libc++. We remove them from the list and communicate their
// existence via flags instead.
- // Similarly, if any libs in this list are statically provided, we omit
- // them from the resolved list and populate the link_objects array instead.
- var resolved_system_libs: std.MultiArrayList(struct {
- name: []const u8,
- lib: Compilation.SystemLib,
- }) = .{};
-
{
- var test_path = std.ArrayList(u8).init(gpa);
- defer test_path.deinit();
-
- var checked_paths = std.ArrayList(u8).init(gpa);
- defer checked_paths.deinit();
-
- var failed_libs = std.ArrayList(struct {
- name: []const u8,
- strategy: SystemLib.SearchStrategy,
- checked_paths: []const u8,
- preferred_mode: std.builtin.LinkMode,
- }).init(arena);
+ var i: usize = 0;
+ while (i < system_libs.count()) {
+ const lib_name = system_libs.keys()[i];
- syslib: for (system_libs.keys(), system_libs.values()) |lib_name, info| {
if (target_util.is_libc_lib_name(target_info.target, lib_name)) {
link_libc = true;
+ system_libs.orderedRemoveAt(i);
continue;
}
if (target_util.is_libcpp_lib_name(target_info.target, lib_name)) {
link_libcpp = true;
+ system_libs.orderedRemoveAt(i);
continue;
}
switch (target_util.classifyCompilerRtLibName(target_info.target, lib_name)) {
.none => {},
.only_libunwind, .both => {
link_libunwind = true;
+ system_libs.orderedRemoveAt(i);
continue;
},
.only_compiler_rt => {
std.log.warn("ignoring superfluous library '{s}': this dependency is fulfilled instead by compiler-rt which zig unconditionally provides", .{lib_name});
+ system_libs.orderedRemoveAt(i);
continue;
},
}
@@ -2593,10 +2580,85 @@ fn buildOutputType(
if (target_info.target.os.tag == .wasi) {
if (wasi_libc.getEmulatedLibCRTFile(lib_name)) |crt_file| {
try wasi_emulated_libs.append(crt_file);
+ system_libs.orderedRemoveAt(i);
continue;
}
}
+ i += 1;
+ }
+ }
+
+ if (comptime builtin.target.isDarwin()) {
+ // If we want to link against frameworks, we need system headers.
+ if (framework_dirs.items.len > 0 or frameworks.count() > 0)
+ want_native_include_dirs = true;
+ }
+
+ if (sysroot == null and cross_target.isNativeOs() and
+ (system_libs.count() != 0 or want_native_include_dirs))
+ {
+ const paths = std.zig.system.NativePaths.detect(arena, target_info) catch |err| {
+ fatal("unable to detect native system paths: {s}", .{@errorName(err)});
+ };
+ for (paths.warnings.items) |warning| {
+ warn("{s}", .{warning});
+ }
+
+ const has_sysroot = if (comptime builtin.target.isDarwin()) outer: {
+ if (std.zig.system.darwin.isDarwinSDKInstalled(arena)) {
+ const sdk = std.zig.system.darwin.getDarwinSDK(arena, target_info.target) orelse
+ break :outer false;
+ native_darwin_sdk = sdk;
+ try clang_argv.ensureUnusedCapacity(2);
+ clang_argv.appendAssumeCapacity("-isysroot");
+ clang_argv.appendAssumeCapacity(sdk.path);
+ break :outer true;
+ } else break :outer false;
+ } else false;
+
+ try clang_argv.ensureUnusedCapacity(paths.include_dirs.items.len * 2);
+ const isystem_flag = if (has_sysroot) "-iwithsysroot" else "-isystem";
+ for (paths.include_dirs.items) |include_dir| {
+ clang_argv.appendAssumeCapacity(isystem_flag);
+ clang_argv.appendAssumeCapacity(include_dir);
+ }
+
+ try clang_argv.ensureUnusedCapacity(paths.framework_dirs.items.len * 2);
+ try framework_dirs.ensureUnusedCapacity(paths.framework_dirs.items.len);
+ const iframework_flag = if (has_sysroot) "-iframeworkwithsysroot" else "-iframework";
+ for (paths.framework_dirs.items) |framework_dir| {
+ clang_argv.appendAssumeCapacity(iframework_flag);
+ clang_argv.appendAssumeCapacity(framework_dir);
+ framework_dirs.appendAssumeCapacity(framework_dir);
+ }
+
+ try lib_dirs.appendSlice(paths.lib_dirs.items);
+ try rpath_list.appendSlice(paths.rpaths.items);
+ }
+
+ // If any libs in this list are statically provided, we omit them from the resolved list
+ // and populate the link_objects array instead.
+ var resolved_system_libs: std.MultiArrayList(struct {
+ name: []const u8,
+ lib: Compilation.SystemLib,
+ }) = .{};
+
+ {
+ var test_path = std.ArrayList(u8).init(gpa);
+ defer test_path.deinit();
+
+ var checked_paths = std.ArrayList(u8).init(gpa);
+ defer checked_paths.deinit();
+
+ var failed_libs = std.ArrayList(struct {
+ name: []const u8,
+ strategy: SystemLib.SearchStrategy,
+ checked_paths: []const u8,
+ preferred_mode: std.builtin.LinkMode,
+ }).init(arena);
+
+ syslib: for (system_libs.keys(), system_libs.values()) |lib_name, info| {
checked_paths.clearRetainingCapacity();
switch (info.search_strategy) {
@@ -2756,54 +2818,6 @@ fn buildOutputType(
}
}
- if (comptime builtin.target.isDarwin()) {
- // If we want to link against frameworks, we need system headers.
- if (framework_dirs.items.len > 0 or frameworks.count() > 0)
- want_native_include_dirs = true;
- }
-
- if (sysroot == null and cross_target.isNativeOs() and
- (resolved_system_libs.len != 0 or want_native_include_dirs))
- {
- const paths = std.zig.system.NativePaths.detect(arena, target_info) catch |err| {
- fatal("unable to detect native system paths: {s}", .{@errorName(err)});
- };
- for (paths.warnings.items) |warning| {
- warn("{s}", .{warning});
- }
-
- const has_sysroot = if (comptime builtin.target.isDarwin()) outer: {
- if (std.zig.system.darwin.isDarwinSDKInstalled(arena)) {
- const sdk = std.zig.system.darwin.getDarwinSDK(arena, target_info.target) orelse
- break :outer false;
- native_darwin_sdk = sdk;
- try clang_argv.ensureUnusedCapacity(2);
- clang_argv.appendAssumeCapacity("-isysroot");
- clang_argv.appendAssumeCapacity(sdk.path);
- break :outer true;
- } else break :outer false;
- } else false;
-
- try clang_argv.ensureUnusedCapacity(paths.include_dirs.items.len * 2);
- const isystem_flag = if (has_sysroot) "-iwithsysroot" else "-isystem";
- for (paths.include_dirs.items) |include_dir| {
- clang_argv.appendAssumeCapacity(isystem_flag);
- clang_argv.appendAssumeCapacity(include_dir);
- }
-
- try clang_argv.ensureUnusedCapacity(paths.framework_dirs.items.len * 2);
- try framework_dirs.ensureUnusedCapacity(paths.framework_dirs.items.len);
- const iframework_flag = if (has_sysroot) "-iframeworkwithsysroot" else "-iframework";
- for (paths.framework_dirs.items) |framework_dir| {
- clang_argv.appendAssumeCapacity(iframework_flag);
- clang_argv.appendAssumeCapacity(framework_dir);
- framework_dirs.appendAssumeCapacity(framework_dir);
- }
-
- try lib_dirs.appendSlice(paths.lib_dirs.items);
- try rpath_list.appendSlice(paths.rpaths.items);
- }
-
const object_format = target_info.target.ofmt;
if (output_mode == .Obj and (object_format == .coff or object_format == .macho)) {
There was a problem hiding this comment.
You reminded me @ifreund that when we link against system libs on Linux GNU, system libs are actually linker scripts which might included a path such as -lgcc_s or similar which will require knowing library search paths by the linker.
|
Indeed... why do you have to be like this gnu :/ |
|
So, this may be a bit counter-intuitive, but I actually think there is a strong argument for moving parsing of these fake .so files into the CLI as well. Hear me out... Lines 2459 to 2471 in 0f5aff3 This code here is in master branch, and checks if The frontend is in the business of knowing which libraries are part of libc, part of compiler_rt, or other, because it needs this information to set up the compilation pipeline - potentially scheduling the creation of compiler_rt to run in parallel, for example. Also, I think it is telling that inside these files are command line arguments so it actually does make sense if you think about it to handle in the command line argument parsing code. |
|
I searched my hard drive for .so files that are actually text files, and came up with a small list of unique test cases. The entire list of unique file basenames in this sample set is: All the files concatenated together, with redundant contents manually removed: As you can see, there are not very many different concepts at play here. All this stuff directly corresponds to command line arguments. |
|
Yeah, I think we should be able to do this in the CLI frontend. As far as I understand, we only need to support a limited ld script syntax to be able to correctly parse those files, and the good news is I've got that implemented in zld already. I could then upstream the corresponding functionality of the linker and make it part of the CLI which would look something like this:
|
|
I think it sounds like the path forward! |
Excellent! |
kubkon
force-pushed
the
frontend-lib-paths
branch
from
889ad14 to
f60d392
Compare
andrewrk
force-pushed
the
frontend-lib-paths
branch
3 times, most recently
from
e2f1abe to
bf33b7f
Compare
andrewrk
force-pushed
the
frontend-lib-paths
branch
2 times, most recently
from
430ae4d to
310db02
Compare
lib/std/zig/system/NativePaths.zig
Outdated
| return self; | ||
| } | ||
|
|
||
| // These do not include headers, so the ones that come with the SDK are preferred. |
There was a problem hiding this comment.
Is this ever valid? I don't think there is a macOS installation that has frameworks exposed at this path? Could you explain your reasoning here?
There was a problem hiding this comment.
Hmm, I do see frameworks at this path. I noticed the headers were missing, but upon further inspection the tbd files are missing too, and in fact I don't see anything here that could be remotely useful for us. So I will remove this path. Thanks!
| self.include_dir = try fs.path.join(args.allocator, &.{ | ||
| sdk.path, "usr/include", | ||
| }); | ||
| self.sys_include_dir = try fs.path.join(args.allocator, &.{ | ||
| sdk.path, "usr/include", |
There was a problem hiding this comment.
I take it we do not want to make framework dir part of LibCInstallation?
There was a problem hiding this comment.
It effectively is, since it can always be found at ../../System/Library/Frameworks which is exactly what detectLibCFromLibCInstallation does. Thankfully the Darwin SDK paths are reasonable, and this property holds. On other operating systems such as Linux, the directories are all over the place, so we have to store multiple different named path roots.
For each library you can specify the preferred mode and search strategy. The old way of setting global state is eliminated.
I expect this to have no functional change.
Apparently tidy doesn't exit with failure status if the input file is not found? wtf tidy
The `null` value here was missed in 0a4d4eb. I hope it is the cause of the CI failures on Windows. The fact that libc++ depends on libc is not important for the CLI and Compilation.create already handles that logic.
Make a bunch of ArrayList objects use arena instead of gpa, eliminating the `defer` expressions, which reduces code size of zig1.wasm by 1%
Regressed in 2006add. References to native_darwin_sdk are no longer kept in the frontend. Instead the darwin SDK is detected as part of NativePaths and as part of LibCInstallation.
* don't assert that the child process doesn't crash * don't give a false negative on warnings printed to stderr Also fix getSdk from the same file in the same way
This path actually has nothing useful in it.
andrewrk
force-pushed
the
frontend-lib-paths
branch
from
cd491f9 to
d0fd67c
Compare
|
I don't think this accounts for mingw's |
|
Please file a bug report and we can get it fixed in 0.11.1. The logic is here: Lines 6240 to 6318 in a327d8b this is growing pains from moving logic from LLD into the frontend, as we move to using our own linker implementations. |
4 participants
The main change here is introducing the concept of preferred link mode and search strategy when resolving libraries.
CLI
The four options to control these flags are:
-search_paths_firstand-search_dylibs_firstcome from the Darwin toolchain and previously only affected those targets, but now they work the same for all targets. The other flags I added by following the naming convention. These arguments are stateful; they affect all subsequent libraries linked by name, such as the flags-l,-weak-l, and-needed-l.search_strategy is no longer passed to Compilation at all; instead it is used in the CLI code only.
System libraries are now included in the cache hash, and thus changes to them will correctly cause cache misses.
In the future, lib_dirs should no longer be passed to Compilation at all, because it is a frontend-only concept.
--sysrootalso now affects all targets the same instead of only affecting Darwin.Improved the error reporting for failure to find a system library. An example error now looks like this:
This is the same default search strategy as when the
zig ccfrontend is used. However, using the Zig CLI,-search_dylibs_firstcan now be used for any target:-staticnow implies-search_static_only:closes #14963
Build System
Previously the build system exposed
-search_paths_firstand-search_dylibs_firstwhich had the ability to retroactively affect all libraries. Now, instead, the build.zig script explicitly chooses the search strategy and preferred link mode for each library independently, and the Compile step emits CLI options to communicate this information to the compiler.macOS
Previously, macOS had a bunch of special casing to make it ignore the native system libraries and header files. This changeset makes macOS do the same thing as Linux, integrating with the host system SDK in the case of a native build.
EDIT:
Fixes #15024
Fixes #16118
Fixes #15963