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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 05/10/2012 18:47, Michael Roth ha scritto:
> 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()

Hmm, this opens another can of worms.  There is a substantial amount of
duplicated code between generated files.  For example,
visit_type_PCIDevice is found in all *.qidl.c files for PCI devices.
Worse, the same is true for the properties array.

Right now, QIDL_DECLARE is a no-op at code-generation time.  Could it be
a marker to generate code for that particular struct?  Then you would
put a normal

struct PCIDevice {
};

declaration in hw/pci.h, and a

QIDL_DECLARE(PCIDevice);

in hw/pci.c that would trigger creation of the visitor etc.  The code
generator can also prepare extern declarations for types that are used
but not defined, for example visit_type_PCIDevice in piix_pci.qidl.c.

Paolo