Re: [PATCH v3] tracetool: generates conitional checks when needed

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Wed, Jun 25, 2025 at 12:30:23PM +0000, Tanish Desai wrote:
> Adds generate_conditional, allowing backends to wrap generate()
> output in a trace_event_get_state(...) check if needed.
> 
> Removes no_check by inlining its logic into trace_foo(...).
> Also ensures the generated code is formatted properly.
> 
> Signed-off-by: Tanish Desai <tanishdesai37@gmail.com>
> ---
>  scripts/tracetool/backend/__init__.py |  3 +++
>  scripts/tracetool/backend/ftrace.py   | 16 +++++++-------
>  scripts/tracetool/backend/log.py      | 26 +++++++++++++----------
>  scripts/tracetool/backend/simple.py   |  4 ++++
>  scripts/tracetool/backend/syslog.py   |  4 ++++
>  scripts/tracetool/format/h.py         | 30 ++++++++++-----------------
>  6 files changed, 46 insertions(+), 37 deletions(-)

This patch does not apply to qemu.git/master. When posting new revisions
of patches, please resend the entire patch series that this belongs to
or send it as a separate patch with the Based-on: <message-id> trailer
to let people (and tools) know which unmerged patch series it depends
on.

> 
> diff --git a/scripts/tracetool/backend/__init__.py b/scripts/tracetool/backend/__init__.py
> index c4456a5efd..dc0806f8d0 100644
> --- a/scripts/tracetool/backend/__init__.py
> +++ b/scripts/tracetool/backend/__init__.py
> @@ -118,6 +118,9 @@ def generate_begin(self, events, group):
>      def generate(self, event, group):
>          self._run_function("generate_%s", event, group)
>  
> +    def generate_conditional(self, hasCondition):
> +        self._run_function("generate_%s_conditional", hasCondition)
> +    
>      def generate_unconditional(self, event, group):
>          self._run_function("generate_%s_unconditional", event, group)
>      
> diff --git a/scripts/tracetool/backend/ftrace.py b/scripts/tracetool/backend/ftrace.py
> index 2d6d608add..d579139532 100644
> --- a/scripts/tracetool/backend/ftrace.py
> +++ b/scripts/tracetool/backend/ftrace.py
> @@ -30,17 +30,15 @@ def generate_h(event, group):
>      if len(event.args) > 0:
>          argnames = ", " + argnames
>  
> -    out('    {',
> -        '        char ftrace_buf[MAX_TRACE_STRLEN];',
> +    out('        char ftrace_buf[MAX_TRACE_STRLEN];',
>          '        int unused __attribute__ ((unused));',
>          '        int trlen;',
>          '#line %(event_lineno)d "%(event_filename)s"',
> -        '            trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,',
> -        '                             "%(name)s " %(fmt)s "\\n" %(argnames)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);',
> -        '    }',
> +        '        trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);',
> +        '        unused = write(trace_marker_fd, ftrace_buf, trlen);',
>          name=event.name,
>          args=event.args,
>          event_lineno=event.lineno,
> @@ -52,3 +50,7 @@ def generate_h(event, group):
>  def generate_h_backend_dstate(event, group):
>      out('    trace_event_get_state_dynamic_by_id(%(event_id)s) || \\',
>          event_id="TRACE_" + event.name.upper())
> +
> +
> +def generate_h_conditional(hasCondition):
> +    hasCondition[0] = hasCondition[0] or True
> diff --git a/scripts/tracetool/backend/log.py b/scripts/tracetool/backend/log.py
> index 35a3aeee55..119e24af04 100644
> --- a/scripts/tracetool/backend/log.py
> +++ b/scripts/tracetool/backend/log.py
> @@ -31,22 +31,22 @@ def generate_h(event, group):
>      if len(event.args) > 0:
>          argnames = ", " + argnames
>  
> -    out('    if (qemu_loglevel_mask(LOG_TRACE)) {',
> -        '        if (message_with_timestamp) {',
> -        '            struct timeval _now;',
> -        '            gettimeofday(&_now, NULL);',
> +    out('        if (qemu_loglevel_mask(LOG_TRACE)) {',
> +        '            if (message_with_timestamp) {',
> +        '                struct timeval _now;',
> +        '                gettimeofday(&_now, NULL);',
>          '#line %(event_lineno)d "%(event_filename)s"',
> -        '            qemu_log("%%d@%%zu.%%06zu:%(name)s " %(fmt)s "\\n",',
> -        '                     qemu_get_thread_id(),',
> -        '                     (size_t)_now.tv_sec, (size_t)_now.tv_usec',
> -        '                     %(argnames)s);',
> +        '                qemu_log("%%d@%%zu.%%06zu:%(name)s " %(fmt)s "\\n",',
> +        '                         qemu_get_thread_id(),',
> +        '                         (size_t)_now.tv_sec, (size_t)_now.tv_usec',
> +        '                         %(argnames)s);',
>          '#line %(out_next_lineno)d "%(out_filename)s"',
> -        '        } else {',
> +        '            } else {',
>          '#line %(event_lineno)d "%(event_filename)s"',
> -        '            qemu_log("%(name)s " %(fmt)s "\\n"%(argnames)s);',
> +        '                qemu_log("%(name)s " %(fmt)s "\\n"%(argnames)s);',
>          '#line %(out_next_lineno)d "%(out_filename)s"',
> +        '            }',
>          '        }',
> -        '    }',
>          event_lineno=event.lineno,
>          event_filename=os.path.relpath(event.filename),
>          name=event.name,
> @@ -57,3 +57,7 @@ def generate_h(event, group):
>  def generate_h_backend_dstate(event, group):
>      out('    trace_event_get_state_dynamic_by_id(%(event_id)s) || \\',
>          event_id="TRACE_" + event.name.upper())
> +
> +    
> +def generate_h_conditional(hasCondition):
> +    hasCondition[0] = hasCondition[0] or True
> diff --git a/scripts/tracetool/backend/simple.py b/scripts/tracetool/backend/simple.py
> index ce8036f5da..316f39727b 100644
> --- a/scripts/tracetool/backend/simple.py
> +++ b/scripts/tracetool/backend/simple.py
> @@ -97,3 +97,7 @@ def generate_c(event, group):
>      out('    trace_record_finish(&rec);',
>          '}',
>          '')
> +
> +
> +def generate_h_conditional(hasCondition):
> +    hasCondition[0] = hasCondition[0] or True
> diff --git a/scripts/tracetool/backend/syslog.py b/scripts/tracetool/backend/syslog.py
> index f84cec641c..a224bd1922 100644
> --- a/scripts/tracetool/backend/syslog.py
> +++ b/scripts/tracetool/backend/syslog.py
> @@ -43,3 +43,7 @@ def generate_h(event, group):
>  def generate_h_backend_dstate(event, group):
>      out('    trace_event_get_state_dynamic_by_id(%(event_id)s) || \\',
>          event_id="TRACE_" + event.name.upper())
> +
> +
> +def generate_h_conditional(hasCondition):
> +    hasCondition[0] = hasCondition[0] or True
> diff --git a/scripts/tracetool/format/h.py b/scripts/tracetool/format/h.py
> index 7bbe6d3148..7a5a32d518 100644
> --- a/scripts/tracetool/format/h.py
> +++ b/scripts/tracetool/format/h.py
> @@ -59,21 +59,10 @@ 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),
> -            args=e.args)
> -
> -        if "disable" not in e.properties:
> -            backend.generate(e, group)
> -
> -        out('}')
> -
>          event_id = 'TRACE_' + e.name.upper()
>          cond = "trace_event_get_state(%s)" % event_id
> -
> +        hasCondition = [False]
> +        backend.generate_conditional(hasCondition)
>          out('',
>              'static inline void %(api)s(%(args)s)',
>              '{',
> @@ -83,13 +72,16 @@ def generate(events, backend, group):
>          if "disable" not in e.properties:
>              backend.generate_unconditional(e, group)
>  
> -        out('    if (%(cond)s) {',
> -            '        %(api_nocheck)s(%(names)s);',
> -            '    }',
> -            '}',
> -            api_nocheck=e.api(e.QEMU_TRACE_NOCHECK),
> -            names=", ".join(e.args.names()),
> +        if hasCondition[0]:
> +            out('    if (%(cond)s) {',
>              cond=cond)
> +        
> +        if "disable" not in e.properties:
> +            backend.generate(e, group)
> +
> +        if hasCondition[0]:
> +            out('    }')
> +        out('}')

This doesn't handle cases where some backends (like dtrace) do not want
trace_event_get_state() but some backends (like simple) do. You can test
that case with ./configure --enable-trace-backends=dtrace,simple.

Both types of backends need to have their generate function
called separately:

   ...backends without the conditional...
   if (trace_event_get_state(...)) {
       ...backends with the conditional...
   }

The hasCondition list argument can be avoided by returning bool from
generate_h_conditional() instead of modifying the argument. That's a
little cleaner than the pass-by-reference trick where each backend has
to logical-or in their value.

The generate_h_conditional() function could also be replace with a
module variable like the existing PUBLIC variable. That way backends can
simply declare what they want instead of implementing a function:

  CHECK_TRACE_EVENT_GET_STATE = True # or False

And the code in h.py would know whether to call the generate function
inside the conditional or not. (I snuck in another suggestion: changing
the name from "conditional", which is a general term, to
"check_trace_event_get_state" to be more specific about what it does.)

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]