From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16087 invoked by alias); 19 Aug 2010 07:49:24 -0000 Received: (qmail 16078 invoked by uid 22791); 19 Aug 2010 07:49:23 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,TW_CP,TW_QT,TW_XF X-Spam-Check-By: sourceware.org Received: from mail-wy0-f169.google.com (HELO mail-wy0-f169.google.com) (74.125.82.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 19 Aug 2010 07:48:49 +0000 Received: by wyg36 with SMTP id 36so2014233wyg.0 for ; Thu, 19 Aug 2010 00:48:46 -0700 (PDT) Received: by 10.216.236.197 with SMTP id w47mr326052weq.114.1282204126540; Thu, 19 Aug 2010 00:48:46 -0700 (PDT) MIME-Version: 1.0 Received: by 10.216.186.205 with HTTP; Thu, 19 Aug 2010 00:48:26 -0700 (PDT) In-Reply-To: References: <201008151946.55330.pedro@codesourcery.com> <4C683A6C.8040501@vmware.com> <201008160009.57924.pedro@codesourcery.com> From: Hui Zhu Date: Thu, 19 Aug 2010 07:49:00 -0000 Message-ID: Subject: Re: [RFA/RFC] tracepoint gdbrsp: add -1 introduce for QTFrame:@var{n} To: Pedro Alves Cc: Michael Snyder , "gdb-patches@sourceware.org" , Eli Zaretskii Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: 2010-08/txt/msg00330.txt.bz2 On Thu, Aug 19, 2010 at 15:43, Hui Zhu wrote: > On Mon, Aug 16, 2010 at 07:09, Pedro Alves wrote: >> On Sunday 15 August 2010 20:05:16, Michael Snyder wrote: >>> Pedro Alves wrote: >>> > On Sunday 15 August 2010 08:16:34, Hui Zhu wrote: >>> >> Sending packet: $QTFrame:ffffffff#fa...Packet received: OK >>> > >>> > I think it is a bug that this is assuming 32-bit, two's complement >>> > on both client/server sides. =A0(not sure it that was what you were >>> > referring to). >>> >>> It's not really assuming that -- it's just assuming that no >>> legitimate frame ID will ever be as high as ffffffff. >> >> I don't know where you're getting that from. =A0If you believe that >> to be true, then you also have to believe Hui's patch is wrong, >> as it documents "-1", not the magic hard limit of "0xffffffff". >> >> Start here: >> >> /* tfind end */ >> static void >> trace_find_end_command (char *args, int from_tty) >> { >> =A0trace_find_command ("-1", from_tty); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ^^ >> } >> >> through here: >> >> /* Worker function for the various flavors of the tfind command. =A0*/ >> void >> tfind_1 (enum trace_find_type type, int num, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0^= ^^^^^^ >> =A0 =A0 =A0 =A0 ULONGEST addr1, ULONGEST addr2, >> =A0 =A0 =A0 =A0 int from_tty) >> { >> =A0int target_frameno =3D -1, target_tracept =3D -1; >> =A0struct frame_id old_frame_id =3D null_frame_id; >> =A0struct breakpoint *tp; >> >> =A0/* Only try to get the current stack frame if we have a chance of >> =A0 =A0 succeeding. =A0In particular, if we're trying to get a first tra= ce >> =A0 =A0 frame while all threads are running, it's not going to succeed, >> =A0 =A0 so leave it with a default value and let the frame comparison >> =A0 =A0 below (correctly) decide to print out the source location of the >> =A0 =A0 trace frame. =A0*/ >> =A0if (!(type =3D=3D tfind_number && num =3D=3D -1) >> =A0 =A0 =A0&& (has_stack_frames () || traceframe_number >=3D 0)) >> =A0 =A0old_frame_id =3D get_frame_id (get_current_frame ()); >> >> =A0target_frameno =3D target_trace_find (type, num, addr1, addr2, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0&target_tracept); >> >> =A0if (type =3D=3D tfind_number >> =A0 =A0 =A0&& num =3D=3D -1 >> =A0 =A0 =A0 =A0 ^^^^^^^^^ >> =A0 =A0 =A0&& target_frameno =3D=3D -1) >> =A0 =A0{ >> =A0 =A0 =A0/* We told the target to get out of tfind mode, and it did. = =A0*/ >> =A0 =A0} >> =A0else if (target_frameno =3D=3D -1) >> =A0 =A0{ >> =A0 =A0 =A0/* A request for a non-existent trace frame has failed. >> =A0 =A0 =A0 =A0 Our response will be different, depending on FROM_TTY: >> >> =A0 =A0 =A0 =A0 If FROM_TTY is true, meaning that this command was >> =A0 =A0 =A0 =A0 typed interactively by the user, then give an error >> =A0 =A0 =A0 =A0 and DO NOT change the state of traceframe_number etc. >> >> =A0 =A0 =A0 =A0 However if FROM_TTY is false, meaning that we're either >> =A0 =A0 =A0 =A0 in a script, a loop, or a user-defined command, then >> =A0 =A0 =A0 =A0 DON'T give an error, but DO change the state of >> =A0 =A0 =A0 =A0 traceframe_number etc. to invalid. >> >> =A0 =A0 =A0 =A0 The rationalle is that if you typed the command, you >> =A0 =A0 =A0 =A0 might just have committed a typo or something, and you'd >> =A0 =A0 =A0 =A0 like to NOT lose your current debugging state. =A0However >> =A0 =A0 =A0 =A0 if you're in a user-defined command or especially in a >> =A0 =A0 =A0 =A0 loop, then you need a way to detect that the command >> =A0 =A0 =A0 =A0 failed WITHOUT aborting. =A0This allows you to write >> =A0 =A0 =A0 =A0 scripts that search thru the trace buffer until the end, >> =A0 =A0 =A0 =A0 and then continue on to do something else. =A0*/ >> >> =A0 =A0 =A0if (from_tty) >> =A0 =A0 =A0 =A0error (_("Target failed to find requested trace frame.")); >> >> >> Notice how you get to the "else if" branch. =A0Choosing any other >> "high enough" random frame id that doesn't exist errors >> out. =A0Only the special case of 0xffffffff being parsed as -1 on >> both target and host actually gets out of tfind mode. >> >> Continuing, the target_trace_find call ends up here: >> >> static int >> remote_trace_find (enum trace_find_type type, int num, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ULONGEST addr1, ULONGEST addr2, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int *tpp) >> { >> =A0struct remote_state *rs =3D get_remote_state (); >> =A0char *p, *reply; >> =A0int target_frameno =3D -1, target_tracept =3D -1; >> >> =A0p =3D rs->buf; >> =A0strcpy (p, "QTFrame:"); >> =A0p =3D strchr (p, '\0'); >> =A0switch (type) >> =A0 =A0{ >> =A0 =A0case tfind_number: >> =A0 =A0 =A0sprintf (p, "%x", num); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0^^^^^^^^^ >> =A0 =A0 =A0break; >> >> ... and here, NUM, is an int, and, is printed with %x. =A0Assuming >> that prints 0xffffffff is actually host dependent (e.g., consider >> ILP64). =A0Granted, likely not a concern on all hosts we care >> about. =A0Still a needless assumption. >> >> The gdbserver side does: >> >> ... >> =A0 =A0 =A0unpack_varlen_hex (packet, &frame); >> =A0 =A0 =A0tfnum =3D (int) frame; >> =A0 =A0 =A0if (tfnum =3D=3D -1) >> =A0 =A0 =A0 =A0{ >> =A0 =A0 =A0 =A0 =A0trace_debug ("Want to stop looking at traceframes"); >> =A0 =A0 =A0 =A0 =A0current_traceframe =3D -1; >> =A0 =A0 =A0 =A0 =A0write_ok (own_buf); >> =A0 =A0 =A0 =A0 =A0return; >> =A0 =A0 =A0 =A0} >> >> Again would break if the target is ILP64, and host is not. >> >> And below that bit of code you'll see that trying to select any >> other high enough frame number that does not exist >> does not get out of tfind mode, but instead returns an error. >> >>> That might also be iffy, but less so.... >> >> I should have mentioned that I consider this a _design_ bug we >> get to live with. =A0Fixing it like I suggested would be incompatible >> with current targets, so, best leave this alone until it actually >> becomes a problem. >> >> Though, this raises the point that Hui's docs don't >> actually match what GDB sends. =A0Not sure what to do. =A0I guess >> I'll just pretend I didn't spot this. =A0;-) >> >>> =A0 IMO, negatives should have an explicit '-' encoding; in >>> > this case, "$QTFrame:-1". =A0Note sure if the RSP docs mention someth= ing >>> > about this. =A0We are careful in some cases (passing thread id's, >>> > I think, is one case), though clearly not everywhere. >> >> -- >> Pedro Alves >> > > Hi, > > I am not sure we need keep this bug in the GDB. =A0Make gdb and > gdbserver support the old version is not a very hard thing. > > I make 3 patch for gdb, gdbserver and doc. > For the gdbserver, it can support the old gdb that use 0xffffffff and > new gdb that use -1. > For the gdb, if you think we =A0need, I can add a switch or something to > make it send 0xffffffff. > > What do you think about it? > > Thanks, > Hui > > 2010-08-19 =A0Hui Zhu =A0 > > =A0 =A0 =A0 =A0* remote.c(remote_trace_find): Handle the negative. > > 2010-08-19 =A0Hui Zhu =A0 > > =A0 =A0 =A0 =A0* tracepoint.c(cmd_qtframe): Handle the negative. > > 2010-08-19 =A0Hui Zhu =A0 > > =A0 =A0 =A0 =A0* gdb.texinfo (Tracepoint Packets): add introduce for -1. > > --- > =A0remote.c | =A0 =A05 ++++- > =A01 file changed, 4 insertions(+), 1 deletion(-) > > --- a/remote.c > +++ b/remote.c > @@ -9979,7 +9979,10 @@ remote_trace_find (enum trace_find_type > =A0 switch (type) > =A0 =A0 { > =A0 =A0 case tfind_number: > - =A0 =A0 =A0sprintf (p, "%x", num); > + =A0 =A0 =A0if (num < 0) > + =A0 =A0 =A0 =A0sprintf (p, "-%x", -num); > + =A0 =A0 =A0else > + =A0 =A0 =A0 =A0sprintf (p, "%x", num); > =A0 =A0 =A0 break; > =A0 =A0 case tfind_pc: > =A0 =A0 =A0 sprintf (p, "pc:%s", phex_nz (addr1, 0)); > > > --- > =A0gdbserver/tracepoint.c | =A0 =A09 +++++++++ > =A01 file changed, 9 insertions(+) > > --- a/gdbserver/tracepoint.c > +++ b/gdbserver/tracepoint.c > @@ -3157,8 +3157,17 @@ cmd_qtframe (char *own_buf) > =A0 =A0 } > =A0 else > =A0 =A0 { > + =A0 =A0 =A0int is_negative =3D 0; > + > + =A0 =A0 =A0if (*packet =3D=3D '-') > + =A0 =A0 =A0 =A0{ > + =A0 =A0 =A0 =A0 =A0is_negative =3D 1; > + =A0 =A0 =A0 =A0 =A0++packet; > + =A0 =A0 =A0 =A0} > =A0 =A0 =A0 unpack_varlen_hex (packet, &frame); > =A0 =A0 =A0 tfnum =3D (int) frame; > + =A0 =A0 =A0if (is_negative) > + =A0 =A0 =A0 =A0tfnum =3D -tfnum; > =A0 =A0 =A0 if (tfnum =3D=3D -1) > =A0 =A0 =A0 =A0{ > =A0 =A0 =A0 =A0 =A0trace_debug ("Want to stop looking at traceframes"); > > > --- > =A0doc/gdb.texinfo | =A0 12 ++++++++++++ > =A01 file changed, 12 insertions(+) > > --- a/doc/gdb.texinfo > +++ b/doc/gdb.texinfo > @@ -33133,6 +33133,18 @@ The selected trace frame records a hit o > > =A0@end table > > +If @var{n} is -1, it mean that stop debugging trace snapshots, > +resume live debugging. > + > +Replies: > +@table @samp > +@item OK > +The packet was understood and carried out. > +@item E @var{NN} > +A badly formed request was detected, or an error was encountered while > +relocating the instruction. > +@end table > + > =A0@item QTFrame:pc:@var{addr} > =A0Like @samp{QTFrame:@var{n}}, but select the first tracepoint frame aft= er the > =A0currently selected frame whose PC is @var{addr}; > Sorry the doc patch is not the right version. following is the current vers= ion. Thanks, Hui 2010-08-19 Hui Zhu * gdb.texinfo (Tracepoint Packets): add introduce for -1. --- doc/gdb.texinfo | 9 +++++++++ 1 file changed, 9 insertions(+) --- a/doc/gdb.texinfo +++ b/doc/gdb.texinfo @@ -33155,6 +33155,15 @@ The selected trace frame records a hit o @end table +If @var{n} is -1, it mean that stop debugging trace snapshots, +resume live debugging. + +Replies: +@table @samp +@item OK +The packet was understood and carried out. +@end table + @item QTFrame:pc:@var{addr} Like @samp{QTFrame:@var{n}}, but select the first tracepoint frame after t= he currently selected frame whose PC is @var{addr};