[PATCH 0/2] tracetool: remove no_check_foo() and if(true){..} blocks

[PATCH 0/2] tracetool: remove no_check_foo() and if(true){..} blocks

[Tanish Desai]

This patch series eliminates unnecessary
if (true) { no_check_foo(...) } blocks and
integrates the no_check_foo(...) logic directly
into trace_foo(...). This results in cleaner,
more maintainable code generation.

A new backend attribute, TRACE_EVENT_GET_STATE,
is introduced. When enabled, it automatically
generates conditional block :
if (trace_event_get_state(...)) { ... }. The
generate() function emits code within this
conditional structure for that backend.

Previously, without TRACE_EVENT_GET_STATE,
each backend was required to manually implement
out("if (trace_event_get_state(...)) {") in its
generate() function, leading to code duplication.

Tanish Desai (2):
  tracetool: add CHECK_TRACE_EVENT_GET_STATE
  tracetool/format: remove redundent trace-event checks

 scripts/tracetool/__init__.py         |  1 -
 scripts/tracetool/backend/__init__.py | 26 ++++++++++++++++-------
 scripts/tracetool/backend/ftrace.py   |  4 +---
 scripts/tracetool/backend/log.py      |  6 ++----
 scripts/tracetool/backend/simple.py   |  9 ++------
 scripts/tracetool/backend/syslog.py   |  8 ++-----
 scripts/tracetool/format/h.py         | 30 ++++++++++-----------------
 7 files changed, 37 insertions(+), 47 deletions(-)

-- 
2.34.1

[PATCH 1/2] tracetool: add CHECK_TRACE_EVENT_GET_STATE

[Tanish Desai]

New attributed added in backends
CHECK_TRACE_EVENT_GET_STATE which when
present and is True wraps the code generated
by generate function in check_trace_event_get_state
check else it is outside the conditional block.

Signed-off-by: Tanish Desai <tanishdesai37@gmail.com>
---
 scripts/tracetool/__init__.py         |  1 -
 scripts/tracetool/backend/__init__.py | 26 ++++++++++++++++-------
 scripts/tracetool/format/h.py         | 30 ++++++++++-----------------
 3 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 2ae2e562d6..d0a02c45d7 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -332,7 +332,6 @@ def formats(self):
         return self._FMT.findall(self.fmt)
 
     QEMU_TRACE               = "trace_%(name)s"
-    QEMU_TRACE_NOCHECK       = "_nocheck__" + QEMU_TRACE
     QEMU_TRACE_TCG           = QEMU_TRACE + "_tcg"
     QEMU_DSTATE              = "_TRACE_%(NAME)s_DSTATE"
     QEMU_BACKEND_DSTATE      = "TRACE_%(NAME)s_BACKEND_DSTATE"
diff --git a/scripts/tracetool/backend/__init__.py b/scripts/tracetool/backend/__init__.py
index 7bfcc86cc5..3c900f01e9 100644
--- a/scripts/tracetool/backend/__init__.py
+++ b/scripts/tracetool/backend/__init__.py
@@ -23,6 +23,8 @@
 Attribute Description
 ========= ====================================================================
 PUBLIC    If exists and is set to 'True', the backend is considered "public".
+CHECK_TRACE_EVENT_GET_STATE    If exists and is set to 'True', generate function
+emits code inside check_trace_event_get_state check.
 ========= ====================================================================
 
 
@@ -101,22 +103,32 @@ class Wrapper:
     def __init__(self, backends, format):
         self._backends = [backend.replace("-", "_") for backend in backends]
         self._format = format.replace("-", "_")
+        self.check_trace_event_get_state = False
         for backend in self._backends:
             assert exists(backend)
         assert tracetool.format.exists(self._format)
+        for backend in self.backend_modules():
+            check_trace_event_get_state = getattr(backend, "CHECK_TRACE_EVENT_GET_STATE", False)
+            self.check_trace_event_get_state = self.check_trace_event_get_state or check_trace_event_get_state
 
-    def _run_function(self, name, *args, **kwargs):
+    def backend_modules(self):
         for backend in self._backends:
