From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26798 invoked by alias); 3 Mar 2013 02:21:40 -0000 Received: (qmail 26764 invoked by uid 22791); 3 Mar 2013 02:21:38 -0000 X-SWARE-Spam-Status: No, hits=-4.2 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL X-Spam-Check-By: sourceware.org Received: from usevmg20.ericsson.net (HELO usevmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 03 Mar 2013 02:21:32 +0000 Received: from EUSAAHC001.ericsson.se (Unknown_Domain [147.117.188.75]) by usevmg20.ericsson.net (Symantec Mail Security) with SMTP id E1.23.02430.BA3B2315; Sun, 3 Mar 2013 03:21:32 +0100 (CET) Received: from EUSAAMB103.ericsson.se ([147.117.188.120]) by EUSAAHC001.ericsson.se ([147.117.188.75]) with mapi id 14.02.0318.004; Sat, 2 Mar 2013 21:21:31 -0500 From: Marc Khouzam To: "yao@codesourcery.com" CC: "gdb-patches@sourceware.org" Subject: RE: [PATCH 1/4] Fix dprintf bugs Date: Sun, 03 Mar 2013 02:21:00 -0000 Message-ID: References: <1361192891-29341-1-git-send-email-yao@codesourcery.com> <1362057362-25324-1-git-send-email-yao@codesourcery.com> In-Reply-To: <1362057362-25324-1-git-send-email-yao@codesourcery.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-IsSubscribed: yes 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 X-SW-Source: 2013-03/txt/msg00067.txt.bz2 > -----Original Message----- > From: gdb-patches-owner@sourceware.org=20 > [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Yao Qi > Sent: Thursday, February 28, 2013 8:16 AM > To: gdb-patches@sourceware.org > Subject: [PATCH 1/4] Fix dprintf bugs >=20 > Hi, > This patch fixes PR15075. Besides this bug, this patch also fixes > some other problems: >=20 > - Two dprintf are set on the same address. Only one printf is > executed. > - dprintf and regular breakpoint are set on the same address, > inferior doesn't stop. >=20 > In order to fix these problems, I don't append "continue" command to > dprintf breakpoint, and execute commands of dprintf after > bpstat_check_breakpoint_conditions if condition is true. The reason I > choose this place (after bpstat_check_breakpoint_conditions instead of > in bpstat_check_breakpoint_conditions) to handle dprintf is that we > have to set BS->stop to zero, but have to update hit count if > condition is true, so we have to do two things together. >=20 > With this patch, GDB executes dprintf commands after > bpstat_check_breakpoint_conditions, the different place where > breakpoint commands are executed. I propose to disable setting > command for dprintf, or disallow user setting commands for dprintf. > If we believe it is important to allow user setting commands for > dprintf (a good use case?), we need a new design like creating struct > dprintf as a 'sub-class' of breakpoint and adding its own field for > printf commands. The code on sending commands to the remote should be > adjusted as well. I've played around with this patch and I like the solution a lot! I like the fact that a user cannot modify the list of commands for dprintf. If they want to use commands, they should use a breakpoint, not a dprintf. It is no longer possible to update the dprintf string without having to create a new dprintf though; but if this is to be supported, I think it should be a specific dprintf operation. I no longer see *stopped/*running events in MI, which is great. I'm still hesitant about the -break-modified event in that case though. I believe the event is triggered because the hit count=20 has changed. For a normal bp, it makes sense to have this event in this case, since execution has stopped and only a single=20 event will be seen (not exactly true for non-stop, but still makes sense, I think). However, for dprintf which is meant to=20 let the inferior continue to run, there could be quite many=20 hit events very quickly. Since we already have some feeback that the dprintf has hit through the actual printf string, I'm leaning towards not having that event for dprintf hits. Furthermore, this event is not being sent when using dprintf-style "agent" anyway. I also saw that conditions are now properly respected for dprintf-style "gdb" and "call". That is great. Conditions are still not respected for=20 style "agent" but that is a separate issue I believe (PR 15180). I did notice that although commands cannot be set for dprintf from the CLI they are not blocked for MI: (gdb) interpreter-exec mi "-break-commands 1 hello" ^done (gdb) info b Num Type Disp Enb Address What 1 dprintf keep y 0x0000000000400570 in main() at loopfirst.c= c:8 hello Finally, it would be nice to have MI commands to deal with dprintf :) Re-using the breakpoint MI commands sound best, although I wonder about two cases: a) Would we be able to specify the printf string in the -break-insert comma= nd? b) How could we change the printf command after a dprintf is created? Will= we need to delete it and create a new one, or would we be able to update it? All that being said, I'm impressed at how many improvements this relatively small patch provides. Nice work. Marc