From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28614 invoked by alias); 17 Jan 2014 21:31:38 -0000 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 Received: (qmail 28603 invoked by uid 89); 17 Jan 2014 21:31:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 17 Jan 2014 21:31:36 +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 s0HLVY0c005576 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 17 Jan 2014 16:31:35 -0500 Received: from psique (ovpn-113-91.phx2.redhat.com [10.3.113.91]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s0HLVV0R012306 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO) for ; Fri, 17 Jan 2014 16:31:32 -0500 From: Sergio Durigan Junior To: GDB Patches Subject: Re: [PATCH] Fix for PR tdep/16397: SystemTap SDT probe support for x86 doesn't work with "triplet operands" References: X-URL: http://www.redhat.com Date: Fri, 17 Jan 2014 21:31:00 -0000 In-Reply-To: (Sergio Durigan Junior's message of "Sun, 12 Jan 2014 02:06:32 -0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-01/txt/msg00684.txt.bz2 On Sunday, January 12 2014, I wrote: > Hi, > > This is the continuation of what Joel proposed on: > > Ping. > Now that I have already submitted and pushed the patch to split > i386_stap_parse_special_token into two smaller functions, it is indeed > simpler to understand this patch. > > It occurs because, on x86, triplet displacement operands are allowed > (like "-4+8-20(%rbp)"), and the current parser for this expression is > buggy. It does not correctly extract the register name from the > expression, which leads to incorrect evaluation. The parser was also > being very "generous" with the expression, so I included a few more > checks to ensure that we're indeed dealing with a triplet displacement > operand. > > This patch also includes testcases for the two different kind of > expressions that can be encountered on x86: the triplet displacement > (explained above) and the three-argument displacement (as in > "(%rbx,%ebx,-8)"). The tests are obviously arch-dependent and are > placed under gdb.arch/. > > OK to apply? > > -- > Sergio > > gdb/ > 2014-01-12 Sergio Durigan Junior > > PR tdep/16397 > * i386-tdep.c (i386_stap_parse_special_token_triplet): Check if a > number comes after the + or - signs. Adjust length of register > name to be extracted. > > gdb/testsuite > 2014-01-12 Sergio Durigan Junior > > PR tdep/16397 > * gdb.arch/amd64-stap-special-operands.exp: New file. > * gdb.arch/amd64-stap-three-arg-disp.S: Likewise. > * gdb.arch/amd64-stap-three-arg-disp.c: Likewise. > * gdb.arch/amd64-stap-triplet.S: Likewise. > * gdb.arch/amd64-stap-triplet.c: Likewise. > > diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c > index 9d1d9e0..26d5d8f 100644 > --- a/gdb/i386-tdep.c > +++ b/gdb/i386-tdep.c > @@ -3639,6 +3639,9 @@ i386_stap_parse_special_token_triplet (struct gdbarch *gdbarch, > got_minus[0] = 1; > } > > + if (!isdigit (*s)) > + return 0; > + > displacements[0] = strtol (s, &endp, 10); > s = endp; > > @@ -3657,6 +3660,9 @@ i386_stap_parse_special_token_triplet (struct gdbarch *gdbarch, > got_minus[1] = 1; > } > > + if (!isdigit (*s)) > + return 0; > + > displacements[1] = strtol (s, &endp, 10); > s = endp; > > @@ -3675,6 +3681,9 @@ i386_stap_parse_special_token_triplet (struct gdbarch *gdbarch, > got_minus[2] = 1; > } > > + if (!isdigit (*s)) > + return 0; > + > displacements[2] = strtol (s, &endp, 10); > s = endp; > > @@ -3690,7 +3699,7 @@ i386_stap_parse_special_token_triplet (struct gdbarch *gdbarch, > if (*s++ != ')') > return 0; > > - len = s - start; > + len = s - start - 1; > regname = alloca (len + 1); > > strncpy (regname, start, len); > diff --git a/gdb/testsuite/gdb.arch/amd64-stap-special-operands.exp b/gdb/testsuite/gdb.arch/amd64-stap-special-operands.exp > new file mode 100644 > index 0000000..f80dfdf > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-stap-special-operands.exp > @@ -0,0 +1,47 @@ > +# Copyright 2013-2014 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 { ![istarget "x86_64-*-*"] && ![istarget "i?86-*-*"] } { > + verbose "Skipping amd64-stap-special-operands.exp" > + return > +} > + > +proc test_probe { probe_name } { > + if { ![runto "-pstap $probe_name"] } { > + fail "run to probe $probe_name" > + return > + } > + > + gdb_test "print \$_probe_argc" " = 1" > + gdb_test "print \$_probe_arg0" " = 10" > +} > + > +standard_testfile amd64-stap-triplet.S > + > +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } { > + untested amd64-stap-special-operands.exp > + return -1 > +} > + > +test_probe "triplet" > + > +standard_testfile amd64-stap-three-arg-disp.S > + > +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } { > + untested amd64-stap-special-operands.exp > + return -1 > +} > + > +test_probe "three_arg" > diff --git a/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.S b/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.S > new file mode 100644 > index 0000000..1e2905c > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.S > @@ -0,0 +1,91 @@ > +/* Copyright (C) 2013-2014 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 . > + > + This file was generated from the equivalent .c file using the > + following command: > + > + #> gcc -S amd64-stap-three-arg-disp.c -o amd64-stap-three-arg-disp.S > + > + Then, the SystemTap SDT probe definition below was tweaked. See below > + for more details. */ > + > + .file "amd64-stap-three-arg-disp.c" > + .text > + .globl main > + .type main, @function > +main: > +.LFB0: > + .cfi_startproc > +# BLOCK 2 seq:0 > +# PRED: ENTRY (fallthru) > + pushq %rbp > + .cfi_def_cfa_offset 16 > + .cfi_offset 6, -16 > + movq %rsp, %rbp > + .cfi_def_cfa_register 6 > + movl %edi, -20(%rbp) > + movq %rsi, -32(%rbp) > + movl $10, -4(%rbp) > +#APP > +# 8 "amd64-stap-three-arg-disp.c" 1 > + 990: nop > +.pushsection .note.stapsdt,"?","note" > +.balign 4 > +.4byte 992f-991f,994f-993f,3 > +991: .asciz "stapsdt" > +992: .balign 4 > +993: .8byte 990b > +.8byte _.stapsdt.base > +.8byte 0 > +.asciz "test" > +.asciz "three_arg" > +/* The probe's argument definition below was tweaked in order to mimic a > + three arg displacement in x86 asm. The original probe argument was: > + > + -4@-4(%rbp) > + > + The argument below is equivalent to that. */ > +.asciz "-4@-4(%rbp,%ebx,0)" > +994: .balign 4 > +.popsection > + > +# 0 "" 2 > +# 8 "amd64-stap-three-arg-disp.c" 1 > + .ifndef _.stapsdt.base > +.pushsection .stapsdt.base,"aG","progbits",.stapsdt.base,comdat > +.weak _.stapsdt.base > +.hidden _.stapsdt.base > +_.stapsdt.base: .space 1 > +.size _.stapsdt.base,1 > +.popsection > +.endif > + > +# 0 "" 2 > +#NO_APP > + movl $0, %eax > +/* Here, we zero-out %ebx in order to use it safely when evaluating > + the probe argument. */ > + movl $0, %ebx > + popq %rbp > + .cfi_def_cfa 7, 8 > +# SUCC: EXIT [100.0%] > + ret > + .cfi_endproc > +.LFE0: > + .size main, .-main > + .ident "GCC: (GNU) 4.7.2 20120921 (Red Hat 4.7.2-2)" > + .section .note.GNU-stack,"",@progbits > diff --git a/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.c b/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.c > new file mode 100644 > index 0000000..666e5ec > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.c > @@ -0,0 +1,31 @@ > +/* Copyright (C) 2013-2014 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 . > + > + This file is not used directly. Please, see the equivalent .S file > + for more details. */ > + > +#include > + > +int > +main (int argc, char *argv[]) > +{ > + int a = 10; > + > + STAP_PROBE1 (test, three_arg, a); > + > + return 0; > +} > diff --git a/gdb/testsuite/gdb.arch/amd64-stap-triplet.S b/gdb/testsuite/gdb.arch/amd64-stap-triplet.S > new file mode 100644 > index 0000000..be22250 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-stap-triplet.S > @@ -0,0 +1,88 @@ > +/* Copyright (C) 2013-2014 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 . > + > + This file was generated from the equivalent .c file using the > + following command: > + > + #> gcc -S amd64-stap-triplet.c -o amd64-stap-triplet.S > + > + Then, the SystemTap SDT probe definition below was tweaked. See below > + for more details. */ > + > + .file "amd64-stap-triplet.c" > + .text > + .globl main > + .type main, @function > +main: > +.LFB0: > + .cfi_startproc > +# BLOCK 2 seq:0 > +# PRED: ENTRY (fallthru) > + pushq %rbp > + .cfi_def_cfa_offset 16 > + .cfi_offset 6, -16 > + movq %rsp, %rbp > + .cfi_def_cfa_register 6 > + movl %edi, -20(%rbp) > + movq %rsi, -32(%rbp) > + movl $10, -4(%rbp) > +#APP > +# 8 "amd64-stap-triplet.c" 1 > + 990: nop > +.pushsection .note.stapsdt,"?","note" > +.balign 4 > +.4byte 992f-991f,994f-993f,3 > +991: .asciz "stapsdt" > +992: .balign 4 > +993: .8byte 990b > +.8byte _.stapsdt.base > +.8byte 0 > +.asciz "test" > +.asciz "triplet" > +/* The probe's argument definition below was tweaked in order to mimic a > + triplet displacement in x86 asm. The original probe argument was: > + > + -4@-4(%rbp) > + > + The argument below is equivalent to that. */ > +.asciz "-4@-4+16-16(%rbp)" > +994: .balign 4 > +.popsection > + > +# 0 "" 2 > +# 8 "amd64-stap-triplet.c" 1 > + .ifndef _.stapsdt.base > +.pushsection .stapsdt.base,"aG","progbits",.stapsdt.base,comdat > +.weak _.stapsdt.base > +.hidden _.stapsdt.base > +_.stapsdt.base: .space 1 > +.size _.stapsdt.base,1 > +.popsection > +.endif > + > +# 0 "" 2 > +#NO_APP > + movl $0, %eax > + popq %rbp > + .cfi_def_cfa 7, 8 > +# SUCC: EXIT [100.0%] > + ret > + .cfi_endproc > +.LFE0: > + .size main, .-main > + .ident "GCC: (GNU) 4.7.2 20120921 (Red Hat 4.7.2-2)" > + .section .note.GNU-stack,"",@progbits > diff --git a/gdb/testsuite/gdb.arch/amd64-stap-triplet.c b/gdb/testsuite/gdb.arch/amd64-stap-triplet.c > new file mode 100644 > index 0000000..0ae2730 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-stap-triplet.c > @@ -0,0 +1,31 @@ > +/* Copyright (C) 2013-2014 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 . > + > + This file is not used directly. Please, see the equivalent .S file > + for more details. */ > + > +#include > + > +int > +main (int argc, char *argv[]) > +{ > + int a = 10; > + > + STAP_PROBE1 (test, triplet, a); > + > + return 0; > +} -- Sergio