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

Jump to bottom

cc-wrapper: make machine configuration configurable #59225

Merged
merged 1 commit into from Apr 21, 2019
Merged

cc-wrapper: make machine configuration configurable #59225

merged 1 commit into from Apr 21, 2019

Conversation

matthewbauer
Copy link
Member

It is useful to make these dynamic and not bake them into gcc. This
means we don’t have to rebuild gcc to change these values. Instead, we
will pass cflags to gcc based on platform values. This was already
done hackily for android gcc (which is multi-target), but not for our
own gccs which are single target.

To accomplish this, we need to add a few things:

  • add ‘arch’ to cpu
  • add NIX_CFLAGS_COMPILE_BEFORE flag (goes before args)
  • set -march everywhere
  • set mcpu, mfpu, mmode, and mtune based on targetPlatform.gcc flags
  • on arm32, set mfloat-abi based on targetPlatform.parsed.abi.float
Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@matthewbauer matthewbauer requested a review from nbp as a code owner April 9, 2019 18:27
@matthewbauer matthewbauer changed the title cc-wrapper: make machine configuration configurable [wip] cc-wrapper: make machine configuration configurable Apr 9, 2019
@matthewbauer matthewbauer changed the base branch from master to staging April 9, 2019 18:27
Ericson2314
Ericson2314 reviewed Apr 9, 2019
View reviewed changes
lib/systems/parse.nix
@@ -86,7 +86,7 @@ rec {
i486 = { bits = 32; significantByte = littleEndian; family = "x86"; };
i586 = { bits = 32; significantByte = littleEndian; family = "x86"; };
i686 = { bits = 32; significantByte = littleEndian; family = "x86"; };
x86_64 = { bits = 64; significantByte = littleEndian; family = "x86"; };
x86_64 = { bits = 64; significantByte = littleEndian; family = "x86"; arch = "x86-64"; };
Copy link
Member

Choose a reason for hiding this comment

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

https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html I think GCC also wants armv7-a etc.

Ericson2314
Ericson2314 approved these changes Apr 9, 2019
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

This is a really great change! Just add those arm arch entries, and let's ship!

@Ericson2314
Copy link
Member

@matthewbauer perhaps we could make a single list go in the file, rather than a bunch of >> statements. That would solve @volth's need for arbitrary extraFlags.

@GrahamcOfBorg GrahamcOfBorg added the 6.topic: stdenv Standard environment label Apr 10, 2019
@alyssais
Copy link
Member

Think this needs a rebase :)

@matthewbauer matthewbauer changed the title [wip] cc-wrapper: make machine configuration configurable cc-wrapper: make machine configuration configurable Apr 18, 2019
@matthewbauer
Copy link
Member Author

Nope I am still working on it. It's just takes a long time to test things out.

@GrahamcOfBorg GrahamcOfBorg removed 6.topic: nixos 6.topic: pantheon The Pantheon desktop environment labels Apr 19, 2019
@matthewbauer matthewbauer force-pushed the dynamic-gcc-config branch 2 times, most recently from f0f46b8 to f6bf5ae Compare April 19, 2019 18:47
@matthewbauer
Copy link
Member Author

@volth for reference here is the issue I am getting with armv7l-hf-multiplatform:

  /nix/store/i7q6m9xyny1r98k5lzd1s7bpzjnnsya9-armv7l-unknown-linux-gnueabihf-binutils-2.31.1/bin/armv7l-unknown-linux-gnueabihf-ld: error: /build/build/elf/librtld.map.o uses VFP register arguments, /nix/store/w1d8rcsfcvawrn7dhj60yhlw3pw6lh45-armv7l-unknown-linux-gnueabihf-stage-static-gcc-debug-7.4.0/lib/gcc/armv7l-unknown-linux-gnueabihf/7.4.0/libgcc.a(_udivmoddi4.o) does not
  /nix/store/i7q6m9xyny1r98k5lzd1s7bpzjnnsya9-armv7l-unknown-linux-gnueabihf-binutils-2.31.1/bin/armv7l-unknown-linux-gnueabihf-ld: failed to merge target specific data of file /nix/store/w1d8rcsfcvawrn7dhj60yhlw3pw6lh45-armv7l-unknown-linux-gnueabihf-stage-static-gcc-debug-7.4.0/lib/gcc/armv7l-unknown-linux-gnueabihf/7.4.0/libgcc.a(_udivmoddi4.o)

This looks bad - it should be using hard floats here but somehow libgcc is built without them!

@matthewbauer
cc-wrapper: make machine configuration configurable
d180cb9 
It is useful to make these dynamic and not bake them into gcc. This
means we don’t have to rebuild gcc to change these values. Instead, we
will pass cflags to gcc based on platform values. This was already
done hackily for android gcc (which is multi-target), but not for our
own gccs which are single target.

To accomplish this, we need to add a few things:

- add ‘arch’ to cpu
- add NIX_CFLAGS_COMPILE_BEFORE flag (goes before args)
- set -march everywhere
- set mcpu, mfpu, mmode, and mtune based on targetPlatform.gcc flags

cc-wrapper: only set -march when it is in the cpu type

Some architectures don’t have a good mapping of -march. For instance
POWER architecture doesn’t support the -march flag at all!

https://gcc.gnu.org/onlinedocs/gcc/RS_002f6000-and-PowerPC-Options.html#RS_002f6000-and-PowerPC-Options
@matthewbauer
Copy link
Member Author

It looks like those platform flags are really needed for single lib gcc. Adding the --with- flags back in seems to fix it. We wouldn't have this problem if only libgcc was built separately from gcc!

@matthewbauer matthewbauer merged commit 7f3fed9 into NixOS:staging Apr 21, 2019
@matthewbauer
Copy link
Member Author

Ok that's annoying

@matthewbauer
Copy link
Member Author

We could either revert this pr or remove the "or" case here:

https://github.com/NixOS/nixpkgs/blob/staging/pkgs/build-support/cc-wrapper/default.nix#L323

to prevent this from breaking things

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 24, 2019 •
edited

Yes if we could make cc-wrapper remove conflicting flags from *before (but not generally!), that sounds correct to me.

matthewbauer pushed a commit that referenced this pull request Apr 25, 2019
@matthewbauer
kernel: fix cross-build
dba65df 
Fix for #59225 regression proposed by @matthewbauer
#59225 (comment)

(cherry picked from commit a6ea72a)
@vcunat vcunat mentioned this pull request Jun 26, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@nbp nbp Awaiting requested review from nbp

@alyssais alyssais Awaiting requested review from alyssais alyssais is a code owner

@FRidh FRidh Awaiting requested review from FRidh

@infinisil infinisil Awaiting requested review from infinisil

@zimbatm zimbatm Awaiting requested review from zimbatm

Assignees
No one assigned
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@matthewbauer @Ericson2314 @alyssais @GrahamcOfBorg