[patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5

[patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5

[Jan Kratochvil]

Hi,

here is the ON_STACK code again, with fixed alignment for i386 SSE.

It is generalized for all OSes on i386/amd64.  I can move it to
{i386,amd64)-linux-tdep.c but I find this code much more lightweight than
i386_push_dummy_call which is already present in i386-tdep.

No regressions on
{x86_64,x86_64-m32,i686}-fedora(15-rawhide)/rhel(5-6)-linux-gnu and for
gdbsever non-extended mode.

For x86_64-fedora17-linux-gnu it fixes:
-FAIL: gdb.cp/gdb2495.exp: Call a function that raises an exception without a handler.
-FAIL: gdb.cp/gdb2495.exp: bt after returning from a popped frame
+PASS: gdb.cp/gdb2495.exp: Call a function that raises an exception without a handler.
+PASS: gdb.cp/gdb2495.exp: bt after returning from a popped frame


Thanks,
Jan


gdb/
2012-03-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* amd64-dicos-tdep.c (amd64_dicos_push_dummy_code): Remove.
	(amd64_dicos_init_abi): Remove its installment.
	* dicos-tdep.c (dicos_init_abi): Remove the
	set_gdbarch_call_dummy_location call.  Update the comment here.
	* i386-dicos-tdep.c (i386_dicos_push_dummy_code): Remove.
	(i386_dicos_init_abi): Remove its installment.
	* i386-tdep.c (i386_push_dummy_code): New function.
	(i386_gdbarch_init): Call set_gdbarch_call_dummy_location, install
	i386_push_dummy_code.

--- a/gdb/amd64-dicos-tdep.c
+++ b/gdb/amd64-dicos-tdep.c
@@ -23,24 +23,6 @@
 #include "amd64-tdep.h"
 #include "dicos-tdep.h"
 
-static CORE_ADDR
-amd64_dicos_push_dummy_code (struct gdbarch *gdbarch,
-			     CORE_ADDR sp, CORE_ADDR funaddr,
-			     struct value **args, int nargs,
-			     struct type *value_type,
-			     CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
-			     struct regcache *regcache)
-{
-  int bplen;
-  CORE_ADDR bppc = sp;
-
-  gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
-  *bp_addr = sp - bplen;
-  *real_pc = funaddr;
-
-  return *bp_addr;
-}
-
 static void
 amd64_dicos_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
@@ -49,8 +31,6 @@ amd64_dicos_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   amd64_init_abi (info, gdbarch);
 
   dicos_init_abi (gdbarch);
-
-  set_gdbarch_push_dummy_code (gdbarch, amd64_dicos_push_dummy_code);
 }
 
 static enum gdb_osabi
--- a/gdb/dicos-tdep.c
+++ b/gdb/dicos-tdep.c
@@ -43,8 +43,8 @@ dicos_init_abi (struct gdbarch *gdbarch)
 
   /* There's no (standard definition of) entry point or a guaranteed
      text location with a symbol where to place the call dummy, so we
-     put it on the stack.  */
-  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+     need it on the stack.  Rely on i386_gdbarch_init used also for
+     amd64 to set up ON_STACK inferior calls.  */
 
   /* DICOS rewinds the PC itself.  */
   set_gdbarch_decr_pc_after_break (gdbarch, 0);
--- a/gdb/i386-dicos-tdep.c
+++ b/gdb/i386-dicos-tdep.c
@@ -22,32 +22,12 @@
 #include "gdb_string.h"
 #include "dicos-tdep.h"
 
-static CORE_ADDR
-i386_dicos_push_dummy_code (struct gdbarch *gdbarch,
-			    CORE_ADDR sp, CORE_ADDR funaddr,
-			    struct value **args, int nargs,
-			    struct type *value_type,
-			    CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
-			    struct regcache *regcache)
-{
-  int bplen;
-  CORE_ADDR bppc = sp;
-
-  gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
-  *bp_addr = sp - bplen;
-  *real_pc = funaddr;
-
-  return *bp_addr;
-}
-
 static void
 i386_dicos_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
   dicos_init_abi (gdbarch);
-
-  set_gdbarch_push_dummy_code (gdbarch, i386_dicos_push_dummy_code);
 }
 
 static enum gdb_osabi
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2326,6 +2326,30 @@ i386_16_byte_align_p (struct type *type)
   return 0;
 }
 
