[PATCH] MI: new timing command

[PATCH] MI: new timing command

[Nick Roberts]

Here's a patch for timing MI commands e.g.

(gdb)
-enable-timings
^done
(gdb)
-break-insert main
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x080484ed",func="main",file="myprog.c",fullname="/home/nickrob/myprog.c",line="73",times="0"},time={wallclock="0.05185",user="0.00800",system="0.00000"}
(gdb)
-enable-timings no
^done
(gdb)
-exec-run
^running
(gdb)
*stopped,reason="breakpoint-hit",bkptno="1",thread-id="0",frame={addr="0x080484ed",func="main",args=[{name="argc",value="1"},{name="argv",value="0xbfb60364"}],file="myprog.c",fullname="/home/nickrob/myprog.c",line="73"}
(gdb)


It's based on Apple's code.  I also have a patch which works for asynchronous
operation but there's probably not much point in submitting that now.

I hope to submit further patches from Apple's code.  Notably:

1) -stack-list-locals --create-varobjs  (hopefully though Vladimir will
    do this one).

2) Event notification.

I thought I'd start with this one as it's straightforward.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2006-12-30  Nick Roberts  <nickrob@snap.net.nz>

	    Based on work by Apple Computer, Inc.
	
	* mi/mi-main.c: Include <sys/resource.h>.
	(current_command_ts, do_timings): New static variables.
	(timestamp, print_diff_now, print_diff, wallclock_diff)
	(user_diff, system_diff): New static timing functions.
	(mi_cmd_enable_timings): New function for new MI command.
	(captured_mi_execute_command, mi_execute_async_cli_command): 
	Call timing functions.

	* mi/mi-cmds.c (mi_cmds): Add entry for new MI command
	-enable-timings.

	* mi/mi-cmds.h (mi_cmd_enable_timings): New extern.

	* mi/mi-parse.h: Include <sys/resource.h>
	(mi_timestamp): New structure.
	(mi_parse): Add mi_timestamp* member.


Index: mi-cmds.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmds.c,v
retrieving revision 1.21
diff -c -p -r1.21 mi-cmds.c
*** mi-cmds.c	5 Jul 2006 19:03:47 -0000	1.21
--- mi-cmds.c	30 Dec 2006 08:42:26 -0000
*************** struct mi_cmd mi_cmds[] =
*** 58,63 ****
--- 58,64 ----
    { "display-enable", { NULL, 0 }, NULL, NULL },
    { "display-insert", { NULL, 0 }, NULL, NULL },
    { "display-list", { NULL, 0 }, NULL, NULL },
+   { "enable-timings", { NULL, 0 }, 0, mi_cmd_enable_timings},
    { "environment-cd", { NULL, 0 }, 0, mi_cmd_env_cd},
    { "environment-directory", { NULL, 0 }, 0, mi_cmd_env_dir},
    { "environment-path", { NULL, 0 }, 0, mi_cmd_env_path},
Index: mi-cmds.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmds.h,v
retrieving revision 1.19
diff -c -p -r1.19 mi-cmds.h
*** mi-cmds.h	23 Dec 2005 18:57:46 -0000	1.19
--- mi-cmds.h	30 Dec 2006 08:42:26 -0000
*************** extern mi_cmd_argv_ftype mi_cmd_data_lis
*** 72,77 ****
--- 72,78 ----
  extern mi_cmd_argv_ftype mi_cmd_data_read_memory;
  extern mi_cmd_argv_ftype mi_cmd_data_write_memory;
  extern mi_cmd_argv_ftype mi_cmd_data_write_register_values;
+ extern mi_cmd_argv_ftype mi_cmd_enable_timings;
  extern mi_cmd_argv_ftype mi_cmd_env_cd;
  extern mi_cmd_argv_ftype mi_cmd_env_dir;
  extern mi_cmd_argv_ftype mi_cmd_env_path;
Index: mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.86
diff -c -p -r1.86 mi-main.c
*** mi-main.c	17 Nov 2006 19:30:41 -0000	1.86
--- mi-main.c	30 Dec 2006 08:42:29 -0000
***************
*** 49,54 ****
--- 49,55 ----
  
  #include <ctype.h>
  #include <sys/time.h>
