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

[Jan Kratochvil <jan dot kratochvil at redhat dot 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?


> 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
+    }
+}