+/* Implementation for set_gdbarch_push_dummy_code.  */
+
+static CORE_ADDR
+i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, CORE_ADDR funaddr,
+		      struct value **args, int nargs, struct type *value_type,
+		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
+		      struct regcache *regcache)
+{
+  int bplen;
+  CORE_ADDR bppc = sp;
+
+  gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
+  sp -= bplen;
+
+  /* amd64_push_dummy_call does alignment on its own but i386_push_dummy_call
+     does not.  ABI requires stack alignment for executables using SSE.  */
+  if (gdbarch_frame_align_p (gdbarch))
+    sp = gdbarch_frame_align (gdbarch, sp);
+
+  *bp_addr = sp;
+  *real_pc = funaddr;
+  return sp;
+}
+
 static CORE_ADDR
 i386_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		      struct regcache *regcache, CORE_ADDR bp_addr, int nargs,
@@ -7372,6 +7396,8 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_get_longjmp_target (gdbarch, i386_get_longjmp_target);
 
   /* Call dummy code.  */
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code (gdbarch, i386_push_dummy_code);
   set_gdbarch_push_dummy_call (gdbarch, i386_push_dummy_call);
   set_gdbarch_frame_align (gdbarch, i386_frame_align);
 

ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5

[Jan Kratochvil]

Just that it is pending.
On Fri, 09 Mar 2012 22:01:17 +0100, Jan Kratochvil wrote:
Hi,

here is the ON_STACK code again, with fixed alignment for i386 SSE.

It is generalized for all OSes on i386/amd64.  I can move it to
{i386,amd64)-linux-tdep.c but I find this code much more lightweight than
i386_push_dummy_call which is already present in i386-tdep.

No regressions on
{x86_64,x86_64-m32,i686}-fedora(15-rawhide)/rhel(5-6)-linux-gnu and for
gdbsever non-extended mode.

For x86_64-fedora17-linux-gnu it fixes:
-FAIL: gdb.cp/gdb2495.exp: Call a function that raises an exception without a handler.
-FAIL: gdb.cp/gdb2495.exp: bt after returning from a popped frame
+PASS: gdb.cp/gdb2495.exp: Call a function that raises an exception without a handler.
+PASS: gdb.cp/gdb2495.exp: bt after returning from a popped frame


Thanks,
Jan


gdb/
2012-03-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* amd64-dicos-tdep.c (amd64_dicos_push_dummy_code): Remove.
	(amd64_dicos_init_abi): Remove its installment.
	* dicos-tdep.c (dicos_init_abi): Remove the
	set_gdbarch_call_dummy_location call.  Update the comment here.
	* i386-dicos-tdep.c (i386_dicos_push_dummy_code): Remove.
	(i386_dicos_init_abi): Remove its installment.
	* i386-tdep.c (i386_push_dummy_code): New function.
	(i386_gdbarch_init): Call set_gdbarch_call_dummy_location, install
	i386_push_dummy_code.

--- a/gdb/amd64-dicos-tdep.c
+++ b/gdb/amd64-dicos-tdep.c
@@ -23,24 +23,6 @@
 #include "amd64-tdep.h"
 #include "dicos-tdep.h"
 
-static CORE_ADDR
-amd64_dicos_push_dummy_code (struct gdbarch *gdbarch,
-			     CORE_ADDR sp, CORE_ADDR funaddr,
-			     struct value **args, int nargs,
-			     struct type *value_type,
-			     CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
-			     struct regcache *regcache)
-{
-  int bplen;
-  CORE_ADDR bppc = sp;
-
-  gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
-  *bp_addr = sp - bplen;
-  *real_pc = funaddr;
-
-  return *bp_addr;
-}
-
 static void
 amd64_dicos_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
@@ -49,8 +31,6 @@ amd64_dicos_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   amd64_init_abi (info, gdbarch);
 
   dicos_init_abi (gdbarch);
-
-  set_gdbarch_push_dummy_code (gdbarch, amd64_dicos_push_dummy_code);
 }
 
 static enum gdb_osabi
--- a/gdb/dicos-tdep.c
+++ b/gdb/dicos-tdep.c
@@ -43,8 +43,8 @@ dicos_init_abi (struct gdbarch *gdbarch)
 
   /* There's no (standard definition of) entry point or a guaranteed
      text location with a symbol where to place the call dummy, so we
-     put it on the stack.  */
-  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+     need it on the stack.  Rely on i386_gdbarch_init used also for
+     amd64 to set up ON_STACK inferior calls.  */
 
   /* DICOS rewinds the PC itself.  */
   set_gdbarch_decr_pc_after_break (gdbarch, 0);
--- a/gdb/i386-dicos-tdep.c
+++ b/gdb/i386-dicos-tdep.c
@@ -22,32 +22,12 @@
 #include "gdb_string.h"
 #include "dicos-tdep.h"
 
-static CORE_ADDR
-i386_dicos_push_dummy_code (struct gdbarch *gdbarch,
-			    CORE_ADDR sp, CORE_ADDR funaddr,
-			    struct value **args, int nargs,
-			    struct type *value_type,
-			    CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
-			    struct regcache *regcache)
-{
-  int bplen;
-  CORE_ADDR bppc = sp;
-
-  gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
-  *bp_addr = sp - bplen;
-  *real_pc = funaddr;
-
-  return *bp_addr;
-}
-
 static void
 i386_dicos_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
   dicos_init_abi (gdbarch);
