This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]