[DTrace-devel] [PATCH 04/20] cg: add dt_cg_trace

[Eugene Loh]

On 5/23/22 14:35, Nick Alcock wrote:

> On 18 May 2022, Eugene Loh via DTrace-devel verbalised:
>
>> My opinion on this... remains unchanged!  The basic functionality seems really attractive, but I don't like the interface.
> Well, Kris suggested one interface and you suggested another
> incompatible one. I have to pick one :)
>> I think it would be nice to mimic the functionality of bpf_trace_printk() more closely.  That is, allow three values to be
>> printed.  Further, let each one be an IMM or REG.  I know we disagreed on what a sensible interface could look like, but that
>> strikes me as a solvable problem.  The current interface with "counter" serving two very
>> different purposes sets the bar low.
> Well yes, if can find a nicer interface I'll use it: but I tried the one
> you suggested (with all the bit-shifts) and I couldn't enter a single
> trace value without screwing it up half a dozen times, which for a
> debugging interface is somewhat unhelpful! I'd need a helper function to
> wrap the tracing helper function and make it usable :P
I guess it's a matter of taste.  To me, in any case, it doesn't seem too 
bad.  We do this sort of thing all the time.  E.g., permissions on a 
file are 0, 1, 2, 3, 4, 5, 6, and 7, meaning --, --x, -w-, -wx, r--, 
r-x, rw-, and rwx, respectively.  The dtrace -xdisasm option takes 
values like 15 (=8|4|2|1) or 8 (last listing only) or 1 (first listing 
only) etc.
>> Further, controlling the operation with debugging= also strikes me as
>> weird.  That build setting is for GCC optimization, while the current
> I intended it as something that controls debugging things that are
> expensive enough that you'd only ever want to turn them on when
> debugging DTrace, and never have them in production. Turning off
> optimization is one of those things: adding stuff to the BPF preamble
> that is never called unless you add explicit debugging calls seems like
> another one to me.
Hmm.  The sense I got from the code and the commit was that the flag was 
related to GCC optimization, code generation, and debugging.
>> Finally I'm curious why the BPF C file is called trace-ptr.c and why that argument is called ptr.  Not a big deal but "ptr" seems
>> like a counterintuitive name.
> Because it's tracing a pointer value? ... actually I had to change it to
> a uint64_t, but it's *conceptually* a uintptr_t :P this will be true
> everywhere but CHERI, where pointers are 128 bits :/ ok that name does
> suck. trace_long.c, perhaps?
"Trace" is used a ton in this code base.  How about adding a new name 
for a new thing.  I think of this thing as a wrapper for 
bpf_trace_printk().  So how about referring to printk()?  E.g. 
dt_cg_printk() and then printk.c.
>
>> Anyhow my $0.02.
> Good points! And I don't like this interface either -- I just like all
> the others proposed so far even less...