-
-  set_gdbarch_push_dummy_code (gdbarch, i386_dicos_push_dummy_code);
 }
 
 static enum gdb_osabi
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2326,6 +2326,30 @@ i386_16_byte_align_p (struct type *type)
   return 0;
 }
 
+/* Implementation for set_gdbarch_push_dummy_code.  */
+
+static CORE_ADDR
+i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, CORE_ADDR funaddr,
+		      struct value **args, int nargs, struct type *value_type,
+		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
+		      struct regcache *regcache)
+{
+  int bplen;
+  CORE_ADDR bppc = sp;
+
+  gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
+  sp -= bplen;
+
+  /* amd64_push_dummy_call does alignment on its own but i386_push_dummy_call
+     does not.  ABI requires stack alignment for executables using SSE.  */
+  if (gdbarch_frame_align_p (gdbarch))
+    sp = gdbarch_frame_align (gdbarch, sp);
+
+  *bp_addr = sp;
+  *real_pc = funaddr;
+  return sp;
+}
+
 static CORE_ADDR
 i386_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		      struct regcache *regcache, CORE_ADDR bp_addr, int nargs,
@@ -7372,6 +7396,8 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_get_longjmp_target (gdbarch, i386_get_longjmp_target);
 
   /* Call dummy code.  */
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code (gdbarch, i386_push_dummy_code);
   set_gdbarch_push_dummy_call (gdbarch, i386_push_dummy_call);
   set_gdbarch_frame_align (gdbarch, i386_frame_align);
 

Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5

[Mark Kettenis]

> Date: Mon, 26 Mar 2012 21:04:14 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -2326,6 +2326,30 @@ i386_16_byte_align_p (struct type *type)
>    return 0;
>  }
>  
> +/* Implementation for set_gdbarch_push_dummy_code.  */
> +
> +static CORE_ADDR
> +i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, CORE_ADDR funaddr,
> +		      struct value **args, int nargs, struct type *value_type,
> +		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> +		      struct regcache *regcache)
> +{
> +  int bplen;
> +  CORE_ADDR bppc = sp;
> +
> +  gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
> +  sp -= bplen;
> +
> +  /* amd64_push_dummy_call does alignment on its own but i386_push_dummy_call
> +     does not.  ABI requires stack alignment for executables using SSE.  */
> +  if (gdbarch_frame_align_p (gdbarch))
> +    sp = gdbarch_frame_align (gdbarch, sp);
> +
> +  *bp_addr = sp;
> +  *real_pc = funaddr;
> +  return sp;
> +}

You're almost certainly right worrying about stack alignment.
However, the comment doesn't make a lot of sense.  For one thing,
amd64_push_dummy_call() doesn't explicitly align the stack.  It just
preserves alignment.  Also, if i386_push_dummy_call() doesn't preserve
alignment (and it sure looks like it doesn't), then aligning the stack
here doesn't help.  In any case, we don't need to to explicitly align
the stack since that's already done bu call_function_by_hand().  So we
only need to make sure the stack stays aligned, which can be easily
done by always allocating 16 bytes of stack space.

Calling gdbarch_breakpoint_from_pc() is also a bit overkill.  The
breakpoint instruction on i386 is pretty much fixed, we know it is
just a single byte and we know it can be placed just about anywhere.

So the simplified version below is perfectly adequate.  We have some
freedom on where to place the breakpoint in the 16-byte stack gap we
create.  I chose to put it up hight such that a small buffer overflow
isn't likely to overwrite the breakpoint instruction.

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.346
diff -u -p -r1.346 i386-tdep.c
--- i386-tdep.c	29 Feb 2012 14:59:41 -0000	1.346
+++ i386-tdep.c	27 Mar 2012 18:30:24 -0000
@@ -2327,6 +2327,21 @@ i386_16_byte_align_p (struct type *type)
 }
 
 static CORE_ADDR
+i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
+		      CORE_ADDR funcaddr,
+		      struct value **args, int nargs,
+		      struct type *value_type,
+		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
+		      struct regcache *regcache)
+{
+  *bp_addr = sp - 1;
+  *real_pc = funcaddr;
+
+  /* Keep the stack aligned.  */
+  return sp - 16;
+}
+
+static CORE_ADDR
 i386_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		      struct regcache *regcache, CORE_ADDR bp_addr, int nargs,
 		      struct value **args, CORE_ADDR sp, int struct_return,
