[RFC] Check original contents at tracepoints

[Stan Shebs <stan at codesourcery dot com>]

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  <stan@codesourcery.com>

   * 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.

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