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: dont inconditionnally include glibc #44538

Closed
wants to merge 2 commits into from
Closed

cc-wrapper: dont inconditionnally include glibc #44538

wants to merge 2 commits into from

Conversation

teto
Copy link
Member

@teto teto commented Aug 6, 2018

... headers even when using -nostdinc or -nolibc (clang attribute)

Motivation for this change

Even with nostdinc/nolibc flags wrapper-cc will look in glibc include directories see #44530

Upon compiling glibcLocales, I have a problem though:

checking for alloca... yes
checking for stdlib.h... (cached) yes
checking whether the C++ compiler supports thread_local... no
running configure fragment for sysdeps/unix/sysv/linux/x86_64/64
running configure fragment for sysdeps/unix/sysv/linux/x86_64
running configure fragment for sysdeps/unix/sysv/linux
checking for unistd.h... (cached) yes
checking installed Linux kernel header files... missing or too old!
configure: error: GNU libc requires kernel header files from
Linux 3.2.0 or later to be installed before configuring.
The kernel header files are found usually in /usr/include/asm and
/usr/include/linux; make sure these directories use files from
Linux 3.2.0 or later.  This check uses <linux/version.h>, so
make sure that file was built correctly when installing the kernel header
files.  To use kernel headers not from /usr/include/linux, use the
configure option --with-headers.
gcc -I.. -I../.. -I../modes -I../asn1 -I../evp -I../../include  -fPIC -DOPENSSL_PIC -DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN -DHAVE_DLFCN_H -Wa,--noexecstack -m64 -DL_ENDIAN -O3 -Wall -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DRC4_ASM -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DWHIRLPOOL_ASM -DGHASH_ASM -DECP_NISTZ256_ASM   -c -o e_aes.o e_aes.c
checking for sys/param.h... yes
note: keeping build directory '/tmp/nix-build-glibc-locales-2.27.drv-0'
builder for '/nix/store/a9n3c7vxj95jp1l8qzfgh5zcn6vxmn71-glibc-locales-2.27.drv' failed with exit code 1
note: keeping build directory '/tmp/nix-build-openssl-1.0.2o.drv-0'
note: keeping build directory '/tmp/nix-build-ghc-8.2.1-binary.drv-0'
cannot build derivation '/nix/store/hhvvpx581bvxsng70l7c8xzqscsgdwri-ShellCheck-0.5.0.drv': 1 dependencies couldn't be built
note: keeping build directory '/tmp/nix-build-bash-interactive-4.4-p23.drv-0'
error: build of '/nix/store/a4122g7iwdy52qr1qb33pvsa3gdhnbjr-bash-interactive-4.4-p23.drv', '/nix/store/hhvvpx581bvxsng70l7c8xzqscsgdwri-ShellCheck-0.5.0.drv' failed

I tried in a nix-shell with the same result

configure flags: --disable-static --prefix=/nix/store/64x91npllljy88insldhgvdnlmh36dda-glibc-locales-2.27 --prefix=/nix/store/64x91npllljy88insldhgvdnlmh36dda-glibc-locales-2.27 --disable-static --prefix=/nix/store/64x91npllljy88insldhgvdnlmh36dda-glibc-locales-2.27 -C --enable-add-ons --enable-obsolete-nsl --enable-obsolete-rpc --sysconfdir=/etc --enable-stackguard-randomization --without-headers --disable-profile
...
running configure fragment for sysdeps/unix/sysv/linux
checking installed Linux kernel header files... missing or too old!
configure: error: GNU libc requires kernel header files from
Linux 3.2.0 or later to be installed before configuring.
The kernel header files are found usually in /usr/include/asm and
/usr/include/linux; make sure these directories use files from
Linux 3.2.0 or later.  This check uses <linux/version.h>, so
make sure that file was built correctly when installing the kernel header
files.  To use kernel headers not from /usr/include/linux, use the
configure option --with-headers.

Looking at the log, it seems that glibc compiles with nostdinc

configure:28: gcc -c -g -O2  -nostdinc -isystem /nix/store/qqcha3j759fl1wj840z0b0hdhkbg061a-gcc-7.3.0/lib/gcc/x86_64-unknown-linux-gnu/7.3.0/include -isystem /nix/store/qqcha3j759fl1wj840z0b0hdhkbg061a-gcc-7.3.0/lib/gcc/x86_64-unknown-linux-gnu/7.3.0/include-fixed -isystem no conftest.c >&5
conftest.c:25:10: fatal error: linux/version.h: No such file or directory
 #include <linux/version.h>
          ^~~~~~~~~~~~~~~~~

