From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33983) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TNn1i-0000Vp-K9 for qemu-devel@nongnu.org; Mon, 15 Oct 2012 11:56:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TNn1c-0004bu-Cn for qemu-devel@nongnu.org; Mon, 15 Oct 2012 11:56:22 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:44379) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TNn1c-0004bi-6O for qemu-devel@nongnu.org; Mon, 15 Oct 2012 11:56:16 -0400 Received: by mail-ob0-f173.google.com with SMTP id wc18so4896080obb.4 for ; Mon, 15 Oct 2012 08:56:15 -0700 (PDT) Sender: fluxion Date: Mon, 15 Oct 2012 10:50:02 -0500 From: Michael Roth Message-ID: <20121015155002.GT16157@illuin> References: <1349372021-31212-1-git-send-email-mdroth@linux.vnet.ibm.com> <1349372021-31212-22-git-send-email-mdroth@linux.vnet.ibm.com> <506EF7C2.1030103@redhat.com> <20121005154119.GL16157@illuin> <506F0265.5080500@redhat.com> <20121005164730.GM16157@illuin> <507C118E.5010106@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <507C118E.5010106@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 21/22] qidl: qidl.h, definitions for qidl annotations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@us.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.com On Mon, Oct 15, 2012 at 03:37:18PM +0200, Paolo Bonzini wrote: > 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 > >>>>> --- > >>>>> 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 > >>>>> + * > >>>>> + * 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 > >>>>> +#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. Hmm, this could work... what I'd probably do though is: - for cases where we're annotating "public" structs that may get pulled into multiple files, use QIDL_DECLARE_PUBLIC() in place of QIDL_DECLARE(). This will tell the code gen/qidl.h to only declare extern declarations for code we'll be linking against to access the generated visitors/properties/etc for that struct - within one or a few common objects that everyone links against (we can add one if one doesn't already exists), we can have, say, #include "hw/pci.h", and in the body we have a QIDL_IMPLEMENT_PUBLIC(PCIDevice), which tells the code gen to inject the code there like it would for QIDL_DECLARE(). If this seems reasonable, I could probably implement this as a follow-up, prior to any large-scale conversions. > > Paolo >