@@ -7372,6 +7387,8 @@ i386_gdbarch_init (struct gdbarch_info i
   set_gdbarch_get_longjmp_target (gdbarch, i386_get_longjmp_target);
 
   /* Call dummy code.  */
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code(gdbarch, i386_push_dummy_code);
   set_gdbarch_push_dummy_call (gdbarch, i386_push_dummy_call);
   set_gdbarch_frame_align (gdbarch, i386_frame_align);
 

Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5

[Joel Brobecker]

Hi Jan,

> So the simplified version below is perfectly adequate.  We have some
> freedom on where to place the breakpoint in the 16-byte stack gap we
> create.  I chose to put it up hight such that a small buffer overflow
> isn't likely to overwrite the breakpoint instruction.

Just wanted to make sure that you'd be OK with Mark's version of
this part of your change... If that's the case, then all is good,
and as soon as part #1 is good to go, so should this one.

Thanks!

> Index: i386-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-tdep.c,v
> retrieving revision 1.346
> diff -u -p -r1.346 i386-tdep.c
> --- i386-tdep.c	29 Feb 2012 14:59:41 -0000	1.346
> +++ i386-tdep.c	27 Mar 2012 18:30:24 -0000
> @@ -2327,6 +2327,21 @@ i386_16_byte_align_p (struct type *type)
>  }
>  
>  static CORE_ADDR
> +i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
> +		      CORE_ADDR funcaddr,
> +		      struct value **args, int nargs,
> +		      struct type *value_type,
> +		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> +		      struct regcache *regcache)
> +{
> +  *bp_addr = sp - 1;
> +  *real_pc = funcaddr;
> +
> +  /* Keep the stack aligned.  */
> +  return sp - 16;
> +}
> +
> +static CORE_ADDR
>  i386_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>  		      struct regcache *regcache, CORE_ADDR bp_addr, int nargs,
>  		      struct value **args, CORE_ADDR sp, int struct_return,
> @@ -7372,6 +7387,8 @@ i386_gdbarch_init (struct gdbarch_info i
>    set_gdbarch_get_longjmp_target (gdbarch, i386_get_longjmp_target);
>  
>    /* Call dummy code.  */
> +  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
> +  set_gdbarch_push_dummy_code(gdbarch, i386_push_dummy_code);
>    set_gdbarch_push_dummy_call (gdbarch, i386_push_dummy_call);
>    set_gdbarch_frame_align (gdbarch, i386_frame_align);
>  

-- 
Joel

Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5

[Jan Kratochvil]

On Tue, 27 Mar 2012 20:53:37 +0200, Mark Kettenis wrote:
> > Date: Mon, 26 Mar 2012 21:04:14 +0200
> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > --- a/gdb/i386-tdep.c
> > +++ b/gdb/i386-tdep.c
> > @@ -2326,6 +2326,30 @@ i386_16_byte_align_p (struct type *type)
> >    return 0;
> >  }
> >  
> > +/* Implementation for set_gdbarch_push_dummy_code.  */
> > +
> > +static CORE_ADDR
> > +i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, CORE_ADDR funaddr,
> > +		      struct value **args, int nargs, struct type *value_type,
> > +		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> > +		      struct regcache *regcache)
> > +{
> > +  int bplen;
> > +  CORE_ADDR bppc = sp;
> > +
> > +  gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
> > +  sp -= bplen;
> > +
> > +  /* amd64_push_dummy_call does alignment on its own but i386_push_dummy_call
> > +     does not.  ABI requires stack alignment for executables using SSE.  */
> > +  if (gdbarch_frame_align_p (gdbarch))
> > +    sp = gdbarch_frame_align (gdbarch, sp);
> > +
> > +  *bp_addr = sp;
> > +  *real_pc = funaddr;
> > +  return sp;
> > +}
> 
> You're almost certainly right worrying about stack alignment.
> However, the comment doesn't make a lot of sense.

I still see the comment correct.  What is specifically wrong with its
statement?


> For one thing, amd64_push_dummy_call() doesn't explicitly align the stack.
> It just preserves alignment.

amd64_push_dummy_call calls amd64_push_arguments which does:
  /* The psABI says that "The end of the input argument area shall be
     aligned on a 16 byte boundary."  */
  sp &= ~0xf;

This will align any previously unaligned stack.  Therefore I do not agree.


> Also, if i386_push_dummy_call() doesn't preserve alignment (and it sure
> looks like it doesn't), then aligning the stack here doesn't help.

The patch helps (it makes working cases which did not work before), I am
unable to find a user visible problem with it.


> In any case, we don't need to to explicitly align
> the stack since that's already done bu call_function_by_hand().  So we
> only need to make sure the stack stays aligned, which can be easily
> done by always allocating 16 bytes of stack space.

Both patches are wrong anyway, they do not fix the new KFAIL testcase below
which I will check in.



