[v2,i386] : Fix PR89221, --enable-frame-pointer does not work as intended

Message ID CAFULd4Z4tJ7OFDDcgWQfRGThxb=pNZgTkujhiiso_N+NaHiCdQ@mail.gmail.com
State New
Headers show
Series
  • [v2,i386] : Fix PR89221, --enable-frame-pointer does not work as intended
Related show

Commit Message

Uros Bizjak Feb. 10, 2019, 7:51 p.m.
On Fri, Feb 8, 2019 at 1:24 PM Uros Bizjak <ubizjak@gmail.com> wrote:

> Attached patch fixes --enable-frame-pointer handling, and this way

> makes a couple of defines in config/i386/sol2.h obsolete.


It turned out that --enable-frame-pointer does not work for multilibs
at all. So, instead of pretending that -m32 on x86_64 and -m64 on i686
works as advertised, unify 32bit and 64bit handling.

2019-02-10  UroŇ° Bizjak  <ubizjak@gmail.com>

    PR target/89221
    * config.gcc (i[34567]86-*-*, x86_64-*-*): Move tests for enable_cld
    and enable_frame_pointer ...
    * configure.ac: ... here.  Update help strings for
    --enable-frame-pointer.
    * configure: Regenerate.
    * config/i386/i386.c (ix86_option_override_internal): Remove
    USE_X86_64_FRAME_POINTER define, use USE_IX86_FRAME_POINTER instead.
    * config/i386/sol2.h (USE_IX86_FRAME_POINTER): Remove.
    (USE_X86_64_FRAME_POINTER): Ditto.

Please note that this fix will re-enable frame pointer for all targets
but linux* or darwin[[8912]]. However, since builds for e.g. cygwin
and mingw survived just well without frame pointers in the mean time,
we should probably list these targets as targets without frame
pointers by default. Maintainers should decide.

Which makes the patch gcc-10 material.

Uros.

Comments

Rainer Orth Feb. 11, 2019, 8:25 a.m. | #1
Hi Uros,

> On Fri, Feb 8, 2019 at 1:24 PM Uros Bizjak <ubizjak@gmail.com> wrote:

>

>> Attached patch fixes --enable-frame-pointer handling, and this way

>> makes a couple of defines in config/i386/sol2.h obsolete.

>

> It turned out that --enable-frame-pointer does not work for multilibs

> at all. So, instead of pretending that -m32 on x86_64 and -m64 on i686

> works as advertised, unify 32bit and 64bit handling.


this is effectively what I came up with when testing the first version
of the patch on i386-pc-solaris2.11 and noticing the -m64 regressions.

I've now re-bootstrapped this exact version without regressions, so from
a Solaris/x86 POV this is good to go.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University
Uros Bizjak May 6, 2019, 3 p.m. | #2
On Mon, Feb 11, 2019 at 9:25 AM Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote:
>

> Hi Uros,

>

> > On Fri, Feb 8, 2019 at 1:24 PM Uros Bizjak <ubizjak@gmail.com> wrote:

> >

> >> Attached patch fixes --enable-frame-pointer handling, and this way

> >> makes a couple of defines in config/i386/sol2.h obsolete.

> >

> > It turned out that --enable-frame-pointer does not work for multilibs

> > at all. So, instead of pretending that -m32 on x86_64 and -m64 on i686

> > works as advertised, unify 32bit and 64bit handling.

>

> this is effectively what I came up with when testing the first version

> of the patch on i386-pc-solaris2.11 and noticing the -m64 regressions.

>

> I've now re-bootstrapped this exact version without regressions, so from

> a Solaris/x86 POV this is good to go.


Now committed to mainline SVN as attached.

Uros.
Index: config/i386/i386-options.c
===================================================================
--- config/i386/i386-options.c	(revision 270904)
+++ config/i386/i386-options.c	(working copy)
@@ -2198,16 +2198,12 @@
 #define USE_IX86_FRAME_POINTER 0
 #endif
 
