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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Tue, Jun 24, 2025 at 11:07:34PM +0530, Tanish Desai wrote:
> > 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.

To me a generic solution that doesn't hard-code trace_event_get_state()
is nicer, but it's okay if you want to introduce the concept of
"conditional" meaning specifically trace_event_get_state() since there
are no other cases where we want to merge two conditionals. I don't
really mind.

> 
> 
> 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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]