[PATCH] Use sized types in tracepoint.

[PATCH] Use sized types in tracepoint.

[Yao Qi]

Hi,
When discussing on the patches on IPA protocol, I start to examine the size
of each field of tracepoint when copying them to `command buffer'.  In the
protocol, each field has its own size, independent of other factors, such
as machine type (32bit vs 64bit), etc.  I started to think about using 'sized'
types in these fields, because it is safe to copy data from one process to
the other one.  This is what this patch does.

These sized types were introduced in C99, and we are using C90 in GDB, IIRC.
Not sure using them in GDB is accpetable.

Regression tested on x86_64-linux.  OK?

gdb/gdbserver:

2012-03-16  Yao Qi  <yao@codesourcery.com>

	* tracepoint.c: Include inttypes.h.
	(struct collect_memory_action): Use sized types.
	(struct tracepoint): Likewise.
	(cmd_qtdp, stop_tracing): Update print specifiers.
	(cmd_qtp, response_tracepoint): Likewise.
	(collect_data_at_tracepoint): Likewise.
	(collect_data_at_step): Likewise.
---
 gdb/gdbserver/tracepoint.c |   32 +++++++++++++++++---------------
 1 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 2144f6f..3662626 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -24,6 +24,7 @@
 #include <unistd.h>
 #include <sys/time.h>
 #include <stddef.h>
+#include <inttypes.h>
 #if HAVE_STDINT_H
 #include <stdint.h>
 #endif
@@ -504,7 +505,7 @@ struct collect_memory_action
 
   ULONGEST addr;
   ULONGEST len;
-  int basereg;
+  int32_t basereg;
 };
 
 /* An 'R' (collect registers) action.  */
@@ -724,7 +725,7 @@ struct tracepoint
 {
   /* The number of the tracepoint, as specified by GDB.  Several
      tracepoint objects here may share a number.  */
-  int number;
+  uint32_t number;
 
   /* Address at which the tracepoint is supposed to trigger.  Several
      tracepoints may share an address.  */
@@ -734,30 +735,30 @@ struct tracepoint
   enum tracepoint_type type;
 
   /* True if the tracepoint is currently enabled.  */
-  int enabled;
+  int8_t enabled;
 
   /* The number of single steps that will be performed after each
      tracepoint hit.  */
-  long step_count;
+  uint64_t step_count;
 
   /* The number of times the tracepoint may be hit before it will
      terminate the entire tracing run.  */
-  long pass_count;
+  uint64_t pass_count;
 
   /* Pointer to the agent expression that is the tracepoint's
      conditional, or NULL if the tracepoint is unconditional.  */
   struct agent_expr *cond;
 
   /* The list of actions to take when the tracepoint triggers.  */
-  int numactions;
+  uint32_t numactions;
   struct tracepoint_action **actions;
 
   /* Count of the times we've hit this tracepoint during the run.
      Note that while-stepping steps are not counted as "hits".  */
-  long hit_count;
+  uint64_t hit_count;
 
   /* Cached sum of the sizes of traceframes created by this point.  */
-  long traceframe_usage;
+  uint64_t traceframe_usage;
 
   CORE_ADDR compiled_cond;
 
@@ -777,7 +778,7 @@ struct tracepoint
   /* The number of bytes displaced by fast tracepoints. It may subsume
      multiple instructions, for multi-byte fast tracepoints.  This
      field is only valid for fast tracepoints.  */
-  int orig_size;
+  uint32_t orig_size;
 
   /* Only for fast tracepoints.  */
   CORE_ADDR obj_addr_on_target;
@@ -2555,7 +2556,7 @@ cmd_qtdp (char *own_buf)
 	}
 
       trace_debug ("Defined %stracepoint %d at 0x%s, "
-		   "enabled %d step %ld pass %ld",
+		   "enabled %d step %" PRIu64 " pass %" PRIu64,
 		   tpoint->type == fast_tracepoint ? "fast "
 		   : tpoint->type == static_tracepoint ? "static " : "",
 		   tpoint->number, paddress (tpoint->address), tpoint->enabled,
@@ -3404,7 +3405,7 @@ stop_tracing (void)
   if (stopping_tracepoint)
     {
       trace_debug ("Stopping the trace because "
-		   "tracepoint %d was hit %ld times",
+		   "tracepoint %d was hit %" PRIu64 " times",
 		   stopping_tracepoint->number,
 		   stopping_tracepoint->pass_count);
       tracing_stop_reason = "tpasscount";
@@ -3682,7 +3683,8 @@ cmd_qtp (char *own_buf)
       return;
     }
 
-  sprintf (own_buf, "V%lx:%lx", tpoint->hit_count, tpoint->traceframe_usage);
+  sprintf (own_buf, "V%" PRIu64 ":%" PRIu64 "", tpoint->hit_count,
+	   tpoint->traceframe_usage);
 }
 
 /* State variables to help return all the tracepoint bits.  */
@@ -3700,7 +3702,7 @@ response_tracepoint (char *packet, struct tracepoint *tpoint)
 {
   char *buf;
 
-  sprintf (packet, "T%x:%s:%c:%lx:%lx", tpoint->number,
+  sprintf (packet, "T%x:%s:%c:%" PRIx64 ":%" PRIx64, tpoint->number,
 	   paddress (tpoint->address),
 	   (tpoint->enabled ? 'E' : 'D'), tpoint->step_count,
 	   tpoint->pass_count);
@@ -4537,7 +4539,7 @@ collect_data_at_tracepoint (struct tracepoint_hit_ctx *ctx, CORE_ADDR stop_pc,
       && stopping_tracepoint == NULL)
     stopping_tracepoint = tpoint;
 
-  trace_debug ("Making new traceframe for tracepoint %d at 0x%s, hit %ld",
+  trace_debug ("Making new traceframe for tracepoint %d at 0x%s, hit %" PRIu64,
 	       tpoint->number, paddress (tpoint->address), tpoint->hit_count);
 
   tframe = add_traceframe (tpoint);
@@ -4574,7 +4576,7 @@ collect_data_at_step (struct tracepoint_hit_ctx *ctx,
   int acti;
 
   trace_debug ("Making new step traceframe for "
-	       "tracepoint %d at 0x%s, step %d of %ld, hit %ld",
+	       "tracepoint %d at 0x%s, step %d of %" PRIu64 ", hit %" PRIu64,
 	       tpoint->number, paddress (tpoint->address),
 	       current_step, tpoint->step_count,
 	       tpoint->hit_count);
-- 
1.7.0.4

Re: [PATCH] Use sized types in tracepoint.

[Mark Kettenis]

> From: Yao Qi <yao@codesourcery.com>
> Date: Fri, 16 Mar 2012 21:46:58 +0800
> 
> Hi, When discussing on the patches on IPA protocol, I start to
> examine the size of each field of tracepoint when copying them to
> `command buffer'.  In the protocol, each field has its own size,
> independent of other factors, such as machine type (32bit vs 64bit),
> etc.  I started to think about using 'sized' types in these fields,
> because it is safe to copy data from one process to the other one.
> This is what this patch does.
> 
> These sized types were introduced in C99, and we are using C90 in
> GDB, IIRC.  Not sure using them in GDB is accpetable.

