From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9473 invoked by alias); 30 Jan 2014 15:16:26 -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 9464 invoked by uid 89); 30 Jan 2014 15:16:25 -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; Thu, 30 Jan 2014 15:16:24 +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 s0UFGKOn023143 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 30 Jan 2014 10:16:20 -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 s0UFGFTe032193 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 30 Jan 2014 10:16:17 -0500 From: Sergio Durigan Junior To: GDB Patches Cc: Joel Brobecker 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: Thu, 30 Jan 2014 15:16:00 -0000 In-Reply-To: (Sergio Durigan Junior's message of "Fri, 17 Jan 2014 19:31:30 -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/msg01058.txt.bz2 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. >> 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