From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5504 invoked by alias); 24 Apr 2010 01:39:35 -0000 Received: (qmail 5445 invoked by uid 22791); 24 Apr 2010 01:39:32 -0000 X-SWARE-Spam-Status: No, hits=0.9 required=5.0 tests=BAYES_50,TW_CP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 24 Apr 2010 01:39:24 +0000 Received: (qmail 13511 invoked from network); 24 Apr 2010 01:39:23 -0000 Received: from unknown (HELO macbook-2.local) (stan@127.0.0.2) by mail.codesourcery.com with ESMTPA; 24 Apr 2010 01:39:23 -0000 Message-ID: <4BD24BC1.4030103@codesourcery.com> Date: Sat, 24 Apr 2010 01:39:00 -0000 From: Stan Shebs User-Agent: Thunderbird 2.0.0.24 (Macintosh/20100228) MIME-Version: 1.0 To: gdb-patches@sourceware.org Subject: [RFC] Check original contents at tracepoints Content-Type: multipart/mixed; boundary="------------000101000309000307060206" 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-04/txt/msg00828.txt.bz2 This is a multi-part message in MIME format. --------------000101000309000307060206 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2642 This is a patch that is a little weird, plus has a portability/usability issue to resolve, so I'm fishing for a little feedback. Our tracing target, among its other complexities, has a JIT, and we're told that it's useful to set tracepoints in JIT code. However, the JIT can unload the tracepointed code and reload other code in its place without telling anybody, so if you start the trace run after that has happened, perhaps in a nonstop mode, the tracepoints get inserted in the middle of instructions, and badness ensues. So the idea of this patch is to record, at definition time, the bytes that the tracepoint expects to overwrite when it is downloaded for the trace run. The agent then compares with what is currently at that address, and refuses to start the run if there is any discrepancy. (In theory this could be a problem for breakpoints also, but breakpoints insert and remove as part of stopping and resuming, so one can catch JIT changes more easily.) Assuming the whole idea has merit, one of the open issues has been to decide how many bytes to record. The patch as it stands has an x64-specific hack that assumes 1 for slow tracepoints, and 5 for fast. In theory this could be a gdbarch property, although the fast tracpoint sal check comes back with a number of instructions being replaced, and that could be used, at least for fast tracepoints. But one of the things that has troubled me is that for 1-byte tracepoints, there is a 1/256 chance of a mistaken acceptance, and perhaps the user/GDB should have the option to check more bytes. But then you get into the possibility of pulling bytes from a different area and problems associated with that. So before getting mired in still more idiosyncrasy, it seems like a good idea to check whether this is of interest for FSF GDB, and to get ideas on how to handle it. Stan 2010-04-23 Stan Shebs * breakpoint.h (struct bp_location): New fields orig_contents and orig_len. * breakpoint.c (check_original_contents): New global. (set_raw_breakpoint): Record original contents. (add_location_to_breakpoint): Ditto. * tracepoint.h (check_original_contents): Declare. * remote.c (PACKET_CheckOriginalContents): New feature enum. (remote_start_remote): Test it. (remote_protocol_features): Add CheckOriginalContents feature. (_initialize_remote): Add it as a config command. (remote_download_tracepoint): Add contents field to tracepoints. * gdb.texinfo (Tracepoint Packets): Describe optional fields. (General Query Packets): Describe CheckOriginalContents. --------------000101000309000307060206 Content-Type: text/plain; x-mac-type="0"; x-mac-creator="0"; name="origcontents-patch-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="origcontents-patch-1" Content-length: 10237 Index: breakpoint.c =================================================================== RCS file: /cvs/src/src/gdb/breakpoint.c,v retrieving revision 1.484 diff -p -r1.484 breakpoint.c *** breakpoint.c 19 Apr 2010 00:48:43 -0000 1.484 --- breakpoint.c 24 Apr 2010 00:51:15 -0000 *************** static int prev_breakpoint_count; *** 416,421 **** --- 416,430 ---- static int tracepoint_count; + /* This is true if we want to send the original program code that will + be overwritten by a tracepoint. This can happen with JIT code that + changes the program code between tracepoint definition and trace + run. We default to 1 so that tracepoint definition at startup does + record the original code, and expect targets that don't want this + in their tracepoint definitions to turn it off. */ + + int check_original_contents = 1; + static struct cmd_list_element *breakpoint_set_cmdlist; static struct cmd_list_element *breakpoint_show_cmdlist; static struct cmd_list_element *save_cmdlist; *************** set_raw_breakpoint (struct gdbarch *gdba *** 5481,5486 **** --- 5490,5498 ---- struct breakpoint *b = set_raw_breakpoint_without_location (gdbarch, bptype); CORE_ADDR adjusted_address; struct gdbarch *loc_gdbarch; + int orig_len = 0; + gdb_byte orig[BREAKPOINT_MAX]; + int err; loc_gdbarch = get_sal_arch (sal); if (!loc_gdbarch) *************** set_raw_breakpoint (struct gdbarch *gdba *** 5497,5502 **** --- 5509,5523 ---- location that's only been partially initialized. */ adjusted_address = adjust_breakpoint_address (loc_gdbarch, sal.pc, b->type); + if (check_original_contents) + { + orig_len = (b->type == bp_fast_tracepoint ? 5 : 1); /* x86-only */ + err = target_read_memory (adjusted_address, orig, orig_len); + /* If we can't read memory, just give up on this feature. */ + if (err) + orig_len = 0; + } + b->loc = allocate_bp_location (b); b->loc->gdbarch = loc_gdbarch; b->loc->requested_address = sal.pc; *************** set_raw_breakpoint (struct gdbarch *gdba *** 5516,5521 **** --- 5537,5549 ---- set_breakpoint_location_function (b->loc); + /* Save the bytes that we are expecting to replace. */ + if (check_original_contents && orig_len) + { + b->loc->orig_len = orig_len; + memcpy (b->loc->orig_contents, orig, orig_len); + } + breakpoints_changed (); return b; *************** add_location_to_breakpoint (struct break *** 6793,6798 **** --- 6821,6838 ---- loc->section = sal->section; set_breakpoint_location_function (loc); + + /* Maybe try to collect bytes at the location. */ + if (check_original_contents) + { + int err; + loc->orig_len = (b->type == bp_fast_tracepoint ? 5 : 1); /* x86-only */ + err = target_read_memory (loc->address, loc->orig_contents, + loc->orig_len); + if (err) + loc->orig_len = 0; + } + return loc; } Index: breakpoint.h =================================================================== RCS file: /cvs/src/src/gdb/breakpoint.h,v retrieving revision 1.118 diff -p -r1.118 breakpoint.h *** breakpoint.h 19 Apr 2010 00:48:43 -0000 1.118 --- breakpoint.h 24 Apr 2010 00:51:16 -0000 *************** struct bp_location *** 331,336 **** --- 331,344 ---- This variable keeps a number of events still to go, when it becomes 0 this location is retired. */ int events_till_retirement; + + /* For targets that have JITs that can change code between definition + time and execution time (for either breakpoints or tracepoints), + this is a copy of memory at the time the breakpoint was defined. */ + gdb_byte orig_contents[BREAKPOINT_MAX]; + + /* This is the number of bytes in orig_contents. */ + int orig_len; }; /* This structure is a collection of function pointers that, if available, Index: remote.c =================================================================== RCS file: /cvs/src/src/gdb/remote.c,v retrieving revision 1.402 diff -p -r1.402 remote.c *** remote.c 16 Apr 2010 07:49:35 -0000 1.402 --- remote.c 24 Apr 2010 00:51:16 -0000 *************** enum { *** 1153,1158 **** --- 1153,1159 ---- PACKET_bc, PACKET_bs, PACKET_TracepointSource, + PACKET_CheckOriginalContents, PACKET_MAX }; *************** remote_start_remote (struct ui_out *uiou *** 2956,2961 **** --- 2957,2965 ---- which later probes to skip. */ remote_query_supported (); + /* Only do the original contents check if the target wants it. */ + check_original_contents = (remote_protocol_packets[PACKET_CheckOriginalContents].support == PACKET_ENABLE); + /* Next, we possibly activate noack mode. If the QStartNoAckMode packet configuration is set to AUTO, *************** static struct protocol_feature remote_pr *** 3462,3467 **** --- 3466,3473 ---- PACKET_bs }, { "TracepointSource", PACKET_DISABLE, remote_supported_packet, PACKET_TracepointSource }, + { "CheckOriginalContents", PACKET_DISABLE, remote_supported_packet, + PACKET_CheckOriginalContents }, }; static char *remote_support_xml; *************** remote_download_tracepoint (struct break *** 9476,9481 **** --- 9482,9492 ---- else warning (_("Target does not support conditional tracepoints, ignoring tp %d cond"), t->number); } + if (check_original_contents && loc->orig_len > 0) + { + sprintf (buf + strlen (buf), ":W%x,", loc->orig_len); + bin2hex (loc->orig_contents, buf + strlen (buf), loc->orig_len); + } if (t->commands || *default_collect) strcat (buf, "-"); *************** Show the maximum size of the address (in *** 10415,10426 **** --- 10426,10441 ---- add_packet_config_cmd (&remote_protocol_packets[PACKET_ConditionalTracepoints], "ConditionalTracepoints", "conditional-tracepoints", 0); + add_packet_config_cmd (&remote_protocol_packets[PACKET_FastTracepoints], "FastTracepoints", "fast-tracepoints", 0); add_packet_config_cmd (&remote_protocol_packets[PACKET_TracepointSource], "TracepointSource", "TracepointSource", 0); + add_packet_config_cmd (&remote_protocol_packets[PACKET_CheckOriginalContents], + "CheckOriginalContents", "CheckOriginalContents", 0); + /* Keep the old ``set remote Z-packet ...'' working. Each individual Z sub-packet has its own set and show commands, but users may have sets to this variable in their .gdbinit files (or in their Index: tracepoint.h =================================================================== RCS file: /cvs/src/src/gdb/tracepoint.h,v retrieving revision 1.35 diff -p -r1.35 tracepoint.h *** tracepoint.h 9 Apr 2010 03:00:58 -0000 1.35 --- tracepoint.h 24 Apr 2010 00:51:16 -0000 *************** extern void tfind_1 (enum trace_find_typ *** 215,218 **** --- 215,220 ---- extern void trace_save (const char *filename, int target_does_save); + extern int check_original_contents; + #endif /* TRACEPOINT_H */ Index: doc/gdb.texinfo =================================================================== RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v retrieving revision 1.713 diff -p -r1.713 gdb.texinfo *** doc/gdb.texinfo 23 Apr 2010 16:20:10 -0000 1.713 --- doc/gdb.texinfo 24 Apr 2010 00:51:17 -0000 *************** These are the currently defined stub fea *** 31278,31283 **** --- 31278,31288 ---- @tab @samp{-} @tab No + @item @samp{CheckOriginalContents} + @tab No + @tab @samp{-} + @tab No + @end multitable These are the currently defined stub features, in more detail: *************** The remote stub accepts and implements t *** 31375,31380 **** --- 31380,31394 ---- The remote stub understands the @samp{QTDPsrc} packet that supplies the source form of tracepoint definitions. + @item CheckOriginalContents + If @value{GDBN} adds an optional @samp{W} field to tracepoint + definition packets, the target will check that the current program + code matches the supplied value of the field; if they do not match, it + will reply with an error and cancel the trace run. This is useful in + cases where a JIT or other dynamic code-changing mechanism may + invalidate the tracepoint definition between the time of its + definition and the tstart command. + @end table @item qSymbol:: *************** tracepoints (@pxref{Tracepoints}). *** 31734,31740 **** @table @samp ! @item QTDP:@var{n}:@var{addr}:@var{ena}:@var{step}:@var{pass}[:F@var{flen}][:X@var{len},@var{bytes}]@r{[}-@r{]} Create a new tracepoint, number @var{n}, at @var{addr}. If @var{ena} is @samp{E}, then the tracepoint is enabled; if it is @samp{D}, then the tracepoint is disabled. @var{step} is the tracepoint's step --- 31748,31754 ---- @table @samp ! @item QTDP:@var{n}:@var{addr}:@var{ena}:@var{step}:@var{pass}@r{[}:@var{options}@r{]}@r{[}-@r{]} Create a new tracepoint, number @var{n}, at @var{addr}. If @var{ena} is @samp{E}, then the tracepoint is enabled; if it is @samp{D}, then the tracepoint is disabled. @var{step} is the tracepoint's step *************** encodings as described below. If the tr *** 31748,31753 **** --- 31762,31790 ---- further @samp{QTDP} packets will follow to specify this tracepoint's actions. + Additional optional fields @var{options} may be supplied. They are + colon-separated, and distinguished by a letter. + + @table @samp + + @item F@var{n} + The tracepoint is to be a fast tracepoint, and @var{n} bytes of + program code (typically the length of one instruction replaced by the + tracepoint) are to be copied elsewhere. + + @item W@var{n},@var{bytes} + The bytes in the hex-encoded sequence @var{bytes} (length @var{n}, a + hexadecimal number) are the original program bytes present at the + tracepoint address. This is useful for checking that a JIT or other + dynamic loading mechanism has not silently invalidated the tracepoint. + + @item X@var{n},@var{bytes} + Defines a tracepoint condition, which consists of a hexadecimal + length, followed by a comma and hex-encoded bytes, in a manner similar + to action encodings as described below. + + @end table + Replies: @table @samp @item OK --------------000101000309000307060206--