Re: [Qemu-devel] [PATCH v5 02/20] qapi.py: add a simple #ifdef conditional

[Marc-André Lureau]

Hi

On Tue, Sep 6, 2016 at 7:58 PM Markus Armbruster <address@hidden> wrote:

> QAPI language design issues, copying Eric.
>
> Marc-André Lureau <address@hidden> writes:
>
> > Hi
> >
> > On Tue, Sep 6, 2016 at 5:00 PM Markus Armbruster <address@hidden>
> wrote:
> >
> >> Marc-André Lureau <address@hidden> writes:
> >>
> >> > Learn to parse #define files provided with -f option, and skip
> >> > undefined #ifdef blocks in the schema.
> >> >
> >> > This is a very simple pre-processing, without stacking support or
> >> > evaluation (it could be implemented if needed).
> >> >
> >> > Signed-off-by: Marc-André Lureau <address@hidden>
> >> > ---
> >> >  scripts/qapi.py | 43 ++++++++++++++++++++++++++++++++++++++++---
> >>
> >> Missing: update of docs/qapi-code-gen.txt.
> >>
> >> >  1 file changed, 40 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> >> > index 21bc32f..d0b8a66 100644
> >> > --- a/scripts/qapi.py
> >> > +++ b/scripts/qapi.py
> >> > @@ -76,6 +76,7 @@ struct_types = []
> >> >  union_types = []
> >> >  events = []
> >> >  all_names = {}
> >> > +defs = []
> >> >
> >> >  #
> >> >  # Parsing the schema into expressions
> >> > @@ -177,6 +178,7 @@ class QAPISchemaParser(object):
> >> >                  self.exprs.append(expr_elem)
> >> >
> >> >      def accept(self):
> >> > +        ok = True
> >> >          while True:
> >> >              self.tok = self.src[self.cursor]
> >> >              self.pos = self.cursor
> >> > @@ -184,7 +186,19 @@ class QAPISchemaParser(object):
> >> >              self.val = None
> >> >
> >> >              if self.tok == '#':
> >> > -                self.cursor = self.src.find('\n', self.cursor)
> >> > +                end = self.src.find('\n', self.cursor)
> >> > +                line = self.src[self.cursor:end+1]
> >> > +                self.cursor = end
> >> > +                sline = line.split()
> >> > +                if len(defs) and len(sline) >= 1 \
> >> > +                   and sline[0] in ['ifdef', 'endif']:
> >> > +                    if sline[0] == 'ifdef':
> >> > +                        ok = sline[1] in defs
> >> > +                    elif sline[0] == 'endif':
> >> > +                        ok = True
> >> > +                    continue
> >> > +            elif not ok:
> >> > +                continue
> >> >              elif self.tok in "{}:,[]":
> >> >                  return
> >> >              elif self.tok == "'":
> >>
> >> Oww, you're abusing comments!  That's horrible :)
> >>
> >> Can we make this real syntax, like everything else, including 'include'?
> >>
> >>
> > We already abuse json, which doesn't support comments either. :) Even
> > without comments, our syntax is wrong and confuse most
> editors/validators.
>
> True.  Python mode works okay for me, though.
>
> Perhaps we've outgrown the JSON syntax.  Or perhaps we've just messed up
> by taking too many liberties with it, with too little thought.  Not
> exactly an excuse for taking *more* liberties :)
>

It depends, json is very limited. Doing everything is json is not
convenient and not necessary. Why not having preprocessor?


> >> Unfortunately, the natural
> >>
> >>     { 'ifdef': 'CONFIG_FOO'
> >>       'then': ...   # ignored unless CONFIG_FOO
> >>       'else': ...   # ignored if CONFIG_FOO (optional)
> >>     }
> >>
> >> is awkward, because the ... have to be a single JSON value, but a QAPI
> >> schema isn't a single JSON value, it's a *sequence* of JSON values.  A
> >> top-level stanza
> >>
> >>     JSON-value1 JSON-value2 ...
> >>
> >> would become
> >>
> >>     [ JSON-value1, JSON-value2, ... ]
> >>
> >> within a conditional.  Blech.
> >>
> >> Could sacrifice the nesting and do
> >>
> >>     { 'ifdef': 'CONFIG_FOO' }
> >>     ...
> >>     { 'endif' }
> >>
> >> Widens the gap between syntax and semantics.  Editors can no longer
> >> easily jump over the conditional (e.g. Emacs forward-sexp).  Nested
> >> conditionals becomes as confusing as in C.  Not exactly elegant, either.
> >>
> >> Yet another option is to add 'ifdef' keys to the definitions
> >> themselves, i.e.
> >>
> >>     { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
> >>       'ifdef': 'TARGET_ARM' }
> >>
> >
> > That's the worst of all options imho, as it makes it hard to filter out
> > unions/enums, ex:
> >
> >  @ -3446,8 +3466,10 @@
> >                                         'testdev': 'ChardevCommon',
> >                                         'stdio'  : 'ChardevStdio',
> >                                         'console': 'ChardevCommon',
> > +#ifdef CONFIG_SPICE
> >                                         'spicevmc' :
> 'ChardevSpiceChannel',
> >                                         'spiceport' : 'ChardevSpicePort',
> > +#endif
> >                                         'vc'     : 'ChardevVC',
> >                                         'ringbuf': 'ChardevRingbuf',
>
> Point taken.
>
> Fixable the same way as always when a definition needs to grow
> additional properties, but the syntax only provides a single value: make
> that single value an object, and the old non-object value syntactic
> sugar for the equivalent object value.  We've previously discussed this
> technique in the context of giving command arguments default values.
> I'm not saying this is what we should do here, only pointing out it's
> possible.
>

