From patchwork Fri May 2 16:14:29 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 792 X-Patchwork-Delegate: alves.ped@gmail.com Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx23.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 2F39E360871 for ; Fri, 2 May 2014 09:14:42 -0700 (PDT) Received: by homiemail-mx23.g.dreamhost.com (Postfix, from userid 14314964) id B781E633FBA51; Fri, 2 May 2014 09:14:41 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx23.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx23.g.dreamhost.com (Postfix) with ESMTPS id 530D6631D480A for ; Fri, 2 May 2014 09:14:41 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:references:date:in-reply-to :message-id:mime-version:content-type; q=dns; s=default; b=Ok4eg 3gV/yGD0VHVcw6M1d5NxAiXnOei84BM6qs+Nlq0K47CQ+w4afMV94TzMWnHHDE8i 9tFQnYGwgSiez+HPqgv6Gt9HYZGPRWWMv0h4AYmfK0fr/SMLyfOziujjTN8DVVlM dpordE9DXgsByOKqKFcX1z35Ze3GGXCrIp6H5w= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:references:date:in-reply-to :message-id:mime-version:content-type; s=default; bh=Z2gymqujiq/ P28CMXwQU8W0uerA=; b=AKONQM1CgxUd1XIWVsJ8DDh7OSfvsor9kiKzt4N+CmI iDoZg2GuZ/sVz+FVceaWc4Nh/GVGvRElgZTi4lyeRNh50Zxuo8G7mT6berN2nUry 7pq0MxkNISkii6nEVS69a7NDiB7BaiGpdB8FclbMedm+ki/cj1p2R3cIIw5YhKdI = Received: (qmail 16663 invoked by alias); 2 May 2014 16:14:38 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 16646 invoked by uid 89); 2 May 2014 16:14: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, 02 May 2014 16:14:36 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s42GEYeH022877 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Fri, 2 May 2014 12:14:34 -0400 Received: from psique ([10.3.113.6]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s42GEUWl008054 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 2 May 2014 12:14:32 -0400 From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches Subject: Re: [PATCH 1/2] Fix PR breakpoints/16889: gdb segfaults when printing ASM SDT arguments References: <1398981131-11720-1-git-send-email-sergiodj@redhat.com> <1398981131-11720-2-git-send-email-sergiodj@redhat.com> <53636901.9090601@redhat.com> X-URL: http://www.redhat.com Date: Fri, 02 May 2014 13:14:29 -0300 In-Reply-To: <53636901.9090601@redhat.com> (Pedro Alves's message of "Fri, 02 May 2014 10:44:33 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes X-DH-Original-To: gdb@patchwork.siddhesh.in On Friday, May 02 2014, Pedro Alves wrote: > On 05/01/2014 10:52 PM, Sergio Durigan Junior wrote: > >> gdb/testsuite/ >> 2014-05-01 Sergio Durigan Junior >> >> PR breakpoints/16889 >> * gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S: New file. >> * gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp: Likewise. > > No "gdb/testsuite/" in the entries. Ops, typo, sorry. >> +#include >> + .file "amd64-stap-optional-prefix.S" > > I think a line break after the include would read better. Done. >> + .text >> + .globl main >> +main: > > > >> + mov %rsp,%rbp >> + pushq $42 >> + STAP_PROBE1(probe, foo, (%rsp)) >> + STAP_PROBE1(probe, bar, -8(%rbp)) > > What controls whether the optional prefix is included? Could > we perhaps add two extra probes that probe the same values, > but use the optional prefix? Something to the effect of: > > STAP_PROBE1(probe, foo, (%rsp)) > STAP_PROBE1(probe, bar, -8(%rbp)) > STAP_PROBE1(probe, foo_prefix, 8@(%rsp)) > STAP_PROBE1(probe, bar_prefix, 8@-8(%rbp)) > > (but I'm clueless on how that is actually written.) If the asm is generated by the compiler, than it is almost guaranteed that the optional prefix will be included. However, since it is optional, if it is a hand-written asm then the user might choose to omit it. Extending the test as you mentioned is OK, but I chose not to do it because the prefix-variant probes are already tested at gdb.base/stap-probe.exp. Either way, I am including them now (and extending the testcase). >> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp >> new file mode 100644 >> index 0000000..9747dc8 >> --- /dev/null >> +++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp >> @@ -0,0 +1,50 @@ >> +# 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 . >> + >> +# This testcase is for PR breakpoints/16889 >> + >> +if { ![istarget "x86_64-*-*"] } { >> + verbose "Skipping amd64-stap-special-operands.exp" >> + return >> +} >> + >> +standard_testfile ".S" > > If you swap these, you can use $testfile: > > standard_testfile ".S" > > if { ![istarget "x86_64-*-*"] } { > verbose "Skipping $testfile.exp" > return > } Done. >> + >> +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } { >> + untested amd64-stap-optional-prefix.exp >> + return -1 >> +} > > Here too. But, prepare_for_testing already calls untested for > you, using the text passed as first argument. Write: > > if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } { > return -1 > } Somehow I didn't know about it. What a lame! Anyway, done. >> + >> +# Run to the first probe (foo). >> + >> +if { ![runto "-pstap foo"] } { >> + fail "run to probe foo" >> + return >> +} >> + >> +# Probe foo's first argument must me == $rsp > > s/me/be ? I think you meant: > > # Probe foo's first argument must be == ($rsp) > > Might even make it easier to read to spell that out > in plain English. > > Otherwise this looks good to me. Done, thanks. Here's what I am going to push by the end of the day if there are no other comments. commit c7c77ebc3aad10cbf7be91e09de839bccdbf06ca Author: Sergio Durigan Junior Date: Thu May 1 18:20:05 2014 -0300 Fix PR breakpoints/16889: gdb segfaults when printing ASM SDT arguments This commit fixes PR breakpoints/16889, which is about a bug that triggers when GDB tries to parse probes whose arguments do not contain the initial (and optional) "N@" part. For reference sake, the de facto format is described here: Anyway, this PR actually uncovered two bugs (related) that were happening while parsing the arguments. The first one was that the parser *was* catching *some* arguments that were missing the "N@" part, but it wasn't correctly setting the argument's type. This was causing a NULL pointer being dereferenced, ouch... The second bug uncovered was that the parser was not catching all of the cases for a probe which did not provide the "N@" part. The fix for that was to simplify the check that the code was making to identify non-prefixed probes. The code is simpler and easier to read now. I am also providing a testcase for this bug, only for x86_64 architectures. gdb/ 2014-05-01 Sergio Durigan Junior PR breakpoints/16889 * stap-probe.c (stap_parse_probe_arguments): Simplify check for non-prefixed probes (i.e., probes whose arguments do not start with "N@"). Always set the argument type to a sane value. gdb/testsuite/ 2014-05-01 Sergio Durigan Junior PR breakpoints/16889 * gdb.arch/amd64-stap-optional-prefix.S: New file. * gdb.arch/amd64-stap-optional-prefix.exp: Likewise. diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c index dbe9f31..ef45495 100644 --- a/gdb/stap-probe.c +++ b/gdb/stap-probe.c @@ -1098,10 +1098,8 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch) Where `N' can be [+,-][4,8]. This is not mandatory, so we check it here. If we don't find it, go to the next state. */ - if ((*cur == '-' && cur[1] != '\0' && cur[2] != '@') - && cur[1] != '@') - arg.bitness = STAP_ARG_BITNESS_UNDEFINED; - else + if ((cur[0] == '-' && isdigit (cur[1]) && cur[2] == '@') + || (isdigit (cur[0]) && cur[1] == '@')) { if (*cur == '-') { @@ -1127,11 +1125,14 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch) } arg.bitness = b; - arg.atype = stap_get_expected_argument_type (gdbarch, b); /* Discard the number and the `@' sign. */ cur += 2; } + else + arg.bitness = STAP_ARG_BITNESS_UNDEFINED; + + arg.atype = stap_get_expected_argument_type (gdbarch, arg.bitness); expr = stap_parse_argument (&cur, arg.atype, gdbarch); diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S new file mode 100644 index 0000000..66689cb --- /dev/null +++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S @@ -0,0 +1,32 @@ +/* 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-optional-prefix.S" + .text + .globl main +main: + mov %rsp,%rbp + pushq $42 + STAP_PROBE1(probe, foo, (%rsp)) + STAP_PROBE1(probe, bar, -8(%rbp)) + STAP_PROBE1(probe, foo_prefix, 8@(%rsp)) + STAP_PROBE1(probe, bar_prefix, 8@-8(%rbp)) + mov %rbp,%rsp + xor %rax,%rax + ret diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp new file mode 100644 index 0000000..c69e54c --- /dev/null +++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp @@ -0,0 +1,68 @@ +# 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 . + +# This testcase is for PR breakpoints/16889 + +standard_testfile ".S" + +if { ![istarget "x86_64-*-*"] } { + verbose "Skipping $testfile.exp" + return +} + +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } { + return -1 +} + +# Helper procedure to go to probe NAME + +proc goto_probe { name } { + global decimal hex + + gdb_test "break -pstap $name" "Breakpoint $decimal at $hex" + gdb_test "continue" "Breakpoint $decimal, main \\(\\) at .*\r\n.*STAP_PROBE1.*${name},.*\\)" +} + +# Helper procedure to test the probe's argument + +proc test_probe_value { value reg_val } { + gdb_test "print \$_probe_argc" "= 1" + gdb_test "print \$_probe_arg0" "= $value" + gdb_test "print \$_probe_arg0 == *((unsigned int *) (${reg_val}))" "= 1" +} + +# Run to the first probe (foo). + +if { ![runto "-pstap foo"] } { + fail "run to probe foo" + return +} + +# Probe foo's first argument must be $rsp + +test_probe_value "42" "\$rsp" + +# Continuing to the second probe (bar) + +goto_probe "bar" +test_probe_value "42" "\$rbp - 8" + +# Now, testing the prefixed probes + +goto_probe "foo_prefix" +test_probe_value "42" "\$rsp" + +goto_probe "bar_prefix" +test_probe_value "42" "\$rbp - 8"