> Calling gdbarch_breakpoint_from_pc() is also a bit overkill.  The
> breakpoint instruction on i386 is pretty much fixed, we know it is
> just a single byte and we know it can be placed just about anywhere.

It is again about different coding style.


> So the simplified version below is perfectly adequate.  We have some
> freedom on where to place the breakpoint in the 16-byte stack gap we
> create.  I chose to put it up hight such that a small buffer overflow
> isn't likely to overwrite the breakpoint instruction.

OK, I will check in the discussed patch (not the one below) with:

2012-06-11  Mark Kettenis  <kettenis@gnu.org>
            Jan Kratochvil  <jan.kratochvil@redhat.com>

	* amd64-dicos-tdep.c (amd64_dicos_push_dummy_code): Remove.
	(amd64_dicos_init_abi): Remove its installment.
	* dicos-tdep.c (dicos_init_abi): Remove the
	set_gdbarch_call_dummy_location call.  Update the comment here.
	* i386-dicos-tdep.c (i386_dicos_push_dummy_code): Remove.
	(i386_dicos_init_abi): Remove its installment.
	* i386-tdep.c (i386_push_dummy_code): New function.
	(i386_gdbarch_init): Call set_gdbarch_call_dummy_location, install
	i386_push_dummy_code.


Thanks,
Jan


gdb/testsuite/
2012-06-11  Jan Kratochvil  <jan.kratochvil@redhat.com>

	New KFAIL for PR tdep/14222
	* gdb.arch/i386-sse-stack-align.S: New file.
	* gdb.arch/i386-sse-stack-align.c: New file.
	* gdb.arch/i386-sse-stack-align.exp: New file.