Ok, but let's find something, if possible simple and convenient, no?


>
> > I think #ifdef is the most straightforward/easy thing to do. My
> experience
> > with doc generations tells me that is really not an issue to reserve the
> > #\w* lines for pre-processing.
>
> Two justifications for doc generators recognizing magic comments: 1. If
> you create a machine processing comments (e.g. formatting them nicely
> for a medium richer than ASCII text, you fundamentally define a comment
> language, embedded in a host language's comments, and 2. what else can
> you do when you can't change the host language?
>
> Neither applies to adding conditional compilation directives to the QAPI
> schema language.  Letting a language grow into its comments is, to be
> frank, disgusting.
>
> Besides, if we truly want the C preprocessor, we should use the C
> preprocessor.  Not implement a half-assed clone of the half-assed hack
> of a macro processor the C preprocessor is.
>

That's possible, although not so easy. First we have to convert all
comments syntax. And then we have to generate files and change the build
sys etc. Doable, but not so great imho. Right now we just need simple
#ifdef conditions that can be easily handled when parsing, far from a full
C preprocessor.


> > And it's a natural fit with the rest of qemu #if conditionals.
>
> It's an awfully unnatural fit with the QAPI schema comment syntax.
>

> ##
> # @Frobs
> #
> # Collection of frobs that need to be frobnicated, except when
> # ifdef CONFIG_NOFROB
> { 'struct': 'Frobs'
>   ...
> }
>
> I stand by 'horrible'.
>

Ok. let's switch the comment syntax to a regular js/c style then?

>
> >> > @@ -1707,15 +1721,36 @@ def gen_params(arg_type, boxed, extra):
> >> >  #
> >> >  # Common command line parsing
> >> >  #
> >> > +def defile(filename):
> >>
> >> From The Collaborative International Dictionary of English v.0.48
> [gcide]:
> >>
> >> Defile \De*file"\ (d[-e]*f[imac]l"), v. t. [OE. defoulen,
> >>    -foilen, to tread down, OF. defouler; de- + fouler to trample
> >>    (see Full, v. t.), and OE. defoulen to foul (influenced in
> >>    form by the older verb defoilen). See File to defile,
> >>    Foul, Defoul.]
> >>    1. To make foul or impure; to make filthy; to dirty; to
> >>       befoul; to pollute.
> >>       [1913 Webster]
> >>
> >>             They that touch pitch will be defiled. --Shak.
> >>       [1913 Webster]
> >>
> >>    2. To soil or sully; to tarnish, as reputation; to taint.
> >>       [1913 Webster]
> >>
> >>             He is . . . among the greatest prelates of this age,
> >>             however his character may be defiled by . . . dirty
> >>             hands.                                --Swift.
> >>       [1913 Webster]
> >>
> >>    3. To injure in purity of character; to corrupt.
> >>       [1913 Webster]
> >>
> >>             Defile not yourselves with the idols of Egypt.
> >>                                                   --Ezek. xx. 7.
> >>       [1913 Webster]
> >>
> >>    4. To corrupt the chastity of; to debauch; to violate; to
> >>       rape.
> >>       [1913 Webster]
> >>
> >>             The husband murder'd and the wife defiled. --Prior.
> >>       [1913 Webster]
> >>
> >>    5. To make ceremonially unclean; to pollute.
> >>       [1913 Webster]
> >>
> >>             That which dieth of itself, or is torn with beasts,
> >>             he shall not eat to defile therewith. --Lev. xxii.
> >>                                                   8.
> >>       [1913 Webster]
> >>
> >> Fitting in a way; you're defiling the poor, innocent comment syntax ;)
> >>
> >
> > ok
>
> Seriously: can we give this function a better name?
>

yes load_define_file?


