[PATCH] qapi/command: Avoid generating unused qmp_marshal_output_T()

[PATCH] qapi/command: Avoid generating unused qmp_marshal_output_T()

[Markus Armbruster]

qmp_marshal_output_T() is only ever called by qmp_marshal_C() for a
command C that returns type T.

We've always generated it as a static function on demand, i.e. when we
generate a call.

Since we split up monolithic generated code into modules (commit
252dc3105fc "qapi: Generate separate .h, .c for each module"), we do
this per module.  As noted in the commit message, this can result in
identical (static) qmp_marshal_output_T() in several modules.  Was
deemed not worth avoiding.

A bit later, we added 'if' conditionals to the schema language (merge
commit 5dafaf4fbce).

When a conditional definition uses a type, then its condition must
imply the type's condition.  We made this the user's responsibility.
Hasn't been an issue in practice.

However, the sharing of qmp_marshal_output_T() among commands
complicates matters.  To avoid both undefined function errors and
unused function warnings, qmp_marshal_output_T() must be defined
exactly when it's used.  It is used when any of the qmp_marshal_C()
calling it is defined, i.e. when any C's condition holds.

The generator uses T's condition instead.  To avoid both error and
warning, T's condition must be the conjunction of all C's conditions.

Unfortunately, this can be impossible:

* Conditional command returning a builtin type

  A builtin type cannot be conditional.  This is noted in a FIXME
  comment.

* Commands in multiple modules where the conjunction differs between
  modules

  An instance of this came up recenrly.  we have unconditional
  commands returning HumanReadableText.  If we add a conditional one
  to a module that does not have unconditional ones, compilation fails
  with "defined but not used".  If we make HumanReadableText
  conditional to fix this module, we break the others.

Instead of complicating the code to compute the conjunction, simplify
it: generate the output marshalling code right into qmp_marshal_C().

This duplicates it when multiple commands return the same type.  The
impact on code size is negligible: qemu-system-x86_64's text segment
grows by 1448 bytes.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.rst | 25 ++++++++------------
 scripts/qapi/commands.py     | 44 ++++++++----------------------------
 2 files changed, 19 insertions(+), 50 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index dfdbeac5a5..091e4f6f4a 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -1804,27 +1804,13 @@ Example::
     $ cat qapi-generated/example-qapi-commands.c
     [Uninteresting stuff omitted...]
 
-    static void qmp_marshal_output_UserDefOne(UserDefOne *ret_in,
-                                    QObject **ret_out, Error **errp)
-    {
-        Visitor *v;
-
-        v = qobject_output_visitor_new_qmp(ret_out);
-        if (visit_type_UserDefOne(v, "unused", &ret_in, errp)) {
-            visit_complete(v, ret_out);
-        }
-        visit_free(v);
-        v = qapi_dealloc_visitor_new();
-        visit_type_UserDefOne(v, "unused", &ret_in, NULL);
-        visit_free(v);
-    }
-
     void qmp_marshal_my_command(QDict *args, QObject **ret, Error **errp)
     {
         Error *err = NULL;
         bool ok = false;
         Visitor *v;
         UserDefOne *retval;
+        Visitor *ov;
         q_obj_my_command_arg arg = {0};
 
         v = qobject_input_visitor_new_qmp(QOBJECT(args));
@@ -1852,7 +1838,14 @@ Example::
             goto out;
         }
 
