diff mbox

[1/3] configure: add option to disable -fstack-protector flags

Message ID 20140111023621.GA27157@amazon.com
State New
Headers show

Commit Message

Noonan, Steven Jan. 11, 2014, 2:36 a.m. UTC 
On Thu, Jan 09, 2014 at 06:18:07PM -0500, Brad Smith wrote:
> On 09/01/14 5:40 PM, Paolo Bonzini wrote:
> >Il 09/01/2014 22:55, Steven Noonan ha scritto:
> >>From: Steven Noonan <snoonan@amazon.com>
> >>
> >>The -fstack-protector flag family is useful for ensuring safety and for
> >>debugging, but has a performance impact. Here's a boot time comparison between
> >>a QEMU build of qemu-system-arm with and without the -fstack-protector-all
> >>flag:
> >>
> >>     # WITHOUT -fstack-protector-all
> >>     [root@localhost ~]# systemd-analyze
> >>     Startup finished in 1.744s (kernel) + 11.345s (initrd) + 47.164s (userspace) = 1min 255ms
> >>
> >>     # WITH -fstack-protector-all
> >>     [root@localhost ~]# systemd-analyze
> >>     Startup finished in 1.843s (kernel) + 12.262s (initrd) + 1min 3.480s (userspace) = 1min 17.587s
> >
> >Can you try -fstack-protector-strong?
> >
> >Probably the right thing to do is to pick in order
> >-fstack-protector-strong, -fstack-protector, and nothing.
> 
> +1
> 

OK, in order to get -fstack-protector-strong I had to get a Fedora box
up and running, so the compiler and build/execution environment are
different. I took three samples from each build, applying a clean 
QCOW2 snapshot before each run to avoid cross-boot confounding
variables:

    # -fstack-protector-all
    Startup finished in 1.810s (kernel) + 12.331s (initrd) + 49.016s (userspace) = 1min 3.159s
    Startup finished in 1.801s (kernel) + 12.287s (initrd) + 47.925s (userspace) = 1min 2.013s
    Startup finished in 1.812s (kernel) + 12.302s (initrd) + 47.995s (userspace) = 1min 2.111s
 
    # -fstack-protector-strong
    Startup finished in 1.744s (kernel) + 11.223s (initrd) + 44.688s (userspace) = 57.657s
    Startup finished in 1.721s (kernel) + 11.222s (initrd) + 44.194s (userspace) = 57.138s
    Startup finished in 1.693s (kernel) + 11.250s (initrd) + 44.426s (userspace) = 57.370s
    
    # -fstack-protector
    Startup finished in 1.705s (kernel) + 11.409s (initrd) + 43.563s (userspace) = 56.677s
    Startup finished in 1.877s (kernel) + 11.137s (initrd) + 43.719s (userspace) = 56.734s
    Startup finished in 1.708s (kernel) + 11.141s (initrd) + 43.628s (userspace) = 56.478s
    
    # no stack protector
    Startup finished in 1.743s (kernel) + 11.190s (initrd) + 43.709s (userspace) = 56.643s
    Startup finished in 1.763s (kernel) + 11.216s (initrd) + 43.767s (userspace) = 56.747s
    Startup finished in 1.711s (kernel) + 11.283s (initrd) + 43.878s (userspace) = 56.873s

There are a few points to note:

  - stack-protector-all doesn't cost nearly as much in this build, for
	reasons unclear to me. It looks like the kernel/initrd timings are
	rather closely reproduced, but the userspace timing varies quite a
	bit.

  - stack-protector-strong's performance hit is indeed much less than
	stack-protector-all.

  - The builds with the plain old -fstack-protector flag and without any
	stack protector are tied for the lead in terms of performance.

Amended patch attached.

Comments

Stefan Weil Jan. 11, 2014, 7:46 a.m. UTC | #1 
Hi Steven,

--disable-stack-protector would also be useful for platforms which make
debugging of executables with stack protection difficult. When I must
debug Windows executables, I always disable stack protection, because
otherwise the stack back traces are unreadable.

So, for MinGW it might be reasonable to set the default to no stack
protection if --enable-debug is selected. This requires some
modifications in your patch.

# Don't set it to "yes" initially:
stack_protector=""

# Do the compile test if it is not "no":
if test "$stack_protector" != "no"; then

The usual logic is do nothing if the user says "no". Run the compile
tests otherwise. Show an error message if the compile tests fail and the
user said "yes". See the code which handles $pie for an example.

Please send your next patch inline - this makes it easier to add comments.

Best regards

Stefan
Paolo Bonzini Jan. 13, 2014, 11:53 a.m. UTC | #2 
Il 11/01/2014 08:46, Stefan Weil ha scritto:
> Hi Steven,
> 
> --disable-stack-protector would also be useful for platforms which make
> debugging of executables with stack protection difficult. When I must
> debug Windows executables, I always disable stack protection, because
> otherwise the stack back traces are unreadable.
> 
> So, for MinGW it might be reasonable to set the default to no stack
> protection if --enable-debug is selected. This requires some
> modifications in your patch.
> 
> # Don't set it to "yes" initially:
> stack_protector=""
> 
> # Do the compile test if it is not "no":
> if test "$stack_protector" != "no"; then