-            func = tracetool.try_import("tracetool.backend." + backend,
-                                        name % self._format, None)[1]
-            if func is not None:
-                func(*args, **kwargs)
+             module = tracetool.try_import("tracetool.backend." + backend)[1]
+             if module is not None:
+                 yield module
+
+    def _run_function(self, name, *args, check_trace_event_get_state=None, **kwargs):
+        for backend in self.backend_modules():
+            func = getattr(backend,name%self._format,None)
+            if func is not None and (check_trace_event_get_state is None or
+                    check_trace_event_get_state == getattr(backend, 'CHECK_TRACE_EVENT_GET_STATE', False)):
+                    func(*args, **kwargs)
 
     def generate_begin(self, events, group):
         self._run_function("generate_%s_begin", events, group)
 
-    def generate(self, event, group):
-        self._run_function("generate_%s", event, group)
+    def generate(self, event, group, check_trace_event_get_state=False):
+        self._run_function("generate_%s", event, group, check_trace_event_get_state=check_trace_event_get_state)
 
     def generate_backend_dstate(self, event, group):
         self._run_function("generate_%s_backend_dstate", event, group)
diff --git a/scripts/tracetool/format/h.py b/scripts/tracetool/format/h.py
index ea126b07ea..0ceb49eef5 100644
--- a/scripts/tracetool/format/h.py
+++ b/scripts/tracetool/format/h.py
@@ -59,33 +59,25 @@ def generate(events, backend, group):
 
         out('    false)')
 
-        # tracer without checks
         out('',
             'static inline void %(api)s(%(args)s)',
             '{',
-            api=e.api(e.QEMU_TRACE_NOCHECK),
+            api=e.api(),
             args=e.args)
 
         if "disable" not in e.properties:
-            backend.generate(e, group)
-
+            backend.generate(e, group, check_trace_event_get_state=False)
+
+        if backend.check_trace_event_get_state:
+            if "disable" not in e.properties:
+                event_id = 'TRACE_' + e.name.upper()
+                cond = "trace_event_get_state(%s)" % event_id
+                out('    if (%(cond)s) {',
+                        cond=cond)
+                backend.generate(e, group, check_trace_event_get_state=True)
+                out('    }')
         out('}')
 
-        cond = "true"
-
-        out('',
-            'static inline void %(api)s(%(args)s)',
-            '{',
-            '    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)
-
     backend.generate_end(events, group)
 
     out('#endif /* TRACE_%s_GENERATED_TRACERS_H */' % group.upper())
-- 
2.34.1

Re: [PATCH 1/2] tracetool: add CHECK_TRACE_EVENT_GET_STATE

[Daniel P. Berrangé]

On Mon, Aug 04, 2025 at 11:20:38AM +0000, Tanish Desai wrote:
> New attributed added in backends
> CHECK_TRACE_EVENT_GET_STATE which when
> present and is True wraps the code generated
> by generate function in check_trace_event_get_state
> check else it is outside the conditional block.
> 
> Signed-off-by: Tanish Desai <tanishdesai37@gmail.com>
> ---
>  scripts/tracetool/__init__.py         |  1 -
>  scripts/tracetool/backend/__init__.py | 26 ++++++++++++++++-------
>  scripts/tracetool/format/h.py         | 30 ++++++++++-----------------
>  3 files changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> index 2ae2e562d6..d0a02c45d7 100644
> --- a/scripts/tracetool/__init__.py
> +++ b/scripts/tracetool/__init__.py
> @@ -332,7 +332,6 @@ def formats(self):
>          return self._FMT.findall(self.fmt)
>  
>      QEMU_TRACE               = "trace_%(name)s"
> -    QEMU_TRACE_NOCHECK       = "_nocheck__" + QEMU_TRACE
>      QEMU_TRACE_TCG           = QEMU_TRACE + "_tcg"
>      QEMU_DSTATE              = "_TRACE_%(NAME)s_DSTATE"
>      QEMU_BACKEND_DSTATE      = "TRACE_%(NAME)s_BACKEND_DSTATE"
> diff --git a/scripts/tracetool/backend/__init__.py b/scripts/tracetool/backend/__init__.py
> index 7bfcc86cc5..3c900f01e9 100644
> --- a/scripts/tracetool/backend/__init__.py
> +++ b/scripts/tracetool/backend/__init__.py
> @@ -23,6 +23,8 @@
>  Attribute Description
>  ========= ====================================================================
>  PUBLIC    If exists and is set to 'True', the backend is considered "public".
> +CHECK_TRACE_EVENT_GET_STATE    If exists and is set to 'True', generate function
> +emits code inside check_trace_event_get_state check.
>  ========= ====================================================================
>  
>  
> @@ -101,22 +103,32 @@ class Wrapper:
>      def __init__(self, backends, format):
>          self._backends = [backend.replace("-", "_") for backend in backends]
>          self._format = format.replace("-", "_")
> +        self.check_trace_event_get_state = False
>          for backend in self._backends:
>              assert exists(backend)
>          assert tracetool.format.exists(self._format)
> +        for backend in self.backend_modules():
> +            check_trace_event_get_state = getattr(backend, "CHECK_TRACE_EVENT_GET_STATE", False)
> +            self.check_trace_event_get_state = self.check_trace_event_get_state or check_trace_event_get_state
>  
> -    def _run_function(self, name, *args, **kwargs):
> +    def backend_modules(self):
>          for backend in self._backends:
> -            func = tracetool.try_import("tracetool.backend." + backend,
> -                                        name % self._format, None)[1]
> -            if func is not None:
> -                func(*args, **kwargs)
> +             module = tracetool.try_import("tracetool.backend." + backend)[1]
> +             if module is not None:
> +                 yield module
> +
> +    def _run_function(self, name, *args, check_trace_event_get_state=None, **kwargs):
> +        for backend in self.backend_modules():
> +            func = getattr(backend,name%self._format,None)
> +            if func is not None and (check_trace_event_get_state is None or
> +                    check_trace_event_get_state == getattr(backend, 'CHECK_TRACE_EVENT_GET_STATE', False)):
> +                    func(*args, **kwargs)

Something here is broken with the "simple" trace backend, as the entire
'.c' file content is no longer generated, causing link errors

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] tracetool: add CHECK_TRACE_EVENT_GET_STATE