>
> >>
> >> > +    f = open(filename, 'r')
> >> > +    while 1:
> >>
> >> while True:
> >>
> >> > +        line = f.readline()
> >> > +        if not line:
> >> > +            break
> >> > +        while line[-2:] == '\\\n':
> >> > +            nextline = f.readline()
> >> > +            if not nextline:
> >> > +                break
> >> > +            line = line + nextline
> >> > +        tmp = line.strip()
> >> > +        if tmp[:1] != '#':
> >> > +            continue
> >> > +        tmp = tmp[1:]
> >> > +        words = tmp.split()
> >> > +        if words[0] != "define":
> >> > +            continue
> >> > +        defs.append(words[1])
> >> > +    f.close()
> >>
> >> This parses Yet Another Language.  Worse, Yet Another Undocumented
> >> Language.  Why not JSON?
> >>
> >
> >> Hmm, peeking ahead to PATCH 04... aha!  This is for reading
> >> config-host.h and config-target.h.  So, this actually doesn't parse
> >> YAUL, it parses C.  Sloppily, of course.
> >>
> >
> > Yes
> >
> >
> >>
> >> I guess we could instead parse config-host.mak and config-target.mak
> >> sloppily.  Not sure which idea is more disgusting :)
> >>
> >> Could we punt evaluating conditionals to the C compiler?  Instead of
> >> emitting TEXT when CONFIG_FOO is defined, emit
> >>
> >>     #ifdef CONFIG_FOO
> >>     TEXT
> >>     #endif
> >>
> >>
> > I don't follow you
>
> Let me try to explain with an example.  Say we make
> query-gic-capabilities conditional on TARGET_ARM in the schema.  In your
> syntax:
>
>     #ifdef TARGET_ARM
>     { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
>     #endif
>
> Now consider qmp-commands.h.  If we evaluate conditionals at QAPI
> generation time, we generate it per target.  For targets with TARGET_ARM
> defined, it contains
>
>     GICCapabilityList *qmp_query_gic_capabilities(Error **errp);
>     void qmp_marshal_query_gic_capabilities(QDict *args, QObject **ret,
> Error **errp);
>
> For the others, it doesn't contain this text.
>
> If we evaluate conditionals at C compile time, we generate
> qmp-commands.h *once*, and it has
>
>     #ifdef TARGET_ARM
>     GICCapabilityList *qmp_query_gic_capabilities(Error **errp);
>     void qmp_marshal_query_gic_capabilities(QDict *args, QObject **ret,
> Error **errp);
>     #endif
>
> It needs to be compiled per target, of course.
>

Doable for commands, but not easily doable for introspect.c. I tried to
implement that at first, it was really complicated. I think we should also
consider simplicity here.


>
> The QAPI generator doesn't interpret the conditional text in any way, it
> just passes it through to the C compiler.
>
> Makes supporting complex conditions easy (we discussed this before, I
> think).  A schema snippet
>
>     #if defined(TARGET_I386) && !defined(TARGET_X86_64)
>     ...
>     #endif
>
> could generate
>
>     #if defined(TARGET_I386) && !defined(TARGET_X86_64)
>     whatever is generated for ...
>     #endif
>
> To do this at QAPI generation time, you need to build an expression
> evaluator into qapi.py.
>

That's relatively easy, I've done that a couple of time, I can do it here
too. Only it was not needed so far.

>
> Also saves us the trouble of reading config-*.h or config-*.mak.
>
>  If needed we could generate a simpler file, like a python config file, or
generating command line arguments, etc.. with just the values we need for
qapi generation. But reading the config-*.h is quite straightforward.


> >> >  def parse_command_line(extra_options="", extra_long_options=[]):
> >> >
> >> >      try:
> >> >          opts, args = getopt.gnu_getopt(sys.argv[1:],
> >> > -                                       "chp:o:" + extra_options,
> >> > +                                       "chp:o:f:" + extra_options,
> >> >                                         ["source", "header",
> "prefix=",
> >> > -                                        "output-dir="] +
> >> extra_long_options)
> >> > +                                        "output-dir=", "--defile="] +
> >>
> >> https://docs.python.org/3/library/getopt.html on the third argument:
> >> "The leading '--' characters should not be included in the option name."
> >>
> >
> > ok
> >
> >
> >>
> >> > +                                       extra_long_options)
> >> >      except getopt.GetoptError as err:
> >> >          print >>sys.stderr, "%s: %s" % (sys.argv[0], str(err))
> >> >          sys.exit(1)
> >> > @@ -1742,6 +1777,8 @@ def parse_command_line(extra_options="",
> extra_long_options=[]):
> >> >              do_c = True
> >> >          elif o in ("-h", "--header"):
> >> >              do_h = True
> >> > +        elif o in ("-f", "--defile"):
> >> > +            defile(a)
> >> >          else:
> >> >              extra_opts.append(oa)
> >>
> >> --
> > Marc-André Lureau
>
-- 
Marc-André Lureau