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

Misc fixes for armv6 bare metal cross #82882

Merged
merged 7 commits into from Mar 19, 2020

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Mar 18, 2020

Motivation for this change

Best to review commit by commit.

The thing that makes this novel is it's doing cross compilation with clang without useLLVM = true, which forces us for the first time to make the ugly clang + gcc hacks work with cross.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

matthewbauer and others added 6 commits March 12, 2020 02:22
newlib is the default for most tools when no kernel is provided. Other
exist, but this seems like a safe default.

(cherry picked from commit 8009c20)
Sometimes it is useful for it to be slightly different. Going forward we
should, however, try to make this fallback rarely needed.
We only want to refer to GCC under these conditions.
Lumping it in with the target platform libraries was incorrect, and
caused eval failures when gcc couldn't be built for the target platform.
 - Cross to bare metal with GCC works

 - Flags are deduplicated

 - Darwin bootstrapping for 8 and 0 closer.

 - Flags are same across versions.
@@ -23,7 +23,7 @@ let
ln -s "${cc}/lib/clang/${release_version}/include" "$rsrc"
ln -s "${targetLlvmLibraries.compiler-rt.out}/lib" "$rsrc/lib"
echo "-resource-dir=$rsrc" >> $out/nix-support/cc-cflags
'' + stdenv.lib.optionalString stdenv.targetPlatform.isLinux ''
'' + stdenv.lib.optionalString (stdenv.targetPlatform.isLinux && tools.clang-unwrapped ? gcc && !(stdenv.targetPlatform.useLLVM or false)) ''
Copy link
Member

Choose a reason for hiding this comment

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

I still don't really understand the use for clang.gcc if clang was built using gcc isn't it equivalent to stdenv.cc.cc? It would be nice not to have that conditional attribute IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is gross, but clang.gcc is not necessary the one used to build clang. clang-unwrapped is target-agnostic, but the gcc appears in its passthru based on the target.

It's all stupid, but at least it is consistent across LLVM versions now. The right thing to do is always build libstdc++ and gcc separately.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that would improve multiple things, anyway this was more of a question than a review comment.

@Ericson2314 Ericson2314 merged commit 19a0b38 into NixOS:master Mar 19, 2020
@Ericson2314 Ericson2314 mentioned this pull request Mar 19, 2020
10 tasks
@Ericson2314 Ericson2314 deleted the armv6-embedded branch March 19, 2020 17:14
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Mar 19, 2020
…llvm10?)

Misc fixes for armv6 bare metal cross

(cherry picked from commit 19a0b38)
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Mar 19, 2020
…edded (+llvm10?)" (revisit rebuilds)

This reverts commit 991f0a5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants