[PATCH] Some tracepoint fixes

[PATCH] Some tracepoint fixes

[Nathan Sidwell]

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

In porting gdb to a new architecture, I came across a number of core gdb
bugs.   Here is the first set of them and addresses the following issues,

1) we did not allow 'extended-remote' targets to use tracepoints.

2) We could only trace architectures with 64 registers, not 256 like
a comment suggested.

3) There was an erroneous comment about tracing memory ranges

4) If a ^D was entered when entering the 'actions' list, we'd create
a NULL action, which would cause a segfault when tracing started.

5) The 'tstatus' command did not actually print any status. testcase
gdb.trace/tfind.exp exepected it to do so.

6) Parsing the tfind responses uses strtol to read hex.  That reads
'FFFFFFFF' as '7FFFFFFF' (and sets errno).  Using sscanf reads that as
-1, as desired.

built and tested on i686-pc-linux-gnu, (and on the unreleased architecture
I ported to) ok?

nathan


-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk


[-- Attachment #2: tracepoint1.patch --]
[-- Type: text/plain, Size: 4311 bytes --]

2005-02-08  Nathan Sidwell  <nathan@codesourcery.com>

	* tracepoint.c (target_is_remote): Allow extended-remote.
	(struct collection_list): Allow 256 registers, like the comment
	said.
	(add_memrange): Fix comment.
	(read_actions): Turn EOF into 'end'.
	(trace_status_command): Print the status.
	(finish_tfind_command): Use sscanf not strtol.

Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.68
diff -c -3 -p -r1.68 tracepoint.c
*** tracepoint.c	2 Feb 2005 00:20:05 -0000	1.68
--- tracepoint.c	8 Feb 2005 11:31:20 -0000
*************** static int
*** 164,170 ****
  target_is_remote (void)
  {
    if (current_target.to_shortname &&
!       strcmp (current_target.to_shortname, "remote") == 0)
      return 1;
    else
      return 0;
--- 164,171 ----
  target_is_remote (void)
  {
    if (current_target.to_shortname &&
!       (strcmp (current_target.to_shortname, "remote") == 0
!        || strcmp (current_target.to_shortname, "extended-remote") == 0))
      return 1;
    else
      return 0;
*************** read_actions (struct tracepoint *t)
*** 860,865 ****
--- 861,869 ----
        else
  	line = gdb_readline (0);
  
+       if (!line)
+ 	line = "end";
+       
        linetype = validate_actionline (&line, t);
        if (linetype == BADLINE)
  	continue;		/* already warned -- collect another line */
*************** struct memrange
*** 1074,1080 ****
  
  struct collection_list
    {
!     unsigned char regs_mask[8];	/* room for up to 256 regs */
      long listsize;
      long next_memrange;
      struct memrange *list;
--- 1078,1084 ----
  
  struct collection_list
    {
!     unsigned char regs_mask[32];	/* room for up to 256 regs */
      long listsize;
      long next_memrange;
      struct memrange *list;
*************** add_memrange (struct collection_list *me
*** 1171,1177 ****
        printf_filtered (",%ld)\n", len);
      }
  
!   /* type: 0 == memory, n == basereg */
    memranges->list[memranges->next_memrange].type = type;
    /* base: addr if memory, offset if reg relative.  */
    memranges->list[memranges->next_memrange].start = base;
--- 1175,1181 ----
        printf_filtered (",%ld)\n", len);
      }
  
!   /* type: -1 == memory, n == basereg */
    memranges->list[memranges->next_memrange].type = type;
    /* base: addr if memory, offset if reg relative.  */
    memranges->list[memranges->next_memrange].start = base;
*************** trace_status_command (char *args, int fr
*** 1873,1878 ****
--- 1877,1886 ----
  
        /* exported for use by the GUI */
        trace_running_p = (target_buf[1] == '1');
+       if (trace_running_p)
+ 	printf_filtered ("Trace is running.\n");
+       else
+ 	printf_filtered ("Trace is not running.\n");
      }
    else
      error ("Trace can only be run on remote targets.");
*************** finish_tfind_command (char *msg,
*** 1888,1893 ****
--- 1896,1902 ----
    CORE_ADDR old_frame_addr;
    struct symbol *old_func;
    char *reply;
+   int len;
  
    old_frame_addr = get_frame_base (get_current_frame ());
    old_func = find_pc_function (read_pc ());
*************** finish_tfind_command (char *msg,
*** 1899,1905 ****
      switch (*reply)
        {
        case 'F':
! 	if ((target_frameno = (int) strtol (++reply, &reply, 16)) == -1)
  	  {
  	    /* A request for a non-existant trace frame has failed.
  	       Our response will be different, depending on FROM_TTY:
--- 1908,1916 ----
      switch (*reply)
        {
        case 'F':
! 	sscanf (reply, "F%X%n", &target_frameno, &len);
! 	reply += len;
! 	if (target_frameno == -1)
  	  {
  	    /* A request for a non-existant trace frame has failed.
  	       Our response will be different, depending on FROM_TTY:
*************** finish_tfind_command (char *msg,
*** 1937,1943 ****
  	  }
  	break;
        case 'T':
! 	if ((target_tracept = (int) strtol (++reply, &reply, 16)) == -1)
  	  error ("Target failed to find requested trace frame.");
  	break;
        case 'O':		/* "OK"? */
--- 1948,1956 ----
  	  }
  	break;
        case 'T':
! 	sscanf (reply, "T%X%n", &target_tracept, &len);
! 	reply += len;
! 	if (target_tracept == -1)
  	  error ("Target failed to find requested trace frame.");
  	break;
        case 'O':		/* "OK"? */

Re: [PATCH] Some tracepoint fixes

[Daniel Jacobowitz]

On Wed, Feb 09, 2005 at 09:19:17AM +0000, Nathan Sidwell wrote:
> In porting gdb to a new architecture, I came across a number of core gdb
> bugs.   Here is the first set of them and addresses the following issues,
> 
> 1) we did not allow 'extended-remote' targets to use tracepoints.
> 
> 2) We could only trace architectures with 64 registers, not 256 like
> a comment suggested.
> 
> 3) There was an erroneous comment about tracing memory ranges
> 
> 4) If a ^D was entered when entering the 'actions' list, we'd create
> a NULL action, which would cause a segfault when tracing started.
> 
> 5) The 'tstatus' command did not actually print any status. testcase
> gdb.trace/tfind.exp exepected it to do so.
> 
> 6) Parsing the tfind responses uses strtol to read hex.  That reads
> 'FFFFFFFF' as '7FFFFFFF' (and sets errno).  Using sscanf reads that as
> -1, as desired.
> 
> built and tested on i686-pc-linux-gnu, (and on the unreleased architecture
> I ported to) ok?

Hi Nathan,

Some bits of this are OK, some aren't (and I'd want Michael's opinion
on them).  In particular, the tracepoint remote protocol packets don't
appear to be documented; so I'm not sure about the strtol change.  Your
change is definitely wrong one way or another, because it depends on
the size of "long" on the host.  You're passing a long* to sscanf where
it expects an unsigned long*.  If we expect a hex-encoded 32-bit
result, then let's parse it that way explicitly.

