[PATCH] Do partial xfers from trace file

[PATCH] Do partial xfers from trace file

[Stan Shebs]

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]

Memory blocks in the trace buffer are limited to 65K (to save a couple 
bytes in the length field, since most blocks are small), and a tester 
trying to collect a quarter-megabyte(!) C++ object ran into trouble with 
that.  The fix is really a target-side thing, but the trace file reader 
needs to cognizant of this detail also.  Fortunately, we can exploit 
GDB's partial xfer mechanism, and just return what we find in one block, 
expecting that GDB will re-request the remainder.

I also made the tfile target has_all_memory, and added an emulation of 
QTro behavior, which lets disassembly and the like work, but rejects 
attempts to print non-constant globals that were not collected.  
Committed to trunk.

Stan

2010-04-05  Stan Shebs  <stan@codesourcery.com>

    * tracepoint.c: Include gdbcore.h.
    (tfile_xfer_partial): Return partial results, also try reading
    from executable.
    (tfile_has_all_memory): New function.
    (init_tfile_ops): Use it.

    * gdb.trace/tfile.c: Add a variable split across two blocks, and a
    constant global.
    * gdb.trace/tfile.exp: Try to print them.


[-- Attachment #2: mempart-patch-1 --]
[-- Type: text/plain, Size: 7089 bytes --]

Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.171
diff -p -r1.171 tracepoint.c
*** tracepoint.c	4 Apr 2010 23:31:28 -0000	1.171
--- tracepoint.c	5 Apr 2010 21:55:59 -0000
***************
*** 45,50 ****
--- 45,51 ----
  #include "filenames.h"
  #include "gdbthread.h"
  #include "stack.h"
+ #include "gdbcore.h"
  
  #include "ax.h"
  #include "ax-gdb.h"
*************** tfile_xfer_partial (struct target_ops *o
*** 3793,3799 ****
  {
    char block_type;
    int pos, gotten;
!   ULONGEST maddr;
    unsigned short mlen;
  
    /* We're only doing regular memory for now.  */
--- 3794,3800 ----
  {
    char block_type;
    int pos, gotten;
!   ULONGEST maddr, amt;
    unsigned short mlen;
  
    /* We're only doing regular memory for now.  */
*************** tfile_xfer_partial (struct target_ops *o
*** 3831,3846 ****
  	    perror_with_name (trace_filename);
  	  else if (gotten < 2)
  	    error (_("Premature end of file while reading trace file"));
! 	  if (maddr <= offset && (offset + len) <= (maddr + mlen))
! 	    {
! 	      gotten = read (trace_fd, readbuf, mlen);
! 	      if (gotten < 0)
! 		perror_with_name (trace_filename);
! 	      else if (gotten < mlen)
! 		error (_("Premature end of file qwhile reading trace file"));
! 
! 	      return mlen;
! 	    }
  	  lseek (trace_fd, mlen, SEEK_CUR);
  	  pos += (8 + 2 + mlen);
  	  break;
--- 3832,3850 ----
  	    perror_with_name (trace_filename);
  	  else if (gotten < 2)
  	    error (_("Premature end of file while reading trace file"));
! 	  /* If the block includes the first part of the desired
! 	     range, return as much it has; GDB will re-request the
! 	     remainder, which might be in a different block of this
! 	     trace frame.  */
! 	  if (maddr <= offset && offset < (maddr + mlen))
!   	    {
! 	      amt = (maddr + mlen) - offset;
! 	      if (amt > len)
! 		amt = len;
! 
! 	      read (trace_fd, readbuf, amt);
! 	      return amt;
!   	    }
  	  lseek (trace_fd, mlen, SEEK_CUR);
  	  pos += (8 + 2 + mlen);
  	  break;
*************** tfile_xfer_partial (struct target_ops *o
*** 3854,3859 ****
--- 3858,3895 ----
  	  break;
  	}
      }
+ 
+   /* It's unduly pedantic to refuse to look at the executable for
+      read-only pieces; so do the equivalent of readonly regions aka
+      QTro packet.  */
+   /* FIXME account for relocation at some point */
+   if (exec_bfd)
+     {
+       asection *s;
+       bfd_size_type size;
+       bfd_vma lma;
+ 
+       for (s = exec_bfd->sections; s; s = s->next)
+ 	{
+ 	  if ((s->flags & SEC_LOAD) == 0 ||
+ 	      (s->flags & SEC_READONLY) == 0)
+ 	    continue;
+ 
+ 	  lma = s->lma;
+ 	  size = bfd_get_section_size (s);
+ 	  if (lma <= offset && offset < (lma + size))
+ 	    {
+ 	      amt = (lma + size) - offset;
+ 	      if (amt > len)
+ 		amt = len;
+ 
+ 	      amt = bfd_get_section_contents (exec_bfd, s,
+ 					      readbuf, offset - lma, amt);
+ 	      return amt;
+ 	    }
+ 	}
+     }
+ 
    /* Indicate failure to find the requested memory block.  */
    return -1;
  }
*************** tfile_get_trace_state_variable_value (in
*** 3923,3928 ****
--- 3959,3970 ----
  }
  
  static int
+ tfile_has_all_memory (struct target_ops *ops)
+ {
+   return 1;
+ }
+ 
+ static int
  tfile_has_memory (struct target_ops *ops)
  {
    return 1;
*************** init_tfile_ops (void)
*** 3958,3963 ****
--- 4000,4006 ----
    /* core_stratum might seem more logical, but GDB doesn't like having
       more than one core_stratum vector.  */
    tfile_ops.to_stratum = process_stratum;
+   tfile_ops.to_has_all_memory = tfile_has_all_memory;
    tfile_ops.to_has_memory = tfile_has_memory;
    tfile_ops.to_has_stack = tfile_has_stack;
    tfile_ops.to_has_registers = tfile_has_registers;
Index: testsuite/gdb.trace/tfile.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/tfile.c,v
retrieving revision 1.3
diff -p -r1.3 tfile.c
*** testsuite/gdb.trace/tfile.c	26 Mar 2010 14:31:48 -0000	1.3
--- testsuite/gdb.trace/tfile.c	5 Apr 2010 21:55:59 -0000
*************** char *tfsizeptr;
*** 15,20 ****
--- 15,24 ----
  
  int testglob = 31415;
  
+ int testglob2 = 271828;
+ 
+ const int constglob = 10000;
+ 
  int
  start_trace_file (char *filename)
  {
*************** finish_trace_file (int fd)
*** 40,51 ****
    close (fd);
  }
  
  void
  write_basic_trace_file (void)
  {
    int fd, int_x;
    short short_x;
-   long long ll_x;
  
    fd = start_trace_file ("basic.tf");
  
--- 44,73 ----
    close (fd);
  }
  
+ 
+ void
+ add_memory_block (char *addr, int size)
+ {
+   short short_x;
+   long long ll_x;
+ 
+   *((char *) trptr) = 'M';
+   trptr += 1;
+   ll_x = (long) addr;
+   memcpy (trptr, &ll_x, sizeof (long long));
+   trptr += sizeof (long long);
+   short_x = size;
+   memcpy (trptr, &short_x, 2);
+   trptr += 2;
+   memcpy (trptr, addr, size);
+   trptr += size;
+ }
+ 
  void
  write_basic_trace_file (void)
  {
    int fd, int_x;
    short short_x;
  
    fd = start_trace_file ("basic.tf");
  
*************** write_basic_trace_file (void)
*** 82,97 ****
    trptr += 2;
    tfsizeptr = trptr;
    trptr += 4;
!   *((char *) trptr) = 'M';
!   trptr += 1;
!   ll_x = (long) &testglob;
!   memcpy (trptr, &ll_x, sizeof (long long));
!   trptr += sizeof (long long);
!   short_x = sizeof (testglob);
!   memcpy (trptr, &short_x, 2);
!   trptr += 2;
!   memcpy (trptr, &testglob, sizeof (testglob));
!   trptr += sizeof (testglob);
    /* Go back and patch in the frame size.  */
    int_x = trptr - tfsizeptr - sizeof (int);
    memcpy (tfsizeptr, &int_x, 4);
--- 104,113 ----
    trptr += 2;
    tfsizeptr = trptr;
    trptr += 4;
!   add_memory_block (&testglob, sizeof (testglob));
!   /* Divide a variable between two separate memory blocks.  */
!   add_memory_block (&testglob2, 1);
!   add_memory_block (((char*) &testglob2) + 1, sizeof (testglob2) - 1);
    /* Go back and patch in the frame size.  */
    int_x = trptr - tfsizeptr - sizeof (int);
    memcpy (tfsizeptr, &int_x, 4);
Index: testsuite/gdb.trace/tfile.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/tfile.exp,v
retrieving revision 1.5
diff -p -r1.5 tfile.exp
*** testsuite/gdb.trace/tfile.exp	4 Apr 2010 23:31:29 -0000	1.5
--- testsuite/gdb.trace/tfile.exp	5 Apr 2010 21:55:59 -0000
*************** gdb_test "tfind 0" \
*** 74,81 ****
--- 74,88 ----
  \#0  write_basic_trace_file ().*" \
      "tfind 0 on trace file"
  
+ # Note that there is no tracepoint collecting these globals, we
+ # just happen to know they are covered by the trace frame.
+ 
  gdb_test "print testglob" " = 31415" "print testglob on trace file"
  
+ gdb_test "print testglob2" " = 271828" "print testglob2 on trace file"
+ 
+ gdb_test "print constglob" " = 10000" "print constglob on trace file"
+ 
  gdb_test "tfind" "Target failed to find requested trace frame." \
      "tfind does not find a second frame in trace file"
  

Re: [PATCH] Do partial xfers from trace file

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1172 bytes --]

On Monday 05 April 2010 18:10:49 Stan Shebs wrote:
> Memory blocks in the trace buffer are limited to 65K (to save a couple
> bytes in the length field, since most blocks are small), and a tester
> trying to collect a quarter-megabyte(!) C++ object ran into trouble with
> that.  The fix is really a target-side thing, but the trace file reader
> needs to cognizant of this detail also.  Fortunately, we can exploit
> GDB's partial xfer mechanism, and just return what we find in one block,
> expecting that GDB will re-request the remainder.
> 
> I also made the tfile target has_all_memory, and added an emulation of
> QTro behavior, which lets disassembly and the like work, but rejects
> attempts to print non-constant globals that were not collected.
> Committed to trunk.

there's some problems with this commit ...

it introduces a warning which breaks with -Werror:
tracepoint.c: In function ‘tfile_xfer_partial’:
tracepoint.c:3895: error: ignoring return value of ‘read’, declared with 
attribute warn_unused_result

that particular piece of code also has slightly broken whitespace:
<space><space><tab><space><space>.......
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] Do partial xfers from trace file

[Stan Shebs]

Mike Frysinger wrote:
> On Monday 05 April 2010 18:10:49 Stan Shebs wrote:
>   
>> Memory blocks in the trace buffer are limited to 65K (to save a couple
>> bytes in the length field, since most blocks are small), and a tester
>> trying to collect a quarter-megabyte(!) C++ object ran into trouble with
>> that.  The fix is really a target-side thing, but the trace file reader
>> needs to cognizant of this detail also.  Fortunately, we can exploit
>> GDB's partial xfer mechanism, and just return what we find in one block,
>> expecting that GDB will re-request the remainder.
>>
>> I also made the tfile target has_all_memory, and added an emulation of
>> QTro behavior, which lets disassembly and the like work, but rejects
>> attempts to print non-constant globals that were not collected.
>> Committed to trunk.
>>     
>
> there's some problems with this commit ...
>
> it introduces a warning which breaks with -Werror:
> tracepoint.c: In function ‘tfile_xfer_partial’:
> tracepoint.c:3895: error: ignoring return value of ‘read’, declared with 
> attribute warn_unused_result
>   

It's a little weird that some people get a complaint and others 
don't...  In any case, it's just an oversight, all the other read 
results are being checked, and so I just committed the fix below.

Stan

2010-04-12  Stan Shebs  <stan@codesourcery.com>

    * tracepoint.c (tfile_xfer_partial): Check read result.

Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.177
diff -p -r1.177 tracepoint.c
*** tracepoint.c    9 Apr 2010 20:46:40 -0000    1.177
--- tracepoint.c    13 Apr 2010 03:52:28 -0000
*************** tfile_xfer_partial (struct target_ops *o
*** 3892,3898 ****
            if (amt > len)
          amt = len;
 
!           read (trace_fd, readbuf, amt);
            return amt;
            }
        lseek (trace_fd, mlen, SEEK_CUR);
--- 3892,3905 ----
            if (amt > len)
          amt = len;
 
!           gotten = read (trace_fd, readbuf, amt);
!           if (gotten < 0)
!         perror_with_name (trace_filename);
!           /* While it's acceptable to return less than was
!          originally asked for, it's not acceptable to return
!          less than what this block claims to contain.  */
!           else if (gotten < amt)
!         error (_("Premature end of file while reading trace file"));
            return amt;
            }
        lseek (trace_fd, mlen, SEEK_CUR);

Re: [PATCH] Do partial xfers from trace file

[Tom Tromey]

>>>>> "Stan" == Stan Shebs <stan@codesourcery.com> writes:

>> tracepoint.c:3895: error: ignoring return value of ‘read’, declared
>> with attribute warn_unused_result

Stan> It's a little weird that some people get a complaint and others
Stan> don't...  In any case, it's just an oversight, all the other read
Stan> results are being checked, and so I just committed the fix below.

There is a distro difference here.  Some distros have warn_unused_result
enabled by default on some functions.

Tom

Re: [PATCH] Do partial xfers from trace file

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 866 bytes --]

On Tuesday 13 April 2010 12:26:38 Tom Tromey wrote:
> >>>>> "Stan" == Stan Shebs <stan@codesourcery.com> writes:
> >> tracepoint.c:3895: error: ignoring return value of ‘read’, declared
> >> with attribute warn_unused_result
> 
> Stan> It's a little weird that some people get a complaint and others
> Stan> don't...  In any case, it's just an oversight, all the other read
> Stan> results are being checked, and so I just committed the fix below.
> 
> There is a distro difference here.  Some distros have warn_unused_result
> enabled by default on some functions.

not exactly ... all distros that keep up with mainline glibc have this 
enumeration.  some distros (like Ubuntu and Gentoo) enable _FORTIFY_SOURCE by 
default in their gcc which signals glibc to apply the warn_unused_result 
attribute to a lot of functions (like read()).
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]