diff --git a/gdb/testsuite/gdb.arch/i386-sse-stack-align.S b/gdb/testsuite/gdb.arch/i386-sse-stack-align.S
new file mode 100755
index 0000000..9411994
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-sse-stack-align.S
@@ -0,0 +1,120 @@
+/* Copyright 2012 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/*
+   gcc -S -o gdb.arch/i386-sse-stack-align.{S,c} -Wall -m32 -msse
+ */
+
+	.file	"i386-sse-stack-align.c"
+	.text
+	.type	foo, @function
+foo:
+.LFB0:
+	.cfi_startproc
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	.cfi_offset 5, -8
+	movl	%esp, %ebp
+	.cfi_def_cfa_register 5
+	subl	$40, %esp
+	movaps	%xmm0, -24(%ebp)
+	movaps	%xmm1, -40(%ebp)
+	movaps	-24(%ebp), %xmm0
+	movaps	-40(%ebp), %xmm1
+	mulps	%xmm1, %xmm0
+	addps	-24(%ebp), %xmm0
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	foo, .-foo
+	.type	f, @function
+f:
+.LFB1:
+	.cfi_startproc
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	.cfi_offset 5, -8
+	movl	%esp, %ebp
+	.cfi_def_cfa_register 5
+	subl	$40, %esp
+	movaps	.LC0, %xmm0
+	movaps	%xmm0, -24(%ebp)
+	movaps	-24(%ebp), %xmm1
+	movaps	-24(%ebp), %xmm0
+	call	foo
+	movaps	%xmm0, -40(%ebp)
+	leal	-40(%ebp), %eax
+	movss	(%eax), %xmm0
+	cvttss2si	%xmm0, %eax
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	f, .-f
+	.type	g, @function
+g:
+.LFB2:
+	.cfi_startproc
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	.cfi_offset 5, -8
+	movl	%esp, %ebp
+	.cfi_def_cfa_register 5
+	subl	$8, %esp
+	call	f
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+	ret
+	.cfi_endproc
+.LFE2:
+	.size	g, .-g
+	.globl	main
+	.type	main, @function
+main:
+.LFB3:
+	.cfi_startproc
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	.cfi_offset 5, -8
+	movl	%esp, %ebp
+	.cfi_def_cfa_register 5
+	andl	$-16, %esp
+	subl	$16, %esp
+	movl	$1, (%esp)
+	call	g
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+	ret
+	.cfi_endproc
+.LFE3:
+	.size	main, .-main
+	.section	.rodata
+	.align 16
+.LC0:
+	.long	1065353216
+	.long	1073741824
+	.long	1077936128
+	.long	1082130432
+	.ident	"GCC: (GNU) 4.6.4 20120611 (prerelease)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.arch/i386-sse-stack-align.c b/gdb/testsuite/gdb.arch/i386-sse-stack-align.c
new file mode 100644
index 0000000..7e856b1
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-sse-stack-align.c
@@ -0,0 +1,46 @@
+/* Copyright 2012 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+typedef float V __attribute__((vector_size(16)));
+
+static V
+foo (V a, V b)
+{
+  return a + b * a;
+}
+
+static __attribute__((noinline,noclone)) int
+f (void)
+{
+  volatile V a = { 1, 2, 3, 4 };
+  volatile V b;
+
+  b = foo (a, a);
+  return b[0];
+}
+
+static __attribute__((noinline,noclone)) int
+g (int x)
+{
+  return f ();
+}
+
+int
+main (void)
+{
+  return g (1);
+}
diff --git a/gdb/testsuite/gdb.arch/i386-sse-stack-align.exp b/gdb/testsuite/gdb.arch/i386-sse-stack-align.exp
new file mode 100644
index 0000000..b2a2458
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-sse-stack-align.exp
@@ -0,0 +1,52 @@
+# Copyright 2012 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if ![is_x86_like_target] {
+    verbose "Skipping x86 SSE stack alignment tests."
+    return
+}
+
+set testfile "i386-sse-stack-align"
+set srcfile ${testfile}.S
+set csrcfile ${testfile}.c
+set executable ${testfile}
+set binfile ${objdir}/${subdir}/${executable}
+set opts {}
+
+if [info exists COMPILE] {
+    set srcfile ${csrcfile}
+    lappend opts debug optimize=-O2 additional_flags=-msse
+}
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $opts] != "" } {
+    unsupported "cannot compile ${srcfile}"
+    return -1
+}
+
+clean_restart $executable
+
+if ![runto_main] then {
+    return -1
+}
+
+set test "print g (1)"
+gdb_test_multiple $test $test {
+    -re " = 2\r\n$gdb_prompt $" {
+	pass $test
+    }
+    -re "Program received signal SIGSEGV, Segmentation fault\\..*\r\n$gdb_prompt $" {
+	kfail tdep/14222 $test
+    }
+}

Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5

[Mark Kettenis]

> Date: Mon, 11 Jun 2012 21:10:08 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> On Tue, 27 Mar 2012 20:53:37 +0200, Mark Kettenis wrote:
> > > Date: Mon, 26 Mar 2012 21:04:14 +0200
> > > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > > --- a/gdb/i386-tdep.c
> > > +++ b/gdb/i386-tdep.c
> > > @@ -2326,6 +2326,30 @@ i386_16_byte_align_p (struct type *type)
> > >    return 0;
> > >  }
> > >  
> > > +/* Implementation for set_gdbarch_push_dummy_code.  */
> > > +
> > > +static CORE_ADDR
> > > +i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, CORE_ADDR funaddr,
> > > +		      struct value **args, int nargs, struct type *value_type,
> > > +		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> > > +		      struct regcache *regcache)
> > > +{
> > > +  int bplen;
> > > +  CORE_ADDR bppc = sp;
> > > +
> > > +  gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
> > > +  sp -= bplen;
> > > +
> > > +  /* amd64_push_dummy_call does alignment on its own but i386_push_dummy_call
> > > +     does not.  ABI requires stack alignment for executables using SSE.  */
> > > +  if (gdbarch_frame_align_p (gdbarch))
> > > +    sp = gdbarch_frame_align (gdbarch, sp);
> > > +
> > > +  *bp_addr = sp;
> > > +  *real_pc = funaddr;
> > > +  return sp;
> > > +}
> > 
> > You're almost certainly right worrying about stack alignment.
> > However, the comment doesn't make a lot of sense.
> 
> I still see the comment correct.  What is specifically wrong with its
> statement?

Well, for one thing it raises the question whether why
i386_push_dummy_call() doesn't properly align the stack.  But it
really isn't clear to me what you're trying to say with this comment.

> > For one thing, amd64_push_dummy_call() doesn't explicitly align the stack.
> > It just preserves alignment.
> 
> amd64_push_dummy_call calls amd64_push_arguments which does:
>   /* The psABI says that "The end of the input argument area shall be
>      aligned on a 16 byte boundary."  */
>   sp &= ~0xf;
> 
> This will align any previously unaligned stack.  Therefore I do not agree.

Right.  Missed that.

> > Also, if i386_push_dummy_call() doesn't preserve alignment (and it sure
> > looks like it doesn't), then aligning the stack here doesn't help.
> 
> The patch helps (it makes working cases which did not work before), I am
> unable to find a user visible problem with it.

Sorry, but that must be sheer luck.

> > In any case, we don't need to to explicitly align
> > the stack since that's already done bu call_function_by_hand().  So we
> > only need to make sure the stack stays aligned, which can be easily
> > done by always allocating 16 bytes of stack space.
> 
> Both patches are wrong anyway, they do not fix the new KFAIL testcase below
> which I will check in.

Like I said, that probably requires i386_push_dummy_call() to properly
align the stack just like amd64_push_dummy_call() does.

> > Calling gdbarch_breakpoint_from_pc() is also a bit overkill.  The
> > breakpoint instruction on i386 is pretty much fixed, we know it is
> > just a single byte and we know it can be placed just about anywhere.
> 
> It is again about different coding style.

