Hi
On Mon, Feb 20, 2023 at 12:10 PM Markus Armbruster <arm...@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lur...@redhat.com> writes:
>
> > Hi Markus
> >
> > On Fri, Feb 17, 2023 at 12:28 PM Markus Armbruster <arm...@redhat.com>
> > wrote:
> >
> >> marcandre.lur...@redhat.com writes:
> >>
> >> > From: Marc-André Lureau <marcandre.lur...@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.lur...@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
