From: Tanish Desai <tanishdesai37@gmail.com>
Generating .rs files makes it possible to support tracing in rust.
This support comprises a new format, and common code that converts
the C expressions in trace-events to Rust. In particular, types
need to be converted, and PRI macros expanded.
As of this commit no backend generates Rust code, but it is already
possible to use tracetool to generate Rust sources; they are not
functional but they compile and contain tracepoint functions.
Signed-off-by: Tanish Desai <tanishdesai37@gmail.com>
[Move Rust argument conversion from Event to Arguments; string
support. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scripts/tracetool/__init__.py | 155 +++++++++++++++++++++++++++++++++
scripts/tracetool/format/rs.py | 71 +++++++++++++++
2 files changed, 226 insertions(+)
create mode 100644 scripts/tracetool/format/rs.py
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index a58d7938658..ea3e83f5adf 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -31,6 +31,49 @@ def error(*lines):
error_write(*lines)
sys.exit(1)
+FMT_TOKEN = re.compile(r'''(?:
+ " ( (?: [^"\\] | \\[\\"abfnrt] | # a string literal
+ \\x[0-9a-fA-F][0-9a-fA-F]) *? ) "
+ | ( PRI [duixX] (?:8|16|32|64|PTR|MAX) ) # a PRIxxx macro
+ | \s+ # spaces (ignored)
+ )''', re.X)
+
+PRI_SIZE_MAP = {
+ '8': 'hh',
+ '16': 'h',
+ '32': '',
+ '64': 'll',
+ 'PTR': 't',
+ 'MAX': 'j',
+}
+
+def expand_format_string(c_fmt, prefix=""):
+ def pri_macro_to_fmt(pri_macro):
+ assert pri_macro.startswith("PRI")
+ fmt_type = pri_macro[3] # 'd', 'i', 'u', or 'x'
+ fmt_size = pri_macro[4:] # '8', '16', '32', '64', 'PTR', 'MAX'
+
+ size = PRI_SIZE_MAP.get(fmt_size, None)
+ if size is None:
+ raise Exception(f"unknown macro {pri_macro}")
+ return size + fmt_type
+
+ result = prefix
+ pos = 0
+ while pos < len(c_fmt):
+ m = FMT_TOKEN.match(c_fmt, pos)
+ if not m:
+ print("No match at position", pos, ":", repr(c_fmt[pos:]), file=sys.stderr)
+ raise Exception("syntax error in trace file")
+ if m[1]:
+ substr = m[1]
+ elif m[2]:
+ substr = pri_macro_to_fmt(m[2])
+ else:
+ substr = ""
+ result += substr
+ pos = m.end()
+ return result
out_lineno = 1
out_filename = '<none>'
@@ -90,6 +133,49 @@ def out(*lines, **kwargs):
"ptrdiff_t",
]
+C_TYPE_KEYWORDS = {"char", "int", "void", "short", "long", "signed", "unsigned"}
+
+C_TO_RUST_TYPE_MAP = {
+ "int": "std::ffi::c_int",
+ "long": "std::ffi::c_long",
+ "long long": "std::ffi::c_longlong",
+ "short": "std::ffi::c_short",
+ "char": "std::ffi::c_char",
+ "bool": "bool",
+ "unsigned": "std::ffi::c_uint",
+ # multiple keywords, keep them sorted
+ "long unsigned": "std::ffi::c_long",
+ "long long unsigned": "std::ffi::c_ulonglong",
+ "short unsigned": "std::ffi::c_ushort",
+ "char unsigned": "u8",
+ "int8_t": "i8",
+ "uint8_t": "u8",
+ "int16_t": "i16",
+ "uint16_t": "u16",
+ "int32_t": "i32",
+ "uint32_t": "u32",
+ "int64_t": "i64",
+ "uint64_t": "u64",
+ "void": "()",
+ "size_t": "usize",
+ "ssize_t": "isize",
+ "uintptr_t": "usize",
+ "ptrdiff_t": "isize",
+}
+
+# Rust requires manual casting of <32-bit types when passing them to
+# variable-argument functions.
+RUST_VARARGS_SMALL_TYPES = {
+ "std::ffi::c_short",
+ "std::ffi::c_ushort",
+ "std::ffi::c_char",
+ "i8",
+ "u8",
+ "i16",
+ "u16",
+ "bool",
+}
+
def validate_type(name):
bits = name.split(" ")
for bit in bits:
@@ -105,6 +191,38 @@ def validate_type(name):
"other complex pointer types should be "
"declared as 'void *'" % name)
+def c_type_to_rust(name):
+ ptr = False
+ const = False
+ name = name.rstrip()
+ if name[-1] == '*':
+ name = name[:-1].rstrip()
+ ptr = True
+ if name[-1] == '*':
+ # pointers to pointers are the same as void*
+ name = "void"
+
+ bits = name.split()
+ if "const" in bits:
+ const = True
+ bits.remove("const")
+ if bits[0] in C_TYPE_KEYWORDS:
+ if "signed" in bits:
+ bits.remove("signed")
+ if len(bits) > 1 and "int" in bits:
+ bits.remove("int")
+ bits.sort()
+ name = ' '.join(bits)
+ else:
+ if len(bits) > 1:
+ raise ValueError("Invalid type '%s'." % name)
+ name = bits[0]
+
+ ty = C_TO_RUST_TYPE_MAP[name.strip()]
+ if ptr:
+ ty = f'*{"const" if const else "mut"} {ty}'
+ return ty
+
class Arguments:
"""Event arguments description."""
@@ -193,6 +311,43 @@ def casted(self):
"""List of argument names casted to their type."""
return ["(%s)%s" % (type_, name) for type_, name in self._args]
+ def rust_decl_extern(self):
+ """Return a Rust argument list for an extern "C" function"""
+ return ", ".join((f"_{name}: {c_type_to_rust(type_)}"
+ for type_, name in self._args))
+
+ def rust_decl(self):
+ """Return a Rust argument list for a tracepoint function"""
+ def decl_type(type_):
+ if type_ == "const char *":
+ return "&std::ffi::CStr"
+ return c_type_to_rust(type_)
+
+ return ", ".join((f"_{name}: {decl_type(type_)}"
+ for type_, name in self._args))
+
+ def rust_call_extern(self):
+ """Return a Rust argument list for a call to an extern "C" function"""
+ def rust_cast(name, type_):
+ if type_ == "const char *":
+ return f"_{name}.as_ptr()"
+ return f"_{name}"
+
+ return ", ".join((rust_cast(name, type_) for type_, name in self._args))
+
+ def rust_call_varargs(self):
+ """Return a Rust argument list for a call to a C varargs function"""
+ def rust_cast(name, type_):
+ if type_ == "const char *":
+ return f"_{name}.as_ptr()"
+
+ type_ = c_type_to_rust(type_)
+ if type_ in RUST_VARARGS_SMALL_TYPES:
+ return f"_{name} as std::ffi::c_int"
+ return f"_{name} /* as {type_} */"
+
+ return ", ".join((rust_cast(name, type_) for type_, name in self._args))
+
class Event(object):
"""Event description.
diff --git a/scripts/tracetool/format/rs.py b/scripts/tracetool/format/rs.py
new file mode 100644
index 00000000000..c4ab0e59d85
--- /dev/null
+++ b/scripts/tracetool/format/rs.py
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+"""
+trace-DIR.rs
+"""
+
+__author__ = "Tanish Desai <tanishdesai37@gmail.com>"
+__copyright__ = "Copyright 2025, Tanish Desai <tanishdesai37@gmail.com>"
+__license__ = "GPL version 2 or (at your option) any later version"
+
+__maintainer__ = "Stefan Hajnoczi"
+__email__ = "stefanha@redhat.com"
+
+
+from tracetool import out
+
+
+def generate(events, backend, group):
+ out('// SPDX-License-Identifier: GPL-2.0-or-later',
+ '// This file is @generated by tracetool, do not edit.',
+ '',
+ '#[allow(unused_imports)]',
+ 'use std::ffi::c_char;',
+ '#[allow(unused_imports)]',
+ 'use util::bindings;',
+ '',
+ '#[inline(always)]',
+ 'fn trace_event_get_state_dynamic_by_id(_id: u16) -> bool {',
+ ' unsafe { (trace_events_enabled_count != 0) && (_id != 0) }',
+ '}',
+ '',
+ 'extern "C" {',
+ ' static mut trace_events_enabled_count: u32;',
+ '}',)
+
+ out('extern "C" {')
+
+ for e in events:
+ out(' static mut %s: u16;' % e.api(e.QEMU_DSTATE))
+ out('}')
+
+ # static state
+ for e in events:
+ if 'disable' in e.properties:
+ enabled = "false"
+ else:
+ enabled = "true"
+
+ backend.generate_begin(events, group)
+
+ for e in events:
+ out('',
+ '#[inline(always)]',
+ '#[allow(dead_code)]',
+ 'pub fn %(api)s(%(args)s)',
+ '{',
+ api=e.api(e.QEMU_TRACE),
+ args=e.args.rust_decl())
+
+ if "disable" not in e.properties:
+ backend.generate(e, group, check_trace_event_get_state=False)
+ if backend.check_trace_event_get_state:
+ event_id = 'TRACE_' + e.name.upper()
+ out(' if trace_event_get_state_dynamic_by_id(unsafe { _%(event_id)s_DSTATE}) {',
+ event_id = event_id,
+ api=e.api())
+ backend.generate(e, group, check_trace_event_get_state=True)
+ out(' }')
+ out('}')
+
+ backend.generate_end(events, group)
--
2.51.0On Fri, Sep 19, 2025 at 01:25:28PM +0200, Paolo Bonzini wrote:
> From: Tanish Desai <tanishdesai37@gmail.com>
>
> Generating .rs files makes it possible to support tracing in rust.
> This support comprises a new format, and common code that converts
> the C expressions in trace-events to Rust. In particular, types
> need to be converted, and PRI macros expanded.
>
> As of this commit no backend generates Rust code, but it is already
> possible to use tracetool to generate Rust sources; they are not
> functional but they compile and contain tracepoint functions.
>
> Signed-off-by: Tanish Desai <tanishdesai37@gmail.com>
> [Move Rust argument conversion from Event to Arguments; string
> support. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> scripts/tracetool/__init__.py | 155 +++++++++++++++++++++++++++++++++
> scripts/tracetool/format/rs.py | 71 +++++++++++++++
> 2 files changed, 226 insertions(+)
> create mode 100644 scripts/tracetool/format/rs.py
>
> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> index a58d7938658..ea3e83f5adf 100644
> --- a/scripts/tracetool/__init__.py
> +++ b/scripts/tracetool/__init__.py
> @@ -31,6 +31,49 @@ def error(*lines):
> error_write(*lines)
> sys.exit(1)
>
> +FMT_TOKEN = re.compile(r'''(?:
> + " ( (?: [^"\\] | \\[\\"abfnrt] | # a string literal
> + \\x[0-9a-fA-F][0-9a-fA-F]) *? ) "
> + | ( PRI [duixX] (?:8|16|32|64|PTR|MAX) ) # a PRIxxx macro
> + | \s+ # spaces (ignored)
> + )''', re.X)
> +
> +PRI_SIZE_MAP = {
> + '8': 'hh',
> + '16': 'h',
> + '32': '',
> + '64': 'll',
> + 'PTR': 't',
> + 'MAX': 'j',
> +}
> +
> +def expand_format_string(c_fmt, prefix=""):
> + def pri_macro_to_fmt(pri_macro):
> + assert pri_macro.startswith("PRI")
> + fmt_type = pri_macro[3] # 'd', 'i', 'u', or 'x'
> + fmt_size = pri_macro[4:] # '8', '16', '32', '64', 'PTR', 'MAX'
> +
> + size = PRI_SIZE_MAP.get(fmt_size, None)
> + if size is None:
> + raise Exception(f"unknown macro {pri_macro}")
> + return size + fmt_type
> +
> + result = prefix
> + pos = 0
> + while pos < len(c_fmt):
> + m = FMT_TOKEN.match(c_fmt, pos)
> + if not m:
> + print("No match at position", pos, ":", repr(c_fmt[pos:]), file=sys.stderr)
> + raise Exception("syntax error in trace file")
> + if m[1]:
> + substr = m[1]
> + elif m[2]:
> + substr = pri_macro_to_fmt(m[2])
> + else:
> + substr = ""
> + result += substr
> + pos = m.end()
> + return result
>
> out_lineno = 1
> out_filename = '<none>'
> @@ -90,6 +133,49 @@ def out(*lines, **kwargs):
> "ptrdiff_t",
> ]
>
> +C_TYPE_KEYWORDS = {"char", "int", "void", "short", "long", "signed", "unsigned"}
> +
> +C_TO_RUST_TYPE_MAP = {
> + "int": "std::ffi::c_int",
> + "long": "std::ffi::c_long",
> + "long long": "std::ffi::c_longlong",
> + "short": "std::ffi::c_short",
> + "char": "std::ffi::c_char",
> + "bool": "bool",
> + "unsigned": "std::ffi::c_uint",
> + # multiple keywords, keep them sorted
> + "long unsigned": "std::ffi::c_long",
> + "long long unsigned": "std::ffi::c_ulonglong",
> + "short unsigned": "std::ffi::c_ushort",
> + "char unsigned": "u8",
> + "int8_t": "i8",
> + "uint8_t": "u8",
> + "int16_t": "i16",
> + "uint16_t": "u16",
> + "int32_t": "i32",
> + "uint32_t": "u32",
> + "int64_t": "i64",
> + "uint64_t": "u64",
> + "void": "()",
> + "size_t": "usize",
> + "ssize_t": "isize",
> + "uintptr_t": "usize",
> + "ptrdiff_t": "isize",
> +}
> +
> +# Rust requires manual casting of <32-bit types when passing them to
> +# variable-argument functions.
> +RUST_VARARGS_SMALL_TYPES = {
> + "std::ffi::c_short",
> + "std::ffi::c_ushort",
> + "std::ffi::c_char",
> + "i8",
> + "u8",
> + "i16",
> + "u16",
> + "bool",
> +}
> +
> def validate_type(name):
> bits = name.split(" ")
> for bit in bits:
> @@ -105,6 +191,38 @@ def validate_type(name):
> "other complex pointer types should be "
> "declared as 'void *'" % name)
>
> +def c_type_to_rust(name):
> + ptr = False
> + const = False
> + name = name.rstrip()
> + if name[-1] == '*':
> + name = name[:-1].rstrip()
> + ptr = True
> + if name[-1] == '*':
> + # pointers to pointers are the same as void*
> + name = "void"
> +
> + bits = name.split()
> + if "const" in bits:
> + const = True
> + bits.remove("const")
> + if bits[0] in C_TYPE_KEYWORDS:
> + if "signed" in bits:
> + bits.remove("signed")
> + if len(bits) > 1 and "int" in bits:
> + bits.remove("int")
> + bits.sort()
> + name = ' '.join(bits)
> + else:
> + if len(bits) > 1:
> + raise ValueError("Invalid type '%s'." % name)
> + name = bits[0]
> +
> + ty = C_TO_RUST_TYPE_MAP[name.strip()]
> + if ptr:
> + ty = f'*{"const" if const else "mut"} {ty}'
> + return ty
> +
> class Arguments:
> """Event arguments description."""
>
> @@ -193,6 +311,43 @@ def casted(self):
> """List of argument names casted to their type."""
> return ["(%s)%s" % (type_, name) for type_, name in self._args]
>
> + def rust_decl_extern(self):
> + """Return a Rust argument list for an extern "C" function"""
> + return ", ".join((f"_{name}: {c_type_to_rust(type_)}"
> + for type_, name in self._args))
> +
> + def rust_decl(self):
> + """Return a Rust argument list for a tracepoint function"""
> + def decl_type(type_):
> + if type_ == "const char *":
> + return "&std::ffi::CStr"
> + return c_type_to_rust(type_)
> +
> + return ", ".join((f"_{name}: {decl_type(type_)}"
> + for type_, name in self._args))
> +
> + def rust_call_extern(self):
> + """Return a Rust argument list for a call to an extern "C" function"""
> + def rust_cast(name, type_):
> + if type_ == "const char *":
> + return f"_{name}.as_ptr()"
> + return f"_{name}"
> +
> + return ", ".join((rust_cast(name, type_) for type_, name in self._args))
> +
> + def rust_call_varargs(self):
> + """Return a Rust argument list for a call to a C varargs function"""
> + def rust_cast(name, type_):
> + if type_ == "const char *":
> + return f"_{name}.as_ptr()"
> +
> + type_ = c_type_to_rust(type_)
> + if type_ in RUST_VARARGS_SMALL_TYPES:
> + return f"_{name} as std::ffi::c_int"
> + return f"_{name} /* as {type_} */"
> +
> + return ", ".join((rust_cast(name, type_) for type_, name in self._args))
> +
>
> class Event(object):
> """Event description.
> diff --git a/scripts/tracetool/format/rs.py b/scripts/tracetool/format/rs.py
> new file mode 100644
> index 00000000000..c4ab0e59d85
> --- /dev/null
> +++ b/scripts/tracetool/format/rs.py
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +"""
> +trace-DIR.rs
> +"""
> +
> +__author__ = "Tanish Desai <tanishdesai37@gmail.com>"
> +__copyright__ = "Copyright 2025, Tanish Desai <tanishdesai37@gmail.com>"
> +__license__ = "GPL version 2 or (at your option) any later version"
> +
> +__maintainer__ = "Stefan Hajnoczi"
> +__email__ = "stefanha@redhat.com"
> +
> +
> +from tracetool import out
> +
> +
> +def generate(events, backend, group):
> + out('// SPDX-License-Identifier: GPL-2.0-or-later',
> + '// This file is @generated by tracetool, do not edit.',
> + '',
> + '#[allow(unused_imports)]',
> + 'use std::ffi::c_char;',
> + '#[allow(unused_imports)]',
> + 'use util::bindings;',
> + '',
> + '#[inline(always)]',
> + 'fn trace_event_get_state_dynamic_by_id(_id: u16) -> bool {',
> + ' unsafe { (trace_events_enabled_count != 0) && (_id != 0) }',
> + '}',
This was translated to Rust from:
/* it's on fast path, avoid consistency checks (asserts) */
#define trace_event_get_state_dynamic_by_id(id) \
(unlikely(trace_events_enabled_count) && _ ## id ## _DSTATE)
The _id != 0 expression is incorrect. The purpose was to check whether
the trace event is currently enabled (i.e. dynamically at runtime).
> + '',
> + 'extern "C" {',
> + ' static mut trace_events_enabled_count: u32;',
> + '}',)
> +
> + out('extern "C" {')
> +
> + for e in events:
> + out(' static mut %s: u16;' % e.api(e.QEMU_DSTATE))
> + out('}')
> +
> + # static state
> + for e in events:
> + if 'disable' in e.properties:
> + enabled = "false"
> + else:
> + enabled = "true"
What is the purpose of this loop? The variable enabled is unused so I
think it can be deleted.
> +
> + backend.generate_begin(events, group)
> +
> + for e in events:
> + out('',
> + '#[inline(always)]',
Tabs snuck in here. This should be indented with spaces.
> + '#[allow(dead_code)]',
> + 'pub fn %(api)s(%(args)s)',
> + '{',
> + api=e.api(e.QEMU_TRACE),
> + args=e.args.rust_decl())
> +
> + if "disable" not in e.properties:
> + backend.generate(e, group, check_trace_event_get_state=False)
> + if backend.check_trace_event_get_state:
> + event_id = 'TRACE_' + e.name.upper()
> + out(' if trace_event_get_state_dynamic_by_id(unsafe { _%(event_id)s_DSTATE}) {',
> + event_id = event_id,
> + api=e.api())
> + backend.generate(e, group, check_trace_event_get_state=True)
> + out(' }')
> + out('}')
> +
> + backend.generate_end(events, group)
> --
> 2.51.0
>
>
On 9/23/25 21:23, Stefan Hajnoczi wrote:
>> + out('// SPDX-License-Identifier: GPL-2.0-or-later',
>> + '// This file is @generated by tracetool, do not edit.',
>> + '',
>> + '#[allow(unused_imports)]',
>> + 'use std::ffi::c_char;',
>> + '#[allow(unused_imports)]',
>> + 'use util::bindings;',
>> + '',
>> + '#[inline(always)]',
>> + 'fn trace_event_get_state_dynamic_by_id(_id: u16) -> bool {',
>> + ' unsafe { (trace_events_enabled_count != 0) && (_id != 0) }',
>> + '}',
>
> This was translated to Rust from:
>
> /* it's on fast path, avoid consistency checks (asserts) */
> #define trace_event_get_state_dynamic_by_id(id) \
> (unlikely(trace_events_enabled_count) && _ ## id ## _DSTATE)
>
> The _id != 0 expression is incorrect. The purpose was to check whether
> the trace event is currently enabled (i.e. dynamically at runtime).
The expression is correct, but the function and argument names are not.
It should be
fn trace_event_state_is_enabled(dstate: u16) -> bool {
unsafe { trace_events_enabled_count } != 0 && dstate != 0
}
>> + # static state
>> + for e in events:
>> + if 'disable' in e.properties:
>> + enabled = "false"
>> + else:
>> + enabled = "true"
>
> What is the purpose of this loop? The variable enabled is unused so I
> think it can be deleted.
The Rust code generator is not emitting any code for disabled
tracepoints. Unlike C, where the disabled tracepoints can produce e.g.
-Wformat warnings, there's no real benefit here.
In the RFC the "enabled" variable was used to produce a const for the
static state; it had no user so I removed it, but I left behind this
dead Python code. Sorry about that!
Paolo
On Wed, Sep 24, 2025 at 09:13:15AM +0200, Paolo Bonzini wrote:
> On 9/23/25 21:23, Stefan Hajnoczi wrote:
> > > + out('// SPDX-License-Identifier: GPL-2.0-or-later',
> > > + '// This file is @generated by tracetool, do not edit.',
> > > + '',
> > > + '#[allow(unused_imports)]',
> > > + 'use std::ffi::c_char;',
> > > + '#[allow(unused_imports)]',
> > > + 'use util::bindings;',
> > > + '',
> > > + '#[inline(always)]',
> > > + 'fn trace_event_get_state_dynamic_by_id(_id: u16) -> bool {',
> > > + ' unsafe { (trace_events_enabled_count != 0) && (_id != 0) }',
> > > + '}',
> >
> > This was translated to Rust from:
> >
> > /* it's on fast path, avoid consistency checks (asserts) */
> > #define trace_event_get_state_dynamic_by_id(id) \
> > (unlikely(trace_events_enabled_count) && _ ## id ## _DSTATE)
> >
> > The _id != 0 expression is incorrect. The purpose was to check whether
> > the trace event is currently enabled (i.e. dynamically at runtime).
>
> The expression is correct, but the function and argument names are not. It
> should be
>
> fn trace_event_state_is_enabled(dstate: u16) -> bool {
> unsafe { trace_events_enabled_count } != 0 && dstate != 0
> }
The generated code is missing DTrace's SDT semaphore (see
generate_h_backend_dstate() in scripts/tracetool/backend/dtrace.py). The
conditional must be taken when a tool like SystemTap or GDB sets the SDT
semaphore. Right now it will not be taken because the conditional only
looks at _ ## id ## _DSTATE and not the SDT semaphore.
Stefan
On Wed, Sep 24, 2025, 20:10 Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > fn trace_event_state_is_enabled(dstate: u16) -> bool {
> > unsafe { trace_events_enabled_count } != 0 && dstate != 0
> > }
>
> The generated code is missing DTrace's SDT semaphore (see
> generate_h_backend_dstate() in scripts/tracetool/backend/dtrace.py). The
> conditional must be taken when a tool like SystemTap or GDB sets the SDT
> semaphore. Right now it will not be taken because the conditional only
> looks at _ ## id ## _DSTATE and not the SDT semaphore.
>
This is private code to trace-*.rs, for use within the tracepoint functions
only; it's not a public "is the tracepoint active" API. The public side in
C does look at the semaphore.
Paolo
On Wed, Sep 24, 2025 at 09:58:04PM +0200, Paolo Bonzini wrote:
> On Wed, Sep 24, 2025, 20:10 Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > > fn trace_event_state_is_enabled(dstate: u16) -> bool {
> > > unsafe { trace_events_enabled_count } != 0 && dstate != 0
> > > }
> >
> > The generated code is missing DTrace's SDT semaphore (see
> > generate_h_backend_dstate() in scripts/tracetool/backend/dtrace.py). The
> > conditional must be taken when a tool like SystemTap or GDB sets the SDT
> > semaphore. Right now it will not be taken because the conditional only
> > looks at _ ## id ## _DSTATE and not the SDT semaphore.
> >
>
> This is private code to trace-*.rs, for use within the tracepoint functions
> only; it's not a public "is the tracepoint active" API. The public side in
> C does look at the semaphore.
You're right, the code is fine just with the function renamed.
Stefan
On 9/25/25 13:50, Stefan Hajnoczi wrote:
> On Wed, Sep 24, 2025 at 09:58:04PM +0200, Paolo Bonzini wrote:
>> On Wed, Sep 24, 2025, 20:10 Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>
>>>> fn trace_event_state_is_enabled(dstate: u16) -> bool {
>>>> unsafe { trace_events_enabled_count } != 0 && dstate != 0
>>>> }
>>>
>>> The generated code is missing DTrace's SDT semaphore (see
>>> generate_h_backend_dstate() in scripts/tracetool/backend/dtrace.py). The
>>> conditional must be taken when a tool like SystemTap or GDB sets the SDT
>>> semaphore. Right now it will not be taken because the conditional only
>>> looks at _ ## id ## _DSTATE and not the SDT semaphore.
>>>
>>
>> This is private code to trace-*.rs, for use within the tracepoint functions
>> only; it's not a public "is the tracepoint active" API. The public side in
>> C does look at the semaphore.
>
> You're right, the code is fine just with the function renamed.
No problem---in fact I have now realized that, for systemtap, I have to
ensure that the semaphore is shared between C and Rust!
Paolo
On Thu, Sep 25, 2025 at 8:40 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 9/25/25 13:50, Stefan Hajnoczi wrote:
> > On Wed, Sep 24, 2025 at 09:58:04PM +0200, Paolo Bonzini wrote:
> >> On Wed, Sep 24, 2025, 20:10 Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >>
> >>>> fn trace_event_state_is_enabled(dstate: u16) -> bool {
> >>>> unsafe { trace_events_enabled_count } != 0 && dstate != 0
> >>>> }
> >>>
> >>> The generated code is missing DTrace's SDT semaphore (see
> >>> generate_h_backend_dstate() in scripts/tracetool/backend/dtrace.py). The
> >>> conditional must be taken when a tool like SystemTap or GDB sets the SDT
> >>> semaphore. Right now it will not be taken because the conditional only
> >>> looks at _ ## id ## _DSTATE and not the SDT semaphore.
> >>>
> >>
> >> This is private code to trace-*.rs, for use within the tracepoint functions
> >> only; it's not a public "is the tracepoint active" API. The public side in
> >> C does look at the semaphore.
> >
> > You're right, the code is fine just with the function renamed.
>
> No problem---in fact I have now realized that, for systemtap, I have to
> ensure that the semaphore is shared between C and Rust!
Is anyone working on the DTrace support? If not, I'll keep it in mind
in case I get some time over the next few weeks.
Stefan
On 9/25/25 17:09, Stefan Hajnoczi wrote: > On Thu, Sep 25, 2025 at 8:40 AM Paolo Bonzini <pbonzini@redhat.com> wrote: >> No problem---in fact I have now realized that, for systemtap, I have to >> ensure that the semaphore is shared between C and Rust! > > Is anyone working on the DTrace support? If not, I'll keep it in mind > in case I get some time over the next few weeks. No, the plan was to use the probe-rs crate, which is almost trivial except that it does not support the shared semaphore. For the shared semaphore probe-rs needs modifications. It's also possible to just import its single file with magic asm[1] into QEMU's trace crate and modify it as needed; and contribute the changes upstream later. There is also the usdt crate, which recently grew support for systemtap SDT as well. It takes a much more complex approach and has many dependencies, as it almost completely reimplements the dtrace compiler. It may be interesting for the future since it is more portable, but for the time being I'd stick with the lower-level probe-rs. Paolo [1] https://github.com/bonzini/probe-rs/blob/extern-semaphore/src/platform/systemtap.rs
On Thu, Sep 25, 2025 at 11:37 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 9/25/25 17:09, Stefan Hajnoczi wrote: > > On Thu, Sep 25, 2025 at 8:40 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > >> No problem---in fact I have now realized that, for systemtap, I have to > >> ensure that the semaphore is shared between C and Rust! > > > > Is anyone working on the DTrace support? If not, I'll keep it in mind > > in case I get some time over the next few weeks. > No, the plan was to use the probe-rs crate, which is almost trivial > except that it does not support the shared semaphore. > > For the shared semaphore probe-rs needs modifications. It's also > possible to just import its single file with magic asm[1] into QEMU's > trace crate and modify it as needed; and contribute the changes upstream > later. > > There is also the usdt crate, which recently grew support for systemtap > SDT as well. It takes a much more complex approach and has many > dependencies, as it almost completely reimplements the dtrace compiler. > It may be interesting for the future since it is more portable, but for > the time being I'd stick with the lower-level probe-rs. Okay, thanks for the link to your semaphore changes. I should be able to play with it at the end of next week. Stefan > > Paolo > > [1] > https://github.com/bonzini/probe-rs/blob/extern-semaphore/src/platform/systemtap.rs >
On Wed, Sep 24, 2025 at 09:13:15AM +0200, Paolo Bonzini wrote:
> On 9/23/25 21:23, Stefan Hajnoczi wrote:
> > > + out('// SPDX-License-Identifier: GPL-2.0-or-later',
> > > + '// This file is @generated by tracetool, do not edit.',
> > > + '',
> > > + '#[allow(unused_imports)]',
> > > + 'use std::ffi::c_char;',
> > > + '#[allow(unused_imports)]',
> > > + 'use util::bindings;',
> > > + '',
> > > + '#[inline(always)]',
> > > + 'fn trace_event_get_state_dynamic_by_id(_id: u16) -> bool {',
> > > + ' unsafe { (trace_events_enabled_count != 0) && (_id != 0) }',
> > > + '}',
> >
> > This was translated to Rust from:
> >
> > /* it's on fast path, avoid consistency checks (asserts) */
> > #define trace_event_get_state_dynamic_by_id(id) \
> > (unlikely(trace_events_enabled_count) && _ ## id ## _DSTATE)
> >
> > The _id != 0 expression is incorrect. The purpose was to check whether
> > the trace event is currently enabled (i.e. dynamically at runtime).
>
> The expression is correct, but the function and argument names are not. It
> should be
>
> fn trace_event_state_is_enabled(dstate: u16) -> bool {
> unsafe { trace_events_enabled_count } != 0 && dstate != 0
> }
>
> > > + # static state
> > > + for e in events:
> > > + if 'disable' in e.properties:
> > > + enabled = "false"
> > > + else:
> > > + enabled = "true"
> >
> > What is the purpose of this loop? The variable enabled is unused so I
> > think it can be deleted.
>
> The Rust code generator is not emitting any code for disabled tracepoints.
> Unlike C, where the disabled tracepoints can produce e.g. -Wformat warnings,
> there's no real benefit here.
>
> In the RFC the "enabled" variable was used to produce a const for the static
> state; it had no user so I removed it, but I left behind this dead Python
> code. Sorry about that!
Is the concept of build time 'disabled' tracepoints actually useful to
still support ? AFAICT we use it in only places, which doesn't make it
sound too compelling:
$ find -name trace-events | xargs grep '^disable'
./target/hppa/trace-events:disable hppa_tlb_flush_ent(void *env, void *ent, uint64_t va_b, uint64_t va_e, uint64_t pa) "env=%p ent=%p va_b=0x%lx va_e=0x%lx pa=0x%lx"
./target/hppa/trace-events:disable hppa_tlb_find_entry(void *env, void *ent, int valid, uint64_t va_b, uint64_t va_e, uint64_t pa) "env=%p ent=%p valid=%d va_b=0x%lx va_e=0x%lx pa=0x%lx"
./target/hppa/trace-events:disable hppa_tlb_find_entry_not_found(void *env, uint64_t addr) "env=%p addr=%08lx"
...snip...
./target/hppa/trace-events:disable hppa_tlb_lpa_success(void *env, uint64_t addr, uint64_t phys) "env=%p addr=0x%lx phys=0x%lx"
./target/hppa/trace-events:disable hppa_tlb_lpa_failed(void *env, uint64_t addr) "env=%p addr=0x%lx"
./target/hppa/trace-events:disable hppa_tlb_probe(uint64_t addr, int level, int want) "addr=0x%lx level=%d want=%d"
./hw/display/trace-events:disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
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 :|
On Wed, Sep 24, 2025 at 3:51 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Sep 24, 2025 at 09:13:15AM +0200, Paolo Bonzini wrote:
> > On 9/23/25 21:23, Stefan Hajnoczi wrote:
> > > > + out('// SPDX-License-Identifier: GPL-2.0-or-later',
> > > > + '// This file is @generated by tracetool, do not edit.',
> > > > + '',
> > > > + '#[allow(unused_imports)]',
> > > > + 'use std::ffi::c_char;',
> > > > + '#[allow(unused_imports)]',
> > > > + 'use util::bindings;',
> > > > + '',
> > > > + '#[inline(always)]',
> > > > + 'fn trace_event_get_state_dynamic_by_id(_id: u16) -> bool {',
> > > > + ' unsafe { (trace_events_enabled_count != 0) && (_id != 0) }',
> > > > + '}',
> > >
> > > This was translated to Rust from:
> > >
> > > /* it's on fast path, avoid consistency checks (asserts) */
> > > #define trace_event_get_state_dynamic_by_id(id) \
> > > (unlikely(trace_events_enabled_count) && _ ## id ## _DSTATE)
> > >
> > > The _id != 0 expression is incorrect. The purpose was to check whether
> > > the trace event is currently enabled (i.e. dynamically at runtime).
> >
> > The expression is correct, but the function and argument names are not. It
> > should be
> >
> > fn trace_event_state_is_enabled(dstate: u16) -> bool {
> > unsafe { trace_events_enabled_count } != 0 && dstate != 0
> > }
> >
> > > > + # static state
> > > > + for e in events:
> > > > + if 'disable' in e.properties:
> > > > + enabled = "false"
> > > > + else:
> > > > + enabled = "true"
> > >
> > > What is the purpose of this loop? The variable enabled is unused so I
> > > think it can be deleted.
> >
> > The Rust code generator is not emitting any code for disabled tracepoints.
> > Unlike C, where the disabled tracepoints can produce e.g. -Wformat warnings,
> > there's no real benefit here.
> >
> > In the RFC the "enabled" variable was used to produce a const for the static
> > state; it had no user so I removed it, but I left behind this dead Python
> > code. Sorry about that!
>
> Is the concept of build time 'disabled' tracepoints actually useful to
> still support ? AFAICT we use it in only places, which doesn't make it
> sound too compelling:
>
> $ find -name trace-events | xargs grep '^disable'
> ./target/hppa/trace-events:disable hppa_tlb_flush_ent(void *env, void *ent, uint64_t va_b, uint64_t va_e, uint64_t pa) "env=%p ent=%p va_b=0x%lx va_e=0x%lx pa=0x%lx"
> ./target/hppa/trace-events:disable hppa_tlb_find_entry(void *env, void *ent, int valid, uint64_t va_b, uint64_t va_e, uint64_t pa) "env=%p ent=%p valid=%d va_b=0x%lx va_e=0x%lx pa=0x%lx"
> ./target/hppa/trace-events:disable hppa_tlb_find_entry_not_found(void *env, uint64_t addr) "env=%p addr=%08lx"
> ...snip...
> ./target/hppa/trace-events:disable hppa_tlb_lpa_success(void *env, uint64_t addr, uint64_t phys) "env=%p addr=0x%lx phys=0x%lx"
> ./target/hppa/trace-events:disable hppa_tlb_lpa_failed(void *env, uint64_t addr) "env=%p addr=0x%lx"
> ./target/hppa/trace-events:disable hppa_tlb_probe(uint64_t addr, int level, int want) "addr=0x%lx level=%d want=%d"
> ./hw/display/trace-events:disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
My recollection is that disabled events were supposed to eliminate the
cost of trace events. Downstreams could disable trace events that they
didn't need.
I'm not aware of any users besides the ones that your grep identified.
We could probably remove the disable keyword without any real impact
on users.
Stefan
On Wed, Sep 24, 2025 at 07:49:49AM -0400, Stefan Hajnoczi wrote:
> On Wed, Sep 24, 2025 at 3:51 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Sep 24, 2025 at 09:13:15AM +0200, Paolo Bonzini wrote:
> > > On 9/23/25 21:23, Stefan Hajnoczi wrote:
> > > > > + out('// SPDX-License-Identifier: GPL-2.0-or-later',
> > > > > + '// This file is @generated by tracetool, do not edit.',
> > > > > + '',
> > > > > + '#[allow(unused_imports)]',
> > > > > + 'use std::ffi::c_char;',
> > > > > + '#[allow(unused_imports)]',
> > > > > + 'use util::bindings;',
> > > > > + '',
> > > > > + '#[inline(always)]',
> > > > > + 'fn trace_event_get_state_dynamic_by_id(_id: u16) -> bool {',
> > > > > + ' unsafe { (trace_events_enabled_count != 0) && (_id != 0) }',
> > > > > + '}',
> > > >
> > > > This was translated to Rust from:
> > > >
> > > > /* it's on fast path, avoid consistency checks (asserts) */
> > > > #define trace_event_get_state_dynamic_by_id(id) \
> > > > (unlikely(trace_events_enabled_count) && _ ## id ## _DSTATE)
> > > >
> > > > The _id != 0 expression is incorrect. The purpose was to check whether
> > > > the trace event is currently enabled (i.e. dynamically at runtime).
> > >
> > > The expression is correct, but the function and argument names are not. It
> > > should be
> > >
> > > fn trace_event_state_is_enabled(dstate: u16) -> bool {
> > > unsafe { trace_events_enabled_count } != 0 && dstate != 0
> > > }
> > >
> > > > > + # static state
> > > > > + for e in events:
> > > > > + if 'disable' in e.properties:
> > > > > + enabled = "false"
> > > > > + else:
> > > > > + enabled = "true"
> > > >
> > > > What is the purpose of this loop? The variable enabled is unused so I
> > > > think it can be deleted.
> > >
> > > The Rust code generator is not emitting any code for disabled tracepoints.
> > > Unlike C, where the disabled tracepoints can produce e.g. -Wformat warnings,
> > > there's no real benefit here.
> > >
> > > In the RFC the "enabled" variable was used to produce a const for the static
> > > state; it had no user so I removed it, but I left behind this dead Python
> > > code. Sorry about that!
> >
> > Is the concept of build time 'disabled' tracepoints actually useful to
> > still support ? AFAICT we use it in only places, which doesn't make it
> > sound too compelling:
> >
> > $ find -name trace-events | xargs grep '^disable'
> > ./target/hppa/trace-events:disable hppa_tlb_flush_ent(void *env, void *ent, uint64_t va_b, uint64_t va_e, uint64_t pa) "env=%p ent=%p va_b=0x%lx va_e=0x%lx pa=0x%lx"
> > ./target/hppa/trace-events:disable hppa_tlb_find_entry(void *env, void *ent, int valid, uint64_t va_b, uint64_t va_e, uint64_t pa) "env=%p ent=%p valid=%d va_b=0x%lx va_e=0x%lx pa=0x%lx"
> > ./target/hppa/trace-events:disable hppa_tlb_find_entry_not_found(void *env, uint64_t addr) "env=%p addr=%08lx"
> > ...snip...
> > ./target/hppa/trace-events:disable hppa_tlb_lpa_success(void *env, uint64_t addr, uint64_t phys) "env=%p addr=0x%lx phys=0x%lx"
> > ./target/hppa/trace-events:disable hppa_tlb_lpa_failed(void *env, uint64_t addr) "env=%p addr=0x%lx"
> > ./target/hppa/trace-events:disable hppa_tlb_probe(uint64_t addr, int level, int want) "addr=0x%lx level=%d want=%d"
> > ./hw/display/trace-events:disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
>
> My recollection is that disabled events were supposed to eliminate the
> cost of trace events. Downstreams could disable trace events that they
> didn't need.
IMHO if that is a required use case, then the dtrace backend is the
answer.
The main premise of tracing is that you can rarely predict what info
will be needed ahead of time, so you need runtime controls. dtrace
gives that with near zero overhead, while QEMU's other backends have
that at the cost of a branch to check the enabled state. Disabling
events at build time feels at odds with the intended use case of
tracing.
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 :|
© 2016 - 2025 Red Hat, Inc.