From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2218 invoked by alias); 11 Jun 2012 19:10:48 -0000 Received: (qmail 1908 invoked by uid 22791); 11 Jun 2012 19:10:43 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,TW_EG,TW_VT,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 11 Jun 2012 19:10:16 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q5BJAFLZ029680 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 11 Jun 2012 15:10:16 -0400 Received: from host2.jankratochvil.net (ovpn-116-33.ams2.redhat.com [10.36.116.33]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q5BJA9r7005852 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 11 Jun 2012 15:10:11 -0400 Date: Mon, 11 Jun 2012 19:10:00 -0000 From: Jan Kratochvil To: Mark Kettenis Cc: gdb-patches@sourceware.org Subject: Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5 Message-ID: <20120611191008.GA29811@host2.jankratochvil.net> References: <20120309210117.GB30432@host2.jankratochvil.net> <20120326190414.GB11001@host2.jankratochvil.net> <201203271853.q2RIrbWf024897@glazunov.sibelius.xs4all.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201203271853.q2RIrbWf024897@glazunov.sibelius.xs4all.nl> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-06/txt/msg00305.txt.bz2 On Tue, 27 Mar 2012 20:53:37 +0200, Mark Kettenis wrote: > > Date: Mon, 26 Mar 2012 21:04:14 +0200 > > From: Jan Kratochvil > > --- 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 Jan Kratochvil * 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 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 . */ + +/* + 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 . */ + +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 . + +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 + } +}