> *************** trace_status_command (char *args, int fr
> *** 1873,1878 ****
> --- 1877,1886 ----
>   
>         /* exported for use by the GUI */
>         trace_running_p = (target_buf[1] == '1');
> +       if (trace_running_p)
> + 	printf_filtered ("Trace is running.\n");
> +       else
> + 	printf_filtered ("Trace is not running.\n");
>       }
>     else
>       error ("Trace can only be run on remote targets.");

That's really weird.  This hasn't been there as far back as the code
has been in the FSF repository, but the testsuite definitely tests for
it... it's pretty useless in the CLI as it is.  Michael, any idea?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Some tracepoint fixes

[Nathan Sidwell]

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

Daniel Jacobowitz wrote:
> On Wed, Feb 09, 2005 at 09:19:17AM +0000, Nathan Sidwell wrote:
> 
>>In porting gdb to a new architecture, I came across a number of core gdb
>>bugs.   Here is the first set of them and addresses the following issues,
>>
>>1) we did not allow 'extended-remote' targets to use tracepoints.
>>
>>2) We could only trace architectures with 64 registers, not 256 like
>>a comment suggested.
>>
>>3) There was an erroneous comment about tracing memory ranges
>>
>>4) If a ^D was entered when entering the 'actions' list, we'd create
>>a NULL action, which would cause a segfault when tracing started.