-        qmp_marshal_output_UserDefOne(retval, ret, errp);
+        ov = qobject_output_visitor_new_qmp(ret);
+        if (visit_type_UserDefOne(ov, "unused", &retval, errp)) {
+            visit_complete(ov, ret);
+        }
+        visit_free(ov);
+        ov = qapi_dealloc_visitor_new();
+        visit_type_UserDefOne(ov, "unused", &retval, NULL);
+        visit_free(ov);
 
         if (trace_event_get_state_backends(TRACE_QMP_EXIT_MY_COMMAND)) {
             g_autoptr(GString) ret_json = qobject_to_json(*ret);
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 7914227382..a82b5a2a5e 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -14,15 +14,12 @@
 """
 
 from typing import (
-    Dict,
     List,
     Optional,
-    Set,
 )
 
 from .common import c_name, mcgen
 from .gen import (
-    QAPIGenC,
     QAPISchemaModularCVisitor,
     build_params,
     gen_features,
@@ -112,11 +109,7 @@ def gen_call(name: str,
 ''')
 
     if ret_type:
-        ret += mcgen('''
-
-    qmp_marshal_output_%(c_name)s(retval, ret, errp);
-''',
-                     c_name=ret_type.c_name())
+        ret += gen_marshal_output(ret_type)
 
     if gen_tracing:
         if ret_type:
@@ -142,22 +135,16 @@ def gen_call(name: str,
 def gen_marshal_output(ret_type: QAPISchemaType) -> str:
     return mcgen('''
 
-static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
-                                QObject **ret_out, Error **errp)
-{
-    Visitor *v;
-
-    v = qobject_output_visitor_new_qmp(ret_out);
-    if (visit_type_%(c_name)s(v, "unused", &ret_in, errp)) {
-        visit_complete(v, ret_out);
+    ov = qobject_output_visitor_new_qmp(ret);
+    if (visit_type_%(c_name)s(ov, "unused", &retval, errp)) {
+        visit_complete(ov, ret);
     }
-    visit_free(v);
-    v = qapi_dealloc_visitor_new();
-    visit_type_%(c_name)s(v, "unused", &ret_in, NULL);
-    visit_free(v);
-}
+    visit_free(ov);
+    ov = qapi_dealloc_visitor_new();
+    visit_type_%(c_name)s(ov, "unused", &retval, NULL);
+    visit_free(ov);
 ''',
-                 c_type=ret_type.c_type(), c_name=ret_type.c_name())
+                 c_name=ret_type.c_name())
 
 
 def build_marshal_proto(name: str,
@@ -209,6 +196,7 @@ def gen_marshal(name: str,
     if ret_type:
         ret += mcgen('''
     %(c_type)s retval;
+    Visitor *ov;
 ''',
                      c_type=ret_type.c_type())
 
@@ -308,11 +296,9 @@ def __init__(self, prefix: str, gen_tracing: bool):
             prefix, 'qapi-commands',
             ' * Schema-defined QAPI/QMP commands', None, __doc__,
             gen_tracing=gen_tracing)
-        self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
         self._gen_tracing = gen_tracing
 
     def _begin_user_module(self, name: str) -> None:
-        self._visited_ret_types[self._genc] = set()
         commands = self._module_basename('qapi-commands', name)
         types = self._module_basename('qapi-types', name)
         visit = self._module_basename('qapi-visit', name)
@@ -386,16 +372,6 @@ def visit_command(self,
                       coroutine: bool) -> None:
         if not gen:
             return
-        # FIXME: If T is a user-defined type, the user is responsible
-        # for making this work, i.e. to make T's condition the
-        # conjunction of the T-returning commands' conditions.  If T
-        # is a built-in type, this isn't possible: the
-        # qmp_marshal_output_T() will be generated unconditionally.
-        if ret_type and ret_type not in self._visited_ret_types[self._genc]:
-            self._visited_ret_types[self._genc].add(ret_type)
-            with ifcontext(ret_type.ifcond,
-                           self._genh, self._genc):
-                self._genc.add(gen_marshal_output(ret_type))
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_command_decl(name, arg_type, boxed,
                                             ret_type, coroutine))
-- 
2.49.0

Re: [PATCH] qapi/command: Avoid generating unused qmp_marshal_output_T()

[Philippe Mathieu-Daudé]

On 4/8/25 15:06, Markus Armbruster wrote:
> qmp_marshal_output_T() is only ever called by qmp_marshal_C() for a
> command C that returns type T.
> 
> We've always generated it as a static function on demand, i.e. when we
> generate a call.
> 
> Since we split up monolithic generated code into modules (commit
> 252dc3105fc "qapi: Generate separate .h, .c for each module"), we do
> this per module.  As noted in the commit message, this can result in
> identical (static) qmp_marshal_output_T() in several modules.  Was
> deemed not worth avoiding.
> 
> A bit later, we added 'if' conditionals to the schema language (merge
> commit 5dafaf4fbce).
> 
> When a conditional definition uses a type, then its condition must
> imply the type's condition.  We made this the user's responsibility.
> Hasn't been an issue in practice.
> 
> However, the sharing of qmp_marshal_output_T() among commands
> complicates matters.  To avoid both undefined function errors and
> unused function warnings, qmp_marshal_output_T() must be defined
> exactly when it's used.  It is used when any of the qmp_marshal_C()
> calling it is defined, i.e. when any C's condition holds.
> 
> The generator uses T's condition instead.  To avoid both error and
> warning, T's condition must be the conjunction of all C's conditions.
> 
> Unfortunately, this can be impossible:
> 
> * Conditional command returning a builtin type
> 
>    A builtin type cannot be conditional.  This is noted in a FIXME
>    comment.
> 
> * Commands in multiple modules where the conjunction differs between
>    modules
> 
>    An instance of this came up recenrly.  we have unconditional
>    commands returning HumanReadableText.  If we add a conditional one
>    to a module that does not have unconditional ones, compilation fails
>    with "defined but not used".  If we make HumanReadableText
>    conditional to fix this module, we break the others.
> 
> Instead of complicating the code to compute the conjunction, simplify
> it: generate the output marshalling code right into qmp_marshal_C().
> 
> This duplicates it when multiple commands return the same type.  The
> impact on code size is negligible: qemu-system-x86_64's text segment
> grows by 1448 bytes.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   docs/devel/qapi-code-gen.rst | 25 ++++++++------------
>   scripts/qapi/commands.py     | 44 ++++++++----------------------------
>   2 files changed, 19 insertions(+), 50 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] qapi/command: Avoid generating unused qmp_marshal_output_T()

[Markus Armbruster]

Queued.