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

GCC has unwanted flags #18995

Closed
andrewrk opened this Issue Sep 27, 2016 · 60 comments

Comments

@andrewrk
Contributor

andrewrk commented Sep 27, 2016

andy@xps:~/tmp$ NIX_DEBUG=true gcc
(some good flags for dealing with /nix/store/* and then...)
  -O2
  -D_FORTIFY_SOURCE=2
  -fstack-protector-strong
  --param
  ssp-buffer-size=4
  -fPIC
  -fno-strict-overflow
  -Wformat
  -Wformat-security
  -Werror=format-security

These flags should be enabled when compiling nix packages, but they should not be always on for general compiler use.

This has caused a broken debugging experience for me on the project I was working on.

Most projects have a "debug build" that has optimizations off, and this prevents that from working correctly.

NixOS version 16.09pre90254.6b20d5b (Flounder)

@andrewrk

This comment has been minimized.

Show comment
Hide comment
@andrewrk

andrewrk Sep 27, 2016

Contributor

I believe this was caused by #12895

Contributor

andrewrk commented Sep 27, 2016

I believe this was caused by #12895

@andrewrk

This comment has been minimized.

Show comment
Hide comment
@andrewrk

andrewrk Sep 27, 2016

Contributor

Not only that, but these flags are in the extraAfter section, which means I can't even override the flags, for example with -Og in a Makefile or build script.

Contributor

andrewrk commented Sep 27, 2016

Not only that, but these flags are in the extraAfter section, which means I can't even override the flags, for example with -Og in a Makefile or build script.

@andrewrk

This comment has been minimized.

Show comment
Hide comment
@andrewrk

andrewrk Sep 27, 2016

Contributor

One workaround is putting

    hardeningDisable = [ "fortify" ];

in a nix-shell. Since I was already using a nix-shell for my environment, this is an acceptable workaround. Still, seems weird to assert -O2 by default and have to disable it to get previously expected behavior.

Contributor

andrewrk commented Sep 27, 2016

One workaround is putting

    hardeningDisable = [ "fortify" ];

in a nix-shell. Since I was already using a nix-shell for my environment, this is an acceptable workaround. Still, seems weird to assert -O2 by default and have to disable it to get previously expected behavior.

@abbradar

This comment has been minimized.

Show comment
Hide comment
@abbradar

abbradar Sep 27, 2016

Member

I think this can be fixed by adding a special flag, NIX_ENFORCE_HARDENING, that is only set in stdenvs. This way the flags won't be enforced in regular gcc invocations and could be disabled in shells by setting this flag to 0.

BTW having -O2 always added is not nice because some applications might want -O3 or -Ofast (which would be overridden). We may want to make this conditional somehow...

Member

abbradar commented Sep 27, 2016

I think this can be fixed by adding a special flag, NIX_ENFORCE_HARDENING, that is only set in stdenvs. This way the flags won't be enforced in regular gcc invocations and could be disabled in shells by setting this flag to 0.

BTW having -O2 always added is not nice because some applications might want -O3 or -Ofast (which would be overridden). We may want to make this conditional somehow...

@rasendubi

This comment has been minimized.

Show comment
Hide comment
@rasendubi
Member

rasendubi commented Sep 27, 2016

@fpletz

This comment has been minimized.

Show comment
Hide comment
@fpletz

fpletz Sep 27, 2016

Member

You can use

hardeningDisable = [ "all" ];

to disable all hardening flags. This should also work if set as an environment variable.

We will work on a solution after 16.09 using gcc spec files that can detect for example if libraries are built or debugging is enabled. This was just our first iteration and we agree that it is not perfect. Changes to the cc-wrapper always require a full rebuild, which is very painful. More feedback on #12895 would've been helpful.

We didn't anticipate and test that those flags would be propagated to regular gcc invocations outside of nix builds. Not entirely sure how to fix it yet but @abbradar's proposal sounds reasonable. I will look into it after 16.09.

-O2 is required for -D_FORTIFY_SOURCE=2. Not sure though if higher optimizations also work.

Member

fpletz commented Sep 27, 2016

You can use

hardeningDisable = [ "all" ];

to disable all hardening flags. This should also work if set as an environment variable.

We will work on a solution after 16.09 using gcc spec files that can detect for example if libraries are built or debugging is enabled. This was just our first iteration and we agree that it is not perfect. Changes to the cc-wrapper always require a full rebuild, which is very painful. More feedback on #12895 would've been helpful.

We didn't anticipate and test that those flags would be propagated to regular gcc invocations outside of nix builds. Not entirely sure how to fix it yet but @abbradar's proposal sounds reasonable. I will look into it after 16.09.

-O2 is required for -D_FORTIFY_SOURCE=2. Not sure though if higher optimizations also work.

@abbradar

This comment has been minimized.

Show comment
Hide comment
@abbradar

abbradar Sep 27, 2016

Member

FWIW I found that level 1 fortifying requires inlining:

As I said in that post, the special fortified functions (those that are available in the form of __$func_chk in the libc.so file and provide warnings at build time, and proper stack traces at runtime) only get enabled if inline functions are enabled, so are totally ignored at -O0 (simply disabling inlines, by using -fno-inline won’t stop them from being used, though).

I haven't been able to find anything except of anecdotal evidence that -O2 is required for level 2 (I'm not questioning your choice, rather I was trying to find a list of optimizations that are required so that we can enable them individually).

Member

abbradar commented Sep 27, 2016

FWIW I found that level 1 fortifying requires inlining:

As I said in that post, the special fortified functions (those that are available in the form of __$func_chk in the libc.so file and provide warnings at build time, and proper stack traces at runtime) only get enabled if inline functions are enabled, so are totally ignored at -O0 (simply disabling inlines, by using -fno-inline won’t stop them from being used, though).

I haven't been able to find anything except of anecdotal evidence that -O2 is required for level 2 (I'm not questioning your choice, rather I was trying to find a list of optimizations that are required so that we can enable them individually).

@fpletz

This comment has been minimized.

Show comment
Hide comment
@fpletz

fpletz Sep 27, 2016

Member

(I'm not questioning your choice, rather I was trying to find a list of optimizations that are required so that we can enable them individually).

I'm always open for suggestions. Thanks! 😃 We'll have to try that.

Member

fpletz commented Sep 27, 2016

(I'm not questioning your choice, rather I was trying to find a list of optimizations that are required so that we can enable them individually).

I'm always open for suggestions. Thanks! 😃 We'll have to try that.

@abbradar

This comment has been minimized.

Show comment
Hide comment
@abbradar

abbradar Sep 27, 2016

Member

Unfortunately it may need peeking into rat nest GCC/glibc's codebases to determine this. Meanwhile I think it's okay to leave -O2.

Member

abbradar commented Sep 27, 2016

Unfortunately it may need peeking into rat nest GCC/glibc's codebases to determine this. Meanwhile I think it's okay to leave -O2.

@copumpkin

This comment has been minimized.

Show comment
Hide comment
@copumpkin

copumpkin Sep 27, 2016

Member

Just as a quick reminder when you talk about gcc: there's also clang to consider on Darwin (and in some cases on Linux too)

Member

copumpkin commented Sep 27, 2016

Just as a quick reminder when you talk about gcc: there's also clang to consider on Darwin (and in some cases on Linux too)

@jxy

This comment has been minimized.

Show comment
Hide comment
@jxy

jxy Sep 29, 2016

So this is why recently gdb suddenly tells me different things in my code and gcc no longer does a good job optimizing my code. I almost thought gcc 5.4 had optimization regressions.

How do I get back a reasonable working gcc? Which nixpkgs version should I revert to?

Does this mean I should put hardeningDisable = [ "all" ]; in all of my custom nix files for my HPC production code, eg, mpich3?

I'm using nixpkgs on a ubuntu without root privilege.

It is NOT okay to leave -Oanything in extraAfter!

jxy commented Sep 29, 2016

So this is why recently gdb suddenly tells me different things in my code and gcc no longer does a good job optimizing my code. I almost thought gcc 5.4 had optimization regressions.

How do I get back a reasonable working gcc? Which nixpkgs version should I revert to?

Does this mean I should put hardeningDisable = [ "all" ]; in all of my custom nix files for my HPC production code, eg, mpich3?

I'm using nixpkgs on a ubuntu without root privilege.

It is NOT okay to leave -Oanything in extraAfter!

@andrewrk

This comment has been minimized.

Show comment
Hide comment
@andrewrk

andrewrk Sep 29, 2016

Contributor

We're all in agreement here @jxy and going to fix it soon. This is why it's called unstable!

The workaround you mentioned is working for me and is probably the reasonable thing to do until this is fixed.

Contributor

andrewrk commented Sep 29, 2016

We're all in agreement here @jxy and going to fix it soon. This is why it's called unstable!

The workaround you mentioned is working for me and is probably the reasonable thing to do until this is fixed.

@jxy

This comment has been minimized.

Show comment
Hide comment
@jxy

jxy Sep 29, 2016

Right, thanks for the workaround. It wasn't easy to find this thread. I guess I'll need some performance regression tests after changing my system.

jxy commented Sep 29, 2016

Right, thanks for the workaround. It wasn't easy to find this thread. I guess I'll need some performance regression tests after changing my system.

@jxy

This comment has been minimized.

Show comment
Hide comment
@jxy

jxy Oct 3, 2016

Does NixOS 16.09 release version have this issue? If so, I'll put off upgrading until this is fixed.

jxy commented Oct 3, 2016

Does NixOS 16.09 release version have this issue? If so, I'll put off upgrading until this is fixed.

@joachifm joachifm referenced this issue Oct 7, 2016

Closed

RFC: Harden(ed) NixOS #7220

0 of 17 tasks complete

@fpletz fpletz added this to the 16.09 milestone Oct 10, 2016

@fpletz fpletz self-assigned this Oct 10, 2016

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Oct 11, 2016

Member

Yes, both 16.09 and unstable/master.

Member

vcunat commented Oct 11, 2016

Yes, both 16.09 and unstable/master.

@fpletz

This comment has been minimized.

Show comment
Hide comment
@fpletz

fpletz Oct 11, 2016

Member

I'm currently working on a fix.

Member

fpletz commented Oct 11, 2016

I'm currently working on a fix.

groxxda added a commit to groxxda/nixpkgs that referenced this issue Oct 13, 2016

stdenv: add hardeningSupported (only gcc5 for now)
This adds an attribute to the cc derivation that indicates which
hardening flags are supported.

Fixes #18995:
nix-shell -p uses stdenvNoCC to build the environment. Thus
stdenv.cc.cc.hardeningSupported is undefined and no hardening flags are
turned on.

groxxda added a commit to groxxda/nixpkgs that referenced this issue Oct 13, 2016

stdenv: add hardeningSupported (only gcc5 for now)
This adds an attribute to the cc derivation that indicates which
hardening flags are supported.

Fixes #18995:
nix-shell -p uses stdenvNoCC to build the environment. Thus
stdenv.cc.cc.hardeningSupported is undefined and no hardening flags are
turned on.

@groxxda groxxda referenced this issue Oct 13, 2016

Closed

WIP: rewrite hardening in plain nix #19512

4 of 10 tasks complete
@andrewrk

This comment has been minimized.

Show comment
Hide comment
@andrewrk

andrewrk Dec 14, 2016

Contributor

even though I knew about this, I temporarily forgot and it caused me to file a bogus issue report on another project: thejoshwolfe/legend-of-swarkland#36

Contributor

andrewrk commented Dec 14, 2016

even though I knew about this, I temporarily forgot and it caused me to file a bogus issue report on another project: thejoshwolfe/legend-of-swarkland#36

@mboisson

This comment has been minimized.

Show comment
Hide comment
@mboisson

mboisson Feb 3, 2017

I'm running into this issue when compiling older versions of GCC.

gcc 4.8 compiles fine, but at run time, it tries to use stack-protector-strong, which is not supported.

I've wasted about 8 hours so far trying to fix this broken thing....
[mboisson@build-node easybuild-easyconfigs]$ gcc --version
gcc: erreur: unrecognized command line option ‘-fstack-protector-strong’
gcc (GCC) 4.8.5

Our fork of nixpkgs is based on 16.09 and is here:
github.com/computecanada/nixpkgs

is there any commit I could pull to fix this ?

mboisson commented Feb 3, 2017

I'm running into this issue when compiling older versions of GCC.

gcc 4.8 compiles fine, but at run time, it tries to use stack-protector-strong, which is not supported.

I've wasted about 8 hours so far trying to fix this broken thing....
[mboisson@build-node easybuild-easyconfigs]$ gcc --version
gcc: erreur: unrecognized command line option ‘-fstack-protector-strong’
gcc (GCC) 4.8.5

Our fork of nixpkgs is based on 16.09 and is here:
github.com/computecanada/nixpkgs

is there any commit I could pull to fix this ?

@globin

This comment has been minimized.

Show comment
Hide comment
@globin

globin Feb 3, 2017

Member

There is no optimal fix yet, workaround is still to export hardeningDisable=all in your shell while developing.

Member

globin commented Feb 3, 2017

There is no optimal fix yet, workaround is still to export hardeningDisable=all in your shell while developing.

@mboisson

This comment has been minimized.

Show comment
Hide comment
@mboisson

mboisson commented Feb 3, 2017

Thanks.

@Ralith

This comment has been minimized.

Show comment
Hide comment
@Ralith

Ralith Feb 13, 2017

Contributor

This cost me untold hours of frustration; eventually I just gave up getting anything useful out of a debugger on NixOS. Eagerly looking forward to a fix.

It's important to me that building nix packages with useful debug info (i.e. without -O2) even with nix-build (rather than nix-shell) is possible as well. For example, debugging software that uses LLVM is much easier with a debug build of LLVM which I'd rather not have to make by hand.

Contributor

Ralith commented Feb 13, 2017

This cost me untold hours of frustration; eventually I just gave up getting anything useful out of a debugger on NixOS. Eagerly looking forward to a fix.

It's important to me that building nix packages with useful debug info (i.e. without -O2) even with nix-build (rather than nix-shell) is possible as well. For example, debugging software that uses LLVM is much easier with a debug build of LLVM which I'd rather not have to make by hand.

@viric

This comment has been minimized.

Show comment
Hide comment
@viric

viric Feb 13, 2017

Member

What about not adding any hardening if IN_NIX_SHELL=1 ? What is bad in this?

Member

viric commented Feb 13, 2017

What about not adding any hardening if IN_NIX_SHELL=1 ? What is bad in this?

@Ralith

This comment has been minimized.

Show comment
Hide comment
@Ralith

Ralith Feb 14, 2017

Contributor

That would be a huge improvement, but would still make it more difficult than necessary to build nix packages with debug symbols. IMO no -O flags should be supplied (let alone in extraAfter!) if debug symbols are requested by the derivation.

Contributor

Ralith commented Feb 14, 2017

That would be a huge improvement, but would still make it more difficult than necessary to build nix packages with debug symbols. IMO no -O flags should be supplied (let alone in extraAfter!) if debug symbols are requested by the derivation.

@edolstra

This comment has been minimized.

Show comment
Hide comment
@edolstra

edolstra Feb 14, 2017

Member

It's not desirable to have nix-shell behave very different from a regular build. The whole point of nix-shell after all is to get an interactive build environment that is otherwise identical to a regular Nix build.

Not supplying -O when debug symbols are requested doesn't sound right either. I do optimized builds with debug symbols all the time... After all, it's pretty useful to be able to get meaningful results out of a production core dump.

However: The real issue here is that add-hardening.sh adds -O2 after the user's CFLAGS. So you might think you're building with -O3, but you're not. That's a pretty critical bug...

A possible fix would be for add-hardening.sh to not add -O2 if any previous -O flag was seen.

Member

edolstra commented Feb 14, 2017

It's not desirable to have nix-shell behave very different from a regular build. The whole point of nix-shell after all is to get an interactive build environment that is otherwise identical to a regular Nix build.

Not supplying -O when debug symbols are requested doesn't sound right either. I do optimized builds with debug symbols all the time... After all, it's pretty useful to be able to get meaningful results out of a production core dump.

However: The real issue here is that add-hardening.sh adds -O2 after the user's CFLAGS. So you might think you're building with -O3, but you're not. That's a pretty critical bug...

A possible fix would be for add-hardening.sh to not add -O2 if any previous -O flag was seen.

@edolstra edolstra modified the milestones: 17.03, 16.09 Feb 14, 2017

@globin

This comment has been minimized.

Show comment
Hide comment
@globin

globin Feb 14, 2017

Member

Just a note if this is not clear, -D_FORTIFY_SOURCE=2 needs at least -O2 to work, but yes generally -O3 should not be overwritten. But I don't think we should not stop adding the -O2 if we see a lower optimisation level or -g because that will remove the hardening on all derivations that have a broken build environment and that are exactly those that need hardening more than others.

Member

globin commented Feb 14, 2017

Just a note if this is not clear, -D_FORTIFY_SOURCE=2 needs at least -O2 to work, but yes generally -O3 should not be overwritten. But I don't think we should not stop adding the -O2 if we see a lower optimisation level or -g because that will remove the hardening on all derivations that have a broken build environment and that are exactly those that need hardening more than others.

@edolstra

This comment has been minimized.

Show comment
Hide comment
@edolstra

edolstra Oct 18, 2017

Member

We got bitten at work by this again today, i.e. a debug build at -O0 silently got changed into -O2. This really needs to be fixed since this issue has been a blocker for the last two releases.

How about this: cc-wrapper adds -O2 only if it hasn't seen a preceding -O flag. If it has seen -O where n >= 2, it does nothing.If is has seen -O0 or -O1, it should fail with an error message like:

cc-wrapper: Hardening is incompatible with -O0. Set hardeningDisable = ["bla"] to disable hardening.

This way hardening can be enabled by default, but we don't get silent optimization with -O0.

(What about -Os?)

Member

edolstra commented Oct 18, 2017

We got bitten at work by this again today, i.e. a debug build at -O0 silently got changed into -O2. This really needs to be fixed since this issue has been a blocker for the last two releases.

How about this: cc-wrapper adds -O2 only if it hasn't seen a preceding -O flag. If it has seen -O where n >= 2, it does nothing.If is has seen -O0 or -O1, it should fail with an error message like:

cc-wrapper: Hardening is incompatible with -O0. Set hardeningDisable = ["bla"] to disable hardening.

This way hardening can be enabled by default, but we don't get silent optimization with -O0.

(What about -Os?)

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Oct 18, 2017

Member

Oh, I had been wondering why my debug builds don't feel like compiled with -O0.

Member

vcunat commented Oct 18, 2017

Oh, I had been wondering why my debug builds don't feel like compiled with -O0.

@gebner

This comment has been minimized.

Show comment
Hide comment
@gebner

gebner Oct 18, 2017

Member

If is has seen -O0 or -O1, it should fail with an error message like:

I agree that it's better to fail loudly instead of silently changing flags. But it won't solve this issue: either way using gcc outside of nixpkgs is broken. nix-shell -p gcc --run "gcc -O0 foo.c" should work and call gcc with the -O0 flag and no further optimization. I really expect this to just work when developing C/C++ code.

IMHO stdenv and the standalone gcc/clang attributes should use different wrappers. stdenv should have hardening enabled by default, and fail with -O0 as you suggested. The standalone attributes should use the minimal wrapper that is necessary to compile programs, but not enable hardening. In this setup, you can still use nix-shell -A to troubleshoot a regular build since it would use the stdenv wrapper.

Member

gebner commented Oct 18, 2017

If is has seen -O0 or -O1, it should fail with an error message like:

I agree that it's better to fail loudly instead of silently changing flags. But it won't solve this issue: either way using gcc outside of nixpkgs is broken. nix-shell -p gcc --run "gcc -O0 foo.c" should work and call gcc with the -O0 flag and no further optimization. I really expect this to just work when developing C/C++ code.

IMHO stdenv and the standalone gcc/clang attributes should use different wrappers. stdenv should have hardening enabled by default, and fail with -O0 as you suggested. The standalone attributes should use the minimal wrapper that is necessary to compile programs, but not enable hardening. In this setup, you can still use nix-shell -A to troubleshoot a regular build since it would use the stdenv wrapper.

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Oct 18, 2017

Member

-O0 silently decreasing hardening might be "unexpected" as well, but outside nix build/shell it seems OK to be without hardening by default. The wrapper would better be the same; stdenv may e.g. set some specific variable like we have IN_NIX_SHELL already.

Member

vcunat commented Oct 18, 2017

-O0 silently decreasing hardening might be "unexpected" as well, but outside nix build/shell it seems OK to be without hardening by default. The wrapper would better be the same; stdenv may e.g. set some specific variable like we have IN_NIX_SHELL already.

@edolstra

This comment has been minimized.

Show comment
Hide comment
@edolstra

edolstra Oct 18, 2017

Member

@gebner Yes, I guess hardeningDisable should be hardeningEnable. I.e. if that variable (which would be set by default in stdenv) is not set, you don't get hardening so -O0 works as expected outside of a Nix build or nix-shell. Inside a nix-shell hardening would still be enabled by default, but you would get an error when using -O0.

Member

edolstra commented Oct 18, 2017

@gebner Yes, I guess hardeningDisable should be hardeningEnable. I.e. if that variable (which would be set by default in stdenv) is not set, you don't get hardening so -O0 works as expected outside of a Nix build or nix-shell. Inside a nix-shell hardening would still be enabled by default, but you would get an error when using -O0.

@Ralith

This comment has been minimized.

Show comment
Hide comment
@Ralith

Ralith Oct 18, 2017

Contributor

@vcunat

outside nix build/shell it seems OK to be without hardening by default

Silently breaking -O0 inside nix-shell is not okay. Using nix-shell to manage development environments is one of Nix's most compelling features.

Having an explicit gcc/clang build input change the behavior would be weird and unintuitive, especially when you want to build a nix package with debug symbols (which is very common when e.g. developing against LLVM). It's not really an improvement on the current state of affairs; in either case, nix silently defeats your efforts unless you know exactly the right magic incantation.

@edolstra

How about this: cc-wrapper adds -O2 only if it hasn't seen a preceding -O flag. If it has seen -O where n >= 2, it does nothing.If is has seen -O0 or -O1, it should fail with an error message like:

Why can't we just place all automatically-injected options before manually supplied ones? This would be massively more intuitive, require less complexity, and protect against similar issues in the future. Yes, there might(!) exist some small number of packages which won't be fully hardened as a result; those packages are broken, and can be fixed.

Contributor

Ralith commented Oct 18, 2017

@vcunat

outside nix build/shell it seems OK to be without hardening by default

Silently breaking -O0 inside nix-shell is not okay. Using nix-shell to manage development environments is one of Nix's most compelling features.

Having an explicit gcc/clang build input change the behavior would be weird and unintuitive, especially when you want to build a nix package with debug symbols (which is very common when e.g. developing against LLVM). It's not really an improvement on the current state of affairs; in either case, nix silently defeats your efforts unless you know exactly the right magic incantation.

@edolstra

How about this: cc-wrapper adds -O2 only if it hasn't seen a preceding -O flag. If it has seen -O where n >= 2, it does nothing.If is has seen -O0 or -O1, it should fail with an error message like:

Why can't we just place all automatically-injected options before manually supplied ones? This would be massively more intuitive, require less complexity, and protect against similar issues in the future. Yes, there might(!) exist some small number of packages which won't be fully hardened as a result; those packages are broken, and can be fixed.

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Oct 18, 2017

Member

@Ralith. I didn't mean to claim that at all. I'm sorry for the misunderstanding. I'm sorry; I'm too tired.

Member

vcunat commented Oct 18, 2017

@Ralith. I didn't mean to claim that at all. I'm sorry for the misunderstanding. I'm sorry; I'm too tired.

@ElvishJerricco

This comment has been minimized.

Show comment
Hide comment
@ElvishJerricco

ElvishJerricco Oct 18, 2017

Contributor

I agree that if the only cost of simply putting the arguments before the supplied ones is that some packages break, that seems like by far the most useful and intuitive approach. The problem is tracking down which packages are broken and verifying that we actually can fix them

Contributor

ElvishJerricco commented Oct 18, 2017

I agree that if the only cost of simply putting the arguments before the supplied ones is that some packages break, that seems like by far the most useful and intuitive approach. The problem is tracking down which packages are broken and verifying that we actually can fix them

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Oct 18, 2017

Member

@Ralith: the problem about nix-shell is that some people expect it to be an interactive nix-build and some use it just to bring packages into scope. EDIT: before I managed to finish the thought properly, Gebner did it for me below.

Member

vcunat commented Oct 18, 2017

@Ralith: the problem about nix-shell is that some people expect it to be an interactive nix-build and some use it just to bring packages into scope. EDIT: before I managed to finish the thought properly, Gebner did it for me below.

@gebner

This comment has been minimized.

Show comment
Hide comment
@gebner

gebner Oct 18, 2017

Member

If hardening is only enabled by stdenv, then

  • nix-shell -p gcc would just bring gcc into scope without hardening, and
  • nix-shell -A hello still works as an interactive nix-build and enables hardening by default.

AFAICT this would address both usages of nix-shell.

Member

gebner commented Oct 18, 2017

If hardening is only enabled by stdenv, then

  • nix-shell -p gcc would just bring gcc into scope without hardening, and
  • nix-shell -A hello still works as an interactive nix-build and enables hardening by default.

AFAICT this would address both usages of nix-shell.

@Ralith

This comment has been minimized.

Show comment
Hide comment
@Ralith

Ralith Oct 18, 2017

Contributor

@vcunat Those aren't mutually exclusive, we just need to make the nix-build semantics sane, i.e. don't clobber explicitly passed options.

@gebner As discussed in my prior comment, that doesn't really improve things. I maintain build environments with a shell.nix containing a derivation and often want debug builds of nix packages, so stdenv very much applies. Adding an explicit build input for a compiler that stdenv would otherwise supply is just another magic incantation required to prevent nix from silently sabotaging me.

Contributor

Ralith commented Oct 18, 2017

@vcunat Those aren't mutually exclusive, we just need to make the nix-build semantics sane, i.e. don't clobber explicitly passed options.

@gebner As discussed in my prior comment, that doesn't really improve things. I maintain build environments with a shell.nix containing a derivation and often want debug builds of nix packages, so stdenv very much applies. Adding an explicit build input for a compiler that stdenv would otherwise supply is just another magic incantation required to prevent nix from silently sabotaging me.

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Oct 18, 2017

Member

Using mkDerivation from nixpkgs is indistinguishable from a real nixpkgs build. If you want something different, you should explicitly differ "in some way".

Member

vcunat commented Oct 18, 2017

Using mkDerivation from nixpkgs is indistinguishable from a real nixpkgs build. If you want something different, you should explicitly differ "in some way".

@Ralith

This comment has been minimized.

Show comment
Hide comment
@Ralith

Ralith Oct 18, 2017

Contributor

@ElvishJerricco To be clear, respecting explicitly supplied arguments will almost certainly not break any packages, it only risks marginally weaken the hardening applied to any packages in the unlikely event that they have hardcoded -O0 in their makefiles or similar. I'm skeptical that there even exist any packages which do this and aren't for some reason specifically unable to use higher optimization levels, in which case this could actually fix some packages.

Contributor

Ralith commented Oct 18, 2017

@ElvishJerricco To be clear, respecting explicitly supplied arguments will almost certainly not break any packages, it only risks marginally weaken the hardening applied to any packages in the unlikely event that they have hardcoded -O0 in their makefiles or similar. I'm skeptical that there even exist any packages which do this and aren't for some reason specifically unable to use higher optimization levels, in which case this could actually fix some packages.

@Ralith

This comment has been minimized.

Show comment
Hide comment
@Ralith

Ralith Oct 18, 2017

Contributor

@vcunat I do not want something different. I want real nixpkgs builds to respect the flags I pass.

Contributor

Ralith commented Oct 18, 2017

@vcunat I do not want something different. I want real nixpkgs builds to respect the flags I pass.

@ElvishJerricco

This comment has been minimized.

Show comment
Hide comment
@ElvishJerricco

ElvishJerricco Oct 18, 2017

Contributor

Frankly, I think cc-wrapper needs to be as simple as possible. The more rules someone has to know in order to use $CC properly, whether in a nix-shell or whatever else, the more likely it is to generate problems like this issue. Rather than adding rules about gcc and stdenv.cc being different and keeping complex rules about which arguments are automatically added where, it makes more sense to me to remove rules and simply say “there may be some arguments prepended to your argument list.”

Contributor

ElvishJerricco commented Oct 18, 2017

Frankly, I think cc-wrapper needs to be as simple as possible. The more rules someone has to know in order to use $CC properly, whether in a nix-shell or whatever else, the more likely it is to generate problems like this issue. Rather than adding rules about gcc and stdenv.cc being different and keeping complex rules about which arguments are automatically added where, it makes more sense to me to remove rules and simply say “there may be some arguments prepended to your argument list.”

@cstrahan

This comment has been minimized.

Show comment
Hide comment
@cstrahan

cstrahan Oct 18, 2017

Contributor

@edolstra et al:

Is my PR (https://github.com/NixOS/nixpkgs/pull/28029/files) not suitable? If not, why? I think it gets us most of the way there - I’m willing to make necessary changes to fit the precise semantics we want with respect to failing fast (e.g. in the presence of multiple -O). Just tell me how to improve it and I’ll do so.

I should note that, as it stands, it makes nix-build and nix-shell behave the same way, while making system-wide installed gcc behave like what most people would expect (unhardened). So that part should please @Ralith, I’d think.

Contributor

cstrahan commented Oct 18, 2017

@edolstra et al:

Is my PR (https://github.com/NixOS/nixpkgs/pull/28029/files) not suitable? If not, why? I think it gets us most of the way there - I’m willing to make necessary changes to fit the precise semantics we want with respect to failing fast (e.g. in the presence of multiple -O). Just tell me how to improve it and I’ll do so.

I should note that, as it stands, it makes nix-build and nix-shell behave the same way, while making system-wide installed gcc behave like what most people would expect (unhardened). So that part should please @Ralith, I’d think.

@andrewrk

This comment has been minimized.

Show comment
Hide comment
@andrewrk

andrewrk Oct 18, 2017

Contributor

I'm going to throw out a crazy idea here:

It's up to each individual application how they want their software compiled. We can have hardening by default, but if they override it to disable hardening, the application wanted it that way. And that decision should be respected, and if we want to counter that decision we should file an upstream bug.

Contributor

andrewrk commented Oct 18, 2017

I'm going to throw out a crazy idea here:

It's up to each individual application how they want their software compiled. We can have hardening by default, but if they override it to disable hardening, the application wanted it that way. And that decision should be respected, and if we want to counter that decision we should file an upstream bug.

@musteresel

This comment has been minimized.

Show comment
Hide comment
@musteresel

musteresel Oct 18, 2017

Contributor

@Ralith Yes, there are packages which must disable optimizations for (parts of) their build. An example is (IIRC, it's been a while since I worked on its sources) CLISP, which does some non-C-standard stuff and thus relies on the optimizer not messing that up.

Contributor

musteresel commented Oct 18, 2017

@Ralith Yes, there are packages which must disable optimizations for (parts of) their build. An example is (IIRC, it's been a while since I worked on its sources) CLISP, which does some non-C-standard stuff and thus relies on the optimizer not messing that up.

@cstrahan

This comment has been minimized.

Show comment
Hide comment
@cstrahan

cstrahan Jan 18, 2018

Contributor

/ping

@edolstra I hate to bug you personally/directly, but would you be able to specify your desired approach to fixing this bug - or if you can't spare the cycles for that, could you name an individual or two that you'd feel comfortable deferring to?

There's enough subtlety to the problem that I doubt everyone here will come to anything approaching a consensus. Nonetheless, the problem needs fixing, and I'd like to do the work to fix it - if someone with authority can state what an acceptable solution is.

Just as a reminder, I have this PR open: #28029

There's some feedback regarding cross compilation from @Ericson2314 that I can address, but I'm first waiting to know if we're going to accept that general approach, a variant of it, or something else entirely.

Contributor

cstrahan commented Jan 18, 2018

/ping

@edolstra I hate to bug you personally/directly, but would you be able to specify your desired approach to fixing this bug - or if you can't spare the cycles for that, could you name an individual or two that you'd feel comfortable deferring to?

There's enough subtlety to the problem that I doubt everyone here will come to anything approaching a consensus. Nonetheless, the problem needs fixing, and I'd like to do the work to fix it - if someone with authority can state what an acceptable solution is.

Just as a reminder, I have this PR open: #28029

There's some feedback regarding cross compilation from @Ericson2314 that I can address, but I'm first waiting to know if we're going to accept that general approach, a variant of it, or something else entirely.

@edolstra

This comment has been minimized.

Show comment
Hide comment
@edolstra

edolstra Jan 19, 2018

Member

@cstrahan The PR looks good to me, thanks. It might be nice to issue an error/warning if there is a conflict between a hardening flag and a user-supplied -O flag, but that's not super important.

Member

edolstra commented Jan 19, 2018

@cstrahan The PR looks good to me, thanks. It might be nice to issue an error/warning if there is a conflict between a hardening flag and a user-supplied -O flag, but that's not super important.

@cstrahan

This comment has been minimized.

Show comment
Hide comment
@cstrahan

cstrahan Jan 26, 2018

Contributor

@edolstra Awesome - thanks! I'll address @Ericson2314's points and try to get it ready for merging soon as I can - probably before the weekend is up.

Contributor

cstrahan commented Jan 26, 2018

@edolstra Awesome - thanks! I'll address @Ericson2314's points and try to get it ready for merging soon as I can - probably before the weekend is up.

cstrahan added a commit to cstrahan/nixpkgs that referenced this issue Mar 6, 2018

@andrewrk

This comment has been minimized.

Show comment
Hide comment
@andrewrk

andrewrk Mar 24, 2018

Contributor

Even though I originally filed the bug, and it bit me again after that, I still forgot about it again and got bit by this issue again, wasting the musl-lib developers' time.

Contributor

andrewrk commented Mar 24, 2018

Even though I originally filed the bug, and it bit me again after that, I still forgot about it again and got bit by this issue again, wasting the musl-lib developers' time.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Mar 26, 2018

Member

Hopefully @cstrahan's PR will be done soon. My bad recently for reviewing it slowly when he took it up again.

Member

Ericson2314 commented Mar 26, 2018

Hopefully @cstrahan's PR will be done soon. My bad recently for reviewing it slowly when he took it up again.

@fpletz fpletz modified the milestones: 18.03, 18.09 Mar 27, 2018

Ericson2314 pushed a commit to obsidiansystems/nixpkgs that referenced this issue Apr 10, 2018

Ericson2314 added a commit that referenced this issue Apr 10, 2018

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