+ #include <sys/resource.h>
  
  enum
    {
*************** struct captured_mi_execute_command_args
*** 81,86 ****
--- 82,93 ----
  int mi_debug_p;
  struct ui_file *raw_stdout;
  
+ /* This is used to pass the current command timestamp
+    down to continuation routines. */
+ static struct mi_timestamp *current_command_ts;
+ 
+ static int do_timings = 0;
+ 
  /* The token of the last asynchronous command */
  static char *last_async_command;
  static char *previous_async_command;
*************** static int get_register (int regnum, int
*** 103,108 ****
--- 110,125 ----
     layer that calls libgdb.  Any operation used in the below should be
     formalized. */
  
+ static void timestamp (struct mi_timestamp *tv);
+ 
+ static void print_diff_now (struct mi_timestamp *start);
+ 
+ static void print_diff (struct mi_timestamp *start, struct mi_timestamp *end);
+ static long wallclock_diff (struct mi_timestamp *start,
+ 			    struct mi_timestamp *end);
+ static long user_diff (struct mi_timestamp *start, struct mi_timestamp *end);
+ static long system_diff (struct mi_timestamp *start, struct mi_timestamp *end);
+ 
  enum mi_cmd_result
  mi_cmd_gdb_exit (char *command, char **argv, int argc)
  {
*************** mi_cmd_exec_continue (char *args, int fr
*** 194,200 ****
  }
  
  /* Interrupt the execution of the target. Note how we must play around
!    with the token varialbes, in order to display the current token in
     the result of the interrupt command, and the previous execution
     token when the target finally stops. See comments in
     mi_cmd_execute. */
--- 211,217 ----
  }
  
  /* Interrupt the execution of the target. Note how we must play around
!    with the token variables, in order to display the current token in
     the result of the interrupt command, and the previous execution
     token when the target finally stops. See comments in
     mi_cmd_execute. */
*************** mi_cmd_data_write_memory (char *command,
*** 1024,1029 ****
--- 1041,1070 ----
    return MI_CMD_DONE;
  }
  
+ enum mi_cmd_result
+ mi_cmd_enable_timings (char *command, char **argv, int argc)
+ {
+   if (argc == 0)
+     do_timings = 1;
+   else if (argc == 1)
+     {
+       if (strcmp (argv[0], "yes") == 0)
+ 	do_timings = 1;
+       else if (strcmp (argv[0], "no") == 0)
+ 	do_timings = 0;
+       else
+ 	goto usage_error;
+     }
+   else
+     goto usage_error;
+     
+   return MI_CMD_DONE;
+ 
+  usage_error:
+   error ("mi_cmd_enable_timings: Usage: %s {yes|no}", command);
+   return MI_CMD_ERROR;
+ }
+ 
  /* Execute a command within a safe environment.
     Return <0 for error; >=0 for ok.
  
*************** captured_mi_execute_command (struct ui_o
*** 1038,1043 ****
--- 1079,1086 ----
      (struct captured_mi_execute_command_args *) data;
    struct mi_parse *context = args->command;
  
+   struct mi_timestamp cmd_finished;
+ 
    switch (context->op)
      {
  
*************** captured_mi_execute_command (struct ui_o
*** 1052,1059 ****
--- 1095,1109 ----
           indication of what action is required and then switch on
           that. */
        args->action = EXECUTE_COMMAND_DISPLAY_PROMPT;
+ 
+       if (do_timings)
+ 	current_command_ts = context->cmd_start;
+ 
        args->rc = mi_cmd_execute (context);
  
+       if (do_timings)
+           timestamp (&cmd_finished);
+ 
        if (!target_can_async_p () || !target_executing)
  	{
  	  /* print the result if there were no errors
*************** captured_mi_execute_command (struct ui_o
*** 1068,1073 ****
--- 1118,1127 ----
  	      fputs_unfiltered ("^done", raw_stdout);
  	      mi_out_put (uiout, raw_stdout);
  	      mi_out_rewind (uiout);
+ 	      /* Have to check cmd_start, since the command could be
+ 		 "mi-enable-timings". */
+ 	      if (do_timings && context->cmd_start)
+ 		  print_diff (context->cmd_start, &cmd_finished);
  	      fputs_unfiltered ("\n", raw_stdout);
  	    }
  	  else if (args->rc == MI_CMD_ERROR)
*************** mi_execute_command (char *cmd, int from_
*** 1163,1168 ****
--- 1217,1230 ----
    if (command != NULL)
      {
        struct gdb_exception result;
+ 
+       if (do_timings)
+ 	{
+ 	  command->cmd_start = (struct mi_timestamp *)
+ 	    xmalloc (sizeof (struct mi_timestamp));
+ 	  timestamp (command->cmd_start);
+ 	}
+ 
        /* FIXME: cagney/1999-11-04: Can this use of catch_exceptions either
           be pushed even further down or even eliminated? */
        args.command = command;
*************** mi_execute_async_cli_command (char *mi, 
*** 1350,1355 ****
--- 1412,1419 ----
        fputs_unfiltered ("*stopped", raw_stdout);
        mi_out_put (uiout, raw_stdout);
        mi_out_rewind (uiout);
+       if (do_timings)
+       	print_diff_now (current_command_ts);
        fputs_unfiltered ("\n", raw_stdout);
        return MI_CMD_QUIET;
      }
*************** _initialize_mi_main (void)
*** 1466,1468 ****
--- 1530,1581 ----
    DEPRECATED_REGISTER_GDBARCH_SWAP (old_regs);
    deprecated_register_gdbarch_swap (NULL, 0, mi_setup_architecture_data);
  }
+ 
+ static void 
+ timestamp (struct mi_timestamp *tv)
+   {
+     gettimeofday (&tv->wallclock, NULL);
+     getrusage (RUSAGE_SELF, &tv->rusage);
+   }
+ 
+ static void 
+ print_diff_now (struct mi_timestamp *start)
+   {
+     struct mi_timestamp now;
+     timestamp (&now);
+     print_diff (start, &now);
+   }
+ 
+ static void 
+ print_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+   {
+     fprintf_unfiltered
+       (raw_stdout,
+        ",time={wallclock=\"%0.5f\",user=\"%0.5f\",system=\"%0.5f\"}", 
+        wallclock_diff (start, end) / 1000000.0, 
+        user_diff (start, end) / 1000000.0, 
+        system_diff (start, end) / 1000000.0);
+   }
+ 
+ static long 
+ wallclock_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+   {
+     return ((end->wallclock.tv_sec - start->wallclock.tv_sec) * 1000000) +
+            (end->wallclock.tv_usec - start->wallclock.tv_usec);
+   }
+ 
+ static long 
+ user_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+   {
+     return 
+      ((end->rusage.ru_utime.tv_sec - start->rusage.ru_utime.tv_sec) * 1000000) +
+       (end->rusage.ru_utime.tv_usec - start->rusage.ru_utime.tv_usec);
+   }
+ 
+ static long 
+ system_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+   {
+     return 
+      ((end->rusage.ru_stime.tv_sec - start->rusage.ru_stime.tv_sec) * 1000000) +
+       (end->rusage.ru_stime.tv_usec - start->rusage.ru_stime.tv_usec);
+   }
Index: mi-parse.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.h,v
retrieving revision 1.5
diff -c -p -r1.5 mi-parse.h
*** mi-parse.h	23 Dec 2005 18:57:46 -0000	1.5
--- mi-parse.h	30 Dec 2006 08:42:29 -0000
***************
*** 24,29 ****
--- 24,37 ----
  
  /* MI parser */
  
+ #include <sys/resource.h>
+ 
+ /* Timestamps for current command and last asynchronous command */
+ struct mi_timestamp {
+     struct timeval wallclock;
+     struct rusage rusage;
+ };
+ 
  enum mi_command_type
    {
      MI_COMMAND, CLI_COMMAND
*************** struct mi_parse
*** 35,40 ****
--- 43,49 ----
      char *command;
      char *token;
      const struct mi_cmd *cmd;
+     struct mi_timestamp *cmd_start;
      char *args;
      char **argv;
      int argc;

Re: [PATCH] MI: new timing command

[Vladimir Prus]

Nick Roberts wrote:

> Here's a patch for timing MI commands e.g.
> 
> (gdb)
> -enable-timings
> ^done
> (gdb)
> -break-insert main
> ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x080484ed",func="main",file="myprog.c",
> fullname="/home/nickrob/myprog.c",line="73",times="0"},time={wallclock="0.05185",user="0.00800",system="0.00000"} 

Great. I think this is really important for optimizing performance of MI.

> It's based on Apple's code.  I also have a patch which works for asynchronous
> operation but there's probably not much point in submitting that now.
> 
> I hope to submit further patches from Apple's code.  Notably:
> 
> 1) -stack-list-locals --create-varobjs  (hopefully though Vladimir will
>     do this one).

It's rather close in my plans. 


> ===================================================================
> RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
> retrieving revision 1.86
> diff -c -p -r1.86 mi-main.c
> *** mi-main.c   17 Nov 2006 19:30:41 -0000      1.86
> --- mi-main.c   30 Dec 2006 08:42:29 -0000
> ***************
> *** 49,54 ****

Do you have a *strong* preference to context diff as opposed to unified?
If yes, I think I can try to cope with context diffs, but unified diffs
are much more readable.

> --- 49,55 ----
>   
>   #include <ctype.h>
>   #include <sys/time.h>
> + #include <sys/resource.h>
>   
>   enum
>     {
> *************** struct captured_mi_execute_command_args
> *** 81,86 ****
> --- 82,93 ----
>   int mi_debug_p;
>   struct ui_file *raw_stdout;
>   
> + /* This is used to pass the current command timestamp
> +    down to continuation routines. */

Two spaces after dot. No, I personally don't think this coding style
guideline makes any sense, but Dan will notice it anyway and you'll
have to change ;-)

More seriously, this comment only says what this variable is
used for, not what it is. For example, the comment might read:

        /* The timestamp of the moment when the current
            command started executing.  Used to ... */

Ah, and looking at the code this variable is used *only* so that
'run' and friend can output the timestamp despite the fact that
they don't emit '^done', so I think this is better
represented in the comment.

> + static struct mi_timestamp *current_command_ts;
> + 
> + static int do_timings = 0;
> + 
>   /* The token of the last asynchronous command */
>   static char *last_async_command;
>   static char *previous_async_command;

> *************** mi_cmd_data_write_memory (char *command,
> *** 1024,1029 ****
> --- 1041,1070 ----
>     return MI_CMD_DONE;
>   }
>   
> + enum mi_cmd_result
> + mi_cmd_enable_timings (char *command, char **argv, int argc)
> + {
> +   if (argc == 0)
> +     do_timings = 1;
> +   else if (argc == 1)
> +     {
> +       if (strcmp (argv[0], "yes") == 0)
> +       do_timings = 1;
> +       else if (strcmp (argv[0], "no") == 0)
> +       do_timings = 0;
> +       else
> +       goto usage_error;

Something looks wrong with indentation above.

> +     }
> +   else
> +     goto usage_error;
> +     
> +   return MI_CMD_DONE;
> + 
> +  usage_error:
> +   error ("mi_cmd_enable_timings: Usage: %s {yes|no}", command);
> +   return MI_CMD_ERROR;
> + }
> + 
>   /* Execute a command within a safe environment.
>      Return <0 for error; >=0 for ok.
>   
> *************** captured_mi_execute_command (struct ui_o
> *** 1038,1043 ****
> --- 1079,1086 ----
>       (struct captured_mi_execute_command_args *) data;
>     struct mi_parse *context = args->command;
>   
> +   struct mi_timestamp cmd_finished;
> + 
>     switch (context->op)
>       {
>   
> *************** captured_mi_execute_command (struct ui_o
> *** 1052,1059 ****
> --- 1095,1109 ----
>            indication of what action is required and then switch on
>            that. */
>         args->action = EXECUTE_COMMAND_DISPLAY_PROMPT;
> + 
> +       if (do_timings)
> +       current_command_ts = context->cmd_start;

I wonder if it's better, instead of having global current_command_ts,
add new global

        struct mi_parse* current_context;

set it here to context:

        current_context = context;

And the user 'current_context' later. That seems to be a more
future-proof solution -- if mi_execute_async_cli_command will
later need something more from mi_parse structure, we won't
need to add yet another global variable.

>         args->rc = mi_cmd_execute (context);
>   
> +       if (do_timings)
> +           timestamp (&cmd_finished);
> + 
>         if (!target_can_async_p () || !target_executing)
>         {
>           /* print the result if there were no errors
> *************** captured_mi_execute_command (struct ui_o
> *** 1068,1073 ****
> --- 1118,1127 ----
>               fputs_unfiltered ("^done", raw_stdout);
>               mi_out_put (uiout, raw_stdout);
>               mi_out_rewind (uiout);
> +             /* Have to check cmd_start, since the command could be
> +                "mi-enable-timings". */

Haven't you named the command 'enable-timings' without 'mi-'?

> +             if (do_timings && context->cmd_start)
> +                 print_diff (context->cmd_start, &cmd_finished);
>               fputs_unfiltered ("\n", raw_stdout);
>             }
>           else if (args->rc == MI_CMD_ERROR)
> *************** mi_execute_command (char *cmd, int from_
> *** 1163,1168 ****
> --- 1217,1230 ----
>     if (command != NULL)
>       {
>         struct gdb_exception result;
> + 
> +       if (do_timings)
> +       {
> +         command->cmd_start = (struct mi_timestamp *)
> +           xmalloc (sizeof (struct mi_timestamp));
> +         timestamp (command->cmd_start);
> +       }
> + 
>         /* FIXME: cagney/1999-11-04: Can this use of catch_exceptions either
>            be pushed even further down or even eliminated? */
>         args.command = command;
> *************** mi_execute_async_cli_command (char *mi, 
> *** 1350,1355 ****
> --- 1412,1419 ----
>         fputs_unfiltered ("*stopped", raw_stdout);
>         mi_out_put (uiout, raw_stdout);
>         mi_out_rewind (uiout);
> +       if (do_timings)
> +               print_diff_now (current_command_ts);
>         fputs_unfiltered ("\n", raw_stdout);
>         return MI_CMD_QUIET;
>       }
> *************** _initialize_mi_main (void)
> *** 1466,1468 ****
> --- 1530,1581 ----
>     DEPRECATED_REGISTER_GDBARCH_SWAP (old_regs);
>     deprecated_register_gdbarch_swap (NULL, 0, mi_setup_architecture_data);
>   }
> + 
> + static void 
> + timestamp (struct mi_timestamp *tv)
> +   {
> +     gettimeofday (&tv->wallclock, NULL);
> +     getrusage (RUSAGE_SELF, &tv->rusage);
> +   }
> + 
> + static void 
> + print_diff_now (struct mi_timestamp *start)
> +   {
> +     struct mi_timestamp now;
> +     timestamp (&now);
> +     print_diff (start, &now);
> +   }
> + 
> + static void 
> + print_diff (struct mi_timestamp *start, struct mi_timestamp *end)
> +   {
> +     fprintf_unfiltered
> +       (raw_stdout,
> +        ",time={wallclock=\"%0.5f\",user=\"%0.5f\",system=\"%0.5f\"}", 
> +        wallclock_diff (start, end) / 1000000.0, 
> +        user_diff (start, end) / 1000000.0, 
> +        system_diff (start, end) / 1000000.0);
> +   }
> + 
> + static long 
> + wallclock_diff (struct mi_timestamp *start, struct mi_timestamp *end)
> +   {
> +     return ((end->wallclock.tv_sec - start->wallclock.tv_sec) * 1000000) +
> +            (end->wallclock.tv_usec - start->wallclock.tv_usec);
> +   }
> + 
> + static long 
> + user_diff (struct mi_timestamp *start, struct mi_timestamp *end)
> +   {
> +     return 
> +      ((end->rusage.ru_utime.tv_sec - start->rusage.ru_utime.tv_sec) * 1000000) +
> +       (end->rusage.ru_utime.tv_usec - start->rusage.ru_utime.tv_usec);
> +   }
> + 
> + static long 
> + system_diff (struct mi_timestamp *start, struct mi_timestamp *end)
> +   {
> +     return 
> +      ((end->rusage.ru_stime.tv_sec - start->rusage.ru_stime.tv_sec) * 1000000) +
> +       (end->rusage.ru_stime.tv_usec - start->rusage.ru_stime.tv_usec);
> +   }

Perhaps the last three functions can be replaced with

static long
timeval_diff (struct timeval* start, start timeval *end)
{
        return (end->tv_sec - start->tv_sec) * 1000000 .....
}

That 'user_diff' seems rather low level on non-reusable.

> + /* Timestamps for current command and last asynchronous command */

Dot at the end of the sentence. The above sounds like this
structure hold two separate timestaps -- for current command
and the last async command. Maybe replacing "and" with "or" will help.

- Volodya

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Vladimir Prus <ghost@cs.msu.su>
> Date: Sat, 30 Dec 2006 15:08:15 +0300
> 
> Two spaces after dot. No, I personally don't think this coding style
> guideline makes any sense

It makes perfect sense if you use Emacs.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > > Two spaces after dot. No, I personally don't think this coding style
 > > guideline makes any sense
 > 
 > It makes perfect sense if you use Emacs.

I like seeing two spaces after each sentence in a paragraph as it makes them
stand out more.  What sense does it make for a single sentence?  Who uses
forward-sentence (M-e) in that case?


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sun, 31 Dec 2006 16:21:42 +1300
> Cc: Vladimir Prus <ghost@cs.msu.su>, gdb-patches@sources.redhat.com
> 
> What sense does it make for a single sentence?  Who uses
> forward-sentence (M-e) in that case?

If you want to quickly get to the end of the sentence, e.g. to add
another one, you might find M-e useful even if the comment has a
single sentence.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > Do you have a *strong* preference to context diff as opposed to unified?
 > If yes, I think I can try to cope with context diffs, but unified diffs
 > are much more readable.

I always use context diffs because that's what Richard Stallman insists on for
Emacs.  However, it's no big deal because if you save the patch as a file and
view it in Emacs in diff-mode (certain names like timings1.diff will
automatically open in this mode), you can toggle between context and unified
diffs from the menu bar.


 > > + /* This is used to pass the current command timestamp
 > > +    down to continuation routines. */
 > 
 > Two spaces after dot. No, I personally don't think this coding style
 > guideline makes any sense, but Dan will notice it anyway and you'll
 > have to change ;-)

OK.

 > More seriously, this comment only says what this variable is
 > used for, not what it is. For example, the comment might read:
 > 
 >         /* The timestamp of the moment when the current
 >             command started executing.  Used to ... */
 > 
 > Ah, and looking at the code this variable is used *only* so that
 > 'run' and friend can output the timestamp despite the fact that
 > they don't emit '^done', so I think this is better
 > represented in the comment.

In Apple's code its used for the asynchronous continuation functions. It's
of type mi_timestamp and called current_command_ts: I think it's better to
let the code speak for itself.

 >...
 > > +       if (strcmp (argv[0], "yes") == 0)
 > > +       do_timings = 1;
 > > +       else if (strcmp (argv[0], "no") == 0)
 > > +       do_timings = 0;
 > > +       else
 > > +       goto usage_error;
 > 
 > Something looks wrong with indentation above.

It's just the way tabs are handled by the diff.

 >...
 > > +       if (do_timings)
 > > +       current_command_ts = context->cmd_start;
 > 
 > I wonder if it's better, instead of having global current_command_ts,
 > add new global
 > 
 >         struct mi_parse* current_context;
 > 
 > set it here to context:
 > 
 >         current_context = context;
 > 
 > And the user 'current_context' later. That seems to be a more
 > future-proof solution -- if mi_execute_async_cli_command will
 > later need something more from mi_parse structure, we won't
 > need to add yet another global variable.

This is a good solution if mi_execute_async_cli_command does eventually need
something more from mi_parse structure later but I don't see why it's needed
now.  But perhaps you have something in mind?

 >...
 > >         args->rc = mi_cmd_execute (context);
 > >   
 > > +       if (do_timings)
 > > +           timestamp (&cmd_finished);
 > > + 
 > >         if (!target_can_async_p () || !target_executing)
 > >         {
 > >           /* print the result if there were no errors
 > > *************** captured_mi_execute_command (struct ui_o
 > > *** 1068,1073 ****
 > > --- 1118,1127 ----
 > >               fputs_unfiltered ("^done", raw_stdout);
 > >               mi_out_put (uiout, raw_stdout);
 > >               mi_out_rewind (uiout);
 > > +             /* Have to check cmd_start, since the command could be
 > > +                "mi-enable-timings". */
 > 
 > Haven't you named the command 'enable-timings' without 'mi-'?

Duh! Yes.  Apple call it -mi-enable-timings but the mi seems implicit to me.

 >...
 > > + static long 
 > > + wallclock_diff (struct mi_timestamp *start, struct mi_timestamp *end)
 > > +   {
 > > +     return ((end->wallclock.tv_sec - start->wallclock.tv_sec) * 1000000) +
 > > +            (end->wallclock.tv_usec - start->wallclock.tv_usec);
 > > +   }
 > > + 
 > > + static long 
 > > + user_diff (struct mi_timestamp *start, struct mi_timestamp *end)
 > > +   {
 > > +     return 
 > > +      ((end->rusage.ru_utime.tv_sec - start->rusage.ru_utime.tv_sec) * 1000000) +
 > > +       (end->rusage.ru_utime.tv_usec - start->rusage.ru_utime.tv_usec);
 > > +   }
 > > + 
 > > + static long 
 > > + system_diff (struct mi_timestamp *start, struct mi_timestamp *end)
 > > +   {
 > > +     return 
 > > +      ((end->rusage.ru_stime.tv_sec - start->rusage.ru_stime.tv_sec) * 1000000) +
 > > +       (end->rusage.ru_stime.tv_usec - start->rusage.ru_stime.tv_usec);
 > > +   }
 > 
 > Perhaps the last three functions can be replaced with
 > 
 > static long
 > timeval_diff (struct timeval* start, start timeval *end)
 > {
 >         return (end->tv_sec - start->tv_sec) * 1000000 .....
 > }
 > 
 > That 'user_diff' seems rather low level on non-reusable.
 > 
 > > + /* Timestamps for current command and last asynchronous command */

Sounds reasonable.  I'll do this in my next patch.

 > Dot at the end of the sentence. The above sounds like this
 > structure hold two separate timestaps -- for current command
 > and the last async command. Maybe replacing "and" with "or" will help.

There are *many* instances of one line comments in this file without a full
stop.  Perhaps the practice is just to give multi-line comments a full stop
(maybe one liners are regarded as phrases).

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Daniel Jacobowitz]

On Sun, Dec 31, 2006 at 07:50:21PM +1300, Nick Roberts wrote:
>  > Dot at the end of the sentence. The above sounds like this
>  > structure hold two separate timestaps -- for current command
>  > and the last async command. Maybe replacing "and" with "or" will help.
> 
> There are *many* instances of one line comments in this file without a full
> stop.  Perhaps the practice is just to give multi-line comments a full stop
> (maybe one liners are regarded as phrases).

The file does contain errors; that's not a good reason to add
additional errors.  The GDB and GNU style is to use the full stop.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > The file does contain errors; that's not a good reason to add
 > additional errors.  The GDB and GNU style is to use the full stop.

It's just a bit anal for a single line comment.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Daniel Jacobowitz]

On Mon, Jan 01, 2007 at 05:09:19PM +1300, Nick Roberts wrote:
>  > The file does contain errors; that's not a good reason to add
>  > additional errors.  The GDB and GNU style is to use the full stop.
> 
> It's just a bit anal for a single line comment.

I don't even understand why we're having this conversation.  Please
follow the coding standard unless you have a good justification not to;
that's why we call it the standard.  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > I don't even understand why we're having this conversation.  Please
 > follow the coding standard unless you have a good justification not to;
 > that's why we call it the standard.  Thanks.

Sure, I didn't say that I wouldn't do it.  I'm just waiting for Eli to tell
me if it works with get_run_time.  Meanwhile here's a revised version of
mi-main.c.  I've even made line breaks before, instead of after, operators!

-- 
Nick                                           http://www.inet.net.nz/~nickrob


*** mi-main.c	02 Jan 2007 10:53:22 +1300	1.87
--- mi-main.c	04 Jan 2007 10:40:53 +1300	
***************
*** 22,28 ****
     Foundation, Inc., 51 Franklin Street, Fifth Floor,
     Boston, MA 02110-1301, USA.  */
  
! /* Work in progress */
  
  #include "defs.h"
  #include "target.h"
--- 22,28 ----
     Foundation, Inc., 51 Franklin Street, Fifth Floor,
     Boston, MA 02110-1301, USA.  */
  
! /* Work in progress.  */
  
  #include "defs.h"
  #include "target.h"
***************
*** 40,47 ****
  #include "interps.h"
  #include "event-loop.h"
  #include "event-top.h"
! #include "gdbcore.h"		/* for write_memory() */
! #include "value.h"		/* for deprecated_write_register_bytes() */
  #include "regcache.h"
  #include "gdb.h"
  #include "frame.h"
--- 40,47 ----
  #include "interps.h"
  #include "event-loop.h"
  #include "event-top.h"
! #include "gdbcore.h"		/* For write_memory().  */
! #include "value.h"		/* For deprecated_write_register_bytes(). */
  #include "regcache.h"
  #include "gdb.h"
  #include "frame.h"
***************
*** 50,62 ****
  #include <ctype.h>
  #include <sys/time.h>
  
  enum
    {
      FROM_TTY = 0
    };
  
  /* Enumerations of the actions that may result from calling
!    captured_mi_execute_command */
  
  enum captured_mi_execute_command_actions
    {
--- 50,66 ----
  #include <ctype.h>
  #include <sys/time.h>
  
+ #if defined HAVE_SYS_RESOURCE_H
+ #include <sys/resource.h>
+ #endif
+ 
  enum
    {
      FROM_TTY = 0
    };
  
  /* Enumerations of the actions that may result from calling
!    captured_mi_execute_command.  */
  
  enum captured_mi_execute_command_actions
    {
*************** enum captured_mi_execute_command_actions
*** 65,87 ****
    };
  
  /* This structure is used to pass information from captured_mi_execute_command
!    to mi_execute_command. */
  struct captured_mi_execute_command_args
  {
!   /* This return result of the MI command (output) */
    enum mi_cmd_result rc;
  
!   /* What action to perform when the call is finished (output) */
    enum captured_mi_execute_command_actions action;
  
!   /* The command context to be executed (input) */
    struct mi_parse *command;
  };
  
  int mi_debug_p;
  struct ui_file *raw_stdout;
  
! /* The token of the last asynchronous command */
  static char *last_async_command;
  static char *previous_async_command;
  char *mi_error_message;
--- 69,97 ----
    };
  
  /* This structure is used to pass information from captured_mi_execute_command
!    to mi_execute_command.  */
  struct captured_mi_execute_command_args
  {
!   /* This return result of the MI command (output).  */
    enum mi_cmd_result rc;
  
!   /* What action to perform when the call is finished (output).  */
    enum captured_mi_execute_command_actions action;
  
!   /* The command context to be executed (input).  */
    struct mi_parse *command;
  };
  
  int mi_debug_p;
  struct ui_file *raw_stdout;
  
! /* This is used to pass the current command timestamp
!    down to continuation routines.  */
! static struct mi_timestamp *current_command_ts;
! 
! static int do_timings = 0;
! 
! /* The token of the last asynchronous command.  */
  static char *last_async_command;
  static char *previous_async_command;
  char *mi_error_message;
*************** static void mi_exec_async_cli_cmd_contin
*** 99,117 ****
  static int register_changed_p (int regnum);
  static int get_register (int regnum, int format);
  
! /* Command implementations. FIXME: Is this libgdb? No.  This is the MI
     layer that calls libgdb.  Any operation used in the below should be
!    formalized. */
  
  enum mi_cmd_result
  mi_cmd_gdb_exit (char *command, char **argv, int argc)
  {
!   /* We have to print everything right here because we never return */
    if (last_async_command)
      fputs_unfiltered (last_async_command, raw_stdout);
    fputs_unfiltered ("^exit\n", raw_stdout);
    mi_out_put (uiout, raw_stdout);
!   /* FIXME: The function called is not yet a formal libgdb function */
    quit_force (NULL, FROM_TTY);
    return MI_CMD_DONE;
  }
--- 109,132 ----
  static int register_changed_p (int regnum);
  static int get_register (int regnum, int format);
  
! /* Command implementations.  FIXME: Is this libgdb? No.  This is the MI
     layer that calls libgdb.  Any operation used in the below should be
!    formalized.  */
! 
! static void timestamp (struct mi_timestamp *tv);
! 
! static void print_diff_now (struct mi_timestamp *start);
! static void print_diff (struct mi_timestamp *start, struct mi_timestamp *end);
  
  enum mi_cmd_result
  mi_cmd_gdb_exit (char *command, char **argv, int argc)
  {
!   /* We have to print everything right here because we never return.  */
    if (last_async_command)
      fputs_unfiltered (last_async_command, raw_stdout);
    fputs_unfiltered ("^exit\n", raw_stdout);
    mi_out_put (uiout, raw_stdout);
!   /* FIXME: The function called is not yet a formal libgdb function.  */
    quit_force (NULL, FROM_TTY);
    return MI_CMD_DONE;
  }
*************** mi_cmd_gdb_exit (char *command, char **a
*** 119,167 ****
  enum mi_cmd_result
  mi_cmd_exec_run (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper */
    return mi_execute_async_cli_command ("run", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_next (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper */
    return mi_execute_async_cli_command ("next", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_next_instruction (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper */
    return mi_execute_async_cli_command ("nexti", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_step (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper */
    return mi_execute_async_cli_command ("step", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_step_instruction (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper */
    return mi_execute_async_cli_command ("stepi", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_finish (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper */
    return mi_execute_async_cli_command ("finish", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_until (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper */
    return mi_execute_async_cli_command ("until", args, from_tty);
  }
  
--- 134,182 ----
  enum mi_cmd_result
  mi_cmd_exec_run (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper.  */
    return mi_execute_async_cli_command ("run", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_next (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper.  */
    return mi_execute_async_cli_command ("next", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_next_instruction (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper.  */
    return mi_execute_async_cli_command ("nexti", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_step (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper.  */
    return mi_execute_async_cli_command ("step", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_step_instruction (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper.  */
    return mi_execute_async_cli_command ("stepi", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_finish (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper.  */
    return mi_execute_async_cli_command ("finish", args, from_tty);
  }
  
  enum mi_cmd_result
  mi_cmd_exec_until (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper.  */
    return mi_execute_async_cli_command ("until", args, from_tty);
  }
  
*************** mi_cmd_exec_return (char *args, int from
*** 172,186 ****
       specified number of frames. */
    if (*args)
      /* Call return_command with from_tty argument equal to 0 so as to
!        avoid being queried. */
      return_command (args, 0);
    else
      /* Call return_command with from_tty argument equal to 0 so as to
!        avoid being queried. */
      return_command (NULL, 0);
  
    /* Because we have called return_command with from_tty = 0, we need
!      to print the frame here. */
    print_stack_frame (get_selected_frame (NULL), 1, LOC_AND_ADDRESS);
  
    return MI_CMD_DONE;
--- 187,201 ----
       specified number of frames. */
    if (*args)
      /* Call return_command with from_tty argument equal to 0 so as to
!        avoid being queried.  */
      return_command (args, 0);
    else
      /* Call return_command with from_tty argument equal to 0 so as to
!        avoid being queried.  */
      return_command (NULL, 0);
  
    /* Because we have called return_command with from_tty = 0, we need
!      to print the frame here.  */
    print_stack_frame (get_selected_frame (NULL), 1, LOC_AND_ADDRESS);
  
    return MI_CMD_DONE;
*************** mi_cmd_exec_return (char *args, int from
*** 189,203 ****
  enum mi_cmd_result
  mi_cmd_exec_continue (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper */
    return mi_execute_async_cli_command ("continue", args, from_tty);
  }
  
! /* Interrupt the execution of the target. Note how we must play around
!    with the token varialbes, in order to display the current token in
     the result of the interrupt command, and the previous execution
!    token when the target finally stops. See comments in
!    mi_cmd_execute. */
  enum mi_cmd_result
  mi_cmd_exec_interrupt (char *args, int from_tty)
  {
--- 204,218 ----
  enum mi_cmd_result
  mi_cmd_exec_continue (char *args, int from_tty)
  {
!   /* FIXME: Should call a libgdb function, not a cli wrapper.  */
    return mi_execute_async_cli_command ("continue", args, from_tty);
  }
  
! /* Interrupt the execution of the target.  Note how we must play around
!    with the token variables, in order to display the current token in
     the result of the interrupt command, and the previous execution
!    token when the target finally stops.  See comments in
!    mi_cmd_execute.  */
  enum mi_cmd_result
  mi_cmd_exec_interrupt (char *args, int from_tty)
  {
*************** mi_cmd_thread_select (char *command, cha
*** 235,241 ****
      rc = gdb_thread_select (uiout, argv[0], &mi_error_message);
  
    /* RC is enum gdb_rc if it is successful (>=0)
!      enum return_reason if not (<0). */
    if ((int) rc < 0 && (enum return_reason) rc == RETURN_ERROR)
      return MI_CMD_ERROR;
    else if ((int) rc >= 0 && rc == GDB_RC_FAIL)
--- 250,256 ----
      rc = gdb_thread_select (uiout, argv[0], &mi_error_message);
  
    /* RC is enum gdb_rc if it is successful (>=0)
!      enum return_reason if not (<0).  */
    if ((int) rc < 0 && (enum return_reason) rc == RETURN_ERROR)
      return MI_CMD_ERROR;
    else if ((int) rc >= 0 && rc == GDB_RC_FAIL)
*************** mi_cmd_data_list_register_names (char *c
*** 280,286 ****
  
    cleanup = make_cleanup_ui_out_list_begin_end (uiout, "register-names");
  
!   if (argc == 0)		/* No args, just do all the regs */
      {
        for (regnum = 0;
  	   regnum < numregs;
--- 295,301 ----
  
    cleanup = make_cleanup_ui_out_list_begin_end (uiout, "register-names");
  
!   if (argc == 0)		/* No args, just do all the regs.  */
      {
        for (regnum = 0;
  	   regnum < numregs;
*************** mi_cmd_data_list_register_names (char *c
*** 294,300 ****
  	}
      }
  
!   /* Else, list of register #s, just do listed regs */
    for (i = 0; i < argc; i++)
      {
        regnum = atoi (argv[i]);
--- 309,315 ----
  	}
      }
  
!   /* Else, list of register #s, just do listed regs.  */
    for (i = 0; i < argc; i++)
      {
        regnum = atoi (argv[i]);
*************** mi_cmd_data_list_changed_registers (char
*** 331,337 ****
  
    cleanup = make_cleanup_ui_out_list_begin_end (uiout, "changed-registers");
  
!   if (argc == 0)		/* No args, just do all the regs */
      {
        for (regnum = 0;
  	   regnum < numregs;
--- 346,352 ----
  
    cleanup = make_cleanup_ui_out_list_begin_end (uiout, "changed-registers");
  
!   if (argc == 0)		/* No args, just do all the regs.  */
      {
        for (regnum = 0;
  	   regnum < numregs;
*************** mi_cmd_data_list_changed_registers (char
*** 352,358 ****
  	}
      }
  
!   /* Else, list of register #s, just do listed regs */
    for (i = 0; i < argc; i++)
      {
        regnum = atoi (argv[i]);
--- 367,373 ----
  	}
      }
  
!   /* Else, list of register #s, just do listed regs.  */
    for (i = 0; i < argc; i++)
      {
        regnum = atoi (argv[i]);
*************** register_changed_p (int regnum)
*** 395,401 ****
  	      register_size (current_gdbarch, regnum)) == 0)
      return 0;
  
!   /* Found a changed register. Return 1. */
  
    memcpy (&old_regs[DEPRECATED_REGISTER_BYTE (regnum)], raw_buffer,
  	  register_size (current_gdbarch, regnum));
--- 410,416 ----
  	      register_size (current_gdbarch, regnum)) == 0)
      return 0;
  
!   /* Found a changed register.  Return 1.  */
  
    memcpy (&old_regs[DEPRECATED_REGISTER_BYTE (regnum)], raw_buffer,
  	  register_size (current_gdbarch, regnum));
*************** register_changed_p (int regnum)
*** 403,415 ****
    return 1;
  }
  
! /* Return a list of register number and value pairs. The valid
     arguments expected are: a letter indicating the format in which to
!    display the registers contents. This can be one of: x (hexadecimal), d
     (decimal), N (natural), t (binary), o (octal), r (raw).  After the
     format argumetn there can be a sequence of numbers, indicating which
!    registers to fetch the content of. If the format is the only argument,
!    a list of all the registers with their values is returned. */
  enum mi_cmd_result
  mi_cmd_data_list_register_values (char *command, char **argv, int argc)
  {
--- 418,430 ----
    return 1;
  }
  
! /* Return a list of register number and value pairs.  The valid
     arguments expected are: a letter indicating the format in which to
!    display the registers contents.  This can be one of: x (hexadecimal), d
     (decimal), N (natural), t (binary), o (octal), r (raw).  After the
     format argumetn there can be a sequence of numbers, indicating which
!    registers to fetch the content of.  If the format is the only argument,
!    a list of all the registers with their values is returned.  */
  enum mi_cmd_result
  mi_cmd_data_list_register_values (char *command, char **argv, int argc)
  {
*************** mi_cmd_data_list_register_values (char *
*** 435,441 ****
  
    list_cleanup = make_cleanup_ui_out_list_begin_end (uiout, "register-values");
  
!   if (argc == 1)		/* No args, beside the format: do all the regs */
      {
        for (regnum = 0;
  	   regnum < numregs;
--- 450,456 ----
  
    list_cleanup = make_cleanup_ui_out_list_begin_end (uiout, "register-values");
  
!   if (argc == 1)	    /* No args, beside the format: do all the regs.  */
      {
        for (regnum = 0;
  	   regnum < numregs;
*************** mi_cmd_data_list_register_values (char *
*** 456,462 ****
  	}
      }
  
!   /* Else, list of register #s, just do listed regs */
    for (i = 1; i < argc; i++)
      {
        regnum = atoi (argv[i]);
--- 471,477 ----
  	}
      }
  
!   /* Else, list of register #s, just do listed regs.  */
    for (i = 1; i < argc; i++)
      {
        regnum = atoi (argv[i]);
*************** mi_cmd_data_list_register_values (char *
*** 487,493 ****
    return MI_CMD_DONE;
  }
  
! /* Output one register's contents in the desired format. */
  static int
  get_register (int regnum, int format)
  {
--- 502,508 ----
    return MI_CMD_DONE;
  }
  
! /* Output one register's contents in the desired format.  */
  static int
  get_register (int regnum, int format)
  {
*************** get_register (int regnum, int format)
*** 539,546 ****
    return 1;
  }
  
! /* Write given values into registers. The registers and values are
!    given as pairs. The corresponding MI command is 
     -data-write-register-values <format> [<regnum1> <value1>...<regnumN> <valueN>]*/
  enum mi_cmd_result
  mi_cmd_data_write_register_values (char *command, char **argv, int argc)
--- 554,561 ----
    return 1;
  }
  
! /* Write given values into registers. T he registers and values are
!    given as pairs.  The corresponding MI command is 
     -data-write-register-values <format> [<regnum1> <value1>...<regnumN> <valueN>]*/
  enum mi_cmd_result
  mi_cmd_data_write_register_values (char *command, char **argv, int argc)
*************** mi_cmd_data_write_register_values (char 
*** 564,575 ****
  
    format = (int) argv[0][0];
  
-   if (!target_has_registers)
-     {
-       mi_error_message = xstrprintf ("mi_cmd_data_write_register_values: No registers.");
-       return MI_CMD_ERROR;
-     }
- 
    if (!(argc - 1))
      {
        mi_error_message = xstrprintf ("mi_cmd_data_write_register_values: No regs and values specified.");
--- 579,584 ----
*************** mi_cmd_data_write_register_values (char 
*** 594,600 ****
  	  /* Get the value as a number.  */
  	  value = parse_and_eval_address (argv[i + 1]);
  
! 	  /* Write it down */
  	  regcache_cooked_write_signed (current_regcache, regnum, value);
  	}
        else
--- 603,609 ----
  	  /* Get the value as a number.  */
  	  value = parse_and_eval_address (argv[i + 1]);
  
! 	  /* Write it down.  */
  	  regcache_cooked_write_signed (current_regcache, regnum, value);
  	}
        else
*************** mi_cmd_data_write_register_values (char 
*** 607,618 ****
  }
  
  #if 0
! /*This is commented out because we decided it was not useful. I leave
!    it, just in case. ezannoni:1999-12-08 */
  
! /* Assign a value to a variable. The expression argument must be in
!    the form A=2 or "A = 2" (I.e. if there are spaces it needs to be
!    quoted. */
  enum mi_cmd_result
  mi_cmd_data_assign (char *command, char **argv, int argc)
  {
--- 616,627 ----
  }
  
  #if 0
! /* This is commented out because we decided it was not useful.  I leave
!    it, just in case.  ezannoni:1999-12-08 */
  
! /* Assign a value to a variable.  The expression argument must be in
!    the form A=2 or "A = 2" i.e. if there are spaces it needs to be
!    quoted.  */
  enum mi_cmd_result
  mi_cmd_data_assign (char *command, char **argv, int argc)
  {
*************** mi_cmd_data_assign (char *command, char 
*** 625,632 ****
        return MI_CMD_ERROR;
      }
  
!   /* NOTE what follows is a clone of set_command(). FIXME: ezannoni
!      01-12-1999: Need to decide what to do with this for libgdb purposes. */
  
    expr = parse_expression (argv[0]);
    old_chain = make_cleanup (free_current_contents, &expr);
--- 634,641 ----
        return MI_CMD_ERROR;
      }
  
!   /* NOTE what follows is a clone of set_command().  FIXME: ezannoni
!      01-12-1999: Need to decide what to do with this for libgdb purposes.  */
  
    expr = parse_expression (argv[0]);
    old_chain = make_cleanup (free_current_contents, &expr);
*************** mi_cmd_data_assign (char *command, char 
*** 636,644 ****
  }
  #endif
  
! /* Evaluate the value of the argument. The argument is an
     expression. If the expression contains spaces it needs to be
!    included in double quotes. */
  enum mi_cmd_result
  mi_cmd_data_evaluate_expression (char *command, char **argv, int argc)
  {
--- 645,653 ----
  }
  #endif
  
! /* Evaluate the value of the argument.  The argument is an
     expression. If the expression contains spaces it needs to be
!    included in double quotes.  */
  enum mi_cmd_result
  mi_cmd_data_evaluate_expression (char *command, char **argv, int argc)
  {
*************** mi_cmd_data_evaluate_expression (char *c
*** 661,667 ****
  
    val = evaluate_expression (expr);
  
!   /* Print the result of the expression evaluation. */
    val_print (value_type (val), value_contents (val),
  	     value_embedded_offset (val), VALUE_ADDRESS (val),
  	     stb->stream, 0, 0, 0, 0);
--- 670,676 ----
  
    val = evaluate_expression (expr);
  
!   /* Print the result of the expression evaluation.  */
    val_print (value_type (val), value_contents (val),
  	     value_embedded_offset (val), VALUE_ADDRESS (val),
  	     stb->stream, 0, 0, 0, 0);
*************** mi_cmd_target_download (char *args, int 
*** 688,694 ****
    return MI_CMD_DONE;
  }
  
! /* Connect to the remote target. */
  enum mi_cmd_result
  mi_cmd_target_select (char *args, int from_tty)
  {
--- 697,703 ----
    return MI_CMD_DONE;
  }
  
! /* Connect to the remote target.  */
  enum mi_cmd_result
  mi_cmd_target_select (char *args, int from_tty)
  {
*************** mi_cmd_target_select (char *args, int fr
*** 698,713 ****
    run = xstrprintf ("target %s", args);
    old_cleanups = make_cleanup (xfree, run);
  
!   /* target-select is always synchronous.  once the call has returned
!      we know that we are connected. */
    /* NOTE: At present all targets that are connected are also
       (implicitly) talking to a halted target.  In the future this may
!      change. */
    execute_command (run, from_tty);
  
    do_cleanups (old_cleanups);
  
!   /* Issue the completion message here. */
    if (last_async_command)
      fputs_unfiltered (last_async_command, raw_stdout);
    fputs_unfiltered ("^connected", raw_stdout);
--- 707,722 ----
    run = xstrprintf ("target %s", args);
    old_cleanups = make_cleanup (xfree, run);
  
!   /* target-select is always synchronous.  Once the call has returned
!      we know that we are connected.  */
    /* NOTE: At present all targets that are connected are also
       (implicitly) talking to a halted target.  In the future this may
!      change.  */
    execute_command (run, from_tty);
  
    do_cleanups (old_cleanups);
  
!   /* Issue the completion message here.  */
    if (last_async_command)
      fputs_unfiltered (last_async_command, raw_stdout);
    fputs_unfiltered ("^connected", raw_stdout);
*************** mi_cmd_target_select (char *args, int fr
*** 721,729 ****
  /* DATA-MEMORY-READ:
  
     ADDR: start address of data to be dumped.
!    WORD-FORMAT: a char indicating format for the ``word''. See 
     the ``x'' command.
!    WORD-SIZE: size of each ``word''; 1,2,4, or 8 bytes
     NR_ROW: Number of rows.
     NR_COL: The number of colums (words per row).
     ASCHAR: (OPTIONAL) Append an ascii character dump to each row.  Use
--- 730,738 ----
  /* DATA-MEMORY-READ:
  
     ADDR: start address of data to be dumped.
!    WORD-FORMAT: a char indicating format for the ``word''.  See 
     the ``x'' command.
!    WORD-SIZE: size of each ``word''; 1,2,4, or 8 bytes.
     NR_ROW: Number of rows.
     NR_COL: The number of colums (words per row).
     ASCHAR: (OPTIONAL) Append an ascii character dump to each row.  Use
*************** mi_cmd_data_read_memory (char *command, 
*** 789,800 ****
  
    /* Extract all the arguments. */
  
!   /* Start address of the memory dump. */
    addr = parse_and_eval_address (argv[0]) + offset;
!   /* The format character to use when displaying a memory word. See
       the ``x'' command. */
    word_format = argv[1][0];
!   /* The size of the memory word. */
    word_size = atol (argv[2]);
    switch (word_size)
      {
--- 798,809 ----
  
    /* Extract all the arguments. */
  
!   /* Start address of the memory dump.  */
    addr = parse_and_eval_address (argv[0]) + offset;
!   /* The format character to use when displaying a memory word.  See
       the ``x'' command. */
    word_format = argv[1][0];
!   /* The size of the memory word.  */
    word_size = atol (argv[2]);
    switch (word_size)
      {
*************** mi_cmd_data_read_memory (char *command, 
*** 818,844 ****
        word_type = builtin_type_int8;
        word_asize = 'b';
      }
!   /* The number of rows */
    nr_rows = atol (argv[3]);
    if (nr_rows <= 0)
      {
        mi_error_message = xstrprintf ("mi_cmd_data_read_memory: invalid number of rows.");
        return MI_CMD_ERROR;
      }
!   /* number of bytes per row. */
    nr_cols = atol (argv[4]);
    if (nr_cols <= 0)
      {
        mi_error_message = xstrprintf ("mi_cmd_data_read_memory: invalid number of columns.");
        return MI_CMD_ERROR;
      }
!   /* The un-printable character when printing ascii. */
    if (argc == 6)
      aschar = *argv[5];
    else
      aschar = 0;
  
!   /* create a buffer and read it in. */
    total_bytes = word_size * nr_rows * nr_cols;
    mbuf = xcalloc (total_bytes, 1);
    make_cleanup (xfree, mbuf);
--- 827,853 ----
        word_type = builtin_type_int8;
        word_asize = 'b';
      }
!   /* The number of rows.  */
    nr_rows = atol (argv[3]);
    if (nr_rows <= 0)
      {
        mi_error_message = xstrprintf ("mi_cmd_data_read_memory: invalid number of rows.");
        return MI_CMD_ERROR;
      }
!   /* Number of bytes per row.  */
    nr_cols = atol (argv[4]);
    if (nr_cols <= 0)
      {
        mi_error_message = xstrprintf ("mi_cmd_data_read_memory: invalid number of columns.");
        return MI_CMD_ERROR;
      }
!   /* The un-printable character when printing ascii.  */
    if (argc == 6)
      aschar = *argv[5];
    else
      aschar = 0;
  
!   /* Create a buffer and read it in.  */
    total_bytes = word_size * nr_rows * nr_cols;
    mbuf = xcalloc (total_bytes, 1);
    make_cleanup (xfree, mbuf);
*************** mi_cmd_data_read_memory (char *command, 
*** 852,858 ****
        return MI_CMD_ERROR;
      }
  
!   /* output the header information. */
    ui_out_field_core_addr (uiout, "addr", addr);
    ui_out_field_int (uiout, "nr-bytes", nr_bytes);
    ui_out_field_int (uiout, "total-bytes", total_bytes);
--- 861,867 ----
        return MI_CMD_ERROR;
      }
  
!   /* Output the header information.  */
    ui_out_field_core_addr (uiout, "addr", addr);
    ui_out_field_int (uiout, "nr-bytes", nr_bytes);
    ui_out_field_int (uiout, "total-bytes", total_bytes);
*************** mi_cmd_data_read_memory (char *command, 
*** 861,867 ****
    ui_out_field_core_addr (uiout, "next-page", addr + total_bytes);
    ui_out_field_core_addr (uiout, "prev-page", addr - total_bytes);
  
!   /* Build the result as a two dimentional table. */
    {
      struct ui_stream *stream = ui_out_stream_new (uiout);
      struct cleanup *cleanup_list_memory;
--- 870,876 ----
    ui_out_field_core_addr (uiout, "next-page", addr + total_bytes);
    ui_out_field_core_addr (uiout, "prev-page", addr - total_bytes);
  
!   /* Build the result as a two dimentional table.  */
    {
      struct ui_stream *stream = ui_out_stream_new (uiout);
      struct cleanup *cleanup_list_memory;
*************** mi_cmd_data_read_memory (char *command, 
*** 931,946 ****
     offset from the beginning of the memory grid row where the cell to
     be written is.
     ADDR: start address of the row in the memory grid where the memory
!    cell is, if OFFSET_COLUMN is specified. Otherwise, the address of
     the location to write to.
!    FORMAT: a char indicating format for the ``word''. See 
     the ``x'' command.
     WORD_SIZE: size of each ``word''; 1,2,4, or 8 bytes
     VALUE: value to be written into the memory address.
  
     Writes VALUE into ADDR + (COLUMN_OFFSET * WORD_SIZE).
  
!    Prints nothing. */
  enum mi_cmd_result
  mi_cmd_data_write_memory (char *command, char **argv, int argc)
  {
--- 940,955 ----
     offset from the beginning of the memory grid row where the cell to
     be written is.
     ADDR: start address of the row in the memory grid where the memory
!    cell is, if OFFSET_COLUMN is specified.  Otherwise, the address of
     the location to write to.
!    FORMAT: a char indicating format for the ``word''.  See 
     the ``x'' command.
     WORD_SIZE: size of each ``word''; 1,2,4, or 8 bytes
     VALUE: value to be written into the memory address.
  
     Writes VALUE into ADDR + (COLUMN_OFFSET * WORD_SIZE).
  
!    Prints nothing.  */
  enum mi_cmd_result
  mi_cmd_data_write_memory (char *command, char **argv, int argc)
  {
*************** mi_cmd_data_write_memory (char *command,
*** 948,954 ****
    char word_format;
    long word_size;
    /* FIXME: ezannoni 2000-02-17 LONGEST could possibly not be big
!      enough when using a compiler other than GCC. */
    LONGEST value;
    void *buffer;
    struct cleanup *old_chain;
--- 957,963 ----
    char word_format;
    long word_size;
    /* FIXME: ezannoni 2000-02-17 LONGEST could possibly not be big
!      enough when using a compiler other than GCC.  */
    LONGEST value;
    void *buffer;
    struct cleanup *old_chain;
*************** mi_cmd_data_write_memory (char *command,
*** 987,1011 ****
        return MI_CMD_ERROR;
      }
  
!   /* Extract all the arguments. */
!   /* Start address of the memory dump. */
    addr = parse_and_eval_address (argv[0]);
!   /* The format character to use when displaying a memory word. See
!      the ``x'' command. */
    word_format = argv[1][0];
    /* The size of the memory word. */
    word_size = atol (argv[2]);
  
!   /* Calculate the real address of the write destination. */
    addr += (offset * word_size);
  
!   /* Get the value as a number */
    value = parse_and_eval_address (argv[3]);
!   /* Get the value into an array */
    buffer = xmalloc (word_size);
    old_chain = make_cleanup (xfree, buffer);
    store_signed_integer (buffer, word_size, value);
!   /* Write it down to memory */
    write_memory (addr, buffer, word_size);
    /* Free the buffer.  */
    do_cleanups (old_chain);
--- 996,1020 ----
        return MI_CMD_ERROR;
      }
  
!   /* Extract all the arguments.  */
!   /* Start address of the memory dump.  */
    addr = parse_and_eval_address (argv[0]);
!   /* The format character to use when displaying a memory word.  See
!      the ``x'' command.  */
    word_format = argv[1][0];
    /* The size of the memory word. */
    word_size = atol (argv[2]);
  
!   /* Calculate the real address of the write destination.  */
    addr += (offset * word_size);
  
!   /* Get the value as a number.  */
    value = parse_and_eval_address (argv[3]);
!   /* Get the value into an array.  */
    buffer = xmalloc (word_size);
    old_chain = make_cleanup (xfree, buffer);
    store_signed_integer (buffer, word_size, value);
!   /* Write it down to memory.  */
    write_memory (addr, buffer, word_size);
    /* Free the buffer.  */
    do_cleanups (old_chain);
*************** mi_cmd_data_write_memory (char *command,
*** 1013,1018 ****
--- 1022,1051 ----
    return MI_CMD_DONE;
  }
  
+ enum mi_cmd_result
+ mi_cmd_enable_timings (char *command, char **argv, int argc)
+ {
+   if (argc == 0)
+     do_timings = 1;
+   else if (argc == 1)
+     {
+       if (strcmp (argv[0], "yes") == 0)
+ 	do_timings = 1;
+       else if (strcmp (argv[0], "no") == 0)
+ 	do_timings = 0;
+       else
+ 	goto usage_error;
+     }
+   else
+     goto usage_error;
+     
+   return MI_CMD_DONE;
+ 
+  usage_error:
+   error ("mi_cmd_enable_timings: Usage: %s {yes|no}", command);
+   return MI_CMD_ERROR;
+ }
+ 
  /* Execute a command within a safe environment.
     Return <0 for error; >=0 for ok.
  
*************** captured_mi_execute_command (struct ui_o
*** 1027,1037 ****
      (struct captured_mi_execute_command_args *) data;
    struct mi_parse *context = args->command;
  
    switch (context->op)
      {
  
      case MI_COMMAND:
!       /* A MI command was read from the input stream */
        if (mi_debug_p)
  	/* FIXME: gdb_???? */
  	fprintf_unfiltered (raw_stdout, " token=`%s' command=`%s' args=`%s'\n",
--- 1060,1072 ----
      (struct captured_mi_execute_command_args *) data;
    struct mi_parse *context = args->command;
  
+   struct mi_timestamp cmd_finished;
+ 
    switch (context->op)
      {
  
      case MI_COMMAND:
!       /* A MI command was read from the input stream.  */
        if (mi_debug_p)
  	/* FIXME: gdb_???? */
  	fprintf_unfiltered (raw_stdout, " token=`%s' command=`%s' args=`%s'\n",
*************** captured_mi_execute_command (struct ui_o
*** 1039,1051 ****
        /* FIXME: cagney/1999-09-25: Rather than this convoluted
           condition expression, each function should return an
           indication of what action is required and then switch on
!          that. */
        args->action = EXECUTE_COMMAND_DISPLAY_PROMPT;
        args->rc = mi_cmd_execute (context);
  
        if (!target_can_async_p () || !target_executing)
  	{
! 	  /* print the result if there were no errors
  
  	     Remember that on the way out of executing a command, you have
  	     to directly use the mi_interp's uiout, since the command could 
--- 1074,1093 ----
        /* FIXME: cagney/1999-09-25: Rather than this convoluted
           condition expression, each function should return an
           indication of what action is required and then switch on
!          that.  */
        args->action = EXECUTE_COMMAND_DISPLAY_PROMPT;
+ 
+       if (do_timings)
+ 	current_command_ts = context->cmd_start;
+ 
        args->rc = mi_cmd_execute (context);
  
+       if (do_timings)
+           timestamp (&cmd_finished);
+ 
        if (!target_can_async_p () || !target_executing)
  	{
! 	  /* Print the result if there were no errors.
  
  	     Remember that on the way out of executing a command, you have
  	     to directly use the mi_interp's uiout, since the command could 
*************** captured_mi_execute_command (struct ui_o
*** 1057,1062 ****
--- 1099,1108 ----
  	      fputs_unfiltered ("^done", raw_stdout);
  	      mi_out_put (uiout, raw_stdout);
  	      mi_out_rewind (uiout);
+ 	      /* Have to check cmd_start, since the command could be
+ 		 -enable-timings.  */
+ 	      if (do_timings && context->cmd_start)
+ 		  print_diff (context->cmd_start, &cmd_finished);
  	      fputs_unfiltered ("\n", raw_stdout);
  	    }
  	  else if (args->rc == MI_CMD_ERROR)
*************** captured_mi_execute_command (struct ui_o
*** 1077,1083 ****
        else if (sync_execution)
  	{
  	  /* Don't print the prompt. We are executing the target in
! 	     synchronous mode. */
  	  args->action = EXECUTE_COMMAND_SUPRESS_PROMPT;
  	  return;
  	}
--- 1123,1129 ----
        else if (sync_execution)
  	{
  	  /* Don't print the prompt. We are executing the target in
! 	     synchronous mode.  */
  	  args->action = EXECUTE_COMMAND_SUPRESS_PROMPT;
  	  return;
  	}
*************** mi_execute_command (char *cmd, int from_
*** 1142,1149 ****
    struct captured_mi_execute_command_args args;
    struct ui_out *saved_uiout = uiout;
  
!   /* This is to handle EOF (^D). We just quit gdb. */
!   /* FIXME: we should call some API function here. */
    if (cmd == 0)
      quit_force (NULL, from_tty);
  
--- 1188,1195 ----
    struct captured_mi_execute_command_args args;
    struct ui_out *saved_uiout = uiout;
  
!   /* This is to handle EOF (^D). We just quit gdb.  */
!   /* FIXME: we should call some API function here.  */
    if (cmd == 0)
      quit_force (NULL, from_tty);
  
*************** mi_execute_command (char *cmd, int from_
*** 1152,1159 ****
    if (command != NULL)
      {
        struct gdb_exception result;
        /* FIXME: cagney/1999-11-04: Can this use of catch_exceptions either
!          be pushed even further down or even eliminated? */
        args.command = command;
        result = catch_exception (uiout, captured_mi_execute_command, &args,
  				RETURN_MASK_ALL);
--- 1198,1213 ----
    if (command != NULL)
      {
        struct gdb_exception result;
+ 
+       if (do_timings)
+ 	{
+ 	  command->cmd_start = (struct mi_timestamp *)
+ 	    xmalloc (sizeof (struct mi_timestamp));
+ 	  timestamp (command->cmd_start);
+ 	}
+ 
        /* FIXME: cagney/1999-11-04: Can this use of catch_exceptions either
!          be pushed even further down or even eliminated?  */
        args.command = command;
        result = catch_exception (uiout, captured_mi_execute_command, &args,
  				RETURN_MASK_ALL);
*************** mi_execute_command (char *cmd, int from_
*** 1162,1168 ****
        if (args.action == EXECUTE_COMMAND_SUPRESS_PROMPT)
  	{
  	  /* The command is executing synchronously.  Bail out early
! 	     suppressing the finished prompt. */
  	  mi_parse_free (command);
  	  return;
  	}
--- 1216,1222 ----
        if (args.action == EXECUTE_COMMAND_SUPRESS_PROMPT)
  	{
  	  /* The command is executing synchronously.  Bail out early
! 	     suppressing the finished prompt.  */
  	  mi_parse_free (command);
  	  return;
  	}
*************** mi_execute_command (char *cmd, int from_
*** 1184,1190 ****
  
    fputs_unfiltered ("(gdb) \n", raw_stdout);
    gdb_flush (raw_stdout);
!   /* print any buffered hook code */
    /* ..... */
  }
  
--- 1238,1244 ----
  
    fputs_unfiltered ("(gdb) \n", raw_stdout);
    gdb_flush (raw_stdout);
!   /* Print any buffered hook code.  */
    /* ..... */
  }
  
*************** mi_cmd_execute (struct mi_parse *parse)
*** 1197,1213 ****
        /* FIXME: We need to save the token because the command executed
           may be asynchronous and need to print the token again.
           In the future we can pass the token down to the func
!          and get rid of the last_async_command */
        /* The problem here is to keep the token around when we launch
           the target, and we want to interrupt it later on.  The
           interrupt command will have its own token, but when the
           target stops, we must display the token corresponding to the
!          last execution command given. So we have another string where
           we copy the token (previous_async_command), if this was
           indeed the token of an execution command, and when we stop we
!          print that one. This is possible because the interrupt
           command, when over, will copy that token back into the
!          default token string (last_async_command). */
  
        if (target_executing)
  	{
--- 1251,1267 ----
        /* FIXME: We need to save the token because the command executed
           may be asynchronous and need to print the token again.
           In the future we can pass the token down to the func
!          and get rid of the last_async_command.  */
        /* The problem here is to keep the token around when we launch
           the target, and we want to interrupt it later on.  The
           interrupt command will have its own token, but when the
           target stops, we must display the token corresponding to the
!          last execution command given.  So we have another string where
           we copy the token (previous_async_command), if this was
           indeed the token of an execution command, and when we stop we
!          print that one.  This is possible because the interrupt
           command, when over, will copy that token back into the
!          default token string (last_async_command).  */
  
        if (target_executing)
  	{
*************** mi_cmd_execute (struct mi_parse *parse)
*** 1234,1248 ****
    else if (parse->cmd->cli.cmd != 0)
      {
        /* FIXME: DELETE THIS. */
!       /* The operation is still implemented by a cli command */
!       /* Must be a synchronous one */
        mi_execute_cli_command (parse->cmd->cli.cmd, parse->cmd->cli.args_p,
  			      parse->args);
        return MI_CMD_DONE;
      }
    else
      {
!       /* FIXME: DELETE THIS. */
        fputs_unfiltered (parse->token, raw_stdout);
        fputs_unfiltered ("^error,msg=\"", raw_stdout);
        fputs_unfiltered ("Undefined mi command: ", raw_stdout);
--- 1288,1302 ----
    else if (parse->cmd->cli.cmd != 0)
      {
        /* FIXME: DELETE THIS. */
!       /* The operation is still implemented by a cli command.  */
!       /* Must be a synchronous one.  */
        mi_execute_cli_command (parse->cmd->cli.cmd, parse->cmd->cli.args_p,
  			      parse->args);
        return MI_CMD_DONE;
      }
    else
      {
!       /* FIXME: DELETE THIS.  */
        fputs_unfiltered (parse->token, raw_stdout);
        fputs_unfiltered ("^error,msg=\"", raw_stdout);
        fputs_unfiltered ("Undefined mi command: ", raw_stdout);
*************** mi_cmd_execute (struct mi_parse *parse)
*** 1254,1261 ****
  }
  
  /* FIXME: This is just a hack so we can get some extra commands going.
!    We don't want to channel things through the CLI, but call libgdb directly */
! /* Use only for synchronous commands */
  
  void
  mi_execute_cli_command (const char *cmd, int args_p, const char *args)
--- 1308,1315 ----
  }
  
  /* FIXME: This is just a hack so we can get some extra commands going.
!    We don't want to channel things through the CLI, but call libgdb directly.
!    Use only for synchronous commands.  */
  
  void
  mi_execute_cli_command (const char *cmd, int args_p, const char *args)
*************** mi_execute_async_cli_command (char *mi, 
*** 1307,1313 ****
      {
        /* NOTE: For synchronous targets asynchronous behavour is faked by
           printing out the GDB prompt before we even try to execute the
!          command. */
        if (last_async_command)
  	fputs_unfiltered (last_async_command, raw_stdout);
        fputs_unfiltered ("^running\n", raw_stdout);
--- 1361,1367 ----
      {
        /* NOTE: For synchronous targets asynchronous behavour is faked by
           printing out the GDB prompt before we even try to execute the
!          command.  */
        if (last_async_command)
  	fputs_unfiltered (last_async_command, raw_stdout);
        fputs_unfiltered ("^running\n", raw_stdout);
*************** mi_execute_async_cli_command (char *mi, 
*** 1319,1325 ****
        /* FIXME: cagney/1999-11-29: Printing this message before
           calling execute_command is wrong.  It should only be printed
           once gdb has confirmed that it really has managed to send a
!          run command to the target. */
        if (last_async_command)
  	fputs_unfiltered (last_async_command, raw_stdout);
        fputs_unfiltered ("^running\n", raw_stdout);
--- 1373,1379 ----
        /* FIXME: cagney/1999-11-29: Printing this message before
           calling execute_command is wrong.  It should only be printed
           once gdb has confirmed that it really has managed to send a
!          run command to the target.  */
        if (last_async_command)
  	fputs_unfiltered (last_async_command, raw_stdout);
        fputs_unfiltered ("^running\n", raw_stdout);
*************** mi_execute_async_cli_command (char *mi, 
*** 1330,1344 ****
    if (!target_can_async_p ())
      {
        /* Do this before doing any printing.  It would appear that some
!          print code leaves garbage around in the buffer. */
        do_cleanups (old_cleanups);
        /* If the target was doing the operation synchronously we fake
!          the stopped message. */
        if (last_async_command)
  	fputs_unfiltered (last_async_command, raw_stdout);
        fputs_unfiltered ("*stopped", raw_stdout);
        mi_out_put (uiout, raw_stdout);
        mi_out_rewind (uiout);
        fputs_unfiltered ("\n", raw_stdout);
        return MI_CMD_QUIET;
      }
--- 1384,1400 ----
    if (!target_can_async_p ())
      {
        /* Do this before doing any printing.  It would appear that some
!          print code leaves garbage around in the buffer.  */
        do_cleanups (old_cleanups);
        /* If the target was doing the operation synchronously we fake
!          the stopped message.  */
        if (last_async_command)
  	fputs_unfiltered (last_async_command, raw_stdout);
        fputs_unfiltered ("*stopped", raw_stdout);
        mi_out_put (uiout, raw_stdout);
        mi_out_rewind (uiout);
+       if (do_timings)
+       	print_diff_now (current_command_ts);
        fputs_unfiltered ("\n", raw_stdout);
        return MI_CMD_QUIET;
      }
*************** _initialize_mi_main (void)
*** 1455,1457 ****
--- 1511,1559 ----
    DEPRECATED_REGISTER_GDBARCH_SWAP (old_regs);
    deprecated_register_gdbarch_swap (NULL, 0, mi_setup_architecture_data);
  }
+ 
+ static void 
+ timestamp (struct mi_timestamp *tv)
+   {
+     long usec;
+ #ifdef HAVE_GETRUSAGE
+     gettimeofday (&tv->wallclock, NULL);
+     getrusage (RUSAGE_SELF, &tv->rusage);
+ #else
+     usec = get_run_time ();
+     tv->wallclock.tv_sec = usec/1000000;
+     tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
+     tv->rusage.ru_utime.tv_sec = 0;
+     tv->rusage.ru_utime.tv_usec = 0;
+     tv->rusage.ru_stime.tv_sec = 0;
+     tv->rusage.ru_stime.tv_usec = 0;
+ #endif
+   }
+ 
+ static void 
+ print_diff_now (struct mi_timestamp *start)
+   {
+     struct mi_timestamp now;
+     timestamp (&now);
+     print_diff (start, &now);
+   }
+ 
+ static long 
+ timeval_diff (struct timeval start, struct timeval end)
+   {
+     return ((end.tv_sec - start.tv_sec) * 1000000)
+       + (end.tv_usec - start.tv_usec);
+   }
+ 
+ static void 
+ print_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+   {
+     fprintf_unfiltered
+       (raw_stdout,
+        ",time={wallclock=\"%0.5f\",user=\"%0.5f\",system=\"%0.5f\"}", 
+        timeval_diff (start->wallclock, end->wallclock) / 1000000.0, 
+        timeval_diff (start->rusage.ru_utime, end->rusage.ru_utime)
+        / 1000000.0, 
+        timeval_diff (start->rusage.ru_stime, end->rusage.ru_stime)
+        / 1000000.0);
+   }

Re: [PATCH] MI: new timing command

[Daniel Jacobowitz]

On Thu, Jan 04, 2007 at 10:49:54AM +1300, Nick Roberts wrote:
>  > I don't even understand why we're having this conversation.  Please
>  > follow the coding standard unless you have a good justification not to;
>  > that's why we call it the standard.  Thanks.
> 
> Sure, I didn't say that I wouldn't do it.  I'm just waiting for Eli to tell
> me if it works with get_run_time.  Meanwhile here's a revised version of
> mi-main.c.  I've even made line breaks before, instead of after, operators!

Please... (from gdb/CONTRIBUTE):

o       Please read your patch before submitting it.

        A patch containing several unrelated changes or
        arbitrary reformats will be returned with a request
        to re-formatting / split it.

If you're volunteering to fix up formatting in the MI files, go right
ahead; patches which only fix formatting problems are obvious, as long
as you've proofread them.  But patches which mix code changes and
formatting changes are pretty much impossible to read.  The one you
just posted, for instance, was 40K and had about twenty lines of actual
new code in it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > If you're volunteering to fix up formatting in the MI files, go right
 > ahead; patches which only fix formatting problems are obvious, as long
 > as you've proofread them.

OK.  I'll commit the formatting changes after you or Eli have approved the
code change.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Vladimir Prus]

Nick Roberts wrote:

>  > Do you have a *strong* preference to context diff as opposed to
>  > unified? If yes, I think I can try to cope with context diffs, but
>  > unified diffs are much more readable.
> 
> I always use context diffs because that's what Richard Stallman insists on
> for
> Emacs.  However, it's no big deal because if you save the patch as a file
> and view it in Emacs in diff-mode (certain names like timings1.diff will
> automatically open in this mode), you can toggle between context and
> unified diffs from the menu bar.

That requires me to (1) save the message (2) open it in Emacs (3) copy-paste
it back to mailer when I have something to say about a patch. That's why I've
asked if you have *strong* preference for context diffs for gdb/MI patches.

>  > More seriously, this comment only says what this variable is
>  > used for, not what it is. For example, the comment might read:
>  > 
>  >         /* The timestamp of the moment when the current
>  >             command started executing.  Used to ... */
>  > 
>  > Ah, and looking at the code this variable is used *only* so that
>  > 'run' and friend can output the timestamp despite the fact that
>  > they don't emit '^done', so I think this is better
>  > represented in the comment.
> 
> In Apple's code its used for the asynchronous continuation functions. It's
> of type mi_timestamp and called current_command_ts: I think it's better to
> let the code speak for itself.

Sorry, I think MI has too much code that fails to speak for itself. Do you suggest
that somebody reading this code should grep for current_command_ts to understand
where this value is set? 

I also think it's is important to note why this variable exists. Why can't we pass
mi_parse* to the relevant function using function parameters? Is there a fundamental
reason to use global variable? 

>  >...
>  > > +       if (do_timings)
>  > > +       current_command_ts = context->cmd_start;
>  > 
>  > I wonder if it's better, instead of having global current_command_ts,
>  > add new global
>  > 
>  >         struct mi_parse* current_context;
>  > 
>  > set it here to context:
>  > 
>  >         current_context = context;
>  > 
>  > And the user 'current_context' later. That seems to be a more
>  > future-proof solution -- if mi_execute_async_cli_command will
>  > later need something more from mi_parse structure, we won't
>  > need to add yet another global variable.
> 
> This is a good solution if mi_execute_async_cli_command does eventually
> need something more from mi_parse structure later but I don't see why it's
> needed
> now.  But perhaps you have something in mind?

Using current_context does not make the patch more complex, and gives us some
future proofing. In fact, I'd say the right solution is to add mi_parse* parameter
to all functions down to mi_executed_async_cli_command, but that's solution is
more complex.

Although, if we're going to have "async commands", using a single global variable
will probably not work.

- Volodya

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sat, 30 Dec 2006 21:47:23 +1300
> 
> Here's a patch for timing MI commands e.g.

Thanks, but where's the documentation to go with it?

> diff -c -p -r1.86 mi-main.c
> *** mi-main.c	17 Nov 2006 19:30:41 -0000	1.86
> --- mi-main.c	30 Dec 2006 08:42:29 -0000
> ***************
> *** 49,54 ****
> --- 49,55 ----
>   
>   #include <ctype.h>
>   #include <sys/time.h>
> + #include <sys/resource.h>

Is sys/resource.h guaranteed to be available on all supported
platforms?  If not, we need to add a configure-time test and enable
this code only under HAVE_GETRUSAGE or some such.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > > Here's a patch for timing MI commands e.g.
 > 
 > Thanks, but where's the documentation to go with it?

There isn't any yet.  I would include it if I was confident that the patch
would get timely review/approval.  I think the code part is easily understood
so I guess I'm asking for approval for that, conditional to tests/documentation
being provided.  This saves wasted effort on my part.

 > > diff -c -p -r1.86 mi-main.c
 > > *** mi-main.c	17 Nov 2006 19:30:41 -0000	1.86
 > > --- mi-main.c	30 Dec 2006 08:42:29 -0000
 > > ***************
 > > *** 49,54 ****
 > > --- 49,55 ----
 > >   
 > >   #include <ctype.h>
 > >   #include <sys/time.h>
 > > + #include <sys/resource.h>
 > 
 > Is sys/resource.h guaranteed to be available on all supported
 > platforms?  If not, we need to add a configure-time test and enable
 > this code only under HAVE_GETRUSAGE or some such.

mi-main.c already uses sys/time.h.  Is sys/resource.h any less widely
distributed?  Is there any way of finding out e.g online database?


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sun, 31 Dec 2006 11:05:35 +1300
> Cc: gdb-patches@sourceware.org
> 
>  > > diff -c -p -r1.86 mi-main.c
>  > > *** mi-main.c	17 Nov 2006 19:30:41 -0000	1.86
>  > > --- mi-main.c	30 Dec 2006 08:42:29 -0000
>  > > ***************
>  > > *** 49,54 ****
>  > > --- 49,55 ----
>  > >   
>  > >   #include <ctype.h>
>  > >   #include <sys/time.h>
>  > > + #include <sys/resource.h>
>  > 
>  > Is sys/resource.h guaranteed to be available on all supported
>  > platforms?  If not, we need to add a configure-time test and enable
>  > this code only under HAVE_GETRUSAGE or some such.
> 
> mi-main.c already uses sys/time.h.  Is sys/resource.h any less widely
> distributed?

I think sys/time.h is much more wide-spread, but I might be wrong.  I
raised the issue because sys/resource.h is relatively seldom used, so
it might be less portable.

> Is there any way of finding out e.g online database?

Sorry, I don't know.  Anyone?

Re: [PATCH] MI: new timing command

[Daniel Jacobowitz]

On Sun, Dec 31, 2006 at 06:21:59AM +0200, Eli Zaretskii wrote:
> > Is there any way of finding out e.g online database?
> 
> Sorry, I don't know.  Anyone?

The autoconf manual has information about this sort of thing.  I see
that libiberty guards it, sometimes with HAVE_GETRUSAGE and other times
with that and HAVE_SYS_RESOURCE_H.

Will libiberty's get_run_time suffice for whatever you were doing,
Nick?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > The autoconf manual has information about this sort of thing.  I see
 > that libiberty guards it, sometimes with HAVE_GETRUSAGE and other times
 > with that and HAVE_SYS_RESOURCE_H.
 > 
 > Will libiberty's get_run_time suffice for whatever you were doing,
 > Nick?

The manual says:

 -- Replacement: long get_run_time (void)
     Returns the time used so far, in microseconds.  If possible, this
     is the time used by this process, else it is the elapsed time
     since the process started.

Without looking at the code, I would guess it's just a wrapper for getrusage
and it uses something like gettimeofday for elapsed time when it can't find
it.  I think if user time isn't available it's best just to make
-enable-timings fail, so I'll use Eli's suggestion.


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Daniel Jacobowitz]

On Sun, Dec 31, 2006 at 06:13:50PM +1300, Nick Roberts wrote:
>  > The autoconf manual has information about this sort of thing.  I see
>  > that libiberty guards it, sometimes with HAVE_GETRUSAGE and other times
>  > with that and HAVE_SYS_RESOURCE_H.
>  > 
>  > Will libiberty's get_run_time suffice for whatever you were doing,
>  > Nick?
> 
> The manual says:
> 
>  -- Replacement: long get_run_time (void)
>      Returns the time used so far, in microseconds.  If possible, this
>      is the time used by this process, else it is the elapsed time
>      since the process started.
> 
> Without looking at the code, I would guess it's just a wrapper for getrusage
> and it uses something like gettimeofday for elapsed time when it can't find
> it.  I think if user time isn't available it's best just to make
> -enable-timings fail, so I'll use Eli's suggestion.

In that case you can copy the necessary guards from that file. 
However, it does more than just getrusage - it also supports
platforms with times() but without getrusage, which IIRC includes
Windows, so it might be better to use it.

I was wondering if we should make this a normal GDB setting, and use5C
"-gdb-set mi profiling on" to enable it.  There's already a maint
setting to do the same thing for the CLI.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: new timing command

[Nick Roberts]

Daniel Jacobowitz writes:
 > On Sun, Dec 31, 2006 at 06:13:50PM +1300, Nick Roberts wrote:
 > >  > The autoconf manual has information about this sort of thing.  I see
 > >  > that libiberty guards it, sometimes with HAVE_GETRUSAGE and other times
 > >  > with that and HAVE_SYS_RESOURCE_H.
 > >  > 
 > >  > Will libiberty's get_run_time suffice for whatever you were doing,
 > >  > Nick?
 > > 
 > > The manual says:
 > > 
 > >  -- Replacement: long get_run_time (void)
 > >      Returns the time used so far, in microseconds.  If possible, this
 > >      is the time used by this process, else it is the elapsed time
 > >      since the process started.
 > > 
 > > Without looking at the code, I would guess it's just a wrapper for getrusage
 > > and it uses something like gettimeofday for elapsed time when it can't find
 > > it.  I think if user time isn't available it's best just to make
 > > -enable-timings fail, so I'll use Eli's suggestion.
 > 
 > In that case you can copy the necessary guards from that file. 
 > However, it does more than just getrusage - it also supports
 > platforms with times() but without getrusage, which IIRC includes
 > Windows, so it might be better to use it.

But as a last resort it returns elapsed time which would be wrong.

 > I was wondering if we should make this a normal GDB setting, and use5C
 > "-gdb-set mi profiling on" to enable it.  There's already a maint
 > setting to do the same thing for the CLI.

The command "maint set profiling on" is relevant for both CLI and MI,
whereas -enable-timings only works in MI.

Here's a new patch which also follows some of Vladimir's suggestions.  If
approved I'll also regenerate and commit configure and config.in using GNU
Autoconf 2.59.


-- 
Nick                                           http://www.inet.net.nz/~nickrob


2006-12-31  Nick Roberts  <nickrob@snap.net.nz>

	    Based on work by Apple Computer, Inc.

	* configure.ac: Test for sys/resource.h and getrusage.

	* mi/mi-main.c: Include <sys/resource.h> if present.
	(current_command_ts, do_timings): New static variables.
	(timestamp, print_diff_now, print_diff, timeval_diff):
	New static timing functions.
	(mi_cmd_enable_timings): New function for new MI command.
	(captured_mi_execute_command, mi_execute_async_cli_command): 
	Call timing functions.

	* mi/mi-cmds.c (mi_cmds): Add entry for new MI command
	-enable-timings.

	* mi/mi-cmds.h (mi_cmd_enable_timings): New extern.

	* mi/mi-parse.h: Include <sys/resource.h> if present.
	(mi_timestamp): New structure.
	(mi_parse): Add mi_timestamp* member.


Index: configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.36
diff -c -p -r1.36 configure.ac
*** configure.ac	22 Nov 2006 17:34:15 -0000	1.36
--- configure.ac	31 Dec 2006 07:39:10 -0000
*************** AC_CHECK_HEADERS(sys/file.h)
*** 355,360 ****
--- 355,361 ----
  AC_CHECK_HEADERS(sys/filio.h)
  AC_CHECK_HEADERS(sys/ioctl.h)
  AC_CHECK_HEADERS(sys/param.h)
+ AC_CHECK_HEADERS(sys/resource.h)
  AC_CHECK_HEADERS(sys/proc.h, [], [],
  [#if HAVE_SYS_PARAM_H
  # include <sys/param.h>
*************** AC_FUNC_ALLOCA
*** 442,447 ****
--- 443,449 ----
  AC_FUNC_MMAP
  AC_FUNC_VFORK
  AC_CHECK_FUNCS(canonicalize_file_name realpath)
+ AC_CHECK_FUNCS(getrusage)
  AC_CHECK_FUNCS(getuid getgid)
  AC_CHECK_FUNCS(poll)
  AC_CHECK_FUNCS(pread64)
Index: mi/mi-cmds.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmds.c,v
retrieving revision 1.21
diff -c -p -r1.21 mi-cmds.c
*** mi/mi-cmds.c	5 Jul 2006 19:03:47 -0000	1.21
--- mi/mi-cmds.c	31 Dec 2006 07:39:11 -0000
*************** struct mi_cmd mi_cmds[] =
*** 58,63 ****
--- 58,64 ----
    { "display-enable", { NULL, 0 }, NULL, NULL },
    { "display-insert", { NULL, 0 }, NULL, NULL },
    { "display-list", { NULL, 0 }, NULL, NULL },
+   { "enable-timings", { NULL, 0 }, 0, mi_cmd_enable_timings},
    { "environment-cd", { NULL, 0 }, 0, mi_cmd_env_cd},
    { "environment-directory", { NULL, 0 }, 0, mi_cmd_env_dir},
    { "environment-path", { NULL, 0 }, 0, mi_cmd_env_path},
Index: mi/mi-cmds.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmds.h,v
retrieving revision 1.19
diff -c -p -r1.19 mi-cmds.h
*** mi/mi-cmds.h	23 Dec 2005 18:57:46 -0000	1.19
--- mi/mi-cmds.h	31 Dec 2006 07:39:11 -0000
*************** extern mi_cmd_argv_ftype mi_cmd_data_lis
*** 72,77 ****
--- 72,78 ----
  extern mi_cmd_argv_ftype mi_cmd_data_read_memory;
  extern mi_cmd_argv_ftype mi_cmd_data_write_memory;
  extern mi_cmd_argv_ftype mi_cmd_data_write_register_values;
+ extern mi_cmd_argv_ftype mi_cmd_enable_timings;
  extern mi_cmd_argv_ftype mi_cmd_env_cd;
  extern mi_cmd_argv_ftype mi_cmd_env_dir;
  extern mi_cmd_argv_ftype mi_cmd_env_path;
Index: mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.86
diff -c -p -r1.86 mi-main.c
*** mi/mi-main.c	17 Nov 2006 19:30:41 -0000	1.86
--- mi/mi-main.c	31 Dec 2006 07:39:14 -0000
***************
*** 50,55 ****
--- 50,59 ----
  #include <ctype.h>
  #include <sys/time.h>
  
+ #if defined HAVE_SYS_RESOURCE_H
+ #include <sys/resource.h>
+ #endif
+ 
  enum
    {
      FROM_TTY = 0
*************** struct captured_mi_execute_command_args
*** 81,86 ****
--- 85,96 ----
  int mi_debug_p;
  struct ui_file *raw_stdout;
  
+ /* This is used to pass the current command timestamp
+    down to continuation routines.  */
+ static struct mi_timestamp *current_command_ts;
+ 
+ static int do_timings = 0;
+ 
  /* The token of the last asynchronous command */
  static char *last_async_command;
  static char *previous_async_command;
*************** static int get_register (int regnum, int
*** 103,108 ****
--- 113,123 ----
     layer that calls libgdb.  Any operation used in the below should be
     formalized. */
  
+ static void timestamp (struct mi_timestamp *tv);
+ 
+ static void print_diff_now (struct mi_timestamp *start);
+ static void print_diff (struct mi_timestamp *start, struct mi_timestamp *end);
+ 
  enum mi_cmd_result
  mi_cmd_gdb_exit (char *command, char **argv, int argc)
  {
*************** mi_cmd_exec_continue (char *args, int fr
*** 194,200 ****
  }
  
  /* Interrupt the execution of the target. Note how we must play around
!    with the token varialbes, in order to display the current token in
     the result of the interrupt command, and the previous execution
     token when the target finally stops. See comments in
     mi_cmd_execute. */
--- 209,215 ----
  }
  
  /* Interrupt the execution of the target. Note how we must play around
!    with the token variables, in order to display the current token in
     the result of the interrupt command, and the previous execution
     token when the target finally stops. See comments in
     mi_cmd_execute. */
*************** mi_cmd_data_write_memory (char *command,
*** 1024,1029 ****
--- 1039,1073 ----
    return MI_CMD_DONE;
  }
  
+ enum mi_cmd_result
+ mi_cmd_enable_timings (char *command, char **argv, int argc)
+ {
+ #ifdef HAVE_GETRUSAGE
+   if (argc == 0)
+     do_timings = 1;
+   else if (argc == 1)
+     {
+       if (strcmp (argv[0], "yes") == 0)
+ 	do_timings = 1;
+       else if (strcmp (argv[0], "no") == 0)
+ 	do_timings = 0;
+       else
+ 	goto usage_error;
+     }
+   else
+     goto usage_error;
+     
+   return MI_CMD_DONE;
+ 
+  usage_error:
+   error ("mi_cmd_enable_timings: Usage: %s {yes|no}", command);
+   return MI_CMD_ERROR;
+ #else
+   error ("GDB compiled without getrusage");
+  return MI_CMD_ERROR;
+ #endif
+ }
+ 
  /* Execute a command within a safe environment.
     Return <0 for error; >=0 for ok.
  
*************** captured_mi_execute_command (struct ui_o
*** 1038,1043 ****
--- 1082,1089 ----
      (struct captured_mi_execute_command_args *) data;
    struct mi_parse *context = args->command;
  
+   struct mi_timestamp cmd_finished;
+ 
    switch (context->op)
      {
  
*************** captured_mi_execute_command (struct ui_o
*** 1052,1059 ****
--- 1098,1112 ----
           indication of what action is required and then switch on
           that. */
        args->action = EXECUTE_COMMAND_DISPLAY_PROMPT;
+ 
+       if (do_timings)
+ 	current_command_ts = context->cmd_start;
+ 
        args->rc = mi_cmd_execute (context);
  
+       if (do_timings)
+           timestamp (&cmd_finished);
+ 
        if (!target_can_async_p () || !target_executing)
  	{
  	  /* print the result if there were no errors
*************** captured_mi_execute_command (struct ui_o
*** 1068,1073 ****
--- 1121,1130 ----
  	      fputs_unfiltered ("^done", raw_stdout);
  	      mi_out_put (uiout, raw_stdout);
  	      mi_out_rewind (uiout);
+ 	      /* Have to check cmd_start, since the command could be
+ 		 -enable-timings. */
+ 	      if (do_timings && context->cmd_start)
+ 		  print_diff (context->cmd_start, &cmd_finished);
  	      fputs_unfiltered ("\n", raw_stdout);
  	    }
  	  else if (args->rc == MI_CMD_ERROR)
*************** mi_execute_command (char *cmd, int from_
*** 1163,1168 ****
--- 1220,1233 ----
    if (command != NULL)
      {
        struct gdb_exception result;
+ 
+       if (do_timings)
+ 	{
+ 	  command->cmd_start = (struct mi_timestamp *)
+ 	    xmalloc (sizeof (struct mi_timestamp));
+ 	  timestamp (command->cmd_start);
+ 	}
+ 
        /* FIXME: cagney/1999-11-04: Can this use of catch_exceptions either
           be pushed even further down or even eliminated? */
        args.command = command;
*************** mi_execute_async_cli_command (char *mi, 
*** 1350,1355 ****
--- 1415,1422 ----
        fputs_unfiltered ("*stopped", raw_stdout);
        mi_out_put (uiout, raw_stdout);
        mi_out_rewind (uiout);
+       if (do_timings)
+       	print_diff_now (current_command_ts);
        fputs_unfiltered ("\n", raw_stdout);
        return MI_CMD_QUIET;
      }
*************** _initialize_mi_main (void)
*** 1466,1468 ****
--- 1533,1572 ----
    DEPRECATED_REGISTER_GDBARCH_SWAP (old_regs);
    deprecated_register_gdbarch_swap (NULL, 0, mi_setup_architecture_data);
  }
+ 
+ static void 
+ timestamp (struct mi_timestamp *tv)
+   {
+ #ifdef HAVE_GETRUSAGE
+     gettimeofday (&tv->wallclock, NULL);
+     getrusage (RUSAGE_SELF, &tv->rusage);
+ #endif
+   }
+ 
+ static void 
+ print_diff_now (struct mi_timestamp *start)
+   {
+     struct mi_timestamp now;
+     timestamp (&now);
+     print_diff (start, &now);
+   }
+ 
+ static long 
+ timval_diff (struct timeval start, struct timeval end)
+   {
+     return ((end.tv_sec - start.tv_sec) * 1000000) +
+       (end.tv_usec - start.tv_usec);
+   }
+ 
+ static void 
+ print_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+   {
+     fprintf_unfiltered
+       (raw_stdout,
+        ",time={wallclock=\"%0.5f\",user=\"%0.5f\",system=\"%0.5f\"}", 
+        timval_diff (start->wallclock, end->wallclock) / 1000000.0, 
+        timval_diff (start->rusage.ru_utime, end->rusage.ru_utime) /
+        1000000.0, 
+        timval_diff (start->rusage.ru_stime, end->rusage.ru_stime) /
+        1000000.0);
+   }
Index: mi/mi-parse.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.h,v
retrieving revision 1.5
diff -c -p -r1.5 mi-parse.h
*** mi/mi-parse.h	23 Dec 2005 18:57:46 -0000	1.5
--- mi/mi-parse.h	31 Dec 2006 07:39:14 -0000
***************
*** 24,29 ****
--- 24,37 ----
  
  /* MI parser */
  
+ #include <sys/resource.h>
+ 
+ /* Timestamps for current command and last asynchronous command */
+ struct mi_timestamp {
+     struct timeval wallclock;
+     struct rusage rusage;
+ };
+ 
  enum mi_command_type
    {
      MI_COMMAND, CLI_COMMAND
*************** struct mi_parse
*** 35,40 ****
--- 43,49 ----
      char *command;
      char *token;
      const struct mi_cmd *cmd;
+     struct mi_timestamp *cmd_start;
      char *args;
      char **argv;
      int argc;

Re: [PATCH] MI: new timing command

[Daniel Jacobowitz]

On Sun, Dec 31, 2006 at 08:42:12PM +1300, Nick Roberts wrote:
> Daniel Jacobowitz writes:
>  > In that case you can copy the necessary guards from that file. 
>  > However, it does more than just getrusage - it also supports
>  > platforms with times() but without getrusage, which IIRC includes
>  > Windows, so it might be better to use it.
> 
> But as a last resort it returns elapsed time which would be wrong.

You keep saying this but I don't see why.  Why is it wrong?  On every
platform where we can do it, we'll print usage; on platforms where we
can't do it, the odds are pretty good that the OS isn't aggressively
scheduling other tasks in while we're running, so wall time is pretty
close to right.

>  > I was wondering if we should make this a normal GDB setting, and use5C
>  > "-gdb-set mi profiling on" to enable it.  There's already a maint
>  > setting to do the same thing for the CLI.
> 
> The command "maint set profiling on" is relevant for both CLI and MI,
> whereas -enable-timings only works in MI.

I wasn't talking about "maint set profiling on", but about "maint time
1".  That doesn't currently work in MI, but it could be made to work
and moved under the normal "set" hierarchy.  Then we could use the same
setting for both CLI and MI.  In that case you wouldn't have to invent
a new command that reimplements the "set" behavior; it could just be a
standard auto_boolean.


-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: new timing command

[Mark Kettenis]

> Date: Sun, 31 Dec 2006 10:15:27 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sun, Dec 31, 2006 at 08:42:12PM +1300, Nick Roberts wrote:
> > Daniel Jacobowitz writes:
> >  > In that case you can copy the necessary guards from that file. 
> >  > However, it does more than just getrusage - it also supports
> >  > platforms with times() but without getrusage, which IIRC includes
> >  > Windows, so it might be better to use it.
> > 
> > But as a last resort it returns elapsed time which would be wrong.
> 
> You keep saying this but I don't see why.  Why is it wrong?  On every
> platform where we can do it, we'll print usage; on platforms where we
> can't do it, the odds are pretty good that the OS isn't aggressively
> scheduling other tasks in while we're running, so wall time is pretty
> close to right.

I agree completely.

Re: [PATCH] MI: new timing command

[Vladimir Prus]

Mark Kettenis wrote:

>> Date: Sun, 31 Dec 2006 10:15:27 -0500
>> From: Daniel Jacobowitz <drow@false.org>
>> 
>> On Sun, Dec 31, 2006 at 08:42:12PM +1300, Nick Roberts wrote:
>> > Daniel Jacobowitz writes:
>> >  > In that case you can copy the necessary guards from that file.
>> >  > However, it does more than just getrusage - it also supports
>> >  > platforms with times() but without getrusage, which IIRC includes
>> >  > Windows, so it might be better to use it.
>> > 
>> > But as a last resort it returns elapsed time which would be wrong.
>> 
>> You keep saying this but I don't see why.  Why is it wrong?  On every
>> platform where we can do it, we'll print usage; on platforms where we
>> can't do it, the odds are pretty good that the OS isn't aggressively
>> scheduling other tasks in while we're running, so wall time is pretty
>> close to right.
> 
> I agree completely.

Is this important? This timing is entirely for diagnostic purposes, 
so why try to make it work on every possible platform. We need to document
that -enable-timing may fail, and that's it.

- Volodya
 

Re: [PATCH] MI: new timing command

[Mark Kettenis]

> X-Spam-Checker-Version: SpamAssassin 3.1.7 (2006-10-05) on 
> 	zigzag.lvk.cs.msu.su
> X-Spam-Level: 
> X-Spam-Status: No, score=-4.4 required=5.5 tests=ALL_TRUSTED,BAYES_00 
> 	autolearn=ham version=3.1.7
> From: Vladimir Prus <ghost@cs.msu.su>
> Date: Sun, 31 Dec 2006 18:37:58 +0300
> X-XS4ALL-DNSBL-Checked: mxdrop37.xs4all.nl checked 158.250.17.23 against DNS blacklists
> X-Virus-Scanned: by XS4ALL Virus Scanner
> X-XS4ALL-Spam-Score: 0.0 () DK_POLICY_SIGNSOME,UNPARSEABLE_RELAY
> X-XS4ALL-Spam: NO
> Envelope-To: mark.kettenis@xs4all.nl
> X-UIDL: 1167579527._smtp.mxdrop37.60237,S=2952
> 
> Mark Kettenis wrote:
> 
> >> Date: Sun, 31 Dec 2006 10:15:27 -0500
> >> From: Daniel Jacobowitz <drow@false.org>
> >> 
> >> On Sun, Dec 31, 2006 at 08:42:12PM +1300, Nick Roberts wrote:
> >> > Daniel Jacobowitz writes:
> >> >  > In that case you can copy the necessary guards from that file.
> >> >  > However, it does more than just getrusage - it also supports
> >> >  > platforms with times() but without getrusage, which IIRC includes
> >> >  > Windows, so it might be better to use it.
> >> > 
> >> > But as a last resort it returns elapsed time which would be wrong.
> >> 
> >> You keep saying this but I don't see why.  Why is it wrong?  On every
> >> platform where we can do it, we'll print usage; on platforms where we
> >> can't do it, the odds are pretty good that the OS isn't aggressively
> >> scheduling other tasks in while we're running, so wall time is pretty
> >> close to right.
> > 
> > I agree completely.
> 
> Is this important? This timing is entirely for diagnostic purposes, 
> so why try to make it work on every possible platform. We need to document
> that -enable-timing may fail, and that's it.

The point is to use get_run_time() from -liberty and never worry about
portability again.

Re: [PATCH] MI: new timing command

[Vladimir Prus]

On Sunday 31 December 2006 19:09, Mark Kettenis wrote:

> > Is this important? This timing is entirely for diagnostic purposes, 
> > so why try to make it work on every possible platform. We need to document
> > that -enable-timing may fail, and that's it.
> 
> The point is to use get_run_time() from -liberty and never worry about
> portability again.

And lose user/system/elapse time distinction? For me, having all those times
on Linux is more valuable than having any time at all on Windows. No because
Windows is not important, but because I doubt there are any plantform specific
MI slowdowns.

- Volodya

Re: [PATCH] MI: new timing command

[Daniel Jacobowitz]

On Sun, Dec 31, 2006 at 07:20:48PM +0300, Vladimir Prus wrote:
> On Sunday 31 December 2006 19:09, Mark Kettenis wrote:
> 
> > > Is this important? This timing is entirely for diagnostic purposes, 
> > > so why try to make it work on every possible platform. We need to document
> > > that -enable-timing may fail, and that's it.
> > 
> > The point is to use get_run_time() from -liberty and never worry about
> > portability again.
> 
> And lose user/system/elapse time distinction? For me, having all those times
> on Linux is more valuable than having any time at all on Windows. No because
> Windows is not important, but because I doubt there are any plantform specific
> MI slowdowns.

I really don't think the divided times are going to be useful.  But
then, I probably won't be the one using them.  If you would prefer to
maintain the distinction between system time and user time, then I
withdraw my objection.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Vladimir Prus <ghost@cs.msu.su>
> Date: Sun, 31 Dec 2006 19:20:48 +0300
> Cc: gdb-patches@sources.redhat.com
> 
> On Sunday 31 December 2006 19:09, Mark Kettenis wrote:
> 
> > > Is this important? This timing is entirely for diagnostic purposes, 
> > > so why try to make it work on every possible platform. We need to document
> > > that -enable-timing may fail, and that's it.
> > 
> > The point is to use get_run_time() from -liberty and never worry about
> > portability again.
> 
> And lose user/system/elapse time distinction?

You don't have to lose it.  How about if we use getrusage directly if
it's available, else the libiberty replacement?  That way, everybody
will be as happy as they can expect, I think.

> For me, having all those times
> on Linux is more valuable than having any time at all on Windows.

You are not the only user of GDB.  There's no reason to gratuitously
confine features to the only platform that you are interested in.
Please remember that getrusage is not even portable enough on Posix
platforms.

> No because Windows is not important, but because I doubt there are
> any plantform specific MI slowdowns.

I'm sure you are wrong.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > > And lose user/system/elapse time distinction?
 > 
 > You don't have to lose it.  How about if we use getrusage directly if
 > it's available, else the libiberty replacement?  That way, everybody
 > will be as happy as they can expect, I think.

If we adopt this approach can you please explain to me how to interpret the
output of get_run_time when getrusage is not availble so I can add it to the
doc.

 > > For me, having all those times
 > > on Linux is more valuable than having any time at all on Windows.
 > 
 > You are not the only user of GDB.

-enable-timing would only be used by developers of frontends to GDB using MI.
Currently Vladimir and myself do almost appear to be the only users.

 >                                    There's no reason to gratuitously
 > confine features to the only platform that you are interested in.

It not gratuitous because generalising it involves extra work in areas
we don't understand.  

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Mon, 1 Jan 2007 17:22:18 +1300
> Cc: Vladimir Prus <ghost@cs.msu.su>, mark.kettenis@xs4all.nl,
> 	gdb-patches@sources.redhat.com
> 
>  > > And lose user/system/elapse time distinction?
>  > 
>  > You don't have to lose it.  How about if we use getrusage directly if
>  > it's available, else the libiberty replacement?  That way, everybody
>  > will be as happy as they can expect, I think.
> 
> If we adopt this approach can you please explain to me how to interpret the
> output of get_run_time when getrusage is not availble so I can add it to the
> doc.

I think it should be interpreted the same as the results from
getrusage.  What made you think it needs special interpretation?

If the problem is that get_run_time returns a single number instead of
3 numbers, I think we should simply display the other 2 as zeroes, or
maybe use some MI convention for results that aren't available, if
there is such a convention.

>  > You are not the only user of GDB.
> 
> -enable-timing would only be used by developers of frontends to GDB using MI.
> Currently Vladimir and myself do almost appear to be the only users.

Let's not enter the argument of how many people we can count that will
use this command.  Features are added to GDB for the benefit of
everybody, not just the couple of people whom we know to need them.

>  >                                    There's no reason to gratuitously
>  > confine features to the only platform that you are interested in.
> 
> It not gratuitous because generalising it involves extra work in areas
> we don't understand.  

What areas are those?  I don't see any particular extra work required
in the simple solution I proposed.  Maybe I'm missing something.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > > >> > But as a last resort it returns elapsed time which would be wrong.
 > > >> 
 > > >> You keep saying this but I don't see why.  Why is it wrong?  On every
 > > >> platform where we can do it, we'll print usage; on platforms where we
 > > >> can't do it, the odds are pretty good that the OS isn't aggressively
 > > >> scheduling other tasks in while we're running, so wall time is pretty
 > > >> close to right.
 > > > 
 > > > I agree completely.
 > > 
 > > Is this important? This timing is entirely for diagnostic purposes, 
 > > so why try to make it work on every possible platform. We need to document
 > > that -enable-timing may fail, and that's it.
 > 
 > The point is to use get_run_time() from -liberty and never worry about
 > portability again.

Let's be realistic.  This isn't a command for general users of GDB, it isn't
even a command for general users of frontends to GDB.  It's a command for
developers of frontends to GDB which currently means just myself and Vladimir,
and maybe a couple of others like Bob Rossi and Alain Magloire.

I'm not familiar with get_run_time but I'm sure we all know getrusage through
the time shell command.  If the frontend appears to be slow I can see if that's
due to MI or other things running on my system.  I'm not sure that I can do
that with get_run_time.  I would like to start with getrusage and then when
there are hoards of developers rushing to develop frontends for GDB using
MI on Windows, I'll be happy to accommodate them.


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Mon, 1 Jan 2007 17:05:01 +1300
> Cc: ghost@cs.msu.su, gdb-patches@sources.redhat.com
> 
> I'm not familiar with get_run_time but I'm sure we all know getrusage through
> the time shell command.  If the frontend appears to be slow I can see if that's
> due to MI or other things running on my system.  I'm not sure that I can do
> that with get_run_time.  I would like to start with getrusage and then when
> there are hoards of developers rushing to develop frontends for GDB using
> MI on Windows, I'll be happy to accommodate them.

Once again, this is not about Windows, this is about _any_ platform
that lacks getrusage.

All you need to do is call get_run_time in the #else branch of the
HAVE_GETRUSAGE test, instead of erroring out.  I'm amazed such a
simple solution causes such a prolonged dispute.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > All you need to do is call get_run_time in the #else branch of the
 > HAVE_GETRUSAGE test, instead of erroring out.  I'm amazed such a
 > simple solution causes such a prolonged dispute.

OK, this patch gives wallclock, user and system time in the output.
In the case when getrusage is not available it the output of get_run_time
is used for the wallclock field and user and system time are given as 0.0.
I presume that is meaningful.  Can you please test it for this latter case?


-- 
Nick                                           http://www.inet.net.nz/~nickrob


2007-01-02  Nick Roberts  <nickrob@snap.net.nz>

	    Based on work by Apple Computer, Inc.

	* configure.ac: Test for sys/resource.h and getrusage.

	* mi/mi-main.c: Include <sys/resource.h> if present.
	(current_command_ts, do_timings): New static variables.
	(timestamp, print_diff_now, print_diff, timeval_diff):
	New static timing functions.
	(mi_cmd_enable_timings): New function for new MI command.
	(captured_mi_execute_command, mi_execute_async_cli_command): 
	Call timing functions.

	* mi/mi-cmds.c (mi_cmds): Add entry for new MI command
	-enable-timings.

	* mi/mi-cmds.h (mi_cmd_enable_timings): New extern.

	* mi/mi-parse.h: Include <sys/resource.h> if present.
	(mi_timestamp): New structure.
	(mi_parse): Add mi_timestamp* member.


Index: configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.36
diff -c -p -r1.36 configure.ac
*** configure.ac	22 Nov 2006 17:34:15 -0000	1.36
--- configure.ac	31 Dec 2006 07:39:10 -0000
*************** AC_CHECK_HEADERS(sys/file.h)
*** 355,360 ****
--- 355,361 ----
  AC_CHECK_HEADERS(sys/filio.h)
  AC_CHECK_HEADERS(sys/ioctl.h)
  AC_CHECK_HEADERS(sys/param.h)
+ AC_CHECK_HEADERS(sys/resource.h)
  AC_CHECK_HEADERS(sys/proc.h, [], [],
  [#if HAVE_SYS_PARAM_H
  # include <sys/param.h>
*************** AC_FUNC_ALLOCA
*** 442,447 ****
--- 443,449 ----
  AC_FUNC_MMAP
  AC_FUNC_VFORK
  AC_CHECK_FUNCS(canonicalize_file_name realpath)
+ AC_CHECK_FUNCS(getrusage)
  AC_CHECK_FUNCS(getuid getgid)
  AC_CHECK_FUNCS(poll)
  AC_CHECK_FUNCS(pread64)
Index: mi/mi-cmds.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmds.c,v
retrieving revision 1.21
diff -c -p -r1.21 mi-cmds.c
*** mi/mi-cmds.c	5 Jul 2006 19:03:47 -0000	1.21
--- mi/mi-cmds.c	31 Dec 2006 07:39:11 -0000
*************** struct mi_cmd mi_cmds[] =
*** 58,63 ****
--- 58,64 ----
    { "display-enable", { NULL, 0 }, NULL, NULL },
    { "display-insert", { NULL, 0 }, NULL, NULL },
    { "display-list", { NULL, 0 }, NULL, NULL },
+   { "enable-timings", { NULL, 0 }, 0, mi_cmd_enable_timings},
    { "environment-cd", { NULL, 0 }, 0, mi_cmd_env_cd},
    { "environment-directory", { NULL, 0 }, 0, mi_cmd_env_dir},
    { "environment-path", { NULL, 0 }, 0, mi_cmd_env_path},
Index: mi/mi-cmds.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmds.h,v
retrieving revision 1.19
diff -c -p -r1.19 mi-cmds.h
*** mi/mi-cmds.h	23 Dec 2005 18:57:46 -0000	1.19
--- mi/mi-cmds.h	31 Dec 2006 07:39:11 -0000
*************** extern mi_cmd_argv_ftype mi_cmd_data_lis
*** 72,77 ****
--- 72,78 ----
  extern mi_cmd_argv_ftype mi_cmd_data_read_memory;
  extern mi_cmd_argv_ftype mi_cmd_data_write_memory;
  extern mi_cmd_argv_ftype mi_cmd_data_write_register_values;
+ extern mi_cmd_argv_ftype mi_cmd_enable_timings;
  extern mi_cmd_argv_ftype mi_cmd_env_cd;
  extern mi_cmd_argv_ftype mi_cmd_env_dir;
  extern mi_cmd_argv_ftype mi_cmd_env_path;
Index: mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.87
diff -c -p -r1.87 mi-main.c
*** mi/mi-main.c	1 Jan 2007 11:17:28 -0000	1.87
--- mi/mi-main.c	1 Jan 2007 22:04:28 -0000
***************
*** 50,55 ****
--- 50,59 ----
  #include <ctype.h>
  #include <sys/time.h>
  
+ #if defined HAVE_SYS_RESOURCE_H
+ #include <sys/resource.h>
+ #endif
+ 
  enum
    {
      FROM_TTY = 0
*************** struct captured_mi_execute_command_args
*** 81,86 ****
--- 85,96 ----
  int mi_debug_p;
  struct ui_file *raw_stdout;
  
+ /* This is used to pass the current command timestamp
+    down to continuation routines.  */
+ static struct mi_timestamp *current_command_ts;
+ 
+ static int do_timings = 0;
+ 
  /* The token of the last asynchronous command */
  static char *last_async_command;
  static char *previous_async_command;
*************** static int get_register (int regnum, int
*** 103,108 ****
--- 113,123 ----
     layer that calls libgdb.  Any operation used in the below should be
     formalized. */
  
+ static void timestamp (struct mi_timestamp *tv);
+ 
+ static void print_diff_now (struct mi_timestamp *start);
+ static void print_diff (struct mi_timestamp *start, struct mi_timestamp *end);
+ 
  enum mi_cmd_result
  mi_cmd_gdb_exit (char *command, char **argv, int argc)
  {
*************** mi_cmd_exec_continue (char *args, int fr
*** 194,200 ****
  }
  
  /* Interrupt the execution of the target. Note how we must play around
!    with the token varialbes, in order to display the current token in
     the result of the interrupt command, and the previous execution
     token when the target finally stops. See comments in
     mi_cmd_execute. */
--- 209,215 ----
  }
  
  /* Interrupt the execution of the target. Note how we must play around
!    with the token variables, in order to display the current token in
     the result of the interrupt command, and the previous execution
     token when the target finally stops. See comments in
     mi_cmd_execute. */
*************** mi_cmd_data_write_register_values (char 
*** 564,575 ****
  
    format = (int) argv[0][0];
  
-   if (!target_has_registers)
-     {
-       mi_error_message = xstrprintf ("mi_cmd_data_write_register_values: No registers.");
-       return MI_CMD_ERROR;
-     }
- 
    if (!(argc - 1))
      {
        mi_error_message = xstrprintf ("mi_cmd_data_write_register_values: No regs and values specified.");
--- 579,584 ----
*************** mi_cmd_data_write_memory (char *command,
*** 1013,1018 ****
--- 1022,1051 ----
    return MI_CMD_DONE;
  }
  
+ enum mi_cmd_result
+ mi_cmd_enable_timings (char *command, char **argv, int argc)
+ {
+   if (argc == 0)
+     do_timings = 1;
+   else if (argc == 1)
+     {
+       if (strcmp (argv[0], "yes") == 0)
+ 	do_timings = 1;
+       else if (strcmp (argv[0], "no") == 0)
+ 	do_timings = 0;
+       else
+ 	goto usage_error;
+     }
+   else
+     goto usage_error;
+     
+   return MI_CMD_DONE;
+ 
+  usage_error:
+   error ("mi_cmd_enable_timings: Usage: %s {yes|no}", command);
+   return MI_CMD_ERROR;
+ }
+ 
  /* Execute a command within a safe environment.
     Return <0 for error; >=0 for ok.
  
*************** captured_mi_execute_command (struct ui_o
*** 1027,1032 ****
--- 1060,1067 ----
      (struct captured_mi_execute_command_args *) data;
    struct mi_parse *context = args->command;
  
+   struct mi_timestamp cmd_finished;
+ 
    switch (context->op)
      {
  
*************** captured_mi_execute_command (struct ui_o
*** 1041,1048 ****
--- 1076,1090 ----
           indication of what action is required and then switch on
           that. */
        args->action = EXECUTE_COMMAND_DISPLAY_PROMPT;
+ 
+       if (do_timings)
+ 	current_command_ts = context->cmd_start;
+ 
        args->rc = mi_cmd_execute (context);
  
+       if (do_timings)
+           timestamp (&cmd_finished);
+ 
        if (!target_can_async_p () || !target_executing)
  	{
  	  /* print the result if there were no errors
*************** captured_mi_execute_command (struct ui_o
*** 1057,1062 ****
--- 1099,1108 ----
  	      fputs_unfiltered ("^done", raw_stdout);
  	      mi_out_put (uiout, raw_stdout);
  	      mi_out_rewind (uiout);
+ 	      /* Have to check cmd_start, since the command could be
+ 		 -enable-timings. */
+ 	      if (do_timings && context->cmd_start)
+ 		  print_diff (context->cmd_start, &cmd_finished);
  	      fputs_unfiltered ("\n", raw_stdout);
  	    }
  	  else if (args->rc == MI_CMD_ERROR)
*************** mi_execute_command (char *cmd, int from_
*** 1152,1157 ****
--- 1198,1211 ----
    if (command != NULL)
      {
        struct gdb_exception result;
+ 
+       if (do_timings)
+ 	{
+ 	  command->cmd_start = (struct mi_timestamp *)
+ 	    xmalloc (sizeof (struct mi_timestamp));
+ 	  timestamp (command->cmd_start);
+ 	}
+ 
        /* FIXME: cagney/1999-11-04: Can this use of catch_exceptions either
           be pushed even further down or even eliminated? */
        args.command = command;
*************** mi_execute_async_cli_command (char *mi, 
*** 1339,1344 ****
--- 1393,1400 ----
        fputs_unfiltered ("*stopped", raw_stdout);
        mi_out_put (uiout, raw_stdout);
        mi_out_rewind (uiout);
+       if (do_timings)
+       	print_diff_now (current_command_ts);
        fputs_unfiltered ("\n", raw_stdout);
        return MI_CMD_QUIET;
      }
*************** _initialize_mi_main (void)
*** 1455,1457 ****
--- 1511,1559 ----
    DEPRECATED_REGISTER_GDBARCH_SWAP (old_regs);
    deprecated_register_gdbarch_swap (NULL, 0, mi_setup_architecture_data);
  }
+ 
+ static void 
+ timestamp (struct mi_timestamp *tv)
+   {
+     long usec;
+ #ifdef HAVE_GETRUSAGE
+     gettimeofday (&tv->wallclock, NULL);
+     getrusage (RUSAGE_SELF, &tv->rusage);
+ #else
+     usec = get_run_time ();
+     tv->wallclock.tv_sec = usec/1000000;
+     tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
+     tv->rusage.ru_utime.tv_sec = 0;
+     tv->rusage.ru_utime.tv_usec = 0;
+     tv->rusage.ru_stime.tv_sec = 0;
+     tv->rusage.ru_stime.tv_usec = 0;
+ #endif
+   }
+ 
+ static void 
+ print_diff_now (struct mi_timestamp *start)
+   {
+     struct mi_timestamp now;
+     timestamp (&now);
+     print_diff (start, &now);
+   }
+ 
+ static long 
+ timeval_diff (struct timeval start, struct timeval end)
+   {
+     return ((end.tv_sec - start.tv_sec) * 1000000) +
+       (end.tv_usec - start.tv_usec);
+   }
+ 
+ static void 
+ print_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+   {
+     fprintf_unfiltered
+       (raw_stdout,
+        ",time={wallclock=\"%0.5f\",user=\"%0.5f\",system=\"%0.5f\"}", 
+        timeval_diff (start->wallclock, end->wallclock) / 1000000.0, 
+        timeval_diff (start->rusage.ru_utime, end->rusage.ru_utime) /
+        1000000.0, 
+        timeval_diff (start->rusage.ru_stime, end->rusage.ru_stime) /
+        1000000.0);
+   }

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Tue, 2 Jan 2007 11:06:56 +1300
> Cc: mark.kettenis@xs4all.nl, ghost@cs.msu.su,
> 	gdb-patches@sources.redhat.com
> 
> OK, this patch gives wallclock, user and system time in the output.
> In the case when getrusage is not available it the output of get_run_time
> is used for the wallclock field and user and system time are given as 0.0.

Thanks.

> Can you please test it for this latter case?

Will do, but it could take me a few days before I have enough free
time to try the patch.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > > OK, this patch gives wallclock, user and system time in the output.
 > > In the case when getrusage is not available it the output of get_run_time
 > > is used for the wallclock field and user and system time are given as 0.0.
 > 
 > Thanks.
 > 
 > > Can you please test it for this latter case?
 > 
 > Will do, but it could take me a few days before I have enough free
 > time to try the patch.

Any progress?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Tue, 16 Jan 2007 18:57:17 +1300
> Cc: mark.kettenis@xs4all.nl, ghost@cs.msu.su,
> 	gdb-patches@sources.redhat.com
> 
>  > > OK, this patch gives wallclock, user and system time in the output.
>  > > In the case when getrusage is not available it the output of get_run_time
>  > > is used for the wallclock field and user and system time are given as 0.0.
>  > 
>  > Thanks.
>  > 
>  > > Can you please test it for this latter case?
>  > 
>  > Will do, but it could take me a few days before I have enough free
>  > time to try the patch.
> 
> Any progress?

Not yet, sorry.  I hoped to get it done this past weekend, but, as you
could read on gdb@, ended up wasting most of it on setting up the test
suite (which I need to test your patch anyway).  Hopefully, this
weekend I will finally get to your patch.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > > Any progress?
 > 
 > Not yet, sorry.  I hoped to get it done this past weekend, but, as you
 > could read on gdb@, ended up wasting most of it on setting up the test
 > suite (which I need to test your patch anyway).  Hopefully, this
 > weekend I will finally get to your patch.

I haven't written a test for this patch (yet ;-)).  I just thought that you
would run GDB with the patch, enable timing and look at the output of a few MI
commands.  I could force it to use get_run_time by changing the header files
but that might not be as convincing.  But there really is no urgency, enjoy
your weekend first.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Wed, 17 Jan 2007 11:29:50 +1300
> Cc: mark.kettenis@xs4all.nl, ghost@cs.msu.su,
> 	gdb-patches@sources.redhat.com
> 
> I haven't written a test for this patch (yet ;-)).

I know, but I still need to make sure that there are no regressions.

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Tue, 16 Jan 2007 18:57:17 +1300
> Cc: mark.kettenis@xs4all.nl, ghost@cs.msu.su,
> 	gdb-patches@sources.redhat.com
> 
>  > > OK, this patch gives wallclock, user and system time in the output.
>  > > In the case when getrusage is not available it the output of get_run_time
>  > > is used for the wallclock field and user and system time are given as 0.0.
>  > 
>  > Thanks.
>  > 
>  > > Can you please test it for this latter case?
>  > 
>  > Will do, but it could take me a few days before I have enough free
>  > time to try the patch.
> 
> Any progress?

I tried, but unfortunately, I cannot compile the patched version.  It
seems like at least one part of your patch was never sent to the list:

	* mi/mi-parse.h: Include <sys/resource.h> if present.
	(mi_timestamp): New structure.
	(mi_parse): Add mi_timestamp* member.

Without this, mi-main.c doesn't compile, because it misses the
definition of the mi_timestamp structure.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > > Any progress?
 > 
 > I tried, but unfortunately, I cannot compile the patched version.  It
 > seems like at least one part of your patch was never sent to the list:
 > 
 > 	* mi/mi-parse.h: Include <sys/resource.h> if present.
 > 	(mi_timestamp): New structure.
 > 	(mi_parse): Add mi_timestamp* member.
 > 
 > Without this, mi-main.c doesn't compile, because it misses the
 > definition of the mi_timestamp structure.

Yes, sorry.  It was part of an earlier patch but got left out somehow.


-- 
Nick                                           http://www.inet.net.nz/~nickrob


Index: mi/mi-parse.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.h,v
retrieving revision 1.5
diff -c -p -r1.5 mi-parse.h
*** mi/mi-parse.h	23 Dec 2005 18:57:46 -0000	1.5
--- mi/mi-parse.h	31 Dec 2006 07:39:14 -0000
***************
*** 24,29 ****
--- 24,37 ----
  
  /* MI parser */
  
+ #include <sys/resource.h>
+ 
+ /* Timestamps for current command and last asynchronous command */
+ struct mi_timestamp {
+     struct timeval wallclock;
+     struct rusage rusage;
+ };
+ 
  enum mi_command_type
    {
      MI_COMMAND, CLI_COMMAND
*************** struct mi_parse
*** 35,40 ****
--- 43,49 ----
      char *command;
      char *token;
      const struct mi_cmd *cmd;
+     struct mi_timestamp *cmd_start;
      char *args;
      char **argv;
      int argc;

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sun, 21 Jan 2007 09:58:18 +1300
> Cc: gdb-patches@sources.redhat.com
> 
>  > I tried, but unfortunately, I cannot compile the patched version.  It
>  > seems like at least one part of your patch was never sent to the list:
>  > 
>  > 	* mi/mi-parse.h: Include <sys/resource.h> if present.
>  > 	(mi_timestamp): New structure.
>  > 	(mi_parse): Add mi_timestamp* member.
>  > 
>  > Without this, mi-main.c doesn't compile, because it misses the
>  > definition of the mi_timestamp structure.
> 
> Yes, sorry.  It was part of an earlier patch but got left out somehow.

Okay, I tried this now, and on a system which has sys/resource.h and
getrusage, it works both with HAVE_SYS_RESOURCE_H and HAVE_GETRUSAGE
and without them (I hacked config.h to get it compiled both ways).

However, I don't see how it can work on systems without sys/resource.h
and without getrusage: your patch uses struct rusage unconditionally:

    mi-parse.h:

    + #include <sys/resource.h>
    + 
    + /* Timestamps for current command and last asynchronous command */
    + struct mi_timestamp {
    +     struct timeval wallclock;
    +     struct rusage rusage;
    + };
    + 

    m-main.c:

    + static void 
    + timestamp (struct mi_timestamp *tv)
    +   {
    +     long usec;
    + #ifdef HAVE_GETRUSAGE
    +     gettimeofday (&tv->wallclock, NULL);
    +     getrusage (RUSAGE_SELF, &tv->rusage);
    + #else
    +     usec = get_run_time ();
    +     tv->wallclock.tv_sec = usec/1000000;
    +     tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
    +     tv->rusage.ru_utime.tv_sec = 0;
    +     tv->rusage.ru_utime.tv_usec = 0;
    +     tv->rusage.ru_stime.tv_sec = 0;
    +     tv->rusage.ru_stime.tv_usec = 0;
    + #endif
    +   }


Note that mi-parse.h includes sys/resource.h unconditionally, which
will fail to compile on systems that don't have that header; and both
mi-parse.h and mi-main.c use struct rusage.

Also, in mi-main.c, there's no need to use gettimeofday only in
the HAVE_GETRUSAGE branch, as its availability is independent of
HAVE_GETRUSAGE.

I suggest to use gettimeofday to compute wallclock time on all
platforms, and avoid using struct rusage in struct mi_timestamp,
e.g. like this:

    struct mi_timestamp {
	struct timeval wallclock;
	struct timeval utime;
	struct timeval stime'
    }

Then in mi-main.c:timestamp you could copy the values from what
getrusage returns to the utime and stime members of mi_timestamp, when
getrusage is available, and if not, assign the value returned by
get_run_time to members of utime.

If you submit a modified patch that does the above, I will test it on
a platform that doesn't have getrusage.

TIA

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> Date: Sat, 27 Jan 2007 16:53:40 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: gdb-patches@sources.redhat.com
> 
>     m-main.c:
> 
>     + static void 
>     + timestamp (struct mi_timestamp *tv)
>     +   {
>     +     long usec;
>     + #ifdef HAVE_GETRUSAGE
>     +     gettimeofday (&tv->wallclock, NULL);
>     +     getrusage (RUSAGE_SELF, &tv->rusage);
>     + #else
>     +     usec = get_run_time ();
>     +     tv->wallclock.tv_sec = usec/1000000;
>     +     tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
>     +     tv->rusage.ru_utime.tv_sec = 0;
>     +     tv->rusage.ru_utime.tv_usec = 0;
>     +     tv->rusage.ru_stime.tv_sec = 0;
>     +     tv->rusage.ru_stime.tv_usec = 0;
>     + #endif
>     +   }

Also note that this line:

>     +     tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;

has a typo: it uses utv_sec instead of tv_usec.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > >     +     usec = get_run_time ();
 > >     +     tv->wallclock.tv_sec = usec/1000000;
 > >     +     tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
 > >     +     tv->rusage.ru_utime.tv_sec = 0;
 > >     +     tv->rusage.ru_utime.tv_usec = 0;
 > >     +     tv->rusage.ru_stime.tv_sec = 0;
 > >     +     tv->rusage.ru_stime.tv_usec = 0;
 > >     + #endif
 > >     +   }
 > 
 > Also note that this line:
 > 
 > >     +     tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
 > 
 > has a typo: it uses utv_sec instead of tv_usec.

I don't think so as get_run_time returns the time in microseconds e.g

usec = 1234567

tv->wallclock.tv_sec = 1234567/1000000 = 1
tv->wallclock.utv_sec = 1234567 - 1000000 = 234567

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Nick Roberts]

 >  > Also note that this line:
 >  > 
 >  > >     +     tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
 >  > 
 >  > has a typo: it uses utv_sec instead of tv_usec.
 > 
 > I don't think so as get_run_time returns the time in microseconds e.g
 > 
 > usec = 1234567
 > 
 > tv->wallclock.tv_sec = 1234567/1000000 = 1
 > tv->wallclock.utv_sec = 1234567 - 1000000 = 234567

Sorry, yes you're right.  I thought you were referring to my arithmetic.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > Okay, I tried this now, and on a system which has sys/resource.h and
 > getrusage, it works both with HAVE_SYS_RESOURCE_H and HAVE_GETRUSAGE
 > and without them (I hacked config.h to get it compiled both ways).

Hopefully you hacked config.h just to save time as my change to configure.ac
should handle that.

 > However, I don't see how it can work on systems without sys/resource.h
 > and without getrusage: your patch uses struct rusage unconditionally:
 > 
 >     mi-parse.h:
 > 
 >     + #include <sys/resource.h>
 >     + 
 >     + /* Timestamps for current command and last asynchronous command */
 >     + struct mi_timestamp {
 >     +     struct timeval wallclock;
 >     +     struct rusage rusage;
 >     + };
 >     + 
 >
 >     m-main.c:
 > 
 >     + static void 
 >     + timestamp (struct mi_timestamp *tv)
 >     +   {
 >     +     long usec;
 >     + #ifdef HAVE_GETRUSAGE
 >     +     gettimeofday (&tv->wallclock, NULL);
 >     +     getrusage (RUSAGE_SELF, &tv->rusage);
 >     + #else
 >     +     usec = get_run_time ();
 >     +     tv->wallclock.tv_sec = usec/1000000;
 >     +     tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
 >     +     tv->rusage.ru_utime.tv_sec = 0;
 >     +     tv->rusage.ru_utime.tv_usec = 0;
 >     +     tv->rusage.ru_stime.tv_sec = 0;
 >     +     tv->rusage.ru_stime.tv_usec = 0;
 >     + #endif
 >     +   }
 > 
 > 
 > Note that mi-parse.h includes sys/resource.h unconditionally, which
 > will fail to compile on systems that don't have that header; and both
 > mi-parse.h and mi-main.c use struct rusage.

Duh!  I see that now.  

 > Also, in mi-main.c, there's no need to use gettimeofday only in
 > the HAVE_GETRUSAGE branch, as its availability is independent of
 > HAVE_GETRUSAGE.

Well it was explained to me that get_run_time basically returns wallclock
time when getrusage isn't defined.  If that is the case, I could just
gettimeofday for both cases and never use get_run_time instead.

 > I suggest to use gettimeofday to compute wallclock time on all
 > platforms, and avoid using struct rusage in struct mi_timestamp,
 > e.g. like this:
 > 
 >     struct mi_timestamp {
 > 	struct timeval wallclock;
 > 	struct timeval utime;
 > 	struct timeval stime'
 >     }
 >
 > Then in mi-main.c:timestamp you could copy the values from what
 > getrusage returns to the utime and stime members of mi_timestamp, when
 > getrusage is available, and if not, assign the value returned by
 > get_run_time to members of utime.

Won't the members wallclock and utime be basically the same now?

 > If you submit a modified patch that does the above, I will test it on
 > a platform that doesn't have getrusage.

Would something like below work?  When getrusage isn't defined I just use
wallclock.  There's no need to define utime or stime in this case as the
values are always 0.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


*** mi-parse.h	28 Jan 2007 10:54:38 +1300	1.6
--- mi-parse.h	28 Jan 2007 10:39:02 +1300	
***************
*** 24,29 ****
--- 24,41 ----
  
  /* MI parser */
  
+ #if defined HAVE_SYS_RESOURCE_H
+ #include <sys/resource.h>
+ #endif
+ 
+ /* Timestamps for current command and last asynchronous command.  */
+ struct mi_timestamp {
+     struct timeval wallclock;
+ #ifdef HAVE_GETRUSAGE
+     struct rusage rusage;
+ #endif
+ };
+ 
  enum mi_command_type
    {
      MI_COMMAND, CLI_COMMAND
*************** struct mi_parse
*** 35,40 ****
--- 47,53 ----
      char *command;
      char *token;
      const struct mi_cmd *cmd;
+     struct mi_timestamp *cmd_start;
      char *args;
      char **argv;
      int argc;


*** mi-main.c	23 Jan 2007 20:21:56 +1300	1.91
--- mi-main.c	28 Jan 2007 10:44:28 +1300	
*************** _initialize_mi_main (void)
*** 1457,1459 ****
--- 1513,1562 ----
    DEPRECATED_REGISTER_GDBARCH_SWAP (old_regs);
    deprecated_register_gdbarch_swap (NULL, 0, mi_setup_architecture_data);
  }
+ 
+ static void 
+ timestamp (struct mi_timestamp *tv)
+   {
+     long usec;
+ #ifdef HAVE_GETRUSAGE
+     gettimeofday (&tv->wallclock, NULL);
+     getrusage (RUSAGE_SELF, &tv->rusage);
+ #else
+     usec = get_run_time ();
+     tv->wallclock.tv_sec = usec/1000000;
+     tv->wallclock.utv_sec = usec - 1000000*tv->wallclock.tv_sec;
+ #endif
+   }
+ 
+ static void 
+ print_diff_now (struct mi_timestamp *start)
+   {
+     struct mi_timestamp now;
+     timestamp (&now);
+     print_diff (start, &now);
+   }
+ 
+ static long 
+ timeval_diff (struct timeval start, struct timeval end)
+   {
+     return ((end.tv_sec - start.tv_sec) * 1000000)
+       + (end.tv_usec - start.tv_usec);
+   }
+ 
+ static void 
+ print_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+   {
+     fprintf_unfiltered
+       (raw_stdout,
+        ",time={wallclock=\"%0.5f\",user=\"%0.5f\",system=\"%0.5f\"}", 
+        timeval_diff (start->wallclock, end->wallclock) / 1000000.0, 
+ #ifdef HAVE_GETRUSAGE
+        timeval_diff (start->rusage.ru_utime, end->rusage.ru_utime)
+        / 1000000.0, 
+        timeval_diff (start->rusage.ru_stime, end->rusage.ru_stime)
+        / 1000000.0
+ #else
+        0, 0
+ #endif
+        );
+   }

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sun, 28 Jan 2007 11:08:45 +1300
> Cc: gdb-patches@sources.redhat.com
> 
> Well it was explained to me that get_run_time basically returns wallclock
> time when getrusage isn't defined.

Not necessarily.  Take a look at getruntime.c: it can use `times' when
that is available.  `times' returns the sum of user and system times,
not the wallclock time.

>  > I suggest to use gettimeofday to compute wallclock time on all
>  > platforms, and avoid using struct rusage in struct mi_timestamp,
>  > e.g. like this:
>  > 
>  >     struct mi_timestamp {
>  > 	struct timeval wallclock;
>  > 	struct timeval utime;
>  > 	struct timeval stime'
>  >     }
>  >
>  > Then in mi-main.c:timestamp you could copy the values from what
>  > getrusage returns to the utime and stime members of mi_timestamp, when
>  > getrusage is available, and if not, assign the value returned by
>  > get_run_time to members of utime.
> 
> Won't the members wallclock and utime be basically the same now?

Not if `times' is used in get_run_time.

> Would something like below work?  When getrusage isn't defined I just use
> wallclock.  There's no need to define utime or stime in this case as the
> values are always 0.

That would work, but I think we shouldn't lose the important case of
platforms which have `times'.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > > Well it was explained to me that get_run_time basically returns wallclock
 > > time when getrusage isn't defined.
 > 
 > Not necessarily.  Take a look at getruntime.c: it can use `times' when
 > that is available.  `times' returns the sum of user and system times,
 > not the wallclock time.

I don't recollect this point being made earlier but anyway I've tried to make
the necessary changes below.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


Other changes as before.  In particular mi-main.c is a pseudo diff as other
previous changes aren't shown.


*** mi-parse.h	28 Jan 2007 10:54:38 +1300	1.6
--- mi-parse.h	28 Jan 2007 18:18:46 +1300	
***************
*** 24,29 ****
--- 24,36 ----
  
  /* MI parser */
  
+ /* Timestamps for current command and last asynchronous command.  */
+ struct mi_timestamp {
+   struct timeval wallclock;
+   struct timeval utime;
+   struct timeval stime;
+ };
+ 
  enum mi_command_type
    {
      MI_COMMAND, CLI_COMMAND
*************** struct mi_parse
*** 35,40 ****
--- 42,48 ----
      char *command;
      char *token;
      const struct mi_cmd *cmd;
+     struct mi_timestamp *cmd_start;
      char *args;
      char **argv;
      int argc;


*** mi-main.c	23 Jan 2007 20:21:56 +1300	1.91
--- mi-main.c	28 Jan 2007 18:15:17 +1300	
*************** _initialize_mi_main (void)
*** 1457,1459 ****
--- 1517,1565 ----
    DEPRECATED_REGISTER_GDBARCH_SWAP (old_regs);
    deprecated_register_gdbarch_swap (NULL, 0, mi_setup_architecture_data);
  }
+ 
+ static void 
+ timestamp (struct mi_timestamp *tv)
+   {
+     long usec;
+     gettimeofday (&tv->wallclock, NULL);
+ #ifdef HAVE_GETRUSAGE
+     getrusage (RUSAGE_SELF, &rusage);
+     tv->utime.tv_sec = rusage.ru_utime.tv_sec;
+     tv->utime.tv_usec = rusage.ru_utime.tv_usec;
+     tv->stime.tv_sec = rusage.ru_stime.tv_sec;
+     tv->stime.tv_usec = rusage.ru_stime.tv_usec;
+ #else
+     usec = get_run_time ();
+     tv->utime.tv_sec = usec/1000000;
+     tv->utime.tv_usec = usec - 1000000*tv->utime.tv_sec;
+     tv->stime.tv_sec = 0;
+     tv->stime.tv_usec = 0;
+ #endif
+   }
+ 
+ static void 
+ print_diff_now (struct mi_timestamp *start)
+   {
+     struct mi_timestamp now;
+     timestamp (&now);
+     print_diff (start, &now);
+   }
+ 
+ static long 
+ timeval_diff (struct timeval start, struct timeval end)
+   {
+     return ((end.tv_sec - start.tv_sec) * 1000000)
+       + (end.tv_usec - start.tv_usec);
+   }
+ 
+ static void 
+ print_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+   {
+     fprintf_unfiltered
+       (raw_stdout,
+        ",time={wallclock=\"%0.5f\",user=\"%0.5f\",system=\"%0.5f\"}", 
+        timeval_diff (start->wallclock, end->wallclock) / 1000000.0, 
+        timeval_diff (start->utime, end->utime) / 1000000.0, 
+        timeval_diff (start->stime, end->stime) / 1000000.0);
+   }

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sun, 28 Jan 2007 18:30:51 +1300
> Cc: gdb-patches@sources.redhat.com
> 
>  > > Well it was explained to me that get_run_time basically returns wallclock
>  > > time when getrusage isn't defined.
>  > 
>  > Not necessarily.  Take a look at getruntime.c: it can use `times' when
>  > that is available.  `times' returns the sum of user and system times,
>  > not the wallclock time.
> 
> I don't recollect this point being made earlier but anyway I've tried to make
> the necessary changes below.

This is okay with me, but please use 1000000L rather than just 1000000
here:

> +     tv->utime.tv_usec = usec - 1000000*tv->utime.tv_sec;

and here:

> + static long 
> + timeval_diff (struct timeval start, struct timeval end)
> +   {
> +     return ((end.tv_sec - start.tv_sec) * 1000000)
> +       + (end.tv_usec - start.tv_usec);
> +   }

This is to avoid a possible overflow of a 32-bit int on systems where
tv_sec is a 32-bit type.

Other than that, this patch can go in, but please show here the exact
patch you commit, since you didn't show it in the message to which I'm
replying.

Thanks.

Re: [PATCH] MI: new timing command

[Nick Roberts]

[-- Attachment #1: message body and .signature --]
[-- Type: text/plain, Size: 2595 bytes --]

 > This is okay with me, but please use 1000000L rather than just 1000000
 > here:
 > 
 > > +     tv->utime.tv_usec = usec - 1000000*tv->utime.tv_sec;
 > 
 > and here:
 >
 > > + static long 
 > > + timeval_diff (struct timeval start, struct timeval end)
 > > +   {
 > > +     return ((end.tv_sec - start.tv_sec) * 1000000)
 > > +       + (end.tv_usec - start.tv_usec);
 > > +   }
 > 
 > This is to avoid a possible overflow of a 32-bit int on systems where
 > tv_sec is a 32-bit type.

I was so immersed in trying to commit my changes correctly that I missed
this so I had to do it retrospectively.

 > Other than that, this patch can go in, but please show here the exact
 > patch you commit, since you didn't show it in the message to which I'm
 > replying.

I am sending it as an attachment with your change listed separately.  Maybe
I've made mistakes (I've not made changes to configury before) but I'll
be available to correct it if necessary.

Thanks for your help.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


*** mi-main.c	03 Feb 2007 11:31:46 +1300	1.92
--- mi-main.c	03 Feb 2007 12:17:14 +1300	
*************** mi_load_progress (const char *section_na
*** 1461,1467 ****
    if (delta.tv_usec < 0)
      {
        delta.tv_sec -= 1;
!       delta.tv_usec += 1000000;
      }
  
    new_section = (previous_sect_name ?
--- 1461,1467 ----
    if (delta.tv_usec < 0)
      {
        delta.tv_sec -= 1;
!       delta.tv_usec += 1000000L;
      }
  
    new_section = (previous_sect_name ?
*************** timestamp (struct mi_timestamp *tv)
*** 1537,1544 ****
      tv->stime.tv_usec = rusage.ru_stime.tv_usec;
  #else
      usec = get_run_time ();
!     tv->utime.tv_sec = usec/1000000;
!     tv->utime.tv_usec = usec - 1000000*tv->utime.tv_sec;
      tv->stime.tv_sec = 0;
      tv->stime.tv_usec = 0;
  #endif
--- 1537,1544 ----
      tv->stime.tv_usec = rusage.ru_stime.tv_usec;
  #else
      usec = get_run_time ();
!     tv->utime.tv_sec = usec/1000000L;
!     tv->utime.tv_usec = usec - 1000000L*tv->utime.tv_sec;
      tv->stime.tv_sec = 0;
      tv->stime.tv_usec = 0;
  #endif
*************** print_diff_now (struct mi_timestamp *sta
*** 1555,1561 ****
  static long 
  timeval_diff (struct timeval start, struct timeval end)
    {
!     return ((end.tv_sec - start.tv_sec) * 1000000)
        + (end.tv_usec - start.tv_usec);
    }
  
--- 1555,1561 ----
  static long 
  timeval_diff (struct timeval start, struct timeval end)
    {
!     return ((end.tv_sec - start.tv_sec) * 1000000L)
        + (end.tv_usec - start.tv_usec);
    }
  

[-- Attachment #2: timing patch --]
[-- Type: application/octet-stream, Size: 19829 bytes --]

Index: configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.41
diff -c -p -r1.41 configure.ac
*** configure.ac	9 Jan 2007 21:34:29 -0000	1.41
--- configure.ac	2 Feb 2007 22:42:59 -0000
*************** AC_CHECK_HEADERS(sys/file.h)
*** 376,381 ****
--- 376,382 ----
  AC_CHECK_HEADERS(sys/filio.h)
  AC_CHECK_HEADERS(sys/ioctl.h)
  AC_CHECK_HEADERS(sys/param.h)
+ AC_CHECK_HEADERS(sys/resource.h)
  AC_CHECK_HEADERS(sys/proc.h, [], [],
  [#if HAVE_SYS_PARAM_H
  # include <sys/param.h>
*************** AC_FUNC_ALLOCA
*** 463,468 ****
--- 464,470 ----
  AC_FUNC_MMAP
  AC_FUNC_VFORK
  AC_CHECK_FUNCS(canonicalize_file_name realpath)
+ AC_CHECK_FUNCS(getrusage)
  AC_CHECK_FUNCS(getuid getgid)
  AC_CHECK_FUNCS(poll)
  AC_CHECK_FUNCS(pread64)
Index: configure
===================================================================
RCS file: /cvs/src/src/gdb/configure,v
retrieving revision 1.220
diff -c -p -r1.220 configure
*** configure	9 Jan 2007 21:34:30 -0000	1.220
--- configure	2 Feb 2007 22:43:47 -0000
*************** fi
*** 9061,9066 ****
--- 9061,9216 ----
  done
  
  
+ for ac_header in sys/resource.h
+ do
+ as_ac_Header=`echo "ac_cv_header_$ac_header" | $as_tr_sh`
+ if eval "test \"\${$as_ac_Header+set}\" = set"; then
+   echo "$as_me:$LINENO: checking for $ac_header" >&5
+ echo $ECHO_N "checking for $ac_header... $ECHO_C" >&6
+ if eval "test \"\${$as_ac_Header+set}\" = set"; then
+   echo $ECHO_N "(cached) $ECHO_C" >&6
+ fi
+ echo "$as_me:$LINENO: result: `eval echo '${'$as_ac_Header'}'`" >&5
+ echo "${ECHO_T}`eval echo '${'$as_ac_Header'}'`" >&6
+ else
+   # Is the header compilable?
+ echo "$as_me:$LINENO: checking $ac_header usability" >&5
+ echo $ECHO_N "checking $ac_header usability... $ECHO_C" >&6
+ cat >conftest.$ac_ext <<_ACEOF
+ /* confdefs.h.  */
+ _ACEOF
+ cat confdefs.h >>conftest.$ac_ext
+ cat >>conftest.$ac_ext <<_ACEOF
+ /* end confdefs.h.  */
+ $ac_includes_default
+ #include <$ac_header>
+ _ACEOF
+ rm -f conftest.$ac_objext
+ if { (eval echo "$as_me:$LINENO: \"$ac_compile\"") >&5
+   (eval $ac_compile) 2>conftest.er1
+   ac_status=$?
+   grep -v '^ *+' conftest.er1 >conftest.err
+   rm -f conftest.er1
+   cat conftest.err >&5
+   echo "$as_me:$LINENO: \$? = $ac_status" >&5
+   (exit $ac_status); } &&
+ 	 { ac_try='test -z "$ac_c_werror_flag"
+ 			 || test ! -s conftest.err'
+   { (eval echo "$as_me:$LINENO: \"$ac_try\"") >&5
+   (eval $ac_try) 2>&5
+   ac_status=$?
+   echo "$as_me:$LINENO: \$? = $ac_status" >&5
+   (exit $ac_status); }; } &&
+ 	 { ac_try='test -s conftest.$ac_objext'
+   { (eval echo "$as_me:$LINENO: \"$ac_try\"") >&5
+   (eval $ac_try) 2>&5
+   ac_status=$?
+   echo "$as_me:$LINENO: \$? = $ac_status" >&5
+   (exit $ac_status); }; }; then
+   ac_header_compiler=yes
+ else
+   echo "$as_me: failed program was:" >&5
+ sed 's/^/| /' conftest.$ac_ext >&5
+ 
+ ac_header_compiler=no
+ fi
+ rm -f conftest.err conftest.$ac_objext conftest.$ac_ext
+ echo "$as_me:$LINENO: result: $ac_header_compiler" >&5
+ echo "${ECHO_T}$ac_header_compiler" >&6
+ 
+ # Is the header present?
+ echo "$as_me:$LINENO: checking $ac_header presence" >&5
+ echo $ECHO_N "checking $ac_header presence... $ECHO_C" >&6
+ cat >conftest.$ac_ext <<_ACEOF
+ /* confdefs.h.  */
+ _ACEOF
+ cat confdefs.h >>conftest.$ac_ext
+ cat >>conftest.$ac_ext <<_ACEOF
+ /* end confdefs.h.  */
+ #include <$ac_header>
+ _ACEOF
+ if { (eval echo "$as_me:$LINENO: \"$ac_cpp conftest.$ac_ext\"") >&5
+   (eval $ac_cpp conftest.$ac_ext) 2>conftest.er1
+   ac_status=$?
+   grep -v '^ *+' conftest.er1 >conftest.err
+   rm -f conftest.er1
+   cat conftest.err >&5
+   echo "$as_me:$LINENO: \$? = $ac_status" >&5
+   (exit $ac_status); } >/dev/null; then
+   if test -s conftest.err; then
+     ac_cpp_err=$ac_c_preproc_warn_flag
+     ac_cpp_err=$ac_cpp_err$ac_c_werror_flag
+   else
+     ac_cpp_err=
+   fi
+ else
+   ac_cpp_err=yes
+ fi
+ if test -z "$ac_cpp_err"; then
+   ac_header_preproc=yes
+ else
+   echo "$as_me: failed program was:" >&5
+ sed 's/^/| /' conftest.$ac_ext >&5
+ 
+   ac_header_preproc=no
+ fi
+ rm -f conftest.err conftest.$ac_ext
+ echo "$as_me:$LINENO: result: $ac_header_preproc" >&5
+ echo "${ECHO_T}$ac_header_preproc" >&6
+ 
+ # So?  What about this header?
+ case $ac_header_compiler:$ac_header_preproc:$ac_c_preproc_warn_flag in
+   yes:no: )
+     { echo "$as_me:$LINENO: WARNING: $ac_header: accepted by the compiler, rejected by the preprocessor!" >&5
+ echo "$as_me: WARNING: $ac_header: accepted by the compiler, rejected by the preprocessor!" >&2;}
+     { echo "$as_me:$LINENO: WARNING: $ac_header: proceeding with the compiler's result" >&5
+ echo "$as_me: WARNING: $ac_header: proceeding with the compiler's result" >&2;}
+     ac_header_preproc=yes
+     ;;
+   no:yes:* )
+     { echo "$as_me:$LINENO: WARNING: $ac_header: present but cannot be compiled" >&5
+ echo "$as_me: WARNING: $ac_header: present but cannot be compiled" >&2;}
+     { echo "$as_me:$LINENO: WARNING: $ac_header:     check for missing prerequisite headers?" >&5
+ echo "$as_me: WARNING: $ac_header:     check for missing prerequisite headers?" >&2;}
+     { echo "$as_me:$LINENO: WARNING: $ac_header: see the Autoconf documentation" >&5
+ echo "$as_me: WARNING: $ac_header: see the Autoconf documentation" >&2;}
+     { echo "$as_me:$LINENO: WARNING: $ac_header:     section \"Present But Cannot Be Compiled\"" >&5
+ echo "$as_me: WARNING: $ac_header:     section \"Present But Cannot Be Compiled\"" >&2;}
+     { echo "$as_me:$LINENO: WARNING: $ac_header: proceeding with the preprocessor's result" >&5
+ echo "$as_me: WARNING: $ac_header: proceeding with the preprocessor's result" >&2;}
+     { echo "$as_me:$LINENO: WARNING: $ac_header: in the future, the compiler will take precedence" >&5
+ echo "$as_me: WARNING: $ac_header: in the future, the compiler will take precedence" >&2;}
+     (
+       cat <<\_ASBOX
+ ## ------------------------------------------ ##
+ ## Report this to the AC_PACKAGE_NAME lists.  ##
+ ## ------------------------------------------ ##
+ _ASBOX
+     ) |
+       sed "s/^/$as_me: WARNING:     /" >&2
+     ;;
+ esac
+ echo "$as_me:$LINENO: checking for $ac_header" >&5
+ echo $ECHO_N "checking for $ac_header... $ECHO_C" >&6
+ if eval "test \"\${$as_ac_Header+set}\" = set"; then
+   echo $ECHO_N "(cached) $ECHO_C" >&6
+ else
+   eval "$as_ac_Header=\$ac_header_preproc"
+ fi
+ echo "$as_me:$LINENO: result: `eval echo '${'$as_ac_Header'}'`" >&5
+ echo "${ECHO_T}`eval echo '${'$as_ac_Header'}'`" >&6
+ 
+ fi
+ if test `eval echo '${'$as_ac_Header'}'` = yes; then
+   cat >>confdefs.h <<_ACEOF
+ #define `echo "HAVE_$ac_header" | $as_tr_cpp` 1
+ _ACEOF
+ 
+ fi
+ 
+ done
+ 
+ 
  for ac_header in sys/proc.h
  do
  as_ac_Header=`echo "ac_cv_header_$ac_header" | $as_tr_sh`
*************** fi
*** 16561,16566 ****
--- 16711,16818 ----
  done
  
  
+ for ac_func in getrusage
+ do
+ as_ac_var=`echo "ac_cv_func_$ac_func" | $as_tr_sh`
+ echo "$as_me:$LINENO: checking for $ac_func" >&5
+ echo $ECHO_N "checking for $ac_func... $ECHO_C" >&6
+ if eval "test \"\${$as_ac_var+set}\" = set"; then
+   echo $ECHO_N "(cached) $ECHO_C" >&6
+ else
+   cat >conftest.$ac_ext <<_ACEOF
+ /* confdefs.h.  */
+ _ACEOF
+ cat confdefs.h >>conftest.$ac_ext
+ cat >>conftest.$ac_ext <<_ACEOF
+ /* end confdefs.h.  */
+ /* Define $ac_func to an innocuous variant, in case <limits.h> declares $ac_func.
+    For example, HP-UX 11i <limits.h> declares gettimeofday.  */
+ #define $ac_func innocuous_$ac_func
+ 
+ /* System header to define __stub macros and hopefully few prototypes,
+     which can conflict with char $ac_func (); below.
+     Prefer <limits.h> to <assert.h> if __STDC__ is defined, since
+     <limits.h> exists even on freestanding compilers.  */
+ 
+ #ifdef __STDC__
+ # include <limits.h>
+ #else
+ # include <assert.h>
+ #endif
+ 
+ #undef $ac_func
+ 
+ /* Override any gcc2 internal prototype to avoid an error.  */
+ #ifdef __cplusplus
+ extern "C"
+ {
+ #endif
+ /* We use char because int might match the return type of a gcc2
+    builtin and then its argument prototype would still apply.  */
+ char $ac_func ();
+ /* The GNU C library defines this for functions which it implements
+     to always fail with ENOSYS.  Some functions are actually named
+     something starting with __ and the normal name is an alias.  */
+ #if defined (__stub_$ac_func) || defined (__stub___$ac_func)
+ choke me
+ #else
+ char (*f) () = $ac_func;
+ #endif
+ #ifdef __cplusplus
+ }
+ #endif
+ 
+ int
+ main ()
+ {
+ return f != $ac_func;
+   ;
+   return 0;
+ }
+ _ACEOF
+ rm -f conftest.$ac_objext conftest$ac_exeext
+ if { (eval echo "$as_me:$LINENO: \"$ac_link\"") >&5
+   (eval $ac_link) 2>conftest.er1
+   ac_status=$?
+   grep -v '^ *+' conftest.er1 >conftest.err
+   rm -f conftest.er1
+   cat conftest.err >&5
+   echo "$as_me:$LINENO: \$? = $ac_status" >&5
+   (exit $ac_status); } &&
+ 	 { ac_try='test -z "$ac_c_werror_flag"
+ 			 || test ! -s conftest.err'
+   { (eval echo "$as_me:$LINENO: \"$ac_try\"") >&5
+   (eval $ac_try) 2>&5
+   ac_status=$?
+   echo "$as_me:$LINENO: \$? = $ac_status" >&5
+   (exit $ac_status); }; } &&
+ 	 { ac_try='test -s conftest$ac_exeext'
+   { (eval echo "$as_me:$LINENO: \"$ac_try\"") >&5
+   (eval $ac_try) 2>&5
+   ac_status=$?
+   echo "$as_me:$LINENO: \$? = $ac_status" >&5
+   (exit $ac_status); }; }; then
+   eval "$as_ac_var=yes"
+ else
+   echo "$as_me: failed program was:" >&5
+ sed 's/^/| /' conftest.$ac_ext >&5
+ 
+ eval "$as_ac_var=no"
+ fi
+ rm -f conftest.err conftest.$ac_objext \
+       conftest$ac_exeext conftest.$ac_ext
+ fi
+ echo "$as_me:$LINENO: result: `eval echo '${'$as_ac_var'}'`" >&5
+ echo "${ECHO_T}`eval echo '${'$as_ac_var'}'`" >&6
+ if test `eval echo '${'$as_ac_var'}'` = yes; then
+   cat >>confdefs.h <<_ACEOF
+ #define `echo "HAVE_$ac_func" | $as_tr_cpp` 1
+ _ACEOF
+ 
+ fi
+ done
+ 
+ 
  
  for ac_func in getuid getgid
  do
Index: config.in
===================================================================
RCS file: /cvs/src/src/gdb/config.in,v
retrieving revision 1.87
diff -c -p -r1.87 config.in
*** config.in	9 Jan 2007 21:34:30 -0000	1.87
--- config.in	2 Feb 2007 22:43:49 -0000
***************
*** 122,127 ****
--- 122,130 ----
  /* Define to 1 if you have the `getpagesize' function. */
  #undef HAVE_GETPAGESIZE
  
+ /* Define to 1 if you have the `getrusage' function. */
+ #undef HAVE_GETRUSAGE
+ 
  /* Define to 1 if you have the `getuid' function. */
  #undef HAVE_GETUID
  
***************
*** 388,393 ****
--- 391,399 ----
  /* Define to 1 if you have the <sys/reg.h> header file. */
  #undef HAVE_SYS_REG_H
  
+ /* Define to 1 if you have the <sys/resource.h> header file. */
+ #undef HAVE_SYS_RESOURCE_H
+ 
  /* Define to 1 if you have the <sys/select.h> header file. */
  #undef HAVE_SYS_SELECT_H
  
Index: mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.91
diff -c -p -r1.91 mi-main.c
*** mi/mi-main.c	23 Jan 2007 20:27:58 -0000	1.91
--- mi/mi-main.c	2 Feb 2007 22:43:51 -0000
***************
*** 50,55 ****
--- 50,63 ----
  #include <ctype.h>
  #include <sys/time.h>
  
+ #if defined HAVE_SYS_RESOURCE_H
+ #include <sys/resource.h>
+ #endif
+ 
+ #ifdef HAVE_GETRUSAGE
+ struct rusage rusage;
+ #endif
+ 
  enum
    {
      FROM_TTY = 0
*************** struct captured_mi_execute_command_args
*** 81,86 ****
--- 89,100 ----
  int mi_debug_p;
  struct ui_file *raw_stdout;
  
+ /* This is used to pass the current command timestamp
+    down to continuation routines.  */
+ static struct mi_timestamp *current_command_ts;
+ 
+ static int do_timings = 0;
+ 
  /* The token of the last asynchronous command */
  static char *last_async_command;
  static char *previous_async_command;
*************** static int get_register (int regnum, int
*** 103,108 ****
--- 117,127 ----
     layer that calls libgdb.  Any operation used in the below should be
     formalized. */
  
+ static void timestamp (struct mi_timestamp *tv);
+ 
+ static void print_diff_now (struct mi_timestamp *start);
+ static void print_diff (struct mi_timestamp *start, struct mi_timestamp *end);
+ 
  enum mi_cmd_result
  mi_cmd_gdb_exit (char *command, char **argv, int argc)
  {
*************** mi_cmd_exec_continue (char *args, int fr
*** 194,200 ****
  }
  
  /* Interrupt the execution of the target. Note how we must play around
!    with the token varialbes, in order to display the current token in
     the result of the interrupt command, and the previous execution
     token when the target finally stops. See comments in
     mi_cmd_execute. */
--- 213,219 ----
  }
  
  /* Interrupt the execution of the target. Note how we must play around
!    with the token variables, in order to display the current token in
     the result of the interrupt command, and the previous execution
     token when the target finally stops. See comments in
     mi_cmd_execute. */
*************** mi_cmd_data_write_memory (char *command,
*** 1013,1018 ****
--- 1032,1061 ----
    return MI_CMD_DONE;
  }
  
+ enum mi_cmd_result
+ mi_cmd_enable_timings (char *command, char **argv, int argc)
+ {
+   if (argc == 0)
+     do_timings = 1;
+   else if (argc == 1)
+     {
+       if (strcmp (argv[0], "yes") == 0)
+ 	do_timings = 1;
+       else if (strcmp (argv[0], "no") == 0)
+ 	do_timings = 0;
+       else
+ 	goto usage_error;
+     }
+   else
+     goto usage_error;
+     
+   return MI_CMD_DONE;
+ 
+  usage_error:
+   error ("mi_cmd_enable_timings: Usage: %s {yes|no}", command);
+   return MI_CMD_ERROR;
+ }
+ 
  /* Execute a command within a safe environment.
     Return <0 for error; >=0 for ok.
  
*************** captured_mi_execute_command (struct ui_o
*** 1027,1032 ****
--- 1070,1077 ----
      (struct captured_mi_execute_command_args *) data;
    struct mi_parse *context = args->command;
  
+   struct mi_timestamp cmd_finished;
+ 
    switch (context->op)
      {
  
*************** captured_mi_execute_command (struct ui_o
*** 1041,1048 ****
--- 1086,1100 ----
           indication of what action is required and then switch on
           that. */
        args->action = EXECUTE_COMMAND_DISPLAY_PROMPT;
+ 
+       if (do_timings)
+ 	current_command_ts = context->cmd_start;
+ 
        args->rc = mi_cmd_execute (context);
  
+       if (do_timings)
+           timestamp (&cmd_finished);
+ 
        if (!target_can_async_p () || !target_executing)
  	{
  	  /* print the result if there were no errors
*************** captured_mi_execute_command (struct ui_o
*** 1057,1062 ****
--- 1109,1118 ----
  	      fputs_unfiltered ("^done", raw_stdout);
  	      mi_out_put (uiout, raw_stdout);
  	      mi_out_rewind (uiout);
+ 	      /* Have to check cmd_start, since the command could be
+ 		 -enable-timings.  */
+ 	      if (do_timings && context->cmd_start)
+ 		  print_diff (context->cmd_start, &cmd_finished);
  	      fputs_unfiltered ("\n", raw_stdout);
  	    }
  	  else if (args->rc == MI_CMD_ERROR)
*************** mi_execute_command (char *cmd, int from_
*** 1152,1157 ****
--- 1208,1221 ----
    if (command != NULL)
      {
        struct gdb_exception result;
+ 
+       if (do_timings)
+ 	{
+ 	  command->cmd_start = (struct mi_timestamp *)
+ 	    xmalloc (sizeof (struct mi_timestamp));
+ 	  timestamp (command->cmd_start);
+ 	}
+ 
        /* FIXME: cagney/1999-11-04: Can this use of catch_exceptions either
           be pushed even further down or even eliminated? */
        args.command = command;
*************** mi_execute_async_cli_command (char *mi, 
*** 1341,1346 ****
--- 1405,1412 ----
        fputs_unfiltered ("*stopped", raw_stdout);
        mi_out_put (uiout, raw_stdout);
        mi_out_rewind (uiout);
+       if (do_timings)
+       	print_diff_now (current_command_ts);
        fputs_unfiltered ("\n", raw_stdout);
        return MI_CMD_QUIET;
      }
*************** _initialize_mi_main (void)
*** 1457,1459 ****
--- 1523,1571 ----
    DEPRECATED_REGISTER_GDBARCH_SWAP (old_regs);
    deprecated_register_gdbarch_swap (NULL, 0, mi_setup_architecture_data);
  }
+ 
+ static void 
+ timestamp (struct mi_timestamp *tv)
+   {
+     long usec;
+     gettimeofday (&tv->wallclock, NULL);
+ #ifdef HAVE_GETRUSAGE
+     getrusage (RUSAGE_SELF, &rusage);
+     tv->utime.tv_sec = rusage.ru_utime.tv_sec;
+     tv->utime.tv_usec = rusage.ru_utime.tv_usec;
+     tv->stime.tv_sec = rusage.ru_stime.tv_sec;
+     tv->stime.tv_usec = rusage.ru_stime.tv_usec;
+ #else
+     usec = get_run_time ();
+     tv->utime.tv_sec = usec/1000000;
+     tv->utime.tv_usec = usec - 1000000*tv->utime.tv_sec;
+     tv->stime.tv_sec = 0;
+     tv->stime.tv_usec = 0;
+ #endif
+   }
+ 
+ static void 
+ print_diff_now (struct mi_timestamp *start)
+   {
+     struct mi_timestamp now;
+     timestamp (&now);
+     print_diff (start, &now);
+   }
+ 
+ static long 
+ timeval_diff (struct timeval start, struct timeval end)
+   {
+     return ((end.tv_sec - start.tv_sec) * 1000000)
+       + (end.tv_usec - start.tv_usec);
+   }
+ 
+ static void 
+ print_diff (struct mi_timestamp *start, struct mi_timestamp *end)
+   {
+     fprintf_unfiltered
+       (raw_stdout,
+        ",time={wallclock=\"%0.5f\",user=\"%0.5f\",system=\"%0.5f\"}", 
+        timeval_diff (start->wallclock, end->wallclock) / 1000000.0, 
+        timeval_diff (start->utime, end->utime) / 1000000.0, 
+        timeval_diff (start->stime, end->stime) / 1000000.0);
+   }
Index: mi/mi-cmds.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmds.c,v
retrieving revision 1.23
diff -c -p -r1.23 mi-cmds.c
*** mi/mi-cmds.c	9 Jan 2007 17:59:08 -0000	1.23
--- mi/mi-cmds.c	2 Feb 2007 22:43:52 -0000
*************** struct mi_cmd mi_cmds[] =
*** 53,58 ****
--- 53,59 ----
    { "data-read-memory", { NULL, 0 }, 0, mi_cmd_data_read_memory},
    { "data-write-memory", { NULL, 0 }, 0, mi_cmd_data_write_memory},
    { "data-write-register-values", { NULL, 0 }, 0, mi_cmd_data_write_register_values},
+   { "enable-timings", { NULL, 0 }, 0, mi_cmd_enable_timings},
    { "environment-cd", { NULL, 0 }, 0, mi_cmd_env_cd},
    { "environment-directory", { NULL, 0 }, 0, mi_cmd_env_dir},
    { "environment-path", { NULL, 0 }, 0, mi_cmd_env_path},
Index: mi/mi-cmds.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmds.h,v
retrieving revision 1.20
diff -c -p -r1.20 mi-cmds.h
*** mi/mi-cmds.h	9 Jan 2007 17:59:08 -0000	1.20
--- mi/mi-cmds.h	2 Feb 2007 22:43:52 -0000
*************** extern mi_cmd_argv_ftype mi_cmd_data_lis
*** 72,77 ****
--- 72,78 ----
  extern mi_cmd_argv_ftype mi_cmd_data_read_memory;
  extern mi_cmd_argv_ftype mi_cmd_data_write_memory;
  extern mi_cmd_argv_ftype mi_cmd_data_write_register_values;
+ extern mi_cmd_argv_ftype mi_cmd_enable_timings;
  extern mi_cmd_argv_ftype mi_cmd_env_cd;
  extern mi_cmd_argv_ftype mi_cmd_env_dir;
  extern mi_cmd_argv_ftype mi_cmd_env_path;
Index: mi/mi-parse.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-parse.h,v
retrieving revision 1.6
diff -c -p -r1.6 mi-parse.h
*** mi/mi-parse.h	9 Jan 2007 17:59:08 -0000	1.6
--- mi/mi-parse.h	2 Feb 2007 22:43:52 -0000
***************
*** 24,29 ****
--- 24,36 ----
  
  /* MI parser */
  
+ /* Timestamps for current command and last asynchronous command.  */
+ struct mi_timestamp {
+   struct timeval wallclock;
+   struct timeval utime;
+   struct timeval stime;
+ };
+ 
  enum mi_command_type
    {
      MI_COMMAND, CLI_COMMAND
*************** struct mi_parse
*** 35,40 ****
--- 42,48 ----
      char *command;
      char *token;
      const struct mi_cmd *cmd;
+     struct mi_timestamp *cmd_start;
      char *args;
      char **argv;
      int argc;

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sat, 3 Feb 2007 12:28:00 +1300
> Cc: gdb-patches@sources.redhat.com
> 
>  > Other than that, this patch can go in, but please show here the exact
>  > patch you commit, since you didn't show it in the message to which I'm
>  > replying.
> 
> I am sending it as an attachment with your change listed separately.  Maybe
> I've made mistakes (I've not made changes to configury before) but I'll
> be available to correct it if necessary.
> 
> Thanks for your help.

And thank you for doing the actual work.

I've proof-read the patches and they seem fine to me.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > >  > I was wondering if we should make this a normal GDB setting, and use5C
 > >  > "-gdb-set mi profiling on" to enable it.  There's already a maint
 > >  > setting to do the same thing for the CLI.
 > > 
 > > The command "maint set profiling on" is relevant for both CLI and MI,
 > > whereas -enable-timings only works in MI.
 > 
 > I wasn't talking about "maint set profiling on", but about "maint time
 > 1".  That doesn't currently work in MI, but it could be made to work
 > and moved under the normal "set" hierarchy.  Then we could use the same
 > setting for both CLI and MI.  In that case you wouldn't have to invent
 > a new command that reimplements the "set" behavior; it could just be a
 > standard auto_boolean.

I wasn't aware of "maint time 1" which does indeed seem very similar.  In that
case don't understand why you suggested "-gdb-set mi profiling on" and not
"-gdb-set mi set time on".  Anyway, in the case of MI, I'm not sure that you
would want the user to be able to turn it on, so I'm not sure that the same
setting is a good idea.  Initially I would just use it manually from the
command line.  I don't currently have a front end that can consume the output
but it clearly makes sense for the future.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: new timing command

[Daniel Jacobowitz]

On Mon, Jan 01, 2007 at 10:16:35PM +1300, Nick Roberts wrote:
> I wasn't aware of "maint time 1" which does indeed seem very similar.  In that
> case don't understand why you suggested "-gdb-set mi profiling on" and not
> "-gdb-set mi set time on".

Indeed, it was a mistake on my part.

> Anyway, in the case of MI, I'm not sure that you
> would want the user to be able to turn it on, so I'm not sure that the same
> setting is a good idea.  Initially I would just use it manually from the
> command line.  I don't currently have a front end that can consume the output
> but it clearly makes sense for the future.

It's just a new field in various outputs; shouldn't a properly written
MI parser ignore them?  Anyway, it might not be wise to make it easy
for end users to enable it, indeed.  That's a good reason to use
-enable-timings as you/Apple did.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: new timing command

[Vladimir Prus]

Nick Roberts wrote:

> + static long
> + timval_diff (struct timeval start, struct timeval end)
> +   {
> +     return ((end.tv_sec - start.tv_sec) * 1000000) +
> +       (end.tv_usec - start.tv_usec);
> +   }

Did you really meant to name a function "timval_diff" as opposed to
"timeval_diff"?

- Volodya

Re: [PATCH] MI: new timing command

[Mark Kettenis]

> Date: Sun, 31 Dec 2006 06:21:59 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Nick Roberts <nickrob@snap.net.nz>
> > Date: Sun, 31 Dec 2006 11:05:35 +1300
> > Cc: gdb-patches@sourceware.org
> > 
> >  > > diff -c -p -r1.86 mi-main.c
> >  > > *** mi-main.c	17 Nov 2006 19:30:41 -0000	1.86
> >  > > --- mi-main.c	30 Dec 2006 08:42:29 -0000
> >  > > ***************
> >  > > *** 49,54 ****
> >  > > --- 49,55 ----
> >  > >   
> >  > >   #include <ctype.h>
> >  > >   #include <sys/time.h>
> >  > > + #include <sys/resource.h>
> >  > 
> >  > Is sys/resource.h guaranteed to be available on all supported
> >  > platforms?  If not, we need to add a configure-time test and enable
> >  > this code only under HAVE_GETRUSAGE or some such.
> > 
> > mi-main.c already uses sys/time.h.  Is sys/resource.h any less widely
> > distributed?
> 
> I think sys/time.h is much more wide-spread, but I might be wrong.  I
> raised the issue because sys/resource.h is relatively seldom used, so
> it might be less portable.
> 
> > Is there any way of finding out e.g online database?
> 
> Sorry, I don't know.  Anyone?

The OpenBSD getrusage(2) manpage says that the getrusage() function
call appeared in 4.2BSD.  Seventh Edition Unix doesn't have it (but
doesn't have <sys/time.h> either).  HP-UX 10.20 and Solaris 2.6 both
have it, so I'm pretty sure all the UNIX-like systems we support have
it.  But POSIX marks it as an XSI extension, so I think we should not
rely on it.

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > > From: Nick Roberts <nickrob@snap.net.nz>
 > > Date: Sat, 30 Dec 2006 21:47:23 +1300
 > > 
 > > Here's a patch for timing MI commands e.g.
 > 
 > Thanks, but where's the documentation to go with it?

Here are some docs but I guess it could still all change.


-- 
Nick                                           http://www.inet.net.nz/~nickrob


2006-12-31  Nick Roberts  <nickrob@snap.net.nz>

	* gdb.texinfo (GDB/MI Miscellaneous Commands): Describe the new
	command -enable-timings.


*** gdb.texinfo	09 Dec 2006 10:52:03 +1300	1.373
--- gdb.texinfo	31 Dec 2006 12:33:02 +1300	
*************** The corresponding @value{GDBN} command i
*** 21419,21424 ****
--- 21419,21468 ----
  (gdb)
  @end smallexample
  
+ @subheading The @code{-enable-timings} Command
+ @findex -enable-timings
+ 
+ @subheading Synopsis
+ 
+ @smallexample
+ -enable-timings [yes | no]
+ @end smallexample
+ 
+ Toggle the printing of the wallclock, user and system times for an MI
+ command as a field in its output.  This command is to help frontend
+ developers optimize the performance of their code.  No argument is
+ equivalent to @samp{yes}.
+ 
+ @subheading @value{GDBN} Command
+ 
+ No equivalent.
+ 
+ @subheading Example
+ 
+ @smallexample
+ (gdb)
+ -enable-timings
+ ^done
+ (gdb)
+ -break-insert main
+ ^done,bkpt=@{number="1",type="breakpoint",disp="keep",enabled="y",
+ addr="0x080484ed",func="main",file="myprog.c",
+ fullname="/home/nickrob/myprog.c",line="73",times="0"@},
+ time=@{wallclock="0.05185",user="0.00800",system="0.00000"@}
+ (gdb)
+ -enable-timings no
+ ^done
+ (gdb)
+ -exec-run
+ ^running
+ (gdb)
+ *stopped,reason="breakpoint-hit",bkptno="1",thread-id="0",
+ frame=@{addr="0x080484ed",func="main",args=[@{name="argc",value="1"@},
+ @{name="argv",value="0xbfb60364"@}],file="myprog.c",
+ fullname="/home/nickrob/myprog.c",line="73"@}
+ (gdb)
+ @end smallexample
+ 
  @node Annotations
  @chapter @value{GDBN} Annotations
  

Re: [PATCH] MI: new timing command

[Vladimir Prus]

Nick Roberts wrote:

> + Toggle the printing of the wallclock, user and system times for an MI
> + command as a field in its output.  This command is to help frontend
> + developers optimize the performance of their code.  

I though it's to help gdb developers to optimize performance of
their code ;-) At least when gdb takes a second for list frames,
there's nothing I can do in a frontend ;-)

> No argument is 
> + equivalent to @samp{yes}.   

I'd suggest to say that this command may return error if 
timing is not available. Otherwise, frontend authors might
not even think this command can fail, and when it fails,
user will see completely buffling error message.

- Volodya

Re: [PATCH] MI: new timing command

[Nick Roberts]

 > > + Toggle the printing of the wallclock, user and system times for an MI
 > > + command as a field in its output. 

Re: [PATCH] MI: new timing command

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sun, 31 Dec 2006 20:46:09 +1300
> Cc: gdb-patches@sourceware.org
> 
> Here are some docs but I guess it could still all change.

Fine with me.

I'd suggest to add a little something about this to NEWS as well.

Thanks.