and that my change make it really effective thus calling configure --without-headers fail ?
My guess is that glibc should always be compiled with withLinuxHeaders to prevent this (which is not the case for glibLocales) but I could be wrong.

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@teto
cc-wrapper: dont inconditionnally include glibc
da6e08b 
... headers even when using -nostdinc or -nolibc (clang attribute)
@teto teto requested a review from Ericson2314 as a code owner August 6, 2018 07:08
@teto
Copy link
Member Author

teto commented Aug 6, 2018

That seems to be it. Swapping the default to true at

nixpkgs/pkgs/development/libraries/glibc/common.nix

Line 30 in 6820e2f

, withLinuxHeaders ? false
fixes it (still compiling though)

@oxij
Copy link
Member

oxij commented Aug 6, 2018

I'm pretty sure this needs to be rebased against staging in any case.

@teto
Copy link
Member Author

teto commented Aug 6, 2018

libc has disappeared from the include folder so it seems a success in that regard but when using clang, I get a /nix/store/XXX-compiler-rt thing in the include pathsm not sure what it is/if it is LGTM (shouldn't imho) so the fix might not be complete.
Anyway what about always defaulting withLinuxHeaders to true when compiling glibc. Seems to solve a real problem but I fear that can be a problem on non-linux distribs.
I will rebase on staging at some point.

@Ericson2314 Ericson2314 changed the base branch from master to staging August 6, 2018 19:17
@Ericson2314
Copy link
Member

Ericson2314 commented Aug 6, 2018 •
edited

The cc-wrapper change seems fine. Not sure what's going on with the staging change however. BTW check out stdenvNoLibs which might also help.

@teto
Copy link
Member Author

teto commented Aug 10, 2018

I realized I did not need -nostdinc after all and as this kind of PRs are really time consuming since I don't have my custom hydra, I'll leave it on hold and hope someone picks it up.
It might be more elegant to export from add_flags.sh NIX_@infixSalt@_CFLAGS_LIBC and then decide in wrapper-cc.sh wether to add libc flags to NIX_@infixSalt@_ CFLAGS_COMPILE.

The call `source add-flags.sh $param1 $param2" is also bash specific (not available in sh).

@dezgeg
Copy link
Contributor

dezgeg commented Aug 10, 2018

Huh, good catch, I am sort of surprised that this hasn't caused problems previously with kernel builds or U-Boot/other firmwarey things.

But yes, if there haven't been problems in practice let's have this on hold until 18.09 is out.

@teto
Copy link
Member Author

teto commented Aug 13, 2018

actually I believe this is the 2nd time I experienced this before another fix. Could that make sense to write a test for it (pass -stdinc -v and grep -n for include folders) ? where should it go ?

@dezgeg
Copy link
Contributor

dezgeg commented Aug 13, 2018

Rather than grepping for implementation details, I'd just test behaviour instead.

I.e. with -nostdlib compiling a file with #include <stdio.h> should fail. With -nostdlib you should be able to compile and link a program which declares void _start() { ... } without getting a multiple definition of '_start'error. And so on.

This can go under pkgs/test/no-standard-files/ or something like that.

@mmahut
Copy link
Member

mmahut commented Aug 19, 2019

Are there any updates on this pull request, please?

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@teto
Copy link
Member Author

teto commented Jul 1, 2020

closing in favor of #91974

@teto teto closed this Jul 1, 2020
Mic92
Mic92 reviewed Jul 1, 2020
View reviewed changes
pkgs/build-support/cc-wrapper/cc-wrapper.sh
@@ -61,6 +52,8 @@ while (( "$n" < "$nParams" )); do
isCpp=1
elif [ "$p" = -nostdlib ]; then
isCpp=-1
elif [ "$p" = -nolibc ]; then
Copy link
Member

Choose a reason for hiding this comment

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

I have not checked this flag yet, but I give it a go in my PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

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

Assignees
No one assigned
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 501+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux-stdenv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@teto @oxij @Ericson2314 @dezgeg @mmahut @Mic92 @GrahamcOfBorg