This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5
- From: Mark Kettenis <mark dot kettenis at xs4all dot nl>
- To: jan dot kratochvil at redhat dot com
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 11 Jun 2012 23:29:08 +0200 (CEST)
- Subject: Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5
- References: <20120309210117.GB30432@host2.jankratochvil.net> <20120326190414.GB11001@host2.jankratochvil.net> <201203271853.q2RIrbWf024897@glazunov.sibelius.xs4all.nl> <20120611191008.GA29811@host2.jankratochvil.net>
> 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
> + }
> +}
>