-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Robust bindgen support #140
Conversation
Whoops... I realized that this approach alone won't work in general because bindgen will still look at older clang anyway (which we can't currently change). So if you have a recent enough clang (I believe, 13 or later), this would indeed work, but this PR is still not complete yet. |
It'd be great if we can merge a solution even if it only works for newer clang versions. |
log.txt |
Huh, this looks pretty bad indeed. So to summarize what had happened here, we are trying to build @TheButlah, can you post what the following command prints? It should say something like $ /Users/ryan.butler/Library/Caches/cargo-zigbuild/0.16.10/zigcc-aarch64-unknown-linux-gnu.sh -E -x c /dev/null -v |
Sure, here you go. I messed with some stuff (specifically installing and uninstalling cross-compilation toolchains, llvm, adding llvm to path, etc) since my original message (I think? I don't remember) and wanted both logs to be in sync, so I just reran the cargo zigbuild for the below logs. command you asked for All |
|
Thank you. Unfortunately all lines have been truncated past 83 columns so I can't easily see the exact commands zig has composed, but it seems clear that |
weird, sorry. Here is the un-truncated output on the not-HEAD version of zig. I'll post the HEAD version shortly. Edit: |
The previous approach to use `--sysroot` was pretty much incomplete, as zig uses multiple include directories to reduce the bundle size. This commit implements a future-proof strategy to determine correct compile options by using `zig cc` itself, which is also what bindgen (transitively via clang-sys) does internally. Also fixes a couple of minor issues: - Handles all three kinds of `BINDGEN_EXTRA_CLANG_ARGS` environment variable correctly (one of them was missing previously). - Existing `BINDGEN_EXTRA_CLANG_ARGS` no longer blocks bindgen support. Collected options are instead appended to them.
Zstd-rs didn't include enough headers and the incomplete bindgen support was simply overlooked. For example, `stdlib.h` didn't work.
Bindgen should have accepted a correct TARGET environment variable, so it makes sense to set only target-specific environment variables. This also fixes a small bug when only the pan-target variable is set, in which case the newly set target-specific variable should first inherit the pan-target value and then more options should have been appended, but the pan-target value was discarded previously.
Only the master version of Zig recognizes it, all existing releases don't and adds proper include directories only when a file with a corresponding extension is added. Note: this will write additional empty files to the cache directory, which should be writable (otherwise everything else will fail anyway). Since the cache directory may end up being cwd, this commit tried to use a specific enough file name to avoid collision.
This is specific to Zig because while Zig ships with the recent enough glibc it dynamically sets `__GLIBC_MINOR__` according to the target. At this point it seems more robust to collect *all* options from `zig cc` output, but this is not as viable because all we can see is cc1 options (`-###` or `-v`), not clang driver options from Zig. It seems that we have no direct means to collect the latter, so we have to be conservative about which options to collect.
It turned out that unused argument warnings (`-Wunused-command-line- argument`) come from cc1, not from driver, and as long as all options are "claimed"---not necessarily used---it doesn't issue warnings. On the other hands, Zig internally adds `-nostdinc` but we didn't because we wanted header-only C++ libraries in the host to be usable, but it turned out that the presence of `__has_include` made this horribly fragile (e.g. some versions of libc++ used `__has_include( <pthread.h>)` to determine pthread support), so we gave up. If this facility is really desired, consider using `-idirafter /usr/include`.
The latter is also a recent addition, and `-nostdinc` already implies `nostdinc++` so we no longer need guarantees from `-stdlib++-isystem`. (We can't still replace duplicate `-c-isystem` and `-cxx-isystem` into a single `-isystem` due to the relocation.)
dce74cc
to
832e423
Compare
832e423
to
e5caa37
Compare
e5caa37
to
87f5092
Compare
This in particular affects `__has_include` new in C++17, which is also how the test internally works. Tested files are chosen so that there is no chance that other OSes have matching header files by accident.
87f5092
to
998830e
Compare
I'm now modestly confident about this PR, having passed the CI. Newer commits should work from all supported clang versions. I've also manually checked against 0.9.0 from my local machine (WSL Ubuntu 18.04 LTS, clang 10.0, glibc 2.27), additional verifications would be appreciated though. @TheButlah I think this PR should work now without any changes---I've now worked around 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.
Thanks! Looks great!
Seems to work! Thanks :3 |
It turned out that #74, which added bindgen support, was pretty much incomplete. For example the following C code will fail to compile during bindgen.
It should be noted that Zig contains multiple libc header files:
zig/lib/libc/glibc/include
etc. are used to build libc on demand. They are not meant to be directly included, and in fact contain a small subset of libc headers which are used by other libc subsystems. Consequently files in this directory will most likely fail to compile; this includesstdlib.h
,errno.h
,pthread.h
andsignal.h
.zig/lib/include
contains header files shared by all targets. They are mostly platform-specific intrinsics.zig/lib/libc/include/$TARGET
contains header files specific to given target. For given target there may be multiple such directories, and some directories are referenced from Zig but not present and ignored by clang driver.zig/lib/libcxx/include
andzig/lib/libcxxabi/include
are included before any other headers because libc++ itself contains some C headers that are not necessarily in libc (e.g.uchar.h
).Right now cargo-zigbuild only concerns with 1, so while some headers won't compile at all, other headers will inadvertently refer to the system library. This PR makes cargo-zigbuild to include 2 and 3, and if required, 4 as well. There are now explicit tests to include (almost) all standard C and C++ header files.
As noted from a lengthy comment that I don't really repeat here, the current strategy of overriding
BINDGEN_EXTRA_CLANG_ARGS
is not really ideal. I have considered alternative approaches, and unfortunately none of them would work out of the box. In the long term we may getclang-sys
fixed and then put target-specificzig cc
wrappers toPATH
so thatclang-sys
can infer a correct search path by itself, but it will take a long time for every one to use a fixedclang-sys
. So I've resorted to this hack for now.