Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Marc-André Lureau]

Hi

On Mon, Feb 20, 2023 at 12:10 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Hi Markus
> >
> > On Fri, Feb 17, 2023 at 12:28 PM Markus Armbruster <armbru@redhat.com>
> > wrote:
> >
> >> marcandre.lureau@redhat.com writes:
> >>
> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >
> >> > The generated code doesn't quite handle the conditional arguments.
> >> > For example, 'bar' in 'test-if-cmd' is not correctly surrounded by #if
> >> > conditions. See generated code in qmp_marshal_test_if_cmd().
> >> >
> >> > Note that if there are multiple optional arguments at the last position,
> >> > there might be compilation issues due to extra comas. I left an assert
> >> > and FIXME for later.
> >> >
> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> > ---
> >> >  scripts/qapi/commands.py                |  4 ++++
> >> >  scripts/qapi/gen.py                     | 19 ++++++++++++++-----
> >> >  scripts/qapi/visit.py                   |  2 ++
> >> >  tests/qapi-schema/qapi-schema-test.json |  3 ++-
> >> >  4 files changed, 22 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> >> > index 79c5e5c3a9..07997d1586 100644
> >> > --- a/scripts/qapi/commands.py
> >> > +++ b/scripts/qapi/commands.py
> >> > @@ -64,9 +64,13 @@ def gen_call(name: str,
> >> >      elif arg_type:
> >> >          assert not arg_type.variants
> >> >          for memb in arg_type.members:
> >> > +            if memb.ifcond.is_present():
> >> > +                argstr += '\n' + memb.ifcond.gen_if()
> >> >              if memb.need_has():
> >> >                  argstr += 'arg.has_%s, ' % c_name(memb.name)
> >> >              argstr += 'arg.%s, ' % c_name(memb.name)
> >> > +            if memb.ifcond.is_present():
> >> > +                argstr += '\n' + memb.ifcond.gen_endif()
> >> >
> >> >      lhs = ''
> >> >      if ret_type:
> >>
> >> @argstr is emitted further down:
> >>
> >>        %(lhs)sqmp_%(name)s(%(args)s&err);
> >>    ''',
> >>                     name=name, args=argstr, lhs=lhs)
> >>
> >>        ret += mcgen('''
> >>        if (err) {
> >>    ''')
> >>
> >> Before the patch, @argstr contains no newlines.  Works.
> >>
> >> After the patch, it may contain newlines, and if it does, intentation is
> >> messed up.  For instance, in the code generated for
> >> qapi-schema-test.json:
> >>
> >>         retval = qmp_test_if_cmd(arg.foo,
> >>     #if defined(TEST_IF_CMD_BAR)
> >>     arg.bar,
> >>     #endif /* defined(TEST_IF_CMD_BAR) */
> >>     &err);
> >>
> >> Strings interpolated into the mcgen() argument should not contain
> >> newlines.  I'm afraid you have to rewrite the code emitting the call.
> >>
> >
> > Why it should not contain newlines?
>
> They mess up indentation.  I think.  It's been a while...  All I really
> know for sure is that the generated code's indentation is messed up
> right there.
>
> > What are you asking exactly? that the caller be changed? (this does not
> > work well if there are multiple optional arguments..)
> >
> >     #if defined(TEST_IF_CMD_BAR)
> >         retval = qmp_test_if_cmd(arg.foo, arg.bar, &err);
> >     #else
> >         retval = qmp_test_if_cmd(arg.foo, &err);
> >     #endif /* defined(TEST_IF_CMD_BAR) */
>
> I'm asking for better indentation.  In handwritten code, we'd do
>
>         retval = qmp_test_if_cmd(arg.foo,
>     #if defined(TEST_IF_CMD_BAR)
>                                  arg.bar,
>     #endif /* defined(TEST_IF_CMD_BAR) */
>                                  &err);
>
> Keeping track of how far to indent the arguments is bothersome in the
> generator, though.  Perhaps we could create infrastructure to make it
> not bothersome, but I'm not asking for that.  Something like this should
> be good enough:
>
>         retval = qmp_test_if_cmd(arg.foo,
>     #if defined(TEST_IF_CMD_BAR)
>                     arg.bar,
>     #endif /* defined(TEST_IF_CMD_BAR) */
>                     &err);
>
> I.e. indent to the function call and then some.

ok, I improved the indentation a bit.

However, I think it would be simpler, and better, if we piped the
generated code to clang-format (when available). I made a simple patch
for that too.

>
> >> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> >> > index b5a8d03e8e..ba57e72c9b 100644
> >> > --- a/scripts/qapi/gen.py
> >> > +++ b/scripts/qapi/gen.py
> >> > @@ -111,22 +111,31 @@ def build_params(arg_type: 
> >> > Optional[QAPISchemaObjectType],
> >> >                   boxed: bool,
> >> >                   extra: Optional[str] = None) -> str:
> >> >      ret = ''
> >> > -    sep = ''
> >> >      if boxed:
> >> >          assert arg_type
> >> >          ret += '%s arg' % arg_type.c_param_type()
> >> > -        sep = ', '
> >> > +        if extra:
> >> > +            ret += ', '
> >> >      elif arg_type:
> >> >          assert not arg_type.variants
> >> > +        n = 0
> >> >          for memb in arg_type.members:
> >> > -            ret += sep
> >> > -            sep = ', '
> >> > +            n += 1
> >> > +            if memb.ifcond.is_present():
> >> > +                ret += '\n' + memb.ifcond.gen_if()
> >> >              if memb.need_has():
> >> >                  ret += 'bool has_%s, ' % c_name(memb.name)
> >> >              ret += '%s %s' % (memb.type.c_param_type(),
> >> >                                c_name(memb.name))
> >> > +            if extra or n != len(arg_type.members):
> >> > +                ret += ', '
> >> > +            else:
> >> > +                # FIXME: optional last argument may break compilation
> >> > +                assert not memb.ifcond.is_present()
> >>
> >> Does the assertion guard against the C compilation failure?
> >
> > Yes
> >
> >>
> >> Is it possible to write schema code that triggers it?
> >
> > Yes, the one we have for TEST_IF_EVENT for example:
> >
> > { 'event': 'TEST_IF_EVENT',
> >   'data': { 'foo': 'TestIfStruct',
> >             'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },
>
> This is the one you put in qapi-schema-test.json less the last
> parameter, so that the conditional parameter becomes the last one.
>
> > produces:
> >
> > void qapi_event_send_test_if_event(TestIfStruct *foo,
> > #if defined(TEST_IF_EVT_BAR)
> > TestIfEnumList *bar,
> > #endif /* defined(TEST_IF_EVT_BAR) */
> > );
> >
> > Which will fail to compile if TEST_IF_EVT_BAR is undefined.
>
> I think it'll fail to compile always, because the parameter list has a
> trailing comma regardless of TEST_IF_EVT_BAR.

Yes, I think I hand-wrote that example, the actual generator does not
leave a trailing comma here.

>
> > So I would rather assert that we don't introduce such a schema, until we
> > fix the code generator. Or we acknowledge the limitation, and treat it as a
> > schema error. Other ideas?
>
> Yes: throw an error.  Assertions are for programming errors.  This isn't
> a programming error, it's a limitation of the current implementation.
>
> How hard would it be to lift the limitation?

Taking this as a problematic example:

void function(first,
#ifdef A
    a,
#endif
#ifdef B
    b
#endif
)

I think it would mean that we would have to pass arguments as a
structure, as they don't have the limitation of trailing coma in
initializers. That would not be idiomatic C though, and we would need
to refactor a lot of code..

Another option is to always pass a dummy last argument? :)

void command(first,
#ifdef A
    a,
#endif
#ifdef B
    b,
#endif
    dummy)


>
> >> > +            if memb.ifcond.is_present():
> >> > +                ret += '\n' + memb.ifcond.gen_endif()
> >> >      if extra:
> >> > -        ret += sep + extra
> >> > +        ret += extra
> >> >      return ret if ret else 'void'
> >> >
> >> >
> >>
> >> Same newline issue as in gen_call().  Generated code:
> >>
> >>     UserDefThree *qmp_test_if_cmd(TestIfStruct *foo,
> >>     #if defined(TEST_IF_CMD_BAR)
> >>     TestIfEnum bar,
> >>     #endif /* defined(TEST_IF_CMD_BAR) */
> >>     Error **errp);
> >>
> >> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> >> > index 26a584ee4c..c56ea4d724 100644
> >> > --- a/scripts/qapi/visit.py
> >> > +++ b/scripts/qapi/visit.py
> >> > @@ -74,11 +74,13 @@ def gen_visit_object_members(name: str,
> >> >      sep = ''
> >> >      for memb in members:
> >> >          if memb.optional and not memb.need_has():
> >> > +            ret += memb.ifcond.gen_if()
> >> >              ret += mcgen('''
> >> >      bool has_%(c_name)s = !!obj->%(c_name)s;
> >> >  ''',
> >> >                           c_name=c_name(memb.name))
> >> >              sep = '\n'
> >> > +            ret += memb.ifcond.gen_endif()
> >> >      ret += sep
> >> >
> >> >      if base:
> >>
> >> This hunk has no effect on the code generated for our schemas as far as
> >> I can tell.  Is it superfluous?  Incorrect?  Gap in test coverage?  Or
> >> am I confused?
> >>
> >>
> > Right, we could change the test this way to exercise it:
> >
> > --- a/tests/qapi-schema/qapi-schema-test.json
> > +++ b/tests/qapi-schema/qapi-schema-test.json
> > @@ -250,7 +250,7 @@
> >  { 'command': 'test-if-cmd',
> >    'data': {
> >      'foo': 'TestIfStruct',
> > -    'bar': { 'type': 'TestIfEnum', 'if': 'TEST_IF_CMD_BAR' } },
> > +    '*bar': { 'type': 'TestIfStruct', 'if': 'TEST_IF_STRUCT' } },
> >    'returns': 'UserDefThree',
>
> Please exercise it :)

ok

>
> >> > diff --git a/tests/qapi-schema/qapi-schema-test.json
> >> b/tests/qapi-schema/qapi-schema-test.json
> >> > index ba7302f42b..baa4e69f63 100644
> >> > --- a/tests/qapi-schema/qapi-schema-test.json
> >> > +++ b/tests/qapi-schema/qapi-schema-test.json
> >> > @@ -258,7 +258,8 @@
> >> >
> >> >  { 'event': 'TEST_IF_EVENT',
> >> >    'data': { 'foo': 'TestIfStruct',
> >> > -            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' }
> >> },
> >> > +            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' },
> >> > +            'baz': 'int' },
> >> >    'if': { 'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT'] } }
> >> >
> >> >  { 'event': 'TEST_IF_EVENT2', 'data': {},
> >>
> >>
>
>

thanks

-- 
Marc-André Lureau