Fix libc_nonshared.a #17702

rootbeer · 2023-10-24T17:44:57Z

Build on #16970 with additional CLs for "weak_hidden_alias" and re-enabling disabled tests.

rootbeer · 2023-10-25T00:10:41Z

CI tests are passing. The two CI failures seem to be native-native-musl failures:

run test std-x86_64-linux-none-Debug-selfhosted-no-lld: error: Illegal instruction at address 0x81febbe
run test std-x86_64-linux-none-Debug-selfhosted-no-lld: error: while executing test 'test.Condition - signal', the following command terminated with signal 4 (expected exited with code 0):

(I'm not clear if there are any subsequent test cases that were skipped because of this failure, or if its isolated to a single test or test file.)

This failure seem unlikely to be due to the changes here because of the "-musl" target .... but they don't look familiar either. I can rebase or wait for other fixes. (Or file a new bug.)

Running this stack locally, I can get my "fstat" repro case to pass with a variety of glibc versions (2.19, 2.25, 2.38, etc). That is an improvement, too.

Oddly, really old glibc versions don't compile (e.g. -target native-native-gnu.2.10) --- lots of missing symbols. I haven't tried them before, so I don't think that's related to this CL. (I'm not sure what the oldest supported glibc should be ... v2.16? v2.0.0?)

squeek502 · 2023-10-25T00:35:41Z

The CI failures are intermittent failures affecting current master as well, so likely unrelated to this PR: https://github.com/ziglang/zig/commits/master

rootbeer · 2023-10-25T04:16:31Z

I think this is good to go then. I'll see if I can add some tests of some sort that cover different glibc versions.

rootbeer · 2023-10-25T04:49:17Z

(I don't mean to steal @lifthrasiir's PRs, which are the bulk of this stack, so I'm happy to donate my patches to his stack if that keeps the attributions correct.)

lifthrasiir · 2023-10-25T05:11:40Z

(I don't mean to steal @lifthrasiir's PRs, which are the bulk of this stack, so I'm happy to donate my patches to his stack if that keeps the attributions correct.)

No problem! It seems that my PR is drifted away (again) from the master anyway, so ~~I'll pull this PR after CI passes.~~ Andy closed the original PR in favor of this :-)

squeek502 · 2023-10-25T23:14:06Z

Seems like it'd probably be good to add a standalone test for this. My initial thought was something very simple to test for #17034 like:

const std = @import("std");

test {
    try std.testing.expectError(error.FileNotFound, std.fs.cwd().statFile("test_file_that_doesnt_exist"));
}

with different glibc_versions in build.zig. Unsure how the host's glibc version would affect the usefulness of the test.

Unsure what a test for #16152 would look like.

rootbeer · 2023-10-25T23:32:52Z

I found test/link/glibc_compat/build.zig, which I expanded with some more tests. I compile some simple code (calls fstat, reallocarray, getauxval, and atexit) against a bunch of different glibc versions and makes sure it runs. I was going to submit it as a separate follow-on PR, but I can add it here if you'd like. See dfde1ae.

rootbeer · 2023-10-25T23:39:42Z

Oh, and my suggested tests are just covering the "positive" cases of glibc symbols being linked correctly. I don't test things like older versions not having newer symbols (e.g., reallocarray should not be found in earlier versions). I could probably figure out how to make a test for this work, if there are some existing tests of linker errors I can follow as an example?

And, I don't test that symbols are left unresolved in the resulting binary (e.g., testing that Zig does not erroneously link in static implementations). I could probably hack a shell script together using objdump or readelf, but I'm not sure that's portable enough to be useful?

andrewrk · 2023-10-27T19:50:58Z

I could probably hack a shell script together using objdump or readelf, but I'm not sure that's portable enough to be useful?

@kubkon might have some build.zig API tips to help with checking objects. For example check out some of the linker tests in test/link/*

rootbeer · 2023-10-28T05:38:12Z

I've rebased to today's changes, and I've added two commits for a glibc test case.

One commit is the basic test case I linked before. It should help ensure glibc exposes symbols at the correct version during compilation. The test is executed too, to see that things work at run-time.

I think I figured out how to make the Elf "checkObject()" tests work against the glibc test case, too. I'm not sure I'm holding the API correctly, but the tests do seem to pass. And they do seem to fail when I break things. (I couldn't get it to check that a "FUNC LOCAL HIDDEN" symbol showed up correctly, see the commented out checkExacts.)

rootbeer · 2023-10-29T19:21:27Z

Heh, tests fail when running executables linked against the newer glibc versions, as the host can't execute them. Will fix that.

rootbeer · 2023-10-30T18:51:28Z

This stack of commits could use some reviews, I think. The underlying commits (from @lifthrasiir) to fix libc_nonshared are the bulk of the change, and they're pretty solid and unchanged by me. In addition to fixing the "fstatat errno problem", this stack removes support for linking glibc versions before v2.17, generally (#17769). And adds some tests that build against different versions of glibc, and check that dynamic symbols are present as expected.

The tests I added to test/link/glibc_compat seem to work, but I'm not sure I'm using the exe.checkObject() APIs to check the work of the linker as well I could.

I added a README.md to lib/libc/glibc/. Anyone could review that (especially if you don't know much about Zig's support for glibc).

The commits that add "bounds checking" to glibc.zig, target.zig, and print_targets.zig seem to work, but I'm not sure if I should use a different approach (e.g., move more of the checks into target.zig), or if the architecture-specific bounds checks are worth the effort.

kubkon · 2023-10-30T20:12:52Z

test/link/glibc_compat/build.zig

+        exe.linkLibC();
+
+        // Only try running the test if the host glibc is known to be good enough
+        if (minor_ver <= running_glibc_minor) {


I wonder if this could be incorporated in our runner much like foreign test runner. What do you think @andrewrk ?

It seems complicated to determine the runtime linker's support if there is an emulator in play? (I guess the test infra could probe for that?) And I also think these does-runtime-support-newer-glibc checks are probably only needed in this test? Other tests should just be using the "default" glibc if they're explicitly linking?

That said, I'm willing to try merging this check into the test infra if folks think that is worthwhile.

Use the SemanticVersion methods for comparison please. Also, I think you should be able to remove this condition since you have skip_foreign_checks = true which is defined to skip the check if the host cannot execute the binary. If the build system needs an improvement to make this work correctly with respect to glibc versions, I'm happy to help with that.

I will work on making the skip_foreign_checks = true check for the glibc supported versions. Without this hack here, the CI machines would build binaries that failed to execute. I can do this as a follow-on to this PR, or will update this PR with the changes if you want to wait for that.

(And I've switched to SemanticVersion.order for the version checking.)

kubkon

Tests look good! I just have a few nits I would like you to try.

test/link/glibc_compat/build.zig

test/link/glibc_compat/glibc_runtime_check.zig

rootbeer · 2023-10-31T02:51:49Z

Rebased and updated with fixes to the test case. (I marked the review feedback that I completed as "resolved" in the UI here, hope that's the right protocol.)

rootbeer · 2023-11-15T16:06:43Z

I believe this PR is ready for more review feedback or for merging. (The open review discussion item is asking about merging the runtime-glibc-version-check into the core test runner or not.)

rootbeer · 2023-11-29T06:45:55Z

I've updated this PR to fix a bug in my glibc bounds checking (to use target.abi.isGnu()).

Also, I discovered that strlcpy and strlcat were new in glibc 2.38, but Zig's glibc header files were not reflecting that. Added CLs to fix that and test them. (These are great for testing the "skip_foreign_checks" path because my locally installed v2.36 glibc doesn't have these symbols.)

I spent a bit of time investigating how to extend Run.zig to gracefully hide/handle the case where a test fails because the host's glibc version isn't sufficient for the generated binary. These dynamic linker failures are fundamentally different from the existing skip_foreign_checks handling in Run.zig because the binary actually executes (the dynamic linker returns 1 and prints a message to stderr) where all the existing failures hidden by skip_foreign_checks are from errors during exec (the test binary doesn't even start from the parent's point of view).

Decoding the dynamic linker failures after the fact seems sketchy. I could add preventative code to runCommand that preemptively skips these tests if they look like they'll fail, but it seems simpler to keep this check in the glibc_compat/build.zig which is presumably where all the glibc multi-version tests will live anyway?

The scope of libc_nonshared.a was greatly changed in glibc 2.33 and 2.34, but only the change from 2.34 was reflected so far. Glibc 2.33 finally switched to versioned symbols for stat functions, meaning that libc_nonshared.a no longer contains them since 2.33. Relevant files were therefore reverted to 2.32 versions and renamed accordingly. This commit also removes errno.c, which was probably added to libc_nonshared.a based on a wrong assumption that glibc/include/errno.h requires glibc/csu/errno.c. In reality errno.h should refer to __libc_errno (not to be confused with the public __errno_location), which should be imported from libc.so. The inclusion of errno.c resulted in wrong compile options as well; this commit fixes them as well. Fixes ziglang#16152

Effectively reverts 3dcd361.

The fstat,lstat,stat,mknod stubs used to build older (before v2.33) glibc versions depend on the weak_hidden_alias macro. It was removed from the glibc libc-symbols header, so patch it back in for the older builds.

glibc variants now support the stat-family of calls correctly, so this test is safe to include.

Compile, link and run a test case against various glibc versions. Exercise symbols that have been probelmatic in the past.

Add a README with an overview of how Zig's glibc support is implemented.

At a minimum required glibc is v2.17, as earlier versions do not define some symbols (e.g., getauxval()) used by the Zig standard library. Additionally, glibc only supports some architectures at more recent versions (e.g., riscv64 support came in glibc v2.27). So add a `glibc_min` field to `available_libcs` for architectures with stronger version requirements. Extend the existing `canBuildLibC` function to check the target against the Zig minimum, and the architecture/os minimum. Also filter the list shown by `zig targets`, too: $ zig targets | jq -c '.glibc' ["2.17.0","2.18.0","2.19.0","2.20.0","2.21.0","2.22.0","2.23.0","2.24.0","2.25.0","2.26.0","2.27.0","2.28.0","2.29.0","2.30.0","2.31.0","2.32.0","2.33.0","2.34.0","2.35.0","2.36.0","2.37.0","2.38.0"] Fixes ziglang#17034 Fixes ziglang#17769

So only expose these in generic-glibc/string.h if Zig is building a v2.38 (or later) glibc stub. Announcement of 2.38 that notes strlcpy and strlcat: https://lists.gnu.org/archive/html/info-gnu/2023-07/msg00010.html

The strlcpy symbol was added in v2.38, so this is a handy symbol for creating binaries that won't run on relatively modern systems (e.g., mine, that has glibc 2.36 installed).

* fix typos and redundancies in docs * use Target.isGnuLibc

andrewrk · 2024-01-05T00:30:35Z

Fantastic work, @rootbeer. Thank you also for keeping this up to date - you did quite a few rebases between when you opened it and when it was merged.

Notable changes: - bump glibc support from 2.34 to 2.38 - fix integration with some glibc symbols: ziglang/zig#17702 - mitigat `error: AccessDenied`, which, according to Noah Santschi-Cooney, also mitigates/fixes the dreaded `error: FileNotFound` which we sometimes see when setting up Bazel worksace. Relevant commit in Zig: ziglang/zig@6b27096 Fixes #133

test/link/glibc_compat/glibc_runtime_check.zig

rootbeer marked this pull request as ready for review October 25, 2023 04:14

rootbeer force-pushed the correct-libc-nonshared branch from 67c7237 to c565672 Compare October 25, 2023 04:15

andrewrk mentioned this pull request Oct 25, 2023

Synchronize what gets compiled into libc_nonshared.a with glibc #16970

Closed

andrewrk added this to the 0.12.0 milestone Oct 25, 2023

rootbeer force-pushed the correct-libc-nonshared branch from c565672 to 3550e35 Compare October 28, 2023 05:31

rootbeer force-pushed the correct-libc-nonshared branch 3 times, most recently from 8b10733 to 17fe336 Compare October 28, 2023 19:20

rootbeer force-pushed the correct-libc-nonshared branch 2 times, most recently from beafee3 to 313d1cc Compare October 30, 2023 18:39

kubkon reviewed Oct 30, 2023

View reviewed changes

kubkon requested changes Oct 30, 2023

View reviewed changes

test/link/glibc_compat/build.zig Outdated Show resolved Hide resolved

test/link/glibc_compat/build.zig Outdated Show resolved Hide resolved

test/link/glibc_compat/glibc_runtime_check.zig Outdated Show resolved Hide resolved

rootbeer force-pushed the correct-libc-nonshared branch from 313d1cc to 1f2f1c8 Compare October 31, 2023 02:46

rootbeer force-pushed the correct-libc-nonshared branch 2 times, most recently from 3482682 to 4a73e20 Compare November 8, 2023 23:43

rootbeer force-pushed the correct-libc-nonshared branch from 4a73e20 to 112d326 Compare November 15, 2023 04:39

rootbeer force-pushed the correct-libc-nonshared branch 2 times, most recently from 6b3d80c to dd18830 Compare December 4, 2023 18:54

rootbeer force-pushed the correct-libc-nonshared branch 2 times, most recently from db8487a to d23b2bf Compare December 14, 2023 07:04

motiejus mentioned this pull request Dec 19, 2023

lstat error does not set errno uber/hermetic_cc_toolchain#133

Closed

rootbeer force-pushed the correct-libc-nonshared branch from d23b2bf to fcd78ba Compare December 26, 2023 18:05

rootbeer force-pushed the correct-libc-nonshared branch from fcd78ba to 8ca5d8a Compare January 3, 2024 04:04

lifthrasiir and others added 10 commits January 4, 2024 17:12

glibc: remove unused stat-related files for 2.33+

f1bd218

Effectively reverts 3dcd361.

glibc: restore "weak_hidden_alias" macro for older glibc versions

2c6372a

The fstat,lstat,stat,mknod stubs used to build older (before v2.33) glibc versions depend on the weak_hidden_alias macro. It was removed from the glibc libc-symbols header, so patch it back in for the older builds.

std/fs/test.zig: Remove work-around for stat() failures on glibc

1564cb0

glibc variants now support the stat-family of calls correctly, so this test is safe to include.

test/link/glibc_compat: test various older glibc versions

42d7b69

Compile, link and run a test case against various glibc versions. Exercise symbols that have been probelmatic in the past.

lib/libc/glibc/: Add README.md

71e8098

Add a README with an overview of how Zig's glibc support is implemented.

glibc: strlcpy and strlcat arrived in glibc 2.38

4a5d73a

So only expose these in generic-glibc/string.h if Zig is building a v2.38 (or later) glibc stub. Announcement of 2.38 that notes strlcpy and strlcat: https://lists.gnu.org/archive/html/info-gnu/2023-07/msg00010.html

test glibc_runtime_check: add strlcpy() check

cbaaf64

The strlcpy symbol was added in v2.38, so this is a handy symbol for creating binaries that won't run on relatively modern systems (e.g., mine, that has glibc 2.36 installed).

minor cosmetic fixups

362460e

* fix typos and redundancies in docs * use Target.isGnuLibc

andrewrk force-pushed the correct-libc-nonshared branch from 8ca5d8a to 362460e Compare January 5, 2024 00:28

andrewrk merged commit 4583667 into ziglang:master Jan 5, 2024
0 of 10 checks passed

squeek502 mentioned this pull request Jan 5, 2024

std.fs: tests for makePath and symlinks #17499

Merged

motiejus mentioned this pull request Jan 5, 2024

zig: upgrade to 0.12.0-dev.2043+6ebeb85ab uber/hermetic_cc_toolchain#140

Merged

rootbeer mentioned this pull request Mar 16, 2024

can't get errno when libc lstat returns -1 #11878

Closed

daurnimator reviewed Apr 29, 2024

View reviewed changes

test/link/glibc_compat/glibc_runtime_check.zig Show resolved Hide resolved

rootbeer deleted the correct-libc-nonshared branch April 29, 2024 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix libc_nonshared.a #17702

Fix libc_nonshared.a #17702

rootbeer commented Oct 24, 2023

rootbeer commented Oct 25, 2023

squeek502 commented Oct 25, 2023

rootbeer commented Oct 25, 2023

rootbeer commented Oct 25, 2023

lifthrasiir commented Oct 25, 2023 •

edited

Loading

squeek502 commented Oct 25, 2023 •

edited

Loading

rootbeer commented Oct 25, 2023

rootbeer commented Oct 25, 2023

andrewrk commented Oct 27, 2023

rootbeer commented Oct 28, 2023

rootbeer commented Oct 29, 2023

rootbeer commented Oct 30, 2023

kubkon Oct 30, 2023

rootbeer Oct 31, 2023

andrewrk Nov 24, 2023

rootbeer Nov 28, 2023

kubkon left a comment

rootbeer commented Oct 31, 2023

rootbeer commented Nov 15, 2023

rootbeer commented Nov 29, 2023 •

edited

Loading

andrewrk commented Jan 5, 2024

Fix libc_nonshared.a #17702

Fix libc_nonshared.a #17702

Conversation

rootbeer commented Oct 24, 2023

rootbeer commented Oct 25, 2023

squeek502 commented Oct 25, 2023

rootbeer commented Oct 25, 2023

rootbeer commented Oct 25, 2023

lifthrasiir commented Oct 25, 2023 • edited Loading

squeek502 commented Oct 25, 2023 • edited Loading

rootbeer commented Oct 25, 2023

rootbeer commented Oct 25, 2023

andrewrk commented Oct 27, 2023

rootbeer commented Oct 28, 2023

rootbeer commented Oct 29, 2023

rootbeer commented Oct 30, 2023

kubkon Oct 30, 2023

Choose a reason for hiding this comment

rootbeer Oct 31, 2023

Choose a reason for hiding this comment

andrewrk Nov 24, 2023

Choose a reason for hiding this comment

rootbeer Nov 28, 2023

Choose a reason for hiding this comment

kubkon left a comment

Choose a reason for hiding this comment

rootbeer commented Oct 31, 2023

rootbeer commented Nov 15, 2023

rootbeer commented Nov 29, 2023 • edited Loading

andrewrk commented Jan 5, 2024

lifthrasiir commented Oct 25, 2023 •

edited

Loading

squeek502 commented Oct 25, 2023 •

edited

Loading

rootbeer commented Nov 29, 2023 •

edited

Loading