qapi/command: Avoid generating unused qmp_marshal_output_T()

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 recently.  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 marshaling 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>
Message-ID: <20250804130602.903904-1-armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
[Commit message typos fixed]

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index d97602f464c7ee6f20a90dbbbc3bbd1a89a7a0c7..3a632b4a648adf26ff8168491e9e95d913d4191e 100644 (file)
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -1809,27 +1809,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));
@@ -1857,7 +1843,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 791422738283da4ea8f35a7bf22035b3bf3cd8ea..a82b5a2a5e62a7d135c70306667981837d0fcb58 100644 (file)
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -14,15 +14,12 @@ See the COPYING file in the top-level directory.
 """
 
 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 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
             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 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
                       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))