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

x86_64-netbsd-none hacks #60

Closed
wants to merge 6 commits into from
Closed

x86_64-netbsd-none hacks #60

wants to merge 6 commits into from

Conversation

lun-4
Copy link

@lun-4 lun-4 commented Jun 7, 2021

The changes here is the result of me attempting to run zig-bootstrap in a NetBSD x86_64 environment, targeting NetBSD x86_64 (so, building LLVM and Zig for the host system, and then cross-compiling LLVM and Zig to the same host system). Running, finding errors, fixing, and repeating step 1.

This should not be merged (hence why I'm starting with a draft PR).

There are comments on commits explaining the build issues that created those fixes. At a high level:

  • Shell best practices. set -u to prevent typos blowing up, and set -x to aid debugging to see which variables are being passed to the commands.
  • Pass on that we are on NetBSD to cmake via the $TARGET_OS_CMAKE mapping.
  • Use clang instead of gcc. -mcpu=baseline is not supported in gcc (tested in gcc7, shipped by netbsd, and gcc10, from my linux system).
    • Should not be merged as the README declares that GCC should work: "C++ compiler capable of building LLVM, Clang, and LLD from source (GCC 5.1+)"
  • Add sincos_netbsd_compat.c to LLVM sourcecode. LLVM mistakengly optimizes sin(x); cos(x); to sincos(x) on a target that does not have it (glibc, musl, freebsd have it, netbsd does not).
    • Disabling the AMDGPU backend to fix it (which is where the undefined symbol: sincos error comes from) does not work, as Zig requires all of the LLVM default targets to be enabled.
    • Should not be merged as this would break the platforms that have sincos due to duplicate symbols? I haven't tested it. Maybe this could be added only for NetBSD? I'm not sure how CMake works.
  • Let the final make "$JOBS" install step have the correct CC and CXX variables. This reverts the second half of dc5d646.
    • Sadly, I no longer remember why I did this. It's possible that it would still work, as the configure step has the variables?

lun-4 added 6 commits June 7, 2021 00:20
this is required for NetBSD as a code path in the AMDGPU backend is
optimized to sincos(x), which is not provided by NetBSD libc.

a compatibility wrapper file provides the missing symbol.
this is inspired by the proposed solution in ziglang/zig#8973

cmake was not accepting the rest of the arguments for CC and CXX, those
wrapper scripts take care of that.

changes beyond the script:
 - use CC=clang because -mcpu=baseline is not accepted by gcc.
 - remove '-target' as i'm targeting native,
    but it should be brought back, this happened before the CC=clang change.
somehow the $CC variable was missing by the time the install lib step
was running. force it back to make it work.
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Thanks for opening this. I want to address some of the underlying issues upstream, rather than merging the workarounds here.

Comment on lines +59 to +68
export CC="$ZIG cc -fno-sanitize=all -mcpu=$MCPU"
export CXX="$ZIG c++ -fno-sanitize=all -mcpu=$MCPU"
echo "#!/bin/sh
env CC=\"clang\" $CC \"\$@\"" > $ROOTDIR/out/host/bin/zigcc
echo "#!/bin/sh
env CC=\"clang\" $CXX \"\$@\"" > $ROOTDIR/out/host/bin/zigcxx
chmod +x $ROOTDIR/out/host/bin/zigcc
chmod +x $ROOTDIR/out/host/bin/zigcxx
export CC="$ROOTDIR/out/host/bin/zigcc"
export CXX="$ROOTDIR/out/host/bin/zigcxx"
Copy link
Member

Choose a reason for hiding this comment

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

I believe this will be solved by ziglang/zig#8960

@@ -144,6 +144,7 @@ add_llvm_target(AMDGPUCodeGen
GCNNSAReassign.cpp
GCNDPPCombine.cpp
SIModeRegister.cpp
sincos_netbsd_compat.c
Copy link
Member

Choose a reason for hiding this comment

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

Interesting... I'll have to look into this

@LemonBoy
Copy link
Contributor

LemonBoy commented Jun 9, 2021

LLVM mistakengly optimizes sin(x); cos(x); to sincos(x) on a target that does not have it (glibc, musl, freebsd have it, netbsd does not).

sincos is a GNU extension that's only used by LLVM if the target ends with -gnu, I suspect something's passing the wrong triple to clang (getting the $CC -v -### output may help here).

@lun-4
Copy link
Author

lun-4 commented Jun 12, 2021

I suspect something's passing the wrong triple to clang (getting the $CC -v -### output may help here).

I'm still due to validate this but my hypothesis is that zig, by identifying netbsd's ABI as gnu (zig targets gives x86_64-netbsd9.2...9.2-gnu as the native target triple), passes that to clang in zig cc. My evidence is that the first build in zig-bootstrap, which is LLVM, except built for the host, worked without problems. The sincos error appared while attempting to cross-compile LLVM to the same target. I believe the fix would be adding -target $TARGET back in the zig cc wrappers, as before, so that it can use none as ABI.

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.

3 participants