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

[Markus Armbruster]

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 :)

>> 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.

> 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.

> 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'.

>> > @@ -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?

>>
>> > +    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.

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.

Also saves us the trouble of reading config-*.h or config-*.mak.

>> >  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