[Paolo Bonzini]

On 8/5/25 22:11, Daniel P. Berrangé wrote:
> Something here is broken with the "simple" trace backend, as the entire
> '.c' file content is no longer generated, causing link errors

I think the bug is here:

+    def generate(self, event, group, check_trace_event_get_state=False):
+        self._run_function("generate_%s", event, group, check_trace_event_get_state=check_trace_event_get_state)

The default must be None, i.e. do not filter at all, instead of False.

Paolo

Re: [PATCH 1/2] tracetool: add CHECK_TRACE_EVENT_GET_STATE

[Tanish Desai]

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

Thanks for pointing out the bug and suggesting a fix for it.

On Wed, Aug 6, 2025 at 4:02 AM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 8/5/25 22:11, Daniel P. Berrangé wrote:
> > Something here is broken with the "simple" trace backend, as the entire
> > '.c' file content is no longer generated, causing link errors
>
> I think the bug is here:
>
Yes this was the bug.

> +    def generate(self, event, group, check_trace_event_get_state=False):
> +        self._run_function("generate_%s", event, group,
> check_trace_event_get_state=check_trace_event_get_state)
>
> The default must be None, i.e. do not filter at all, instead of False.
>
> Paolo
>
>

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

[PATCH 2/2] tracetool/format: remove redundent trace-event checks

[Tanish Desai]

Remove redundent if(check_trace_event) check
from individual backends.
Adding CHECK_TRACE_EVENT_GET_STATE in log,syslog,
dtrace and simple backend.

Signed-off-by: Tanish Desai <tanishdesai37@gmail.com>
---
 scripts/tracetool/backend/ftrace.py | 4 +---
 scripts/tracetool/backend/log.py    | 6 ++----
 scripts/tracetool/backend/simple.py | 9 ++-------
 scripts/tracetool/backend/syslog.py | 8 ++------
 4 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/scripts/tracetool/backend/ftrace.py b/scripts/tracetool/backend/ftrace.py
index 5fa30ccc08..b38f527461 100644
--- a/scripts/tracetool/backend/ftrace.py
+++ b/scripts/tracetool/backend/ftrace.py
@@ -16,6 +16,7 @@
 
 
 PUBLIC = True
+CHECK_TRACE_EVENT_GET_STATE = True
 
 
 def generate_h_begin(events, group):
@@ -32,18 +33,15 @@ def generate_h(event, group):
         '        char ftrace_buf[MAX_TRACE_STRLEN];',
         '        int unused __attribute__ ((unused));',
         '        int trlen;',
