Re: [PATCH] MI: new timing command

[Eli Zaretskii <eliz@gnu.org>]

> 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