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 :|
