cc-wrapper: Pass shellcheck and other cleanups #27879

Ericson2314 · 2017-08-02T18:41:22Z

Motivation for this change

In many cases, this involved taking @orivej's and @edolstra's recent ld-wrapper improvements, and applying then elsewhere.

The only semantics changes I know of are

Fix bad word-splitting---already done with ld-wrapper with no issues
Remove unused and probably wrong LD=---didn't take into account cross and in cc-wrapper pointed to unwrapped ld while in ld-wrapper pointed to wrapped ld.

Things done

Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers.

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built stdenv on platform(s)
- NixOS
- macOS
- Linux
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/)
Fits CONTRIBUTING.md.

CC @orivej

mention-bot · 2017-08-02T18:41:24Z

@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @vcunat and @abbradar to be potential reviewers.

Ericson2314

Here's some things I'd like feedback on (along with me discovering my own mistake :)).

Ericson2314 · 2017-08-02T18:45:48Z

pkgs/build-support/cc-wrapper/ld-wrapper.sh

@@ -69,7 +73,7 @@ relocatable=
 # Find all -L... switches for rpath, and relocatable flags for build id.
 if [ "$NIX_DONT_SET_RPATH" != 1 ] || [ "$NIX_SET_BUILD_ID" = 1 ]; then
    prev=
-    for p in "${params[@]}" "${extra[@]}"; do
+    for p in "${extraBefore[@]}" "${params[@]}" "${extraAfter[@]}"; do


I think not including extraBefore here was an oversight of whoever added it originally. Note that the debugging code below originally also skipped it.

Ericson2314 · 2017-08-02T18:46:35Z

pkgs/build-support/cc-wrapper/setup-hook.sh

 if [ -n "@coreutils_bin@" ]; then
    addToSearchPath _PATH @coreutils_bin@/bin
 fi

-if [ -z "$crossConfig" ]; then
-  ENV_PREFIX=""
+# shellcheck disable=SC2157


This was the same one as above---eager copy paste---and unneeded.

Ericson2314 · 2017-08-02T18:48:13Z

pkgs/build-support/cc-wrapper/add-flags.sh

@@ -1,28 +1,37 @@
 # `-B@out@/bin' forces cc to use ld-wrapper.sh when calling ld.
 export NIX_CFLAGS_COMPILE="-B@out@/bin/ $NIX_CFLAGS_COMPILE"

+# One line export + assign means missing files will not fail script. Not sure
+# whether thsi is intentional.


Somebody should weigh in on this.

thsi is probably not intentional

I weigh in on "thsi" most likely being a typo. Does that count?

Wait, we already check for file existence, so this is definitely isn't needed.

Ericson2314 · 2017-08-02T18:48:33Z

pkgs/build-support/cc-wrapper/add-hardening.sh

@@ -1,26 +1,41 @@
 hardeningFlags=(fortify stackprotector pic strictoverflow format relro bindnow)
-hardeningFlags+=("${hardeningEnable[@]}")
+# Intentionally word-split in case 'hardeningEnable' is defined in Nix.
+hardeningFlags+=(${hardeningEnable[@]})


Semantics change, but all the flags are sans-whitespace so it seems fine.

Ericson2314 · 2017-08-02T18:49:21Z

pkgs/build-support/cc-wrapper/add-hardening.sh

+# `set -u`.
+for flag in ${hardeningDisable[@]} @hardening_unsupported_flags@
+do
+  hardeningDisableMap[$flag]=1


I think this is clearer than the regex stuff that was there before---and shellcheck said the regex stuff was done wrong anyways such that only string literals would match.

Ericson2314 · 2017-08-02T18:49:48Z

pkgs/build-support/cc-wrapper/add-hardening.sh

      case $flag in
        fortify)
          if [[ -n "$NIX_DEBUG" ]]; then echo HARDENING: enabling fortify >&2; fi
          hardeningCFlags+=('-O2' '-D_FORTIFY_SOURCE=2')
          ;;
        stackprotector)
          if [[ -n "$NIX_DEBUG" ]]; then echo HARDENING: enabling stackprotector >&2; fi
