From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3779 invoked by alias); 2 Sep 2014 16:33: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 3725 invoked by uid 89); 2 Sep 2014 16:33:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.1 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 02 Sep 2014 16:33:24 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s82GXMVT000906 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 2 Sep 2014 12:33:23 -0400 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s82GXLX6025277 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO) for ; Tue, 2 Sep 2014 12:33:22 -0400 From: Sergio Durigan Junior To: GDB Patches Subject: Re: [PATCH] Fix for PR gdb/17235: possible bug extracting systemtap probe operand References: <87vbpf9cg4.fsf@redhat.com> X-URL: http://www.redhat.com Date: Tue, 02 Sep 2014 16:33:00 -0000 In-Reply-To: <87vbpf9cg4.fsf@redhat.com> (Sergio Durigan Junior's message of "Tue, 26 Aug 2014 17:18:51 -0400") Message-ID: <874mwqkmni.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00068.txt.bz2 On Tuesday, August 26 2014, I wrote: > Hi there, Ping. > This patch is a fix to PR gdb/17235. The bug is about an unused > variable that got declared and set during one of the parsing phases of > an SDT probe's argument. I took the opportunity to rewrite some of the > code to improve the parsing. The bug was actually a thinko, because > what I wanted to do in the code was to discard the number on the string > being parsed. > > During this portion, the code identifies that it is dealing with an > expression that begins with a sign ('+', '-' or '~'). This means that > the expression could be: > > - a numeric literal (e.g., '+5') > - a register displacement (e.g., '-4(%rsp)') > - a subexpression (e.g., '-(2*3)') > > So, after saving the sign and moving forward 1 char, now the code needs > to know if there is a digit followed by a register displacement prefix > operand (e.g., '(' on x86_64). If yes, then it is a register > operation. If not, then it will be handled recursively, and the code > will later apply the requested operation on the result (either a '+', a > '-' or a '~'). > > With the bug, the code was correctly discarding the digit (though using > strtol unnecessarily), but it wasn't properly dealing with > subexpressions when the register indirection prefix was '(', like on > x86_64. This patch also fixes this bug, and includes a testcase. It > passes on x86_64 Fedora 20. > > OK to check it in? > > Thanks, > > -- > Sergio > GPG key ID: 0x65FC5E36 > Please send encrypted e-mail if possible > http://sergiodj.net/ > > gdb/ChangeLog: > 2014-08-26 Sergio Durigan Junior > > PR gdb/17235 > * stap-probe.c (stap_parse_single_operand): Delete unused variable > 'number'. New variable 'has_digit'. Rewrite code to deal with > subexpressions on SDT probes. > > gdb/testsuite/ChangeLog: > 2014-08-26 Sergio Durigan Junior > > PR gdb/17235 > * gdb.arch/amd64-stap-wrong-subexp.exp: New file. > * gdb.arch/amd64-stap-wrong-subexp.S: Likewise. > > diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c > index 84714b5..23202d7 100644 > --- a/gdb/stap-probe.c > +++ b/gdb/stap-probe.c > @@ -753,9 +753,9 @@ stap_parse_single_operand (struct stap_parse_info *p) > if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+') > { > char c = *p->arg; > - int number; > /* We use this variable to do a lookahead. */ > const char *tmp = p->arg; > + int has_digit = 0; > > /* Skipping signal. */ > ++tmp; > @@ -772,26 +772,19 @@ stap_parse_single_operand (struct stap_parse_info *p) > if (p->inside_paren_p) > tmp = skip_spaces_const (tmp); > > - if (isdigit (*tmp)) > + while (isdigit (*tmp)) > { > - char *endp; > - > - number = strtol (tmp, &endp, 10); > - tmp = endp; > + /* We skip the digit here because we are only interested in > + knowing what kind of unary operation this is. The digit > + will be handled by one of the functions that will be > + called below ('stap_parse_argument_conditionally' or > + 'stap_parse_register_operand'). */ > + ++tmp; > + has_digit = 1; > } > > - if (!stap_is_register_indirection_prefix (gdbarch, tmp, NULL)) > - { > - /* This is not a displacement. We skip the operator, and deal > - with it later. */ > - ++p->arg; > - stap_parse_argument_conditionally (p); > - if (c == '-') > - write_exp_elt_opcode (&p->pstate, UNOP_NEG); > - else if (c == '~') > - write_exp_elt_opcode (&p->pstate, UNOP_COMPLEMENT); > - } > - else > + if (has_digit && stap_is_register_indirection_prefix (gdbarch, tmp, > + NULL)) > { > /* If we are here, it means it is a displacement. The only > operations allowed here are `-' and `+'. */ > @@ -801,6 +794,17 @@ stap_parse_single_operand (struct stap_parse_info *p) > > stap_parse_register_operand (p); > } > + else > + { > + /* This is not a displacement. We skip the operator, and > + deal with it when the recursion returns. */ > + ++p->arg; > + stap_parse_argument_conditionally (p); > + if (c == '-') > + write_exp_elt_opcode (&p->pstate, UNOP_NEG); > + else if (c == '~') > + write_exp_elt_opcode (&p->pstate, UNOP_COMPLEMENT); > + } > } > else if (isdigit (*p->arg)) > { > diff --git a/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.S b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.S > new file mode 100644 > index 0000000..2848462 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.S > @@ -0,0 +1,27 @@ > +/* Copyright (C) 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 . */ > + > +#include > + > + .file "amd64-stap-wrong-subexp.S" > + .text > + .globl main > +main: > + STAP_PROBE1(probe, foo, -4@$-3($4+$3)) > + STAP_PROBE2(probe, bar, -4@-($4), -4@$-3+($22/$2)-$16) > + xor %rax,%rax > + ret > diff --git a/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.exp b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.exp > new file mode 100644 > index 0000000..5a1ad53 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.exp > @@ -0,0 +1,41 @@ > +# Copyright 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-*-*"] || ![is_lp64_target] } { > + verbose "Skipping amd64-stap-wrong-subexp.exp" > + return > +} > + > +standard_testfile amd64-stap-wrong-subexp.S > + > +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } { > + untested amd64-stap-wrong-subexp.exp > + return -1 > +} > + > +proc goto_probe { probe_name } { > + if { ![runto "-pstap $probe_name"] } { > + fail "run to probe $probe_name" > + return > + } > +} > + > +goto_probe foo > +gdb_test "print \$_probe_arg0" "Invalid operator `\\\(' on expression .*" \ > + "print probe foo arg0" > + > +goto_probe bar > +gdb_test "print \$_probe_arg0" " = -4" "print probe bar arg0" > +gdb_test "print \$_probe_arg1" " = -8" "print probe bar arg1" -- Sergio GPG key ID: 0x65FC5E36 Please send encrypted e-mail if possible http://sergiodj.net/