Skip to content
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

std.zig.system.NativeTargetInfo: use getconf for detecting glibc version on glibc systems #12567

Closed
wants to merge 1 commit into from
Closed

std.zig.system.NativeTargetInfo: use getconf for detecting glibc version on glibc systems #12567

wants to merge 1 commit into from

Conversation

BratishkaErik
Copy link
Contributor

@BratishkaErik BratishkaErik commented Aug 22, 2022

Closes #6469 and closes #11137
Signed-off-by: Eric Joldasov bratishkaerik@getgoogleoff.me

error.InvalidGnuLibCVersion,
=> break :glibc_ver,
};
const getconf = std.ChildProcess.exec(.{ .allocator = allocator, .argv = &.{ "getconf", "GNU_LIBC_VERSION" } }) catch unreachable;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC all glibc systems (even more, all POSIX systems https://pubs.opengroup.org/onlinepubs/000095399/utilities/getconf.html) has this program, and if not, it is a bug in systems itself.

}
const glibc_ver_from_stdout = getconf.stdout["glibc ".len..];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to man:

_CS_GNU_LIBC_VERSION (GNU C library only; since glibc 2.3.2)
A string which identifies the GNU C library version on
this system (e.g., "glibc 2.3.4").

@@ -641,167 +598,15 @@ pub fn abiAndDynamicLinkerFromFile(
if (builtin.target.os.tag == .linux and result.target.isGnuLibC() and
Copy link
Contributor Author

@BratishkaErik BratishkaErik Aug 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But anyway this will be not executed on a non-glibc systems

@BratishkaErik
Copy link
Contributor Author

BratishkaErik commented Aug 22, 2022

@topolarity does this fixes #11137 (comment)
and #11137 (comment) for you?

@BratishkaErik
Copy link
Contributor Author

@topolarity does this fixes #11137 (comment) and #11137 (comment) for you?

Oops, I forgot that zig doesn't provide glibc 2.35 yet :(

@BratishkaErik
Copy link
Contributor Author

@topolarity does this fixes #11137 (comment) and #11137 (comment) for you?

Oops, I forgot that zig doesn't provide glibc 2.35 yet :(

error: zig does not yet provide glibc version 2.35, the max provided version is 2.34
error: unable to build glibc shared objects: InvalidTargetGLibCVersion
error: zig...
error: The following command exited with error code 1:
/home/bratishkaerik/zig/build/zig build-exe --stack 33554432 /home/bratishkaerik/zig/src/main.zig -lc /home/bratishkaerik/zig/build/zigcpp/libzigcpp.a /usr/lib/llvm/14/lib64/libclang-cpp.so.14 /usr/lib64/liblldMinGW.so /usr/lib64/liblldELF.so /usr/lib64/liblldCOFF.so /usr/lib64/liblldWasm.so /usr/lib64/liblldMachO.so /usr/lib64/liblldCommon.so /usr/lib/llvm/14/lib64/libLLVM-14.so /usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/libstdc++.so -lunwind --cache-dir /home/bratishkaerik/zig/zig-cache --global-cache-dir /home/bratishkaerik/.cache/zig --name zig --pkg-begin build_options /home/bratishkaerik/zig/zig-cache/options/7X62S_IHrJHJyONEQXfH1Bt-Y-ZrGbNtZVv-QoWvD9uq09Hl8g4BY__8D6284ocN --pkg-end -I /usr/include -fno-build-id --enable-cache 
error: the following build command failed with exit code 1:
/home/bratishkaerik/zig/zig-cache/o/87897b425d0e88a403c948b1ad22e5dc/build /home/bratishkaerik/zig/build/zig /home/bratishkaerik/zig /home/bratishkaerik/zig/zig-cache /home/bratishkaerik/.cache/zig -p stage2 -Dstatic-llvm=false -Denable-llvm=true

@BratishkaErik
Copy link
Contributor Author

My suggestion:

diff --git a/src/glibc.zig b/src/glibc.zig
index ceb60ff09..d714d4aa1 100644
--- a/src/glibc.zig
+++ b/src/glibc.zig
@@ -745,14 +745,7 @@ pub fn buildSharedObjects(comp: *Compilation) !void {
                     return error.InvalidTargetGLibCVersion;
                 },
             }
-        } else {
-            const latest_index = metadata.all_versions.len - 1;
-            // TODO Expose via compile error mechanism instead of log.
-            log.err("zig does not yet provide glibc version {}, the max provided version is {}", .{
-                target_version, metadata.all_versions[latest_index],
-            });
-            return error.InvalidTargetGLibCVersion;
-        };
+        } else metadata.all_versions.len - 1;
 
         {
             var map_contents = std.ArrayList(u8).init(arena);

@topolarity
Copy link
Contributor

topolarity commented Aug 24, 2022

@topolarity does this fixes #11137 (comment) and #11137 (comment) for you?

Looks like if Zig dynamically links libc (e.g. -Dstatic-llvm=false), then this is still unresolved
Latest push resolved this

@topolarity
Copy link
Contributor

@topolarity does this fixes #11137 (comment) and #11137 (comment) for you?

Oops, I forgot that zig doesn't provide glibc 2.35 yet :(

IIUC we should still be able to use the system glibc in this case, right?

@BratishkaErik
Copy link
Contributor Author

BratishkaErik commented Aug 24, 2022

@topolarity does this fixes #11137 (comment) and #11137 (comment) for you?

Looks like if Zig dynamically links libc (e.g. -Dstatic-llvm=false), then this is still unresolved

I force–pushed after this, can you try this version too? (and I'm using dynamic linking, too) ea7c4bc

@BratishkaErik
Copy link
Contributor Author

IIUC we should still be able to use the system glibc in this case, right?

I'm hitting this #12567 (comment)

@topolarity
Copy link
Contributor

I force–pushed recently, can you try this version too? (and I'm using dynamic linking, too) ea7cb4c

Yep, I was out of date! This appears to be resolved for me 👍

Re: the glibc version. Zig will currently fallback to using system-provided libc if you ask for system libraries (e.g. if you add -lz to the build-exe invocation, it will work). Perhaps it's worth performing the same fallback if Zig does not provide the requested version of libc but the system does?

@BratishkaErik
Copy link
Contributor Author

I force–pushed recently, can you try this version too? (and I'm using dynamic linking, too) ea7cb4c

Yep, I was out of date! This appears to be resolved for me 👍

Re: the glibc version. Zig will currently fallback to using system-provided libc if you ask for system libraries (e.g. if you add -lz to the build-exe invocation, it will work). Perhaps it's worth performing the same fallback if Zig does not provide the requested version of libc but the system does?

Something like?:
[if glibc newer than supplied] —> (use system libc) —> [if system doesn't provide headers] —> (use latest supplied libc available)

Copy link
Contributor

@topolarity topolarity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Debian (bookworm), NixOS (unstable), and Arch - all of which are affected by #6469 and #11137 and are fixed by this PR.

This change does mean linking libc will be broken on OS's with too-new libc (e.g. NixOS and Arch) until we have a fallback to link against system libc or update Zig-provided glibc, but this still seems like a step in the right direction even without that

P.S. Not sure what the CI failures are, but they seem unrelated?

lib/std/zig/system/NativeTargetInfo.zig Show resolved Hide resolved
@topolarity
Copy link
Contributor

Something like?:
[if glibc newer than supplied] —> (use system libc) —> [if system doesn't provide headers] —> (use latest supplied libc available)

Sounds OK to me - most of the logic should already be present in Compilation.zig. Might be as simple as updating canBuildLibC to account for the libc version

If you're up to tackling that, feel free to update this PR or start a new one - whichever you prefer 🙂

@BratishkaErik
Copy link
Contributor Author

BratishkaErik commented Aug 29, 2022

[if system doesn't provide headers] —> (use latest supplied libc available) ❌
01e3000

stage2: error if requested glibc version too high
Falling back to the max provided glibc version is insufficient as linking to shared objects compiled against the requested version will fail.

[if glibc newer than supplied] —> (use system libc) ❌
Unfortunately, not all users have required headers, our only option is updating glibc (IIRC from IRC, only core team member should do this)

@topolarity
Copy link
Contributor

Ah, I misunderstood and thought you had meant use the Zig headers to link against the system-provided glibc.

Good catch, let's stay faithful to the requested glibc version

andrewrk added a commit that referenced this pull request Sep 9, 2022
Previously, this code would fail to detect glibc version because it
relied on libc.so.6 being a symlink which revealed the answer. On modern
distros, this is no longer the case.

This new strategy finds the path to libc.so.6 from /usr/bin/env, then
inspects the .dynstr section of libc.so.6, looking for symbols that
start with "GLIBC_2.". It then parses those as semantic versions and
takes the maximum value as the system-native glibc version.

closes #6469
   see #11137
closes #12567
@andrewrk
Copy link
Member

andrewrk commented Sep 9, 2022

I created #12788 which supersedes this PR and does not depend on getconf.

…ion on glibc systems

Signed-off-by: Eric Joldasov <bratishkaerik@getgoogleoff.me>
andrewrk added a commit that referenced this pull request Sep 9, 2022
Previously, this code would fail to detect glibc version because it
relied on libc.so.6 being a symlink which revealed the answer. On modern
distros, this is no longer the case.

This new strategy finds the path to libc.so.6 from /usr/bin/env, then
inspects the .dynstr section of libc.so.6, looking for symbols that
start with "GLIBC_2.". It then parses those as semantic versions and
takes the maximum value as the system-native glibc version.

closes #6469
   see #11137
closes #12567
andrewrk added a commit that referenced this pull request Sep 9, 2022
Previously, this code would fail to detect glibc version because it
relied on libc.so.6 being a symlink which revealed the answer. On modern
distros, this is no longer the case.

This new strategy finds the path to libc.so.6 from /usr/bin/env, then
inspects the .dynstr section of libc.so.6, looking for symbols that
start with "GLIBC_2.". It then parses those as semantic versions and
takes the maximum value as the system-native glibc version.

closes #6469
   see #11137
closes #12567
@BratishkaErik BratishkaErik deleted the fix-glibc-version-detecting branch September 11, 2022 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-lc doesn't link c_nonshared zig detects wrong libc version
3 participants