Re: [Qemu-devel] [PATCH v3 21/22] qidl: qidl.h, definitions for qidl annotations

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Fri, Oct 05, 2012 at 05:53:09PM +0200, Paolo Bonzini wrote:
> Il 05/10/2012 17:41, Michael Roth ha scritto:
> > On Fri, Oct 05, 2012 at 05:07:46PM +0200, Paolo Bonzini wrote:
> >> Il 04/10/2012 19:33, Michael Roth ha scritto:
> >>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >>> ---
> >>>  qidl.h |  113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 113 insertions(+)
> >>>  create mode 100644 qidl.h
> >>>
> >>> diff --git a/qidl.h b/qidl.h
> >>> new file mode 100644
> >>> index 0000000..eae0202
> >>> --- /dev/null
> >>> +++ b/qidl.h
> >>> @@ -0,0 +1,113 @@
> >>> +/*
> >>> + * QEMU IDL Macros/stubs
> >>> + *
> >>> + * See docs/qidl.txt for usage information.
> >>> + *
> >>> + * Copyright IBM, Corp. 2012
> >>> + *
> >>> + * Authors:
> >>> + *  Michael Roth    <mdroth@linux.vnet.ibm.com>
> >>> + *
> >>> + * This work is licensed under the terms of the GNU GPLv2 or later.
> >>> + * See the COPYING file in the top-level directory.
> >>> + *
> >>> + */
> >>> +
> >>> +#ifndef QIDL_H
> >>> +#define QIDL_H
> >>> +
> >>> +#include <glib.h>
> >>> +#include "qapi/qapi-visit-core.h"
> >>> +#include "qemu/object.h"
> >>> +#include "hw/qdev-properties.h"
> >>> +
> >>> +#ifdef QIDL_GEN
> >>> +
> >>> +/* we pass the code through the preprocessor with QIDL_GEN defined to parse
> >>> + * structures as they'd appear after preprocessing, and use the following
> >>> + * definitions mostly to re-insert the initial macros/annotations so they
> >>> + * stick around for the parser to process
> >>> + */
> >>> +#define QIDL(...) QIDL(__VA_ARGS__)
> >>> +#define QIDL_START(name, ...) QIDL_START(name, ##__VA_ARGS__)
> >>> +
> >>> +#define QIDL_VISIT_TYPE(name, v, s, f, e)
> >>> +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp)
> >>> +#define QIDL_PROPERTIES(name)
> >>
> >> Ok, a few questions...
> >>
> >> Why do you need these to expand to nothing in the QIDL_GEN case?
> >>
> > 
> > They don't need to, I was just trying to be explicit about what
> > directives were relevant to the parser and which ones were relevant to
> > the actually compiled code. It was more a development "aid" than
> > anything else though, so I think we can drop the special handling and
> > clean these up a bit.
> 
> Yes, thanks!
> 
> >>> +#define QIDL_DECLARE(name, ...) \
> >>
> >> Can QIDL_DECLARE replace QIDL_ENABLED as the magic detection string for
> >> qidl compilation?
> >>
> > 
> > In some cases the declarations will come via #include'd headers, so the
> > only way to do that reliable is to run it through the preprocessor
> > first, which is how things were done in v1. But running everything
> > through cpp adds substantial overhead, and just because a QIDL-fied
> > struct is included in a C file, it doesn't mean that the C file intends
> > to use any qidl-generated code.
> 
> Ok, I guess I need to see some example.  We can clean it up later if we
> find a more clever way to do things.

This was the main example I hit (not yet rebased):

https://github.com/mdroth/qemu/commit/d8ea7c7a882e2fcbd0a9b7ab9ea47a389f87d31b

As part of that patch We add annotations to PCIDevice in pci.h, which then gets
pulled in from quite a few devices. So we end up with *.qidl.c files for devices
that don't expose a "state" property or even have a QIDL_DECLARE() directive.

If we were to scan for QIDL_DECLARE() in advance of running it through
the preprocessor, we'd address a lot of those case. But then we miss
cases like this:

https://github.com/mdroth/qemu/commit/2199f721daebd5c3961069bdd51de80a5b4fa827

where, in pci.c, we use code generated from declarations in pci_internals.h even
though pci.c doesn't contain a QIDL_DECLARE()

We could in theory scan for
QIDL_PROPERTIES()/QIDL_SCHEMA_ADD_LINK()/QIDL_VISIT_TYPE() to avoid the
need for QIDL_ENABLE(), but to me the latter approach seemed like it
would scale better if we were to leverage QIDL for other things in the
future.

> 
> Paolo
>