Apart from this little detail, the patch looks good.

Steven, please resend the patch as a top-level message with the patch
inline rather than attached.  This is needed so that Anthony's script
will pick it up.  Including these changes would be nice too.

Paolo

> The usual logic is do nothing if the user says "no". Run the compile
> tests otherwise. Show an error message if the compile tests fail and the
> user said "yes". See the code which handles $pie for an example.
> 
> Please send your next patch inline - this makes it easier to add comments.
> 
> Best regards
> 
> Stefan
>
diff mbox

Patch

From cad2b5990debede2bc8dc8552904b2ef9e14af22 Mon Sep 17 00:00:00 2001
From: Steven Noonan <snoonan@amazon.com>
Date: Mon, 6 Jan 2014 13:39:20 -0800
Subject: [PATCH] configure: add option to disable -fstack-protector flags

The -fstack-protector flag family is useful for ensuring safety and for
debugging, but has a performance impact. Here are some boot time comparisons of
the various versions of -fstack-protector using qemu-system-arm on an x86_64
host:

    # -fstack-protector-all
    Startup finished in 1.810s (kernel) + 12.331s (initrd) + 49.016s (userspace) = 1min 3.159s
    Startup finished in 1.801s (kernel) + 12.287s (initrd) + 47.925s (userspace) = 1min 2.013s
    Startup finished in 1.812s (kernel) + 12.302s (initrd) + 47.995s (userspace) = 1min 2.111s

    # -fstack-protector-strong
    Startup finished in 1.744s (kernel) + 11.223s (initrd) + 44.688s (userspace) = 57.657s
    Startup finished in 1.721s (kernel) + 11.222s (initrd) + 44.194s (userspace) = 57.138s
    Startup finished in 1.693s (kernel) + 11.250s (initrd) + 44.426s (userspace) = 57.370s

    # -fstack-protector
    Startup finished in 1.705s (kernel) + 11.409s (initrd) + 43.563s (userspace) = 56.677s
    Startup finished in 1.877s (kernel) + 11.137s (initrd) + 43.719s (userspace) = 56.734s
    Startup finished in 1.708s (kernel) + 11.141s (initrd) + 43.628s (userspace) = 56.478s

    # no stack protector
    Startup finished in 1.743s (kernel) + 11.190s (initrd) + 43.709s (userspace) = 56.643s
    Startup finished in 1.763s (kernel) + 11.216s (initrd) + 43.767s (userspace) = 56.747s
    Startup finished in 1.711s (kernel) + 11.283s (initrd) + 43.878s (userspace) = 56.873s

This patch introduces a configure option to disable the stack protector
entirely, and conditional stack protector flag selection (in order, based on
availability): -fstack-protector-strong, -fstack-protector, no stack protector.

Signed-off-by: Steven Noonan <snoonan@amazon.com>
---
 configure | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 07b6be3..a485e94 100755
--- a/configure
+++ b/configure
@@ -147,6 +147,7 @@  audio_win_int=""
 cc_i386=i386-pc-linux-gnu-gcc
 libs_qga=""
 debug_info="yes"
+stack_protector="yes"
 
 # Don't accept a target_list environment variable.
 unset target_list
@@ -879,6 +880,10 @@  for opt do
   ;;
   --disable-werror) werror="no"
   ;;
+  --enable-stack-protector) stack_protector="yes"
+  ;;
+  --disable-stack-protector) stack_protector="no"
+  ;;
   --disable-curses) curses="no"
   ;;
   --enable-curses) curses="yes"
@@ -1117,6 +1122,7 @@  echo "  --enable-sparse          enable sparse checker"
 echo "  --disable-sparse         disable sparse checker (default)"
 echo "  --disable-strip          disable stripping binaries"
 echo "  --disable-werror         disable compilation abort on warning"
+echo "  --disable-stack-protector disable GCC-provided stack protection"
 echo "  --disable-sdl            disable SDL"
 echo "  --enable-sdl             enable SDL"
 echo "  --disable-gtk            disable gtk UI"
@@ -1298,9 +1304,15 @@  for flag in $gcc_flags; do
     fi
 done
 
-if compile_prog "-Werror -fstack-protector-all" "" ; then
-    QEMU_CFLAGS="$QEMU_CFLAGS -fstack-protector-all"
-    LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,-fstack-protector-all"
+if test "$stack_protector" = "yes" ; then
+  gcc_flags="-fstack-protector-strong -fstack-protector"
+  for flag in $gcc_flags; do
+    if compile_prog "-Werror $flag" "" ; then
+      QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+      LIBTOOLFLAGS="$LIBTOOLFLAGS -Wc,$flag"
+      break
+    fi
+  done
 fi
 
 # Workaround for http://gcc.gnu.org/PR55489.  Happens with -fPIE/-fPIC and
-- 
1.8.5.2