-#ifndef USE_X86_64_FRAME_POINTER
-#define USE_X86_64_FRAME_POINTER 0
-#endif
-
   /* Set the default values for switches whose default depends on TARGET_64BIT
      in case they weren't overwritten by command line options.  */
   if (TARGET_64BIT_P (opts->x_ix86_isa_flags))
     {
       if (opts->x_optimize >= 1 && !opts_set->x_flag_omit_frame_pointer)
-	opts->x_flag_omit_frame_pointer = !USE_X86_64_FRAME_POINTER;
+	opts->x_flag_omit_frame_pointer = !USE_IX86_FRAME_POINTER;
       if (opts->x_flag_asynchronous_unwind_tables
 	  && !opts_set->x_flag_unwind_tables
 	  && TARGET_64BIT_MS_ABI)
Index: config/i386/sol2.h
===================================================================
--- config/i386/sol2.h	(revision 270904)
+++ config/i386/sol2.h	(working copy)
@@ -248,9 +248,6 @@
 #define ASAN_REJECT_SPEC \
   DEF_ARCH64_SPEC("%e:-fsanitize=address is not supported in this configuration")
 
-#define USE_IX86_FRAME_POINTER 1
-#define USE_X86_64_FRAME_POINTER 1
-
 #undef NO_PROFILE_COUNTERS
 
 #undef MCOUNT_NAME
Index: config.gcc
===================================================================
--- config.gcc	(revision 270913)
+++ config.gcc	(working copy)
@@ -607,12 +607,6 @@
 		echo "This target does not support --with-abi."
 		exit 1
 	fi
-	if test "x$enable_cld" = xyes; then
-		tm_defines="${tm_defines} USE_IX86_CLD=1"
-	fi
-	if test "x$enable_frame_pointer" = xyes; then
-		tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1"
-	fi
 	;;
 x86_64-*-*)
 	case ${with_abi} in
@@ -633,12 +627,6 @@
 		echo "Unknown ABI used in --with-abi=$with_abi"
 		exit 1
 	esac
-	if test "x$enable_cld" = xyes; then
-		tm_defines="${tm_defines} USE_IX86_CLD=1"
-	fi
-	if test "x$enable_frame_pointer" = xyes; then
-		tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1"
-	fi
 	;;
 arm*-*-*)
 	tm_p_file="arm/arm-flags.h ${tm_p_file} arm/aarch-common-protos.h"
Index: configure
===================================================================
--- configure	(revision 270904)
+++ configure	(working copy)
@@ -1688,8 +1688,7 @@
   --enable-leading-mingw64-underscores
                           enable leading underscores on 64 bit mingw targets
   --enable-cld            enable -mcld by default for 32bit x86
-  --enable-frame-pointer  enable -fno-omit-frame-pointer by default for 32bit
-                          x86
+  --enable-frame-pointer  enable -fno-omit-frame-pointer by default for x86
   --disable-win32-registry
                           disable lookup of installation paths in the Registry
                           on Windows hosts
@@ -12199,8 +12198,7 @@
 
 case $target_os in
 linux* | darwin[8912]*)
-  # Enable -fomit-frame-pointer by default for Linux and Darwin with
-  # DWARF2.
+  # Enable -fomit-frame-pointer by default for Linux and Darwin with DWARF2.
   enable_frame_pointer=no
   ;;
 *)
@@ -12211,6 +12209,17 @@
 fi
 
 
+case $target in
+i[34567]86-*-* | x86_64-*-*)
+	if test "x$enable_cld" = xyes; then
+		tm_defines="${tm_defines} USE_IX86_CLD=1"
+	fi
+	if test "x$enable_frame_pointer" = xyes; then
+		tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1"
+	fi
+	;;
+esac
+
 # Windows32 Registry support for specifying GCC installation paths.
 # Check whether --enable-win32-registry was given.
 if test "${enable_win32_registry+set}" = set; then :
@@ -18646,7 +18655,7 @@
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 18649 "configure"
+#line 18658 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -18752,7 +18761,7 @@
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 18755 "configure"
+#line 18764 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
Index: configure.ac
===================================================================
--- configure.ac	(revision 270904)
+++ configure.ac	(working copy)
@@ -1881,12 +1881,11 @@
 
 AC_ARG_ENABLE(frame-pointer,
 [AS_HELP_STRING([--enable-frame-pointer],
-		[enable -fno-omit-frame-pointer by default for 32bit x86])], [],
+		[enable -fno-omit-frame-pointer by default for x86])], [],
 [
 case $target_os in
 linux* | darwin[[8912]]*)
-  # Enable -fomit-frame-pointer by default for Linux and Darwin with
-  # DWARF2.
+  # Enable -fomit-frame-pointer by default for Linux and Darwin with DWARF2.
   enable_frame_pointer=no
   ;;
 *)
@@ -1895,6 +1894,17 @@
 esac
 ])
 
