Re: [Qemu-devel] [PATCH v4 09/20] instrument: Add basic control interface

["Emilio G. Cota" <cota@braap.org>]

On Wed, Sep 06, 2017 at 20:59:02 +0300, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
(snip)
> +QI_VPUBLIC void qi_set_fini(qi_fini_fn fn, void *data)
> +{
> +    ERROR_IF(!instr_get_state(), "called outside instrumentation");
> +    instr_set_event(fini_fn, fn);
> +    instr_set_event(fini_data, data);
> +}

Why are these QI_VPUBLIC attributes here? Those are useful for DSO's, not
for executables --by using -rdynamic, all non-static symbols in the
executable are already visible.

> diff --git a/instrument/control.h b/instrument/control.h
> new file mode 100644
> index 0000000000..f2b085f69b
> --- /dev/null
> +++ b/instrument/control.h
(snip)
> + * Instrumentation state of current host thread. Used to ensure instrumentation
> + * clients use QEMU's API only in expected points.
> + */
> +typedef enum {
> +    INSTR_STATE_DISABLE,
> +    INSTR_STATE_ENABLE,
> +} InstrState;

I find this unnecessarily ugly for the little gain we get, i.e. asserts against
calling API code from QEMU.. seems unlikely to me (although admittedly I think
the qemu-internal API is unnecessarily complex/verbose, so maybe
you're better off with these checks).

(snip)
> +/**
> + * instr_get_event:
> + *
> + * Get value set by instrumentation library.
> + */
> +#define instr_get_event(name)                   \
> +    atomic_load_acquire(&instr_event__ ## name)
> +
> +/**
> + * instr_get_event:
> + *
> + * Set value from instrumentation library.
> + */
> +#define instr_set_event(name, fn)               \
> +    atomic_store_release(&instr_event__ ## name, fn)

This isn't enough to decide whether to call instrumentation, especially for
TCG. We need TB's to know what to call, and update that mask with async
work, just like we do with tracing. Check out my alternative patchset.

Also, a single function pointer cannot work for more than one plugin. But
I see you have an XXX when there's more than one plugin, so it's OK for now.
I used RCU lists for this, which at least gives you a time in the future
at which things become visible/invisible by other threads -- this is important
when unloading an instrumenter, since you don't want to clear important stuff
(e.g. dlclose) before you're sure no further callbacks to it are possible.
[no, the atomic_acquire/release isn't enough!]

(snip)
> diff --git a/instrument/load.c b/instrument/load.c
> index a57401102a..e180f03429 100644
> --- a/instrument/load.c
> +++ b/instrument/load.c
> @@ -11,6 +11,8 @@
>  #include "qemu-common.h"
>  
>  #include <dlfcn.h>
> +#include "instrument/control.h"
> +#include "instrument/events.h"
>  #include "instrument/load.h"
>  #include "qemu/config-file.h"
>  #include "qemu/error-report.h"
> @@ -105,8 +107,11 @@ InstrLoadError instr_load(const char * path, int argc, const char ** argv,
>          res = INSTR_LOAD_DLERROR;
>          goto err;
>      }
> +    instr_set_event(fini_fn, NULL);
>  
> +    instr_set_state(INSTR_STATE_ENABLE);
>      main_res = main_cb(argc, argv);
> +    instr_set_state(INSTR_STATE_DISABLE);
>  
>      if (main_res != 0) {
>          res = INSTR_LOAD_ERROR;
> @@ -136,6 +141,14 @@ InstrUnloadError instr_unload(int64_t handle_id)
>          goto out;
>      }
>  
> +    qi_fini_fn fini_fn = instr_get_event(fini_fn);
> +    if (fini_fn) {
> +        void *fini_data = instr_get_event(fini_data);
> +        fini_fn(fini_data);
> +    }
> +
> +    instr_set_event(fini_fn, NULL);
> +

Is fini really that useful? Doesn't the tool just die with QEMU once QEMU exits?
At the end of the day, the tool could register its own atexit hook.

		E.