From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29218 invoked by alias); 30 Jan 2014 15:36:03 -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 29206 invoked by uid 89); 30 Jan 2014 15:36:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: glazunov.sibelius.xs4all.nl Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 30 Jan 2014 15:36:01 +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 s0UFZp4Q004944; Thu, 30 Jan 2014 16:35:51 +0100 (CET) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3/Submit) id s0UFZp3N013895; Thu, 30 Jan 2014 16:35:51 +0100 (CET) Date: Thu, 30 Jan 2014 15:36:00 -0000 Message-Id: <201401301535.s0UFZp3N013895@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: sergiodj@redhat.com CC: gdb-patches@sourceware.org, brobecker@adacore.com In-reply-to: (message from Sergio Durigan Junior on Thu, 30 Jan 2014 13:16:15 -0200) Subject: Re: [PATCH] Fix for PR tdep/16397: SystemTap SDT probe support for x86 doesn't work with "triplet operands" References: X-SW-Source: 2014-01/txt/msg01059.txt.bz2 > From: Sergio Durigan Junior > Date: Thu, 30 Jan 2014 13:16:15 -0200 > > On Friday, January 17 2014, I wrote: > > > On Sunday, January 12 2014, I wrote: > > > >> Hi, > >> > >> This is the continuation of what Joel proposed on: > >> > >> > > > > Ping. > > Ping^2. No objection to this going in (other than the unsafe use of isdigit(3)) but I don't claim to know wnything of the SDT stuff. > >> 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 > > -- > Sergio >