> Some bits of this are OK, some aren't (and I'd want Michael's opinion
> on them).  In particular, the tracepoint remote protocol packets don't
> appear to be documented; so I'm not sure about the strtol change.  Your
> change is definitely wrong one way or another, because it depends on
> the size of "long" on the host.  You're passing a long* to sscanf where
> it expects an unsigned long*.  If we expect a hex-encoded 32-bit
> result, then let's parse it that way explicitly.

ok. I've installed the attached patch, which are the uncontraversial
bits.  I'll address the strtol one separately.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk


[-- Attachment #2: tracepoint1a.patch --]
[-- Type: text/plain, Size: 2388 bytes --]

2005-03-08  Nathan Sidwell  <nathan@codesourcery.com>

	* tracepoint.c (target_is_remote): Allow extended-remote.
	(struct collection_list): Allow 256 registers, like the comment
	said.
	(add_memrange): Fix comment.
	(read_actions): Turn EOF into 'end'.

Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.68
diff -c -3 -p -r1.68 tracepoint.c
*** tracepoint.c	2 Feb 2005 00:20:05 -0000	1.68
--- tracepoint.c	8 Feb 2005 11:31:20 -0000
*************** static int
*** 164,170 ****
  target_is_remote (void)
  {
    if (current_target.to_shortname &&
!       strcmp (current_target.to_shortname, "remote") == 0)
      return 1;
    else
      return 0;
--- 164,171 ----
  target_is_remote (void)
  {
    if (current_target.to_shortname &&
!       (strcmp (current_target.to_shortname, "remote") == 0
!        || strcmp (current_target.to_shortname, "extended-remote") == 0))
      return 1;
    else
      return 0;
*************** read_actions (struct tracepoint *t)
*** 860,865 ****
--- 861,869 ----
        else
  	line = gdb_readline (0);
  
+       if (!line)
+ 	line = "end";
+       
        linetype = validate_actionline (&line, t);
        if (linetype == BADLINE)
  	continue;		/* already warned -- collect another line */
*************** struct memrange
*** 1074,1080 ****
  
  struct collection_list
    {
!     unsigned char regs_mask[8];	/* room for up to 256 regs */
      long listsize;
      long next_memrange;
      struct memrange *list;
--- 1078,1084 ----
  
  struct collection_list
    {
!     unsigned char regs_mask[32];	/* room for up to 256 regs */
      long listsize;
      long next_memrange;
      struct memrange *list;
*************** add_memrange (struct collection_list *me
*** 1171,1177 ****
        printf_filtered (",%ld)\n", len);
      }
  
!   /* type: 0 == memory, n == basereg */
    memranges->list[memranges->next_memrange].type = type;
    /* base: addr if memory, offset if reg relative.  */
    memranges->list[memranges->next_memrange].start = base;
--- 1175,1181 ----
        printf_filtered (",%ld)\n", len);
      }
  
!   /* type: -1 == memory, n == basereg */
    memranges->list[memranges->next_memrange].type = type;
    /* base: addr if memory, offset if reg relative.  */
    memranges->list[memranges->next_memrange].start = base;