-          hardeningCFlags+=('-fstack-protector-strong' '--param ssp-buffer-size=4')
+          hardeningCFlags+=('-fstack-protector-strong' '--param' 'ssp-buffer-size=4')


N.B. bugs like this weren't being caught because extra word splitting on arrays.

Ericson2314 · 2017-08-02T18:50:07Z

pkgs/build-support/cc-wrapper/cc-wrapper.sh

@@ -19,16 +25,17 @@ source @out@/nix-support/utils.sh
 # For instance, figure out if linker flags should be passed.
 # GCC prints annoying warnings when they are not needed.
 dontLink=0
-getVersion=0


This was unused.

Ericson2314 · 2017-08-02T18:53:32Z

pkgs/build-support/cc-wrapper/cc-wrapper.sh

 fi

 if [ -n "$NIX_CC_WRAPPER_EXEC_HOOK" ]; then
    source "$NIX_CC_WRAPPER_EXEC_HOOK"
 fi

 PATH="$path_backup"
-exec @prog@ ${extraBefore[@]} "${params[@]}" "${extraAfter[@]}"
+exec @prog@ "${extraBefore[@]}" "${params[@]}" "${extraAfter[@]}"


Semantics change, but the old version was a mistake. I fixed a hardening flag bug uncovered by this; hope there aren't others.

@orivej did something the same fix for ld-wrapper, and I guess it was fine there?

Ericson2314 · 2017-08-02T18:54:24Z

pkgs/build-support/cc-wrapper/gnat-wrapper.sh

        if [ "${p:0:3}" = -L/ ] && badPath "${p:2}"; then
-            skip $p
+            skip "${p:2}"


So the "skip" thing does say "bad path", but not knowing what flag it came from is strictly less useful. I'm conflicted.

Ericson2314 · 2017-08-02T18:58:39Z

pkgs/build-support/cc-wrapper/gnat-wrapper.sh

@@ -18,7 +24,6 @@ source @out@/nix-support/utils.sh
 # Figure out if linker flags should be passed.  GCC prints annoying
 # warnings when they are not needed.
 dontLink=0


This is a dead variable, but used in commented code, so I guess I'll leave it for now.

Ericson2314 · 2017-08-03T23:16:36Z

In #27672 I made cc-wrapper robust with undefined variables with set -u. That makes me extra confident in this commit, as it is included in that.

grahamc

I'm worried about the n+=1:

[grahamc@Morbo:~]$ declare -i n=0

[grahamc@Morbo:~]$ i+=1

[grahamc@Morbo:~]$ echo $i
1

[grahamc@Morbo:~]$ i+=1

[grahamc@Morbo:~]$ echo $i
11

otherwise this looks great

grahamc · 2017-08-02T18:57:24Z

pkgs/build-support/cc-wrapper/add-flags.sh

@@ -1,28 +1,37 @@
 # `-B@out@/bin' forces cc to use ld-wrapper.sh when calling ld.
 export NIX_CFLAGS_COMPILE="-B@out@/bin/ $NIX_CFLAGS_COMPILE"

+# One line export + assign means missing files will not fail script. Not sure
+# whether thsi is intentional.


thsi is probably not intentional

@orivej

In many cases, this involved taking @orivej's and @edolstra's recent ld-wrapper improvements, and applying then elsewhere.

Ericson2314 · 2017-08-04T16:54:46Z

OK I think I dealt with all comments.

Ericson2314 · 2017-08-04T16:57:16Z

pkgs/build-support/cc-wrapper/cc-wrapper.sh

@@ -116,23 +125,22 @@ if [[ "$isCpp" = 1 ]]; then
    NIX_CFLAGS_LINK+=" $NIX_CXXSTDLIB_LINK"
 fi

-LD=@ldPath@/ld


This would be the unwrapped ld, which seems wrong.

Ericson2314 · 2017-08-04T16:58:41Z

