From 14207e1cf83737443636a25534075427f65c1de4 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Sun, 24 Mar 2024 23:45:25 -0700 Subject: [PATCH] Build with traps on signed overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is UB, we should not be doing it, and we can cheaply turn it into crashes reliably. We would much rather have crashes than the program doing something silly. Benchmarks, but i wonder if they are nonsense because they get identical times across compilers?! | Command | Mean [ms] | Min [ms] | Max [ms] | Relative | |:---|---:|---:|---:|---:| | `result-clang/bin/nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix` | 375.5 ± 24.0 | 353.8 | 408.8 | 1.00 | | `result-gcc/bin/nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix` | 407.9 ± 26.0 | 385.1 | 449.5 | 1.09 ± 0.10 | | `result-clangsan/bin/nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix` | 382.2 ± 26.6 | 354.9 | 419.0 | 1.02 ± 0.10 | | `result-gccsan/bin/nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix` | 408.6 ± 24.6 | 384.5 | 441.9 | 1.09 ± 0.10 | | Command | Mean [s] | Min [s] | Max [s] | Relative | |:---|---:|---:|---:|---:| | `result-clang/bin/nix search --no-eval-cache github:nixos/nixpkgs/e1fa12d4f6c6fe19ccb59cac54b5b3f25e160870 hello` | 17.199 ± 0.167 | 16.930 | 17.499 | 1.01 ± 0.01 | | `result-gcc/bin/nix search --no-eval-cache github:nixos/nixpkgs/e1fa12d4f6c6fe19ccb59cac54b5b3f25e160870 hello` | 17.409 ± 0.126 | 17.242 | 17.633 | 1.02 ± 0.01 | | `result-clangsan/bin/nix search --no-eval-cache github:nixos/nixpkgs/e1fa12d4f6c6fe19ccb59cac54b5b3f25e160870 hello` | 17.080 ± 0.137 | 16.879 | 17.350 | 1.00 | | `result-gccsan/bin/nix search --no-eval-cache github:nixos/nixpkgs/e1fa12d4f6c6fe19ccb59cac54b5b3f25e160870 hello` | 17.396 ± 0.160 | 17.131 | 17.660 | 1.02 ± 0.01 | | Command | Mean [s] | Min [s] | Max [s] | Relative | |:---|---:|---:|---:|---:| | `result-clang/bin/nix eval --raw --impure --expr 'with import {}; system'` | 6.267 ± 0.069 | 6.197 | 6.415 | 1.02 ± 0.01 | | `result-gcc/bin/nix eval --raw --impure --expr 'with import {}; system'` | 6.232 ± 0.045 | 6.180 | 6.311 | 1.01 ± 0.01 | | `result-clangsan/bin/nix eval --raw --impure --expr 'with import {}; system'` | 6.162 ± 0.020 | 6.133 | 6.196 | 1.00 | | `result-gccsan/bin/nix eval --raw --impure --expr 'with import {}; system'` | 6.229 ± 0.031 | 6.199 | 6.289 | 1.01 ± 0.01 | | Command | Mean [s] | Min [s] | Max [s] | Relative | |:---|---:|---:|---:|---:| | `GC_INITIAL_HEAP_SIZE=10g result-clang/bin/nix eval --raw --impure --expr 'with import {}; system'` | 4.683 ± 0.044 | 4.630 | 4.761 | 1.00 | | `GC_INITIAL_HEAP_SIZE=10g result-gcc/bin/nix eval --raw --impure --expr 'with import {}; system'` | 4.750 ± 0.041 | 4.680 | 4.812 | 1.01 ± 0.01 | | `GC_INITIAL_HEAP_SIZE=10g result-clangsan/bin/nix eval --raw --impure --expr 'with import {}; system'` | 4.703 ± 0.040 | 4.640 | 4.760 | 1.00 ± 0.01 | | `GC_INITIAL_HEAP_SIZE=10g result-gccsan/bin/nix eval --raw --impure --expr 'with import {}; system'` | 4.766 ± 0.037 | 4.727 | 4.844 | 1.02 ± 0.01 | Change-Id: I616ca3eab670317587d47b41870d8ac963c019ae --- local.mk | 9 ++++++++- meson.build | 17 +++++++++++++++++ mk/libraries.mk | 6 +----- package.nix | 4 +++- 4 files changed, 29 insertions(+), 7 deletions(-) diff --git a/local.mk b/local.mk index 3f3abb9f0..a756c8272 100644 --- a/local.mk +++ b/local.mk @@ -1,4 +1,11 @@ -GLOBAL_CXXFLAGS += -Wno-deprecated-declarations -Werror=switch +# 2024-03-24: jade benchmarked the default sanitize reporting in clang and got +# a regression of about 10% on hackage-packages.nix with clang. So we are trapping instead. +# +# This has an overhead of 0-4% on gcc and unmeasurably little on clang, in +# Nix evaluation benchmarks. +DEFAULT_SANITIZE_FLAGS = -fsanitize=signed-integer-overflow -fsanitize-undefined-trap-on-error +GLOBAL_CXXFLAGS += -Wno-deprecated-declarations -Werror=switch $(DEFAULT_SANITIZE_FLAGS) +GLOBAL_LDFLAGS += $(DEFAULT_SANITIZE_FLAGS) # Allow switch-enum to be overridden for files that do not support it, usually because of dependency headers. ERROR_SWITCH_ENUM = -Werror=switch-enum diff --git a/meson.build b/meson.build index 0a6a6b4b1..b7eb41089 100644 --- a/meson.build +++ b/meson.build @@ -302,6 +302,23 @@ add_project_arguments( language : 'cpp', ) +if cxx.get_id() in ['gcc', 'clang'] + # 2024-03-24: jade benchmarked the default sanitize reporting in clang and got + # a regression of about 10% on hackage-packages.nix with clang. So we are trapping instead. + # + # This has an overhead of 0-4% on gcc and unmeasurably little on clang, in + # Nix evaluation benchmarks. + # + # N.B. Meson generates a completely nonsense warning here: + # https://github.com/mesonbuild/meson/issues/9822 + # Both of these args cannot be written in the default meson configuration. + # b_sanitize=signed-integer-overflow is ignored, and + # -fsanitize-undefined-trap-on-error is not representable. + sanitize_args = ['-fsanitize=signed-integer-overflow', '-fsanitize-undefined-trap-on-error'] + add_project_arguments(sanitize_args, language: 'cpp') + add_project_link_arguments(sanitize_args, language: 'cpp') +endif + add_project_link_arguments('-pthread', language : 'cpp') if cxx.get_linker_id() in ['ld.bfd', 'ld.gold'] add_project_link_arguments('-Wl,--no-copy-dt-needed-entries', language : 'cpp') diff --git a/mk/libraries.mk b/mk/libraries.mk index 1bc73d7f7..f9d427b10 100644 --- a/mk/libraries.mk +++ b/mk/libraries.mk @@ -78,11 +78,7 @@ define build-library $(1)_LDFLAGS += -undefined suppress -flat_namespace endif else - ifndef HOST_DARWIN - ifndef HOST_CYGWIN - $(1)_LDFLAGS += -Wl,-z,defs - endif - endif + # -Wl,-z,defs is broken with sanitizers on Linux/clang at least. endif ifndef HOST_DARWIN diff --git a/package.nix b/package.nix index 3c4971605..ef3557387 100644 --- a/package.nix +++ b/package.nix @@ -287,7 +287,9 @@ in stdenv.mkDerivation (finalAttrs: { strictDeps = true; - hardeningDisable = lib.optional stdenv.hostPlatform.isStatic "pie"; + # strictoverflow is disabled because we trap on signed overflow instead + hardeningDisable = [ "strictoverflow" ] + ++ lib.optional stdenv.hostPlatform.isStatic "pie"; meta.platforms = lib.platforms.unix;