+case $target in
+i[[34567]]86-*-* | x86_64-*-*)
+	if test "x$enable_cld" = xyes; then
+		tm_defines="${tm_defines} USE_IX86_CLD=1"
+	fi
+	if test "x$enable_frame_pointer" = xyes; then
+		tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1"
+	fi
+	;;
+esac
+
 # Windows32 Registry support for specifying GCC installation paths.
 AC_ARG_ENABLE(win32-registry,
 [AS_HELP_STRING([--disable-win32-registry],

Patch

Index: config/i386/sol2.h
===================================================================
--- config/i386/sol2.h	(revision 268670)
+++ config/i386/sol2.h	(working copy)
@@ -248,9 +248,6 @@ 
 #define ASAN_REJECT_SPEC \
   DEF_ARCH64_SPEC("%e:-fsanitize=address is not supported in this configuration")
 
-#define USE_IX86_FRAME_POINTER 1
-#define USE_X86_64_FRAME_POINTER 1
-
 #undef NO_PROFILE_COUNTERS
 
 #undef MCOUNT_NAME
Index: config.gcc
===================================================================
--- config.gcc	(revision 268670)
+++ config.gcc	(working copy)
@@ -604,12 +604,6 @@ 
 		echo "This target does not support --with-abi."
 		exit 1
 	fi
-	if test "x$enable_cld" = xyes; then
-		tm_defines="${tm_defines} USE_IX86_CLD=1"
-	fi
-	if test "x$enable_frame_pointer" = xyes; then
-		tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1"
-	fi
 	;;
 x86_64-*-*)
 	case ${with_abi} in
@@ -630,12 +624,6 @@ 
 		echo "Unknown ABI used in --with-abi=$with_abi"
 		exit 1
 	esac
-	if test "x$enable_cld" = xyes; then
-		tm_defines="${tm_defines} USE_IX86_CLD=1"
-	fi
-	if test "x$enable_frame_pointer" = xyes; then
-		tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1"
-	fi
 	;;
 arm*-*-*)
 	tm_p_file="arm/arm-flags.h ${tm_p_file} arm/aarch-common-protos.h"
Index: configure
===================================================================
--- configure	(revision 268670)
+++ configure	(working copy)
@@ -1688,8 +1688,7 @@ 
   --enable-leading-mingw64-underscores
                           enable leading underscores on 64 bit mingw targets
   --enable-cld            enable -mcld by default for 32bit x86
-  --enable-frame-pointer  enable -fno-omit-frame-pointer by default for 32bit
-                          x86
+  --enable-frame-pointer  enable -fno-omit-frame-pointer by default for x86
   --disable-win32-registry
                           disable lookup of installation paths in the Registry
                           on Windows hosts
@@ -12199,8 +12198,7 @@ 
 
 case $target_os in
 linux* | darwin[8912]*)
-  # Enable -fomit-frame-pointer by default for Linux and Darwin with
-  # DWARF2.
+  # Enable -fomit-frame-pointer by default for Linux and Darwin with DWARF2.
   enable_frame_pointer=no
   ;;
 *)
@@ -12211,6 +12209,17 @@ 
 fi
 
 
+case $target in
+i[34567]86-*-* | x86_64-*-*)
+	if test "x$enable_cld" = xyes; then
+		tm_defines="${tm_defines} USE_IX86_CLD=1"
+	fi
+	if test "x$enable_frame_pointer" = xyes; then
+		tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1 USE_X86_64_FRAME_POINTER=1"
+	fi
+	;;
+esac
+
 # Windows32 Registry support for specifying GCC installation paths.
 # Check whether --enable-win32-registry was given.
 if test "${enable_win32_registry+set}" = set; then :
@@ -18646,7 +18655,7 @@ 
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 18640 "configure"
+#line 18658 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -18752,7 +18761,7 @@ 
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 18746 "configure"
+#line 18764 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -25141,7 +25150,7 @@ 
           no)
             ;;
           *)
-            as_fn_error "'$enableval' is an invalid value for --enable-standard-branch-protection.\
+            as_fn_error $? "'$enableval' is an invalid value for --enable-standard-branch-protection.\
   Valid choices are 'yes' and 'no'." "$LINENO" 5
             ;;
         esac
Index: configure.ac
===================================================================
--- configure.ac	(revision 268670)
+++ configure.ac	(working copy)
@@ -1881,12 +1881,11 @@ 
 
 AC_ARG_ENABLE(frame-pointer,
 [AS_HELP_STRING([--enable-frame-pointer],
-		[enable -fno-omit-frame-pointer by default for 32bit x86])], [],
+		[enable -fno-omit-frame-pointer by default for x86])], [],
 [
 case $target_os in
 linux* | darwin[[8912]]*)
-  # Enable -fomit-frame-pointer by default for Linux and Darwin with
-  # DWARF2.
+  # Enable -fomit-frame-pointer by default for Linux and Darwin with DWARF2.
   enable_frame_pointer=no
   ;;
 *)
@@ -1895,6 +1894,17 @@ 
 esac
 ])
 
+case $target in
+i[[34567]]86-*-* | x86_64-*-*)
+	if test "x$enable_cld" = xyes; then
+		tm_defines="${tm_defines} USE_IX86_CLD=1"
+	fi
+	if test "x$enable_frame_pointer" = xyes; then
+		tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1 USE_X86_64_FRAME_POINTER=1"
+	fi
+	;;
+esac
+
 # Windows32 Registry support for specifying GCC installation paths.
 AC_ARG_ENABLE(win32-registry,
 [AS_HELP_STRING([--disable-win32-registry],