We have gnulib/stdint.h so using typedefs like int32_t should be ok.
But I don't think we have support for the PRIxxx macros, so the
changes to the printing code aren't ok I'm afraid.

> Regression tested on x86_64-linux.  OK?
> 
> gdb/gdbserver:
> 
> 2012-03-16  Yao Qi  <yao@codesourcery.com>
> 
> 	* tracepoint.c: Include inttypes.h.
> 	(struct collect_memory_action): Use sized types.
> 	(struct tracepoint): Likewise.
> 	(cmd_qtdp, stop_tracing): Update print specifiers.
> 	(cmd_qtp, response_tracepoint): Likewise.
> 	(collect_data_at_tracepoint): Likewise.
> 	(collect_data_at_step): Likewise.
> ---
>  gdb/gdbserver/tracepoint.c |   32 +++++++++++++++++---------------
>  1 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index 2144f6f..3662626 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -24,6 +24,7 @@
>  #include <unistd.h>
>  #include <sys/time.h>
>  #include <stddef.h>
> +#include <inttypes.h>
>  #if HAVE_STDINT_H
>  #include <stdint.h>
>  #endif
> @@ -504,7 +505,7 @@ struct collect_memory_action
>  
>    ULONGEST addr;
>    ULONGEST len;
> -  int basereg;
> +  int32_t basereg;
>  };
>  
>  /* An 'R' (collect registers) action.  */
> @@ -724,7 +725,7 @@ struct tracepoint
>  {
>    /* The number of the tracepoint, as specified by GDB.  Several
>       tracepoint objects here may share a number.  */
> -  int number;
> +  uint32_t number;
>  
>    /* Address at which the tracepoint is supposed to trigger.  Several
>       tracepoints may share an address.  */
> @@ -734,30 +735,30 @@ struct tracepoint
>    enum tracepoint_type type;
>  
>    /* True if the tracepoint is currently enabled.  */
> -  int enabled;
> +  int8_t enabled;
>  
>    /* The number of single steps that will be performed after each
>       tracepoint hit.  */
> -  long step_count;
> +  uint64_t step_count;
>  
>    /* The number of times the tracepoint may be hit before it will
>       terminate the entire tracing run.  */
> -  long pass_count;
> +  uint64_t pass_count;
>  
>    /* Pointer to the agent expression that is the tracepoint's
>       conditional, or NULL if the tracepoint is unconditional.  */
>    struct agent_expr *cond;
>  
>    /* The list of actions to take when the tracepoint triggers.  */
> -  int numactions;
> +  uint32_t numactions;
>    struct tracepoint_action **actions;
>  
>    /* Count of the times we've hit this tracepoint during the run.
>       Note that while-stepping steps are not counted as "hits".  */
> -  long hit_count;
> +  uint64_t hit_count;
>  
>    /* Cached sum of the sizes of traceframes created by this point.  */
> -  long traceframe_usage;
> +  uint64_t traceframe_usage;
>  
>    CORE_ADDR compiled_cond;
>  
> @@ -777,7 +778,7 @@ struct tracepoint
>    /* The number of bytes displaced by fast tracepoints. It may subsume
>       multiple instructions, for multi-byte fast tracepoints.  This
>       field is only valid for fast tracepoints.  */
> -  int orig_size;
> +  uint32_t orig_size;
>  
>    /* Only for fast tracepoints.  */
>    CORE_ADDR obj_addr_on_target;
> @@ -2555,7 +2556,7 @@ cmd_qtdp (char *own_buf)
>  	}
>  
>        trace_debug ("Defined %stracepoint %d at 0x%s, "
> -		   "enabled %d step %ld pass %ld",
> +		   "enabled %d step %" PRIu64 " pass %" PRIu64,
>  		   tpoint->type == fast_tracepoint ? "fast "
>  		   : tpoint->type == static_tracepoint ? "static " : "",
>  		   tpoint->number, paddress (tpoint->address), tpoint->enabled,
> @@ -3404,7 +3405,7 @@ stop_tracing (void)
>    if (stopping_tracepoint)
>      {
>        trace_debug ("Stopping the trace because "
> -		   "tracepoint %d was hit %ld times",
> +		   "tracepoint %d was hit %" PRIu64 " times",
>  		   stopping_tracepoint->number,
>  		   stopping_tracepoint->pass_count);
>        tracing_stop_reason = "tpasscount";
> @@ -3682,7 +3683,8 @@ cmd_qtp (char *own_buf)
>        return;
>      }
>  
> -  sprintf (own_buf, "V%lx:%lx", tpoint->hit_count, tpoint->traceframe_usage);
> +  sprintf (own_buf, "V%" PRIu64 ":%" PRIu64 "", tpoint->hit_count,
> +	   tpoint->traceframe_usage);
>  }
>  
>  /* State variables to help return all the tracepoint bits.  */
> @@ -3700,7 +3702,7 @@ response_tracepoint (char *packet, struct tracepoint *tpoint)
>  {
>    char *buf;
>  
> -  sprintf (packet, "T%x:%s:%c:%lx:%lx", tpoint->number,
> +  sprintf (packet, "T%x:%s:%c:%" PRIx64 ":%" PRIx64, tpoint->number,
>  	   paddress (tpoint->address),
>  	   (tpoint->enabled ? 'E' : 'D'), tpoint->step_count,
>  	   tpoint->pass_count);
> @@ -4537,7 +4539,7 @@ collect_data_at_tracepoint (struct tracepoint_hit_ctx *ctx, CORE_ADDR stop_pc,
>        && stopping_tracepoint == NULL)
>      stopping_tracepoint = tpoint;
>  
> -  trace_debug ("Making new traceframe for tracepoint %d at 0x%s, hit %ld",
> +  trace_debug ("Making new traceframe for tracepoint %d at 0x%s, hit %" PRIu64,
>  	       tpoint->number, paddress (tpoint->address), tpoint->hit_count);
>  
>    tframe = add_traceframe (tpoint);
> @@ -4574,7 +4576,7 @@ collect_data_at_step (struct tracepoint_hit_ctx *ctx,
>    int acti;
>  
>    trace_debug ("Making new step traceframe for "
> -	       "tracepoint %d at 0x%s, step %d of %ld, hit %ld",
> +	       "tracepoint %d at 0x%s, step %d of %" PRIu64 ", hit %" PRIu64,
>  	       tpoint->number, paddress (tpoint->address),
>  	       current_step, tpoint->step_count,
>  	       tpoint->hit_count);
> -- 
> 1.7.0.4
> 
> 

Re: [PATCH] Use sized types in tracepoint.

[Pedro Alves]

On 03/16/2012 03:50 PM, Mark Kettenis wrote:

> We have gnulib/stdint.h so using typedefs like int32_t should be ok.


GDBserver doesn't use gnulib, unfortunately.  Thought, it looks like
the Linux, Windows and Lynx backends already unconditionally include stdint.h.
The NTO backend doesn't, so I'm not sure if we can include it unconditionally.

On 03/16/2012 01:46 PM, Yao Qi wrote:
> +#include <inttypes.h>
>  #if HAVE_STDINT_H
       ^^^^^^^^^^^^^
>  #include <stdint.h>
>  #endif

So I'd like to see that HAVE_STDINT_H removed first.  If this causes
trouble, the option would be for gdbserver to use gnulib proper.

-- 
Pedro Alves

Re: [PATCH] Use sized types in tracepoint.

[Yao Qi]

On 03/16/2012 11:57 PM, Pedro Alves wrote:
> On 03/16/2012 03:50 PM, Mark Kettenis wrote:
> 
>> We have gnulib/stdint.h so using typedefs like int32_t should be ok.
> 
> 
> GDBserver doesn't use gnulib, unfortunately.  Thought, it looks like
> the Linux, Windows and Lynx backends already unconditionally include stdint.h.
> The NTO backend doesn't, so I'm not sure if we can include it unconditionally.
> 

Unless I miss something, I don't see NTO-backend use stdint.h.

$ grep stdint *.c *.h
ax.c:     be to import stdint.h from gnulib.  */
linux-i386-ipa.c:#include <stdint.h>
lynx-i386-low.c:#include <stdint.h>
lynx-ppc-low.c:#include <stdint.h>
thread-db.c:#include <stdint.h>
tracepoint.c:#include <stdint.h>
win32-low.c:#include <stdint.h>

tracepoint.c is the only one file using stdint.h conditionally.

> On 03/16/2012 01:46 PM, Yao Qi wrote:
>> +#include <inttypes.h>
>>  #if HAVE_STDINT_H
>        ^^^^^^^^^^^^^
>>  #include <stdint.h>
>>  #endif
> 
> So I'd like to see that HAVE_STDINT_H removed first.  If this causes
> trouble, the option would be for gdbserver to use gnulib proper.
> 

What kind of trouble you expect to see?  Unable to build?  I removed
HAVE_STDINT_H, and successfully build gdbserver on linux, gdbserver on
mingw32, and gdbserver on tic6x-uclinux.  Is it qualified as "cause no
trouble"?

In parallel, I am writing a patch to use gnulib in gdbserver.  I am not
sure how to place gnulib source tree.  We have to choices here,
  - Import gnulib separately in gdbserver.  We have to gnulib source
trees in gdb and gdbserver respectively, some duplication.  However, we
gain some flexibility when gdb and gdbserver wants to import different
modules of gnulib.
  - Use gnulib in gdb.  Since gnulib is designed as a sub-dir of any
project, if we don't import gnulib in gdbserver, we have to copy gnulib
source to gdbserver source tree during build, and remove it when
finished.  Since we have a single source of gnulib, we have to import
any modules that GDB or GDBserver needs.

Former approach looks better for me.  What do you think?

-- 
Yao (齐尧)

Re: [PATCH] Use sized types in tracepoint.

[Pedro Alves]

On 04/10/2012 02:59 PM, Yao Qi wrote:

> On 03/16/2012 11:57 PM, Pedro Alves wrote:
>> On 03/16/2012 03:50 PM, Mark Kettenis wrote:
>>
>>> We have gnulib/stdint.h so using typedefs like int32_t should be ok.
>>
>>
>> GDBserver doesn't use gnulib, unfortunately.  Thought, it looks like
>> the Linux, Windows and Lynx backends already unconditionally include stdint.h.
>> The NTO backend doesn't, so I'm not sure if we can include it unconditionally.
>>
> 
> Unless I miss something, I don't see NTO-backend use stdint.h.
> 
> $ grep stdint *.c *.h
> ax.c:     be to import stdint.h from gnulib.  */
> linux-i386-ipa.c:#include <stdint.h>
> lynx-i386-low.c:#include <stdint.h>
> lynx-ppc-low.c:#include <stdint.h>
> thread-db.c:#include <stdint.h>
> tracepoint.c:#include <stdint.h>
> win32-low.c:#include <stdint.h>
> 
> tracepoint.c is the only one file using stdint.h conditionally.


That's the point.  The NTO backend doesn't include stdint.h
unconditionally like the others, and the tracepoint.c inclusion
is guarded, so we don't know if including stdint.h unconditionally
works on NTO.  It most probably does.  Added Aleksandar to CC.

> 
>> On 03/16/2012 01:46 PM, Yao Qi wrote:
>>> +#include <inttypes.h>
>>>  #if HAVE_STDINT_H
>>        ^^^^^^^^^^^^^
>>>  #include <stdint.h>
>>>  #endif
>>
>> So I'd like to see that HAVE_STDINT_H removed first.  If this causes
>> trouble, the option would be for gdbserver to use gnulib proper.
>>
> 
> What kind of trouble you expect to see?  Unable to build?


I don't expect any real trouble, but yes, that'd be a possibility.

> I removed
> HAVE_STDINT_H, and successfully build gdbserver on linux, gdbserver on
> mingw32, and gdbserver on tic6x-uclinux.  Is it qualified as "cause no
> trouble"?


That was very much already expected, since in all those, stdint.h is
already included unconditionally.

> In parallel, I am writing a patch to use gnulib in gdbserver.  I am not
> sure how to place gnulib source tree.  We have to choices here,
>   - Import gnulib separately in gdbserver.  We have to gnulib source
> trees in gdb and gdbserver respectively, some duplication.  However, we
> gain some flexibility when gdb and gdbserver wants to import different
> modules of gnulib.
>   - Use gnulib in gdb.  Since gnulib is designed as a sub-dir of any
> project, if we don't import gnulib in gdbserver, we have to copy gnulib
> source to gdbserver source tree during build, and remove it when
> finished.  Since we have a single source of gnulib, we have to import
> any modules that GDB or GDBserver needs.
> 
> Former approach looks better for me.  What do you think?


I'd prefer to have only one copy of gnulib, until we find a real need
to have separate copies.  Is it really true that gnulib only works if
it is in a direct src subdir?

-- 
Pedro Alves

Re: [PATCH] Use sized types in tracepoint.

[Aleksandar Ristovski]

On 12-04-10 10:17 AM, Pedro Alves wrote:
> we don't know if including stdint.h unconditionally
> works on NTO.  It most probably does.  Added Aleksandar to CC.

Yes, it works for NTO.


Thanks,

Aleksandar

Re: [PATCH] Use sized types in tracepoint.

[Aleksandar Ristovski]

On 12-04-10 10:17 AM, Pedro Alves wrote:
> we don't know if including stdint.h unconditionally
> works on NTO.  It most probably does.  Added Aleksandar to CC.

Yes, it works for NTO.


Thanks,

Aleksandar

Re: [PATCH] Use sized types in tracepoint.

[Yao Qi]

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

On 04/10/2012 10:45 PM, Aleksandar Ristovski wrote:
> On 12-04-10 10:17 AM, Pedro Alves wrote:
>> we don't know if including stdint.h unconditionally
>> works on NTO.  It most probably does.  Added Aleksandar to CC.
> 
> Yes, it works for NTO.
> 

Here is a small patch to include stdint.h unconditionally in gdbserver.

-- 
Yao (齐尧)

[-- Attachment #2: 0009-Include-stdint.h-unconditionally.patch --]
[-- Type: text/x-patch, Size: 597 bytes --]

gdb/gdbserver:

2012-04-12  Yao Qi  <yao@codesourcery.com>

	* tracepoint.c: Include stdint.h unconditionally.
---
 gdb/gdbserver/tracepoint.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 2144f6f..f06e3c0 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -24,9 +24,8 @@
 #include <unistd.h>
 #include <sys/time.h>
 #include <stddef.h>
-#if HAVE_STDINT_H
 #include <stdint.h>
-#endif
+
 #include "ax.h"
 
 /* This file is built for both GDBserver, and the in-process
-- 
1.7.0.4

gdbserver: include stdint.h unconditionally.

[Pedro Alves]

On 04/12/2012 02:50 PM, Yao Qi wrote:
> On 04/10/2012 10:45 PM, Aleksandar Ristovski wrote:
>> > On 12-04-10 10:17 AM, Pedro Alves wrote:
>>> >> we don't know if including stdint.h unconditionally
>>> >> works on NTO.  It most probably does.  Added Aleksandar to CC.
>> >
>> > Yes, it works for NTO.
>> >
> Here is a small patch to include stdint.h unconditionally in gdbserver.

(The NTO discovery is now mostly irrelevant since gdbserver pulls
stdint.h from gnulib.)

> gdb/gdbserver:
> 
> 2012-04-12  Yao Qi  <yao@codesourcery.com>
> 
> 	* tracepoint.c: Include stdint.h unconditionally.


Okay, thanks.

-- 
Pedro Alves

Re: [PATCH] Use sized types in tracepoint.

[Tom Tromey]

Mark> We have gnulib/stdint.h so using typedefs like int32_t should be ok.
Mark> But I don't think we have support for the PRIxxx macros, so the
Mark> changes to the printing code aren't ok I'm afraid.

The gnulib inttypes.h module defines these.

Tom

Re: [PATCH] Use sized types in tracepoint.

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Fri, 16 Mar 2012 13:04:57 -0600
> 
> Mark> We have gnulib/stdint.h so using typedefs like int32_t should be ok.
> Mark> But I don't think we have support for the PRIxxx macros, so the
> Mark> changes to the printing code aren't ok I'm afraid.
> 
> The gnulib inttypes.h module defines these.

But unless I'm missing something, we don't have that module yet, so it
would have to be imported first.

Re: [PATCH] Use sized types in tracepoint.

[Tom Tromey]

>>>>> "Mark" == Mark Kettenis <mark.kettenis@xs4all.nl> writes:

Tom> The gnulib inttypes.h module defines these.

Mark> But unless I'm missing something, we don't have that module yet, so it
Mark> would have to be imported first.

Yes, and I suppose wired up to gdbserver.
Importing it is easy, wiring it up...

Tom