pkgs/build-support/cc-wrapper/ld-wrapper.sh

@@ -49,18 +54,17 @@ if [ "$NIX_ENFORCE_PURITY" = 1 -a -n "$NIX_STORE" \
    params=("${rest[@]}")
 fi

-LD=@prog@


This however would be the wrapped ld. Since cc will fork ld-wrapper, the old LD=..., assuming LD was already an export so it affected forked processes' environments (!), would be clobbered anyways.

Ericson2314 · 2017-08-04T17:00:23Z

pkgs/build-support/cc-wrapper/add-flags.sh

@@ -1,28 +1,37 @@
 # `-B@out@/bin' forces cc to use ld-wrapper.sh when calling ld.
 export NIX_CFLAGS_COMPILE="-B@out@/bin/ $NIX_CFLAGS_COMPILE"

+# Export and assign separately in order that a failing $(..) will fail
+# the script.


Added back better comment without typo.

Ericson2314 requested review from grahamc and edolstra August 2, 2017 18:41

Ericson2314 commented Aug 2, 2017

View reviewed changes

Ericson2314 added the 1.severity: mass-rebuild label Aug 2, 2017

Ericson2314 commented Aug 2, 2017

View reviewed changes

Ericson2314 force-pushed the cc-wrapper-shellcheck branch 4 times, most recently from 7f7ed3c to de957cc Compare August 3, 2017 20:06

globin approved these changes Aug 3, 2017

View reviewed changes

Ericson2314 mentioned this pull request Aug 4, 2017

cc-wrapper: Hygiene and set -u #27672

Merged

9 tasks

grahamc reviewed Aug 4, 2017

View reviewed changes

cc-wrapper: Pass shellcheck and other cleanups

6463fd3

In many cases, this involved taking @orivej's and @edolstra's recent ld-wrapper improvements, and applying then elsewhere.

Ericson2314 force-pushed the cc-wrapper-shellcheck branch from de957cc to 6463fd3 Compare August 4, 2017 16:47

Ericson2314 commented Aug 4, 2017

View reviewed changes

LnL7 approved these changes Aug 4, 2017

View reviewed changes

Ericson2314 merged commit fdd07f6 into NixOS:staging Aug 4, 2017

Ericson2314 deleted the cc-wrapper-shellcheck branch August 4, 2017 18:19

FRidh mentioned this pull request Aug 7, 2017

ld-wrapper: Optimize expanding rpath #27657

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cc-wrapper: Pass shellcheck and other cleanups #27879

cc-wrapper: Pass shellcheck and other cleanups #27879

Ericson2314 commented Aug 2, 2017 •

edited

mention-bot commented Aug 2, 2017

Ericson2314 left a comment

Ericson2314 Aug 2, 2017

Ericson2314 Aug 2, 2017

Ericson2314 Aug 2, 2017

grahamc Aug 2, 2017

copumpkin Aug 2, 2017

Ericson2314 Aug 2, 2017 •

edited

Ericson2314 Aug 2, 2017

Ericson2314 Aug 2, 2017

Ericson2314 Aug 2, 2017

Ericson2314 Aug 2, 2017

Ericson2314 Aug 2, 2017

Ericson2314 Aug 2, 2017

Ericson2314 Aug 2, 2017

Ericson2314 commented Aug 3, 2017

grahamc left a comment •

edited

grahamc Aug 2, 2017

Ericson2314 commented Aug 4, 2017

Ericson2314 Aug 4, 2017

Ericson2314 Aug 4, 2017

Ericson2314 Aug 4, 2017

cc-wrapper: Pass shellcheck and other cleanups #27879

cc-wrapper: Pass shellcheck and other cleanups #27879

Conversation

Ericson2314 commented Aug 2, 2017 • edited

Motivation for this change

Things done

mention-bot commented Aug 2, 2017

Ericson2314 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Aug 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Aug 3, 2017

grahamc left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Aug 2, 2017 •

edited

Ericson2314 Aug 2, 2017 •

edited

grahamc left a comment •

edited