From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16809 invoked by alias); 9 Apr 2013 17:50:29 -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 16799 invoked by uid 89); 9 Apr 2013 17:50:29 -0000 X-Spam-SWARE-Status: No, score=-8.9 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 09 Apr 2013 17:50:28 +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 r39HoLZx002209 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 9 Apr 2013 13:50:21 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r39HoISu019446; Tue, 9 Apr 2013 13:50:19 -0400 Message-ID: <516454DA.9040109@redhat.com> Date: Tue, 09 Apr 2013 23:31:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Hui Zhu CC: Eli Zaretskii , Hui Zhu , gdb-patches@sourceware.org, marc.khouzam@ericsson.com Subject: Re: [PATCH] add -s option to make -break-insert support dprintf References: <515451EA.1000200@mentor.com> <83y5d7wpvq.fsf@gnu.org> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00249.txt.bz2 Hi Hui, Thanks for the patch. New MI features need a NEWS entry. On 03/29/2013 08:01 AM, Hui Zhu wrote: > + if (hardware && dprintf) > + error (_("-break-insert: -h and -s cannot be use together")); "cannot be used" > @@ -180,11 +189,14 @@ mi_cmd_break_insert (char *command, char > regular non-jump based tracepoints. */ > type_wanted = (tracepoint > ? (hardware ? bp_fast_tracepoint : bp_tracepoint) > - : (hardware ? bp_hardware_breakpoint : bp_breakpoint)); > - ops = tracepoint ? &tracepoint_breakpoint_ops : &bkpt_breakpoint_ops; > + : (hardware ? bp_hardware_breakpoint > + : (dprintf ? bp_dprintf : bp_breakpoint))); > + ops = tracepoint ? &tracepoint_breakpoint_ops > + : (dprintf ? &dprintf_breakpoint_ops > + : &bkpt_breakpoint_ops); This is getting unnecessarily hard for humans to grok. Write instead as (untested): if (tracepoint) { /* move existing comment on fast tracepoints here */ type_wanted = hardware ? bp_fast_tracepoint : bp_tracepoint; ops = &tracepoint_breakpoint_ops; } else if (dprintf) { type_wanted = bp_dprintf; ops = &dprintf_breakpoint_ops; } else { type_wanted = hardware ? bp_hardware_breakpoint : bp_breakpoint; ops = &bkpt_breakpoint_ops; } > +@item -s "@var{template},@var{expression}[,@var{expression}@dots{}]" > +Set a dynamic printf breakpoint, described in @ref{Dynamic Printf}. > +The @var{location}, @var{template} and @var{expression} should be > +within double quotations and be escaped by being preceded with a backslash. Please remove the "location" mention here. It's stale. I think you either say "double quotation marks" or "double quotes", never "double quotation". > > 2013-03-29 Hui Zhu > > * breakpoint.c (dprintf_breakpoint_ops): Remove its static. > * breakpoint.h (dprintf_breakpoint_ops): Add extern. > * mi/mi-cmd-break.c (mi_cmd_break_insert): Describe the "-s" option. Not really describing... "Handle"? > 2013-03-29 Hui Zhu > > * gdb.mi/Makefile.in (PROGS): Add "mi-dprintf". > * gdb.mi/mi-dprintf.c, gdb.mi/mi-dprintf.h: New. Missing reference to mi-dprintf.exp. > + foo (loc++); > + foo (loc++); > + foo (loc++); > + return g; > +} > + > +#include Headers at the top, please. > +/* Make sure function 'malloc' is linked into program. On some bare-metal > + port, if we don't use 'malloc', it will not be linked in program. 'malloc' > + is needed, otherwise we'll see such error message > + > +standard_testfile .c .c is the default. > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { > + untested mi-dprintf.exp http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#A.22untested.22_calls > +set target_can_dprintf 1 This should start out as 0. More below. > +set msg "Set dprintf style to agent" > +mi_gdb_test "1set dprintf-style agent" "\[^\n\]*\r\ndone" > +gdb_expect { > + -re "\\^done" { > + pass "$msg - can do" and be set to 1 here. Should expect ${mi_gdb_prompt} too. > + } > + -re ".*" { Should expect ${mi_gdb_prompt} too. But what this actually expecting? Is it: "warning: Target cannot run dprintf commands, falling back to GDB printf" ? Please adjust the -re accordingly. > + set target_can_dprintf 0 > + pass "$msg - cannot do" > + } > + timeout { > + fail "resume all, waiting for program exit (timeout)" Certainly "resume all" is a pasto here. Related to the comment to "set target_can_dprintf 1" above, e.g., this failure path didn't set target_can_dprintf to 0. > + } > +} > + > +if $target_can_dprintf { Why do I get: PASS: gdb.mi/mi-dprintf.exp: Set dprintf style to agent - cannot do with gdbserver? (The test tries the same thing with a few different options. I suspect it could be simplified with loops and with_test_prefix.) -- Pedro Alves