-        '        if (trace_event_get_state(%(event_id)s)) {',
         '#line %(event_lineno)d "%(event_filename)s"',
         '            trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,',
         '                             "%(name)s " %(fmt)s "\\n" %(argnames)s);',
         '#line %(out_next_lineno)d "%(out_filename)s"',
         '            trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);',
         '            unused = write(trace_marker_fd, ftrace_buf, trlen);',
-        '        }',
         '    }',
         name=event.name,
         args=event.args,
-        event_id="TRACE_" + event.name.upper(),
         event_lineno=event.lineno,
         event_filename=event.filename,
         fmt=event.fmt.rstrip("\n"),
diff --git a/scripts/tracetool/backend/log.py b/scripts/tracetool/backend/log.py
index eb50ceea34..d8406d226b 100644
--- a/scripts/tracetool/backend/log.py
+++ b/scripts/tracetool/backend/log.py
@@ -16,6 +16,7 @@
 
 
 PUBLIC = True
+CHECK_TRACE_EVENT_GET_STATE = True
 
 
 def generate_h_begin(events, group):
@@ -28,14 +29,11 @@ def generate_h(event, group):
     if len(event.args) > 0:
         argnames = ", " + argnames
 
-    cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper())
-
-    out('    if (%(cond)s && qemu_loglevel_mask(LOG_TRACE)) {',
+    out('    if (qemu_loglevel_mask(LOG_TRACE)) {',
         '#line %(event_lineno)d "%(event_filename)s"',
         '        qemu_log("%(name)s " %(fmt)s "\\n"%(argnames)s);',
         '#line %(out_next_lineno)d "%(out_filename)s"',
         '    }',
-        cond=cond,
         event_lineno=event.lineno,
         event_filename=event.filename,
         name=event.name,
diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
index 7c84c06b20..623ea7d8ed 100644
--- a/scripts/tracetool/backend/simple.py
+++ b/scripts/tracetool/backend/simple.py
@@ -16,7 +16,7 @@
 
 
 PUBLIC = True
-
+CHECK_TRACE_EVENT_GET_STATE = True
 
 def is_string(arg):
     strtype = ('const char*', 'char*', 'const char *', 'char *')
@@ -36,13 +36,8 @@ def generate_h_begin(events, group):
 
 
 def generate_h(event, group):
-    event_id = 'TRACE_' + event.name.upper()
-    cond = "trace_event_get_state(%s)" % event_id
-    out('    if (%(cond)s) {',
-        '        _simple_%(api)s(%(args)s);',
-        '    }',
+    out('        _simple_%(api)s(%(args)s);',
         api=event.api(),
-        cond=cond,
         args=", ".join(event.args.names()))
 
 
diff --git a/scripts/tracetool/backend/syslog.py b/scripts/tracetool/backend/syslog.py
index 3f82e54aab..04ec85717a 100644
--- a/scripts/tracetool/backend/syslog.py
+++ b/scripts/tracetool/backend/syslog.py
@@ -16,6 +16,7 @@
 
 
 PUBLIC = True
+CHECK_TRACE_EVENT_GET_STATE = True
 
 
 def generate_h_begin(events, group):
@@ -28,14 +29,9 @@ def generate_h(event, group):
     if len(event.args) > 0:
         argnames = ", " + argnames
 
-    cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper())
-
-    out('    if (%(cond)s) {',
-        '#line %(event_lineno)d "%(event_filename)s"',
+    out('#line %(event_lineno)d "%(event_filename)s"',
         '        syslog(LOG_INFO, "%(name)s " %(fmt)s %(argnames)s);',
         '#line %(out_next_lineno)d "%(out_filename)s"',
-        '    }',
-        cond=cond,
         event_lineno=event.lineno,
         event_filename=event.filename,
         name=event.name,
-- 
2.34.1

Re: [PATCH 2/2] tracetool/format: remove redundent trace-event checks

[Daniel P. Berrangé]

On Mon, Aug 04, 2025 at 11:20:39AM +0000, Tanish Desai wrote:
> Remove redundent if(check_trace_event) check
> from individual backends.
> Adding CHECK_TRACE_EVENT_GET_STATE in log,syslog,
> dtrace and simple backend.
> 
> Signed-off-by: Tanish Desai <tanishdesai37@gmail.com>
> ---
>  scripts/tracetool/backend/ftrace.py | 4 +---
>  scripts/tracetool/backend/log.py    | 6 ++----
>  scripts/tracetool/backend/simple.py | 9 ++-------
>  scripts/tracetool/backend/syslog.py | 8 ++------
>  4 files changed, 7 insertions(+), 20 deletions(-)
> 
> diff --git a/scripts/tracetool/backend/ftrace.py b/scripts/tracetool/backend/ftrace.py
> index 5fa30ccc08..b38f527461 100644
> --- a/scripts/tracetool/backend/ftrace.py
> +++ b/scripts/tracetool/backend/ftrace.py
> @@ -16,6 +16,7 @@
>  
>  
>  PUBLIC = True
> +CHECK_TRACE_EVENT_GET_STATE = True
>  
>  
>  def generate_h_begin(events, group):
> @@ -32,18 +33,15 @@ def generate_h(event, group):
>          '        char ftrace_buf[MAX_TRACE_STRLEN];',
>          '        int unused __attribute__ ((unused));',
>          '        int trlen;',
> -        '        if (trace_event_get_state(%(event_id)s)) {',
>          '#line %(event_lineno)d "%(event_filename)s"',
>          '            trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,',
>          '                             "%(name)s " %(fmt)s "\\n" %(argnames)s);',
>          '#line %(out_next_lineno)d "%(out_filename)s"',
>          '            trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);',
>          '            unused = write(trace_marker_fd, ftrace_buf, trlen);',
> -        '        }',
>          '    }',
>          name=event.name,
>          args=event.args,
> -        event_id="TRACE_" + event.name.upper(),
>          event_lineno=event.lineno,
>          event_filename=event.filename,
>          fmt=event.fmt.rstrip("\n"),
> diff --git a/scripts/tracetool/backend/log.py b/scripts/tracetool/backend/log.py
> index eb50ceea34..d8406d226b 100644
> --- a/scripts/tracetool/backend/log.py
> +++ b/scripts/tracetool/backend/log.py
> @@ -16,6 +16,7 @@
>  
>  
>  PUBLIC = True
> +CHECK_TRACE_EVENT_GET_STATE = True
>  
>  
>  def generate_h_begin(events, group):
> @@ -28,14 +29,11 @@ def generate_h(event, group):
>      if len(event.args) > 0:
>          argnames = ", " + argnames
>  
> -    cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper())
> -
> -    out('    if (%(cond)s && qemu_loglevel_mask(LOG_TRACE)) {',
> +    out('    if (qemu_loglevel_mask(LOG_TRACE)) {',
>          '#line %(event_lineno)d "%(event_filename)s"',
>          '        qemu_log("%(name)s " %(fmt)s "\\n"%(argnames)s);',
>          '#line %(out_next_lineno)d "%(out_filename)s"',
>          '    }',
> -        cond=cond,
>          event_lineno=event.lineno,
>          event_filename=event.filename,
>          name=event.name,

This change results in bad indentation of the generated inline function
nested if conditions. e.g. the result looks like this:

static inline void trace_test_blah(void *context, const char *filename)
{
    if (trace_event_get_state(TRACE_TEST_BLAH)) {
    if (qemu_loglevel_mask(LOG_TRACE)) {
#line 4 "trace-events"
        qemu_log("test_blah " "Blah context=%p filename=%s" "\n", context, filename);
#line 29 "../../../../../../../../dev/stdout"
    }
    }
}


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/2] tracetool: remove no_check_foo() and if(true){..} blocks

[Daniel P. Berrangé]

On Mon, Aug 04, 2025 at 11:20:37AM +0000, Tanish Desai wrote:
> This patch series eliminates unnecessary
> if (true) { no_check_foo(...) } blocks and
> integrates the no_check_foo(...) logic directly
> into trace_foo(...). This results in cleaner,
> more maintainable code generation.
> 
> A new backend attribute, TRACE_EVENT_GET_STATE,
> is introduced. When enabled, it automatically
> generates conditional block :
> if (trace_event_get_state(...)) { ... }. The
> generate() function emits code within this
> conditional structure for that backend.
> 
> Previously, without TRACE_EVENT_GET_STATE,
> each backend was required to manually implement
> out("if (trace_event_get_state(...)) {") in its
> generate() function, leading to code duplication.

FWIW, I rebased your patches on top of this series
just posted, which is how I found the two bugs
pointed out against the patches

  https://lists.nongnu.org/archive/html/qemu-devel/2025-08/msg00942.html


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|