Well, KISS is an important engineering principle if you ask me.

> > So the simplified version below is perfectly adequate.  We have some
> > freedom on where to place the breakpoint in the 16-byte stack gap we
> > create.  I chose to put it up hight such that a small buffer overflow
> > isn't likely to overwrite the breakpoint instruction.
> 
> OK, I will check in the discussed patch (not the one below) with:
> 
> 2012-06-11  Mark Kettenis  <kettenis@gnu.org>
>             Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* amd64-dicos-tdep.c (amd64_dicos_push_dummy_code): Remove.
> 	(amd64_dicos_init_abi): Remove its installment.
> 	* dicos-tdep.c (dicos_init_abi): Remove the
> 	set_gdbarch_call_dummy_location call.  Update the comment here.
> 	* i386-dicos-tdep.c (i386_dicos_push_dummy_code): Remove.
> 	(i386_dicos_init_abi): Remove its installment.
> 	* i386-tdep.c (i386_push_dummy_code): New function.
> 	(i386_gdbarch_init): Call set_gdbarch_call_dummy_location, install
> 	i386_push_dummy_code.

Fine.

> gdb/testsuite/
> 2012-06-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	New KFAIL for PR tdep/14222
> 	* gdb.arch/i386-sse-stack-align.S: New file.
> 	* gdb.arch/i386-sse-stack-align.c: New file.
> 	* gdb.arch/i386-sse-stack-align.exp: New file.
> 
> diff --git a/gdb/testsuite/gdb.arch/i386-sse-stack-align.S b/gdb/testsuite/gdb.arch/i386-sse-stack-align.S
> new file mode 100755
> index 0000000..9411994
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-sse-stack-align.S
> @@ -0,0 +1,120 @@
> +/* Copyright 2012 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/*
> +   gcc -S -o gdb.arch/i386-sse-stack-align.{S,c} -Wall -m32 -msse
> + */
> +
> +	.file	"i386-sse-stack-align.c"
> +	.text
> +	.type	foo, @function
> +foo:
> +.LFB0:
> +	.cfi_startproc
> +	pushl	%ebp
> +	.cfi_def_cfa_offset 8
> +	.cfi_offset 5, -8
> +	movl	%esp, %ebp
> +	.cfi_def_cfa_register 5
> +	subl	$40, %esp
> +	movaps	%xmm0, -24(%ebp)
> +	movaps	%xmm1, -40(%ebp)
> +	movaps	-24(%ebp), %xmm0
> +	movaps	-40(%ebp), %xmm1
> +	mulps	%xmm1, %xmm0
> +	addps	-24(%ebp), %xmm0
> +	leave
> +	.cfi_restore 5
> +	.cfi_def_cfa 4, 4
> +	ret
> +	.cfi_endproc
> +.LFE0:
> +	.size	foo, .-foo
> +	.type	f, @function
> +f:
> +.LFB1:
> +	.cfi_startproc
> +	pushl	%ebp
> +	.cfi_def_cfa_offset 8
> +	.cfi_offset 5, -8
> +	movl	%esp, %ebp
> +	.cfi_def_cfa_register 5
> +	subl	$40, %esp
> +	movaps	.LC0, %xmm0
> +	movaps	%xmm0, -24(%ebp)
> +	movaps	-24(%ebp), %xmm1
> +	movaps	-24(%ebp), %xmm0
> +	call	foo
> +	movaps	%xmm0, -40(%ebp)
> +	leal	-40(%ebp), %eax
> +	movss	(%eax), %xmm0
> +	cvttss2si	%xmm0, %eax
> +	leave
> +	.cfi_restore 5
> +	.cfi_def_cfa 4, 4
> +	ret
> +	.cfi_endproc
> +.LFE1:
> +	.size	f, .-f
> +	.type	g, @function
> +g:
> +.LFB2:
> +	.cfi_startproc
> +	pushl	%ebp
> +	.cfi_def_cfa_offset 8
> +	.cfi_offset 5, -8
> +	movl	%esp, %ebp
> +	.cfi_def_cfa_register 5
> +	subl	$8, %esp
> +	call	f
> +	leave
> +	.cfi_restore 5
> +	.cfi_def_cfa 4, 4
> +	ret
> +	.cfi_endproc
> +.LFE2:
> +	.size	g, .-g
> +	.globl	main
> +	.type	main, @function
> +main:
> +.LFB3:
> +	.cfi_startproc
> +	pushl	%ebp
> +	.cfi_def_cfa_offset 8
> +	.cfi_offset 5, -8
> +	movl	%esp, %ebp
> +	.cfi_def_cfa_register 5
> +	andl	$-16, %esp
> +	subl	$16, %esp
> +	movl	$1, (%esp)
> +	call	g
> +	leave
> +	.cfi_restore 5
> +	.cfi_def_cfa 4, 4
> +	ret
> +	.cfi_endproc
> +.LFE3:
> +	.size	main, .-main
> +	.section	.rodata
> +	.align 16
> +.LC0:
> +	.long	1065353216
> +	.long	1073741824
> +	.long	1077936128
> +	.long	1082130432
> +	.ident	"GCC: (GNU) 4.6.4 20120611 (prerelease)"
> +	.section	.note.GNU-stack,"",@progbits
> diff --git a/gdb/testsuite/gdb.arch/i386-sse-stack-align.c b/gdb/testsuite/gdb.arch/i386-sse-stack-align.c
> new file mode 100644
> index 0000000..7e856b1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-sse-stack-align.c
> @@ -0,0 +1,46 @@
> +/* Copyright 2012 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +typedef float V __attribute__((vector_size(16)));
> +
> +static V
> +foo (V a, V b)
> +{
> +  return a + b * a;
> +}
> +
> +static __attribute__((noinline,noclone)) int
> +f (void)
> +{
> +  volatile V a = { 1, 2, 3, 4 };
> +  volatile V b;
> +
> +  b = foo (a, a);
> +  return b[0];
> +}
> +
> +static __attribute__((noinline,noclone)) int
> +g (int x)
> +{
> +  return f ();
> +}
> +
> +int
> +main (void)
> +{
> +  return g (1);
> +}
> diff --git a/gdb/testsuite/gdb.arch/i386-sse-stack-align.exp b/gdb/testsuite/gdb.arch/i386-sse-stack-align.exp
> new file mode 100644
> index 0000000..b2a2458
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-sse-stack-align.exp
> @@ -0,0 +1,52 @@
> +# Copyright 2012 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +if ![is_x86_like_target] {
> +    verbose "Skipping x86 SSE stack alignment tests."
> +    return
> +}
> +
> +set testfile "i386-sse-stack-align"
> +set srcfile ${testfile}.S
> +set csrcfile ${testfile}.c
> +set executable ${testfile}
> +set binfile ${objdir}/${subdir}/${executable}
> +set opts {}
> +
> +if [info exists COMPILE] {
> +    set srcfile ${csrcfile}
> +    lappend opts debug optimize=-O2 additional_flags=-msse
> +}
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $opts] != "" } {
> +    unsupported "cannot compile ${srcfile}"
> +    return -1
> +}
> +
> +clean_restart $executable
> +
> +if ![runto_main] then {
> +    return -1
> +}
> +
> +set test "print g (1)"
> +gdb_test_multiple $test $test {
> +    -re " = 2\r\n$gdb_prompt $" {
> +	pass $test
> +    }
> +    -re "Program received signal SIGSEGV, Segmentation fault\\..*\r\n$gdb_prompt $" {
> +	kfail tdep/14222 $test
> +    }
> +}
> 

Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5

[Jan Kratochvil]

On Mon, 11 Jun 2012 23:29:08 +0200, Mark Kettenis wrote:
> > On Tue, 27 Mar 2012 20:53:37 +0200, Mark Kettenis wrote:
> > > Also, if i386_push_dummy_call() doesn't preserve alignment (and it sure
> > > looks like it doesn't), then aligning the stack here doesn't help.
> > 
> > The patch helps (it makes working cases which did not work before), I am
> > unable to find a user visible problem with it.
> 
> Sorry, but that must be sheer luck.

Here is no luck, your patch also does not do anything good with the alignment
as the new KFAIL tdep/14222 proves.  16 bytes shift is still better than
1 byte shift (as was in amd64-dicos-tdep.c and i386-dicos-tdep.c) as 1 byte
shift always breaks SSE inferiors.  But whether 16 bytes subtraction or
16 bytes modulo works is just the "sheer luck" depending on this or that case.

I will check GCC parameters alignment and fix PR tdep/14222 first otherwise
the code does not make sense.


> Like I said, that probably requires i386_push_dummy_call() to properly
> align the stack just like amd64_push_dummy_call() does.

Yes, PR tdep/14222.


> > It is again about different coding style.
> 
> Well, KISS is an important engineering principle if you ask me.

(a) The previous code in amd64-dicos-tdep.c and i386-dicos-tdep.c already used
    gdbarch_breakpoint_from_pc.  So moving a code is more simple patch than
    moving + changing the code.
(b) Your code:
      *bp_addr = sp - 1;
    I find definitely less readable than
      gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
    What is "- 1" there?  There is no comment for it.  If anything you should
    have written:
      /* 1 is size of the i386 breakpoint instruction.  */
      *bp_addr = sp - 1;
    But in such case I find more obvious it is more readable to put there just:
      gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);


Jan