Re: [PATCH 08/16] tracetool: Add Rust format support

["Daniel P. Berrangé" <berrange@redhat.com>]

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