* [patch 2/3] Align stack for SSE (PR tdep/14222)
@ 2012-06-13 15:50 Jan Kratochvil
2012-06-13 21:20 ` Mark Kettenis
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kratochvil @ 2012-06-13 15:50 UTC (permalink / raw)
To: gdb-patches
Hi,
this is mostly independent patch. But it does not make sense to discuss
alignment in [patch 3/3] when it has been already broken.
http://groups.google.com/group/ia32-abi/browse_thread/thread/4f9b3e5069943bf1
says that gcc-4.6 aligns stack to 32 bytes. I do not see it anywhere:
80482e8: 83 e4 f0 and $0xfffffff0,%esp
It is aligned just to 16 bytes. Also checked that Intel CPU documentation
does not require any more alignment than 16 bytes (sure it may be less
effective but traps never occur). Therefore I consider amd64-tdep.c to be
correct and I have not changed its existing 16-bytes alignment.
No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu.
Thanks,
Jan
gdb/
2012-06-13 Jan Kratochvil <jan.kratochvil@redhat.com>
PR tdep/14222
* i386-tdep.c (i386_push_dummy_call): Align to 16 bytes unconditionally.
gdb/testsuite/
2012-06-13 Jan Kratochvil <jan.kratochvil@redhat.com>
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.
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2399,9 +2399,11 @@ i386_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
if (!write_pass)
{
- if (have_16_byte_aligned_arg)
- args_space = align_up (args_space, 16);
sp -= args_space;
+
+ /* ABI (since gcc-4.6?) says that caller should have SP aligned on
+ a 16 byte boundary. */
+ sp &= ~0xf;
}
}
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-sse-stack-align.S
@@ -0,0 +1,214 @@
+/* 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 g0, @function
+g0:
+.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 g0, .-g0
+ .type g1, @function
+g1:
+.LFB3:
+ .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
+.LFE3:
+ .size g1, .-g1
+ .type g2, @function
+g2:
+.LFB4:
+ .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
+.LFE4:
+ .size g2, .-g2
+ .type g3, @function
+g3:
+.LFB5:
+ .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
+.LFE5:
+ .size g3, .-g3
+ .type g4, @function
+g4:
+.LFB6:
+ .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
+.LFE6:
+ .size g4, .-g4
+ .globl main
+ .type main, @function
+main:
+.LFB7:
+ .cfi_startproc
+ pushl %ebp
+ .cfi_def_cfa_offset 8
+ .cfi_offset 5, -8
+ movl %esp, %ebp
+ .cfi_def_cfa_register 5
+ pushl %ebx
+ andl $-16, %esp
+ subl $16, %esp
+ .cfi_offset 3, -12
+ call g0
+ movl %eax, %ebx
+ movl $1, (%esp)
+ call g1
+ addl %eax, %ebx
+ movl $2, 4(%esp)
+ movl $1, (%esp)
+ call g2
+ addl %eax, %ebx
+ movl $3, 8(%esp)
+ movl $2, 4(%esp)
+ movl $1, (%esp)
+ call g3
+ addl %eax, %ebx
+ movl $4, 12(%esp)
+ movl $3, 8(%esp)
+ movl $2, 4(%esp)
+ movl $1, (%esp)
+ call g4
+ addl %ebx, %eax
+ movl -4(%ebp), %ebx
+ leave
+ .cfi_restore 5
+ .cfi_def_cfa 4, 4
+ .cfi_restore 3
+ ret
+ .cfi_endproc
+.LFE7:
+ .size main, .-main
+ .section .rodata
+ .align 16
+.LC0:
+ .long 1065353216
+ .long 1073741824
+ .long 1077936128
+ .long 1082130432
+ .ident "GCC: (GNU) 4.6.4 20120612 (prerelease)"
+ .section .note.GNU-stack,"",@progbits
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-sse-stack-align.c
@@ -0,0 +1,70 @@
+/* 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
+g0 (void)
+{
+ return f ();
+}
+
+static __attribute__((noinline, noclone)) int
+g1 (int p1)
+{
+ return f ();
+}
+
+static __attribute__((noinline, noclone)) int
+g2 (int p1, int p2)
+{
+ return f ();
+}
+
+static __attribute__((noinline, noclone)) int
+g3 (int p1, int p2, int p3)
+{
+ return f ();
+}
+
+static __attribute__((noinline, noclone)) int
+g4 (int p1, int p2, int p3, int p4)
+{
+ return f ();
+}
+
+int
+main (void)
+{
+ return g0 () + g1 (1) + g2 (1, 2) + g3 (1, 2, 3) + g4 (1, 2, 3, 4);
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-sse-stack-align.exp
@@ -0,0 +1,60 @@
+# 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 args ""
+foreach i {0 1 2 3 4} {
+ set test "print g$i ($args)"
+ gdb_test_multiple $test $test {
+ -re " = 2\r\n$gdb_prompt $" {
+ pass $test
+ }
+ -re "Program received signal SIGSEGV, Segmentation fault\\..*\r\n$gdb_prompt $" {
+ fail $test
+ }
+ }
+
+ if {$args != ""} {
+ set args "$args, "
+ }
+ set args "$args[expr $i + 1]"
+}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch 2/3] Align stack for SSE (PR tdep/14222)
2012-06-13 15:50 [patch 2/3] Align stack for SSE (PR tdep/14222) Jan Kratochvil
@ 2012-06-13 21:20 ` Mark Kettenis
2012-06-13 21:31 ` Jan Kratochvil
0 siblings, 1 reply; 3+ messages in thread
From: Mark Kettenis @ 2012-06-13 21:20 UTC (permalink / raw)
To: jan.kratochvil; +Cc: gdb-patches
> Date: Wed, 13 Jun 2012 17:50:20 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
>
> Hi,
>
> this is mostly independent patch. But it does not make sense to discuss
> alignment in [patch 3/3] when it has been already broken.
>
> http://groups.google.com/group/ia32-abi/browse_thread/thread/4f9b3e5069943bf1
> says that gcc-4.6 aligns stack to 32 bytes. I do not see it anywhere:
> 80482e8: 83 e4 f0 and $0xfffffff0,%esp
> It is aligned just to 16 bytes. Also checked that Intel CPU documentation
> does not require any more alignment than 16 bytes (sure it may be less
> effective but traps never occur). Therefore I consider amd64-tdep.c to be
> correct and I have not changed its existing 16-bytes alignment.
>
> gdb/
> 2012-06-13 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> PR tdep/14222
> * i386-tdep.c (i386_push_dummy_call): Align to 16 bytes unconditionally.
Committed a better diff (see below) that removes the now unused
have_16_byte_aligned_arg variable.
> gdb/testsuite/
> 2012-06-13 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> 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.
Can you commit this testsuite bit?
2012-06-13 Mark Kettenis <kettenis@gnu.org>
Jan Kratochvil <jan.kratochvil@redhat.com>
PR tdep/14222
* i386-tdep.c (i386_push_dummy_call): Unconditionally align the
stack on a 16-byte boundary.
Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.352
diff -u -p -r1.352 i386-tdep.c
--- i386-tdep.c 13 Jun 2012 20:29:15 -0000 1.352
+++ i386-tdep.c 13 Jun 2012 21:15:44 -0000
@@ -2351,7 +2351,6 @@ i386_push_dummy_call (struct gdbarch *gd
for (write_pass = 0; write_pass < 2; write_pass++)
{
int args_space_used = 0;
- int have_16_byte_aligned_arg = 0;
if (struct_return)
{
@@ -2389,19 +2388,20 @@ i386_push_dummy_call (struct gdbarch *gd
else
{
if (i386_16_byte_align_p (value_enclosing_type (args[i])))
- {
- args_space = align_up (args_space, 16);
- have_16_byte_aligned_arg = 1;
- }
+ args_space = align_up (args_space, 16);
args_space += align_up (len, 4);
}
}
if (!write_pass)
{
- if (have_16_byte_aligned_arg)
- args_space = align_up (args_space, 16);
sp -= args_space;
+
+ /* The original System V ABI only requires word alignment,
+ but modern incarnations need 16-byte alignment in order
+ to support SSE. Since wasting a few bytes here isn't
+ harmful we unconditionally enforce 16-byte alignment. */
+ sp &= ~0xf;
}
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch 2/3] Align stack for SSE (PR tdep/14222)
2012-06-13 21:20 ` Mark Kettenis
@ 2012-06-13 21:31 ` Jan Kratochvil
0 siblings, 0 replies; 3+ messages in thread
From: Jan Kratochvil @ 2012-06-13 21:31 UTC (permalink / raw)
To: Mark Kettenis; +Cc: gdb-patches
On Wed, 13 Jun 2012 23:19:28 +0200, Mark Kettenis wrote:
> Committed a better diff (see below) that removes the now unused
> have_16_byte_aligned_arg variable.
True, thanks.
> > gdb/testsuite/
> > 2012-06-13 Jan Kratochvil <jan.kratochvil@redhat.com>
> >
> > 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.
>
> Can you commit this testsuite bit?
It is easier during regressions analysis to have fixes + tests together in
a single commit.
Checked in:
http://sourceware.org/ml/gdb-cvs/2012-06/msg00103.html
Jan
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-06-13 21:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-13 15:50 [patch 2/3] Align stack for SSE (PR tdep/14222) Jan Kratochvil
2012-06-13 21:20 ` Mark Kettenis
2012-06-13 21:31 ` Jan Kratochvil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox