Re: [Patch] Dryrun framework.

["Carlos O'Donell" <carlos at redhat dot com>]

On 04/09/2013 03:22 PM, OndÅej BÃlka wrote:
> Hi,
> 
> Dryrun is framework to supply real world data for microbenchmarks
> and analyze workloads.
> 
> To keep first commit simple I only added code to handle exp.
> Others will follow.
> 
> One question is where to save data? For memset I produced 20MB file
> so adding to git is out of question.
> Is possible http/ftp at sourceware where we could upload these files.
> 
> Another issue leaking information. So far I for memset/memcpy/strlen
> need only to know positions and I doubt that somebody can reconstruct
> from it password.
> 
> Compiling directory with given function creates three files:
> 
> record.so: To record function calls write 'LD_PRELOAD=dir/record.so command' 
> or add record.so to /ld.so.preload for global recording.
> 
> replay: Calls function as recorded.
> 
> show: displays recorded data.
> 
> OK to commit?

Not yet. Look at my comments and post v2.

> Ondra
> 
> 	* dryrun/common/common.h: New file.
> 	* dryrun/common/record_common.h: New file.
> 	* dryrun/common/replay_common.h: New file.
> 	* dryrun/exp/Makefile: New file.
> 	* dryrun/exp/layout.h: New file.
> 	* dryrun/exp/record.c: New file.
> 	* dryrun/exp/replay.c: New file.
> 	* dryrun/exp/show.c: New file.
> 	* dryrun/sysdeps/generic.h: New file.

* Is it -Wall -pedantic clean?

* You need copyright headers on *all* of your files.

* All hard coded constants like 48, 1000, and such need
  to go into a header somewhere.

* In general you need to follow GNU coding style for everything.

e.g.

int
main (void)
{
...
}

not

int main (void)
{
...
}

* Failure to check return on almost all functions that
  could return errors.
 
> From 563629a9f59f1d2926e5b234ed4bd84d20930ce4 Mon Sep 17 00:00:00 2001
> From: Ondrej Bilka <neleai@seznam.cz>
> Date: Tue, 9 Apr 2013 20:34:53 +0200
> Subject: [PATCH] dryrun framework
> 
> ---
>  dryrun/common/common.h        |   24 +++++++++++++++++++++
>  dryrun/common/record_common.h |   39 ++++++++++++++++++++++++++++++++++
>  dryrun/common/replay_common.h |   24 +++++++++++++++++++++
>  dryrun/exp/Makefile           |    4 +++
>  dryrun/exp/layout.h           |    5 ++++
>  dryrun/exp/record.c           |   18 +++++++++++++++
>  dryrun/exp/replay.c           |   10 ++++++++
>  dryrun/exp/show.c             |    9 +++++++
>  dryrun/sysdeps/generic.h      |   47 +++++++++++++++++++++++++++++++++++++++++
>  9 files changed, 180 insertions(+), 0 deletions(-)
>  create mode 100644 dryrun/common/common.h
>  create mode 100644 dryrun/common/record_common.h
>  create mode 100644 dryrun/common/replay_common.h
>  create mode 100644 dryrun/exp/Makefile
>  create mode 100644 dryrun/exp/layout.h
>  create mode 100644 dryrun/exp/record.c
>  create mode 100644 dryrun/exp/replay.c
>  create mode 100644 dryrun/exp/show.c
>  create mode 100644 dryrun/sysdeps/generic.h
> 
> diff --git a/dryrun/common/common.h b/dryrun/common/common.h
> new file mode 100644
> index 0000000..c7a0148
> --- /dev/null
> +++ b/dryrun/common/common.h

Header.

> @@ -0,0 +1,24 @@
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "../sysdeps/generic.h"
> +
> +typedef struct
> +{
> +  uint64_t time_start;
> +  uint64_t time_end;
> +  uint64_t backtrace;
> +} common_s;
> +
> +#include "layout.h"
> +
> +typedef struct
> +{
> +  char binary_name[48];

Magic constant.

> +  uint64_t size,capacity;
> +  rec items[];
> +} header;
> +
> +static void init_fn();
> +static header *h;
> diff --git a/dryrun/common/record_common.h b/dryrun/common/record_common.h
> new file mode 100644
> index 0000000..bbd13eb
> --- /dev/null
> +++ b/dryrun/common/record_common.h
> @@ -0,0 +1,39 @@
> +#include "common.h"
> +
> +__attribute__ ((constructor)) static void __start_profile ()

Coding style.

> +{
> +  h = malloc (sizeof (header) + 1000 * sizeof (rec));
> +  h->size = 0;
> +  h->capacity = 1000;

Magic constants.

> +  init_fn ();
> +}
> +
> +
> +__attribute__ ((destructor)) static void __end_profile ()
> +{
> +  int i, j;
> +  binary_name (h->binary_name);
> +  FILE* fi = fopen (FNAME, "a");
> +  if (fi)
> +    {
> +      //lock_file (fi);
> +      fwrite (h, sizeof (header) + h->size * sizeof (rec), 1, fi);
> +      fclose (fi);
> +    }
> +}
> +
> +rec* add_record ()
> +{
> +  h->size++;
> +  if (h->size == h->capacity)
> +    {
> +      h->capacity *= 2;
> +      h = realloc (h, sizeof (header) + h->capacity * sizeof (rec));
> +    }
> +
> +  rec* r = h->items + (h->size - 1);
> +  r->common.time_start = get_ticks ();
> +  r->common.backtrace = backtrace2 ();
> +
> +  return r;
> +}
> diff --git a/dryrun/common/replay_common.h b/dryrun/common/replay_common.h
> new file mode 100644
> index 0000000..04c322b
> --- /dev/null
> +++ b/dryrun/common/replay_common.h
> @@ -0,0 +1,24 @@
> +#include "common.h"
> +
> +void replay (rec* r, int size);
> +
> +int main (int argc, char** argv)
> +{
> +  long i;
> +  rec* buf;
> +  char* fname = argv[1];
> +  if (argc == 1)
> +    fname = FNAME;
> +  FILE* fi = fopen (fname, "r");
> +  h = malloc (sizeof (header));
> +  while (!feof (fi))
> +    {
> +      fread (h, sizeof (header), 1, fi);
> +      buf = malloc (h->size * sizeof (rec));
> +      fread (buf, h->size * sizeof (rec), 1, fi);
> +      printf ("replaying %s\n", h->binary_name);
> +
> +      replay (buf, h->size);
> +      free (buf);
> +    }
> +}
> diff --git a/dryrun/exp/Makefile b/dryrun/exp/Makefile
> new file mode 100644
> index 0000000..1e4cae6
> --- /dev/null
> +++ b/dryrun/exp/Makefile
> @@ -0,0 +1,4 @@
> +all:
> +	gcc -I. -I../common -g -DFNAME='"${PWD}/record.rec"' record.c -lm -lrt -ldl -shared -fPIC -o record.so
> +	gcc -I. -I../common -g -DFNAME='"${PWD}/record.rec"' replay.c -lm -lrt -o replay
> +	gcc -I. -I../common -g -DFNAME='"${PWD}/record.rec"' show.c -lm -lrt -o show

Should be integrated into glibc's build framwork
and callable from top-level as `make dryrun`.

Don't hard code the record file output, add an argument
or env var that is read to set the recording file output.

Otherwise default otuput should always be into $(CWD).

> diff --git a/dryrun/exp/layout.h b/dryrun/exp/layout.h
> new file mode 100644
> index 0000000..660f481
> --- /dev/null
> +++ b/dryrun/exp/layout.h
> @@ -0,0 +1,5 @@
> +typedef struct
> +{
> +  common_s common;
> +  double x;
> +} rec;
> diff --git a/dryrun/exp/record.c b/dryrun/exp/record.c
> new file mode 100644
> index 0000000..8208a99
> --- /dev/null
> +++ b/dryrun/exp/record.c
> @@ -0,0 +1,18 @@
> +#include <dlfcn.h>
> +#include "record_common.h"
> +
> +static void *libm_handle;
> +static double (*fn) (double);
> +
> +static void init_fn ()
> +{
> +  libm_handle = dlopen ("libm.so", RTLD_NOW);

Failure to check return.

> +  fn = dlsym (libm_handle, "exp");

Failure to check return.

> +}
> +
> +double exp (double x)
> +{
> +  rec *r = add_record ();
> +  r->x = x;
> +  return fn (x);
> +}
> diff --git a/dryrun/exp/replay.c b/dryrun/exp/replay.c
> new file mode 100644
> index 0000000..a07fdea
> --- /dev/null
> +++ b/dryrun/exp/replay.c
> @@ -0,0 +1,10 @@
> +#include "replay_common.h"
> +
> +void replay (rec *r, int size)
> +{
> +  int i;
> +  for (i = 0; i < size; i++)
> +    {
> +      exp (r[i].x);
> +    }
> +}
> diff --git a/dryrun/exp/show.c b/dryrun/exp/show.c
> new file mode 100644
> index 0000000..89e88bd
> --- /dev/null
> +++ b/dryrun/exp/show.c
> @@ -0,0 +1,9 @@
> +#include "replay_common.h"
> +void replay (rec *r,int size)
> +{
> +  int i;
> +  for (i = 0; i < size; i++)
> +    {
> +      printf ("%lf\n", r[i].x);
> +    }
> +}
> diff --git a/dryrun/sysdeps/generic.h b/dryrun/sysdeps/generic.h
> new file mode 100644
> index 0000000..8e5e5c4
> --- /dev/null
> +++ b/dryrun/sysdeps/generic.h
> @@ -0,0 +1,47 @@
> +#include <stdint.h>
> +#include <time.h>
> +#include <dlfcn.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <fcntl.h>
> +
> +
> +/* TODO Placeholder function.  */
> +/* We can use backtrace as it depends on libgcc which causes dlopen->backtrace->dlopen infinite loop.  */

Line too long.

If we don't have backtrace support remove it from the code.

> +static uint64_t __attribute__ ((noinline)) backtrace2 ()
> +{
> +  return 0;
> +}
> +
> +/* TODO now only linux specific.  */

We shouldn't have any todo's, Just comment that it's linux specific.

> +static void binary_name (char *x)
> +{
> +  int i;
> +  sprintf (x, "/proc/%i/cmdline", getpid ());
> +  FILE *f=fopen (x, "r");
> +  for (i = 0 ; i < 48 ; i++)
> +    x[i] = 0;
> +  fgets (x, 48, f);
> +  x[47] = 0;
> +}
> +
> +/* TODO add platform specific clocks that measure cycles.  */
> +static uint64_t  __attribute__ ((noinline)) get_ticks ()
> +{
> +  struct timespec ts;
> +  clock_gettime (CLOCK_REALTIME, &ts);

Failure to check return.

> +  return 1000000000 * ts.tv_sec + ts.tv_nsec;
> +}
> +
> +static void lock_file (FILE *fi)
> +{
> +  lock_file (fi);
> +  struct flock fl;
> +  int fd = fileno (fi);
> +  fl.l_type = F_WRLCK;
> +  fl.l_whence = SEEK_SET;
> +  fl.l_start = 0;
> +  fl.l_len = 0;
> +  fl.l_pid = getpid ();
> +  fcntl (fd, F_SETLKW, &fl);
> +}
> 

Cheers,
Carlos.