From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3961 invoked by alias); 11 Jun 2012 21:29:34 -0000 Received: (qmail 3949 invoked by uid 22791); 11 Jun 2012 21:29:33 -0000 X-SWARE-Spam-Status: No, hits=-2.8 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,TW_EG,TW_VT,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 11 Jun 2012 21:29:18 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3) with ESMTP id q5BLTBl8029569; Mon, 11 Jun 2012 23:29:11 +0200 (CEST) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3/Submit) id q5BLT8ck007831; Mon, 11 Jun 2012 23:29:08 +0200 (CEST) Date: Mon, 11 Jun 2012 21:29:00 -0000 Message-Id: <201206112129.q5BLT8ck007831@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: jan.kratochvil@redhat.com CC: gdb-patches@sourceware.org In-reply-to: <20120611191008.GA29811@host2.jankratochvil.net> (message from Jan Kratochvil on Mon, 11 Jun 2012 21:10:08 +0200) 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> 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/msg00319.txt.bz2 > Date: Mon, 11 Jun 2012 21:10:08 +0200 > From: Jan Kratochvil > > 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? 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 > 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. Fine. > 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 > + } > +} >