Re: [PATCH v3 2/3] scripts/qapi-gen.py: add --add-trace-events option

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 18.01.2022 13:27, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> Add and option to generate trace events. We should generate both trace
>>> events and trace-events files for further trace events code generation.
>> 
>> Can you explain why we want trace generation to be optional?
>
> Because I failed make it work for tests and qga.. And seems there no good reason for it: there now trace events for now neither in tests nor in qga.
>
> I've now tried again.
>
> It doesn't work, as I understand, the problem is qga subdir goes after trace subdir, so when we generate trace headers, we didn't yet processed qga subdir.
>
> And I can't just move qga above trace: qga depends on qemutil variable so it should go after it. And if I put 'trace' subdir under qemuutil declaration it doesn't work too (seems because qemuutil depends on trace_ss)..
>
> So, supporting auto-generated trace points for qga qmp commands requires some deeper refactoring.

Similar trouble with tests?

The normal case seems to be "generate trace code", with an exception for
cases where our build system defeats that.  Agree?

If yes, I'd prefer to default to "generate trace code", and have an
option to suppress it, with suitable TODO comment(s) explaining why.

[...]

>>> @@ -122,10 +167,17 @@ def gen_marshal_decl(name: str) -> str:
>>>                    proto=build_marshal_proto(name))
>>>   
>>>   
>>> +def gen_trace(name: str) -> str:
>>> +    name = c_name(name)
>>> +    return f"""\
>>> +qmp_enter_{name}(const char *json) "%s"\n
>>> +qmp_exit_{name}(const char *result, bool succeeded) "%s %d"\n"""
>> 
>> Why not mcgen()?
>
> Hmm.. Here we don't need any indentation for sure. Do you think we still want mcgen for consistancy and not use f-string?

Let's stick to mcgen() for generating code.

>> The generated FOO.trace-events look like this:
>> 
>>      $ cat bld-clang/qapi/qapi-commands-control.trace-events
>>      qmp_enter_qmp_capabilities(const char *json) "%s"
>> 
>>      qmp_exit_qmp_capabilities(const char *result, bool succeeded) "%s %d"
>>      qmp_enter_query_version(const char *json) "%s"
>> 
>>      qmp_exit_query_version(const char *result, bool succeeded) "%s %d"
>>      qmp_enter_query_commands(const char *json) "%s"
>> 
>>      qmp_exit_query_commands(const char *result, bool succeeded) "%s %d"
>>      qmp_enter_quit(const char *json) "%s"
>> 
>>      qmp_exit_quit(const char *result, bool succeeded) "%s %d"
>> 
>> Either drop the blank lines, or put them between the pairs instead of
>> within.  I'd do the former.
>> 
>> We generate lots of empty FOO.trace-events.  I guess that's okay.
>> 
>>> +
>> 
>> scripts/qapi/commands.py:176:1: E302 expected 2 blank lines, found 1
>> 
>>>   def gen_marshal(name: str,
>>>                   arg_type: Optional[QAPISchemaObjectType],
>>>                   boxed: bool,
>>> -                ret_type: Optional[QAPISchemaType]) -> str:
>>> +                ret_type: Optional[QAPISchemaType],
>>> +                add_trace_events: bool) -> str:
>>>       have_args = boxed or (arg_type and not arg_type.is_empty())
>>>       if have_args:
>>>           assert arg_type is not None
>>> @@ -180,7 +232,7 @@ def gen_marshal(name: str,
>>>       }
>>>   ''')
>>>   
>>> -    ret += gen_call(name, arg_type, boxed, ret_type)
>>> +    ret += gen_call(name, arg_type, boxed, ret_type, add_trace_events)
>>>   
>>>       ret += mcgen('''
>>>   
>>> @@ -238,11 +290,12 @@ def gen_register_command(name: str,
>>>   
>>>   
>>>   class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
>>> -    def __init__(self, prefix: str):
>>> +    def __init__(self, prefix: str, add_trace_events: bool):
>>>           super().__init__(
>>>               prefix, 'qapi-commands',
>>>               ' * Schema-defined QAPI/QMP commands', None, __doc__)
>>>           self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
>>> +        self.add_trace_events = add_trace_events
>>>   
>>>       def _begin_user_module(self, name: str) -> None:
>>>           self._visited_ret_types[self._genc] = set()
>>> @@ -261,6 +314,15 @@ def _begin_user_module(self, name: str) -> None:
>>>   
>>>   ''',
>>>                                commands=commands, visit=visit))
>>> +
>>> +        if self.add_trace_events and c_name(commands) != 'qapi_commands':
>>> +            self._genc.add(mcgen('''
>>> +#include "trace/trace-qapi.h"
>>> +#include "qapi/qmp/qjson.h"
>>> +#include "trace/trace-%(nm)s_trace_events.h"
>>> +''',
>>> +                                 nm=c_name(commands)))
>> 
>> Why c_name(commands), and not just commands?
>
> Because generated files has underscores instead of '-'. Looking at code, I think it's because underscorify() in trace/meson.build when we create group_name variable.

I see.  We first generate output modules like

    qapi/qapi-commands-machine-target.c
    qapi/qapi-commands-machine-target.h
    qapi/qapi-commands-machine-target.trace-events

and then from the latter their trace code like

    trace/trace-qapi_commands_machine_target_trace_events.h
    trace/trace-qapi_commands_machine_target_trace_events.c

These file names is ugly, but not this patch's fault.

Normally, the foo/bar/*.c include "trace.h" (handwritten one-liner),
which includes "trace/trace-foo_bar.h" generated from
foo/bar/trace-events.

Here, we include them directly, because we generate per file, not per
directory.

To actually match .underscorify(), you have to use c_name(commands,
protect=False).

Also add a comment that points to the .underscorify().

[...]