Re: [PATCH v2 2/3] tracetool: introduce generate_unconditional

[Tanish Desai <tanishdesai37@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3386 bytes --]

> 1. nocheck isn't necessary anymore. The body of nocheck could be inlined
>   here instead to simplify the generated code.
Yes I agree.I will remove nocheck and inline the body of nocheck in
trace-foo
> 2. "if (%(cond)s) {" is only useful for backends that implement
>   .generate(). For example, if only the dtrace backend is enabled then
>   "if (trace_event_get_state(...)) {}" will be emitted unnecessarily.
Yes, we should remove unnecessary if (trace_event_get_state(...)){}
blocks.

But It is difficult because backend.generate() calls
_run_function("generate_%s", event, group) which in turn loops on all
backends.Format can't call generate for individual backends.I will need
to make this map in scripts/tracetool/backend/__init__.py:_run_function().(I
think this
will not be a good thing to do).

possible fix would be to create in scripts/tracetool/backend/__init__.py
def is_conditional(self, cond_check):
    self._run_function("generate_%s_conditional", cond_check)
now cond_check will be passed to all backends and backend's will have
def is_h_conditional(cond_check):
    cond_check = cond_check or True

Finally if cond_check==True in h.py I will generate "if
(trace_event_get_state(...)) {"
else not. As the same condition is re-used this solution would work well.
Since h.py only handles reused/shared logic, it's safe to assume consistent
conditions.
If a new backend requires a different condition, it's better handled
directly in backend/*.py.


On Tue, Jun 24, 2025 at 8:07 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Fri, Jun 20, 2025 at 02:37:19PM +0000, Tanish Desai wrote:
> > diff --git a/scripts/tracetool/format/h.py
> b/scripts/tracetool/format/h.py
> > index ea126b07ea..89d54b9aff 100644
> > --- a/scripts/tracetool/format/h.py
> > +++ b/scripts/tracetool/format/h.py
> > @@ -76,13 +76,17 @@ def generate(events, backend, group):
> >          out('',
> >              'static inline void %(api)s(%(args)s)',
> >              '{',
> > -            '    if (%(cond)s) {',
> > +            api=e.api(),
> > +            args=e.args)
> > +
> > +        if "disable" not in e.properties:
> > +            backend.generate_unconditional(e, group)
> > +
> > +        out('    if (%(cond)s) {',
> >              '        %(api_nocheck)s(%(names)s);',
> >              '    }',
> >              '}',
> > -            api=e.api(),
> >              api_nocheck=e.api(e.QEMU_TRACE_NOCHECK),
> > -            args=e.args,
> >              names=", ".join(e.args.names()),
> >              cond=cond)
>
> Two thoughts:
>
> 1. nocheck isn't necessary anymore. The body of nocheck could be inlined
>    here instead to simplify the generated code.
>
> 2. "if (%(cond)s) {" is only useful for backends that implement
>    .generate(). For example, if only the dtrace backend is enabled then
>    "if (trace_event_get_state(...)) {}" will be emitted unnecessarily.
>
> Maybe backends should have a .condition() interface so that
> scripts/tracetool/format/h.py:generate() can first collect a dict[cond]
> -> backend. Then it iterates over the map, calling backend.generate()
> within "if (%(cond)s) { ... }". That way only the conditions that are
> actually needed are generated and multiple backends that have the same
> condition will share the same if statement.
>
> Stefan
>

[-- Attachment #2: Type: text/html, Size: 5263 bytes --]