From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35243) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TNkrV-0000KQ-75 for qemu-devel@nongnu.org; Mon, 15 Oct 2012 09:37:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TNkrO-0000Md-3l for qemu-devel@nongnu.org; Mon, 15 Oct 2012 09:37:41 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:60937) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TNkrN-0000MZ-OD for qemu-devel@nongnu.org; Mon, 15 Oct 2012 09:37:34 -0400 Received: by mail-pb0-f45.google.com with SMTP id rp2so4877612pbb.4 for ; Mon, 15 Oct 2012 06:37:32 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <507C118E.5010106@redhat.com> Date: Mon, 15 Oct 2012 15:37:18 +0200 From: Paolo Bonzini MIME-Version: 1.0 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> In-Reply-To: <20121005164730.GM16157@illuin> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: Michael Roth Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@us.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.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 >>>>> --- >>>>> 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. Paolo