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 <stefa...@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
> >
signature.asc
Description: PGP signature
