Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> Hi
>
> On Tue, Sep 6, 2016 at 7:58 PM Markus Armbruster <arm...@redhat.com> wrote:
>
>> QAPI language design issues, copying Eric.
>>
>> Marc-André Lureau <marcandre.lur...@gmail.com> writes:
>>
>> > Hi
>> >
>> > On Tue, Sep 6, 2016 at 5:00 PM Markus Armbruster <arm...@redhat.com>
>> wrote:
>> >
>> >> Marc-André Lureau <marcandre.lur...@redhat.com> 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 <marcandre.lur...@redhat.com>
>> >> > ---
>> >> > 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?
In language design, a preprocessor transforming text is a cop out.
With a simple and regular syntax such as JSON, syntax tree
transformations are at least as easy to grasp, and make more sense.
Compare the C preprocessor (text-transforming water pistol, tacked on,
way too complex for what it can do) to Lisp macros (syntax-transforming
auto-cannon, compile-time language identical to run-time language) to
C++ templates (syntax-transforming auto-cannon, completely separate
compile-time language, fully understood by about three people, one has
since died, one has forgotten everything, and one has gone crazy).
I'm not asking for Lisp macros here. Sometimes, the most practical
solution is to cop out. I just want us to think before we cop out.
>> >> 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 agree it needs to be simple, both the interface (QAPI language) and
the implementation. However, I don't like "first past the post". I
prefer to explore the design space a bit.
So let me explore the "add 'ifdef' keys to definitions" corner of the
QAPI language design space.
Easily done for top-level definitions, because they're all JSON objects.
Could even add it to the include directive if we wanted to.
Less easily done for enumeration, struct, union and alternate members.
Note that command and event arguments specified inline are a special
case of struct members.
The "can't specify additional stuff for struct members" problem isn't
new. We hacked around it to specify "optional": encode it into the
member name. Doesn't scale. We need to solve the problem to be able to
specify default values, and we already decided how: have an JSON object
instead of a mere JSON string, make the string syntax sugar for {
'type': STRING }. See commit 6b5abc7 and the discussion in qemu-devel
leading up to it. For consistency, we'll do it for union and alternate
members, too.
That leaves just enumeration members. The same technique applies.
If I remember correctly, we only need conditional commands right now, to
avoid regressing query-commands. The more complicated member work could
be done later.
>> > 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.
Things always start that way. Trouble is they rarely stay that way.
>> > 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?
Switching to JavaScript comment syntax is an option. Painful loss of
git-blame information, though.
>> >> > @@ -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?
read_config_h?
>> >> > + 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.
Of course.
I'd like to have a look at qapi-introspect.py myself.
>> 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.
See "Trouble is they rarely stay that way" above.
>> 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.
Yes.
> But reading the config-*.h is quite straightforward.
Until somebody gets clever in configure, and things break.
We can ignore this detail for now.
[...]
