[DTrace-devel] [PATCH v3 1/2] Add support for the print() action
Alan Maguire
alan.maguire at oracle.com
Thu Nov 23 10:53:41 UTC 2023
On 22/11/2023 12:47, Nick Alcock wrote:
> On 22 Nov 2023, Alan Maguire via DTrace-devel verbalised:
>
>> print() [1] prints the address, type and associated values of the data pointed
>> to by the pointer passed in. For example:
>
> Oh this is *so pretty*. Lovely code, lovely results :)
>
> I have a pile of CTF niggles about the actual printing function, though,
> unsurprisingly. Most are trivial, a few are... less so.
>
thanks for the review Nick! Replies below...
>> $ /sbin/dtrace -n 'fbt::ip_rcv_core:entry { print((struct sk_buff *)arg0); }'
>> dtrace: description 'fbt::ip_rcv_core:entry ' matched 1 probe
>> CPU ID FUNCTION:NAME
>> 0 75596 ip_output:entry 0xffff89ab58f88200 = *
>> (struct sk_buff) {
>> (struct) {
>> .sk = (struct sock *)0xffff9b7ad5df2f80,
>> .ip_defrag_offset = (int)-706793600,
>> },
>> (struct) {
>> .tstamp = (ktime_t)504907694897760,
>> .skb_mstamp_ns = (u64)504907694897760,
>> },
>> .cb = (char [48]) [
>> ],
>> (struct) {
>> (struct) {
>> ._skb_refdst = (long unsigned int)18446633550675199489,
>> .destructor = (void (*)(*) ())0xffffffffb66c0b80,
>> },
>> .tcp_tsorted_anchor = (struct list_head) {
>> .next = (struct list_head *)0xffff9b7ad9cc4601,
>> .prev = (struct list_head *)0xffffffffb66c0b80,
>> },
>> ._sk_redir = (long unsigned int)18446633550675199489,
>> },
>> ._nfct = (long unsigned int)18446633547046898499,
>> .len = (unsigned int)8788,
>> .data_len = (unsigned int)8736,
>> .hdr_len = (__u16)320,
>> .cloned = (__u8)1,
>> .ip_summed = (__u8)2,
>
> It's even columnized right!
>
> (Presumably (struct) could also be (union) etc. It looks like it would
> take you extra work for that not to be true, since this is literally the
> ctf_type_name().)
>
>> Signed-off-by: Alan Maguire <alan.maguire at oracle.com>
>
> Shame about the 64K limit, but honestly with strsize limits being what
> they are I thought you'd be stuck with a much smaller limit.
>
>> [1] https://docs.oracle.com/en/operating-systems/solaris/oracle-solaris/11.4/dtrace-guide/print-action.html#GUID-533E6BD9-8DE8-474E-9770-96F84244911C
>> ---
>> include/dtrace/actions_defines.h | 1 +
>> include/dtrace/options_defines.h | 3 +-
>> libdtrace/dt_cg.c | 48 +++++
>> libdtrace/dt_consume.c | 26 +++
>> libdtrace/dt_errtags.h | 3 +-
>> libdtrace/dt_impl.h | 6 +-
>> libdtrace/dt_open.c | 2 +
>> libdtrace/dt_options.c | 25 +++
>> libdtrace/dt_printf.c | 302 ++++++++++++++++++++++++++++++-
>> libdtrace/dt_printf.h | 2 +
>> 10 files changed, 413 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/dtrace/actions_defines.h b/include/dtrace/actions_defines.h
>> index f161df7c..4ff4fb83 100644
>> --- a/include/dtrace/actions_defines.h
>> +++ b/include/dtrace/actions_defines.h
>> @@ -37,6 +37,7 @@
>> #define DTRACEACT_LIBACT 5 /* library-controlled action */
>> #define DTRACEACT_TRACEMEM 6 /* tracemem() action */
>> #define DTRACEACT_PCAP 7 /* pcap() action */
>> +#define DTRACEACT_PRINT 8 /* print() action */
>>
>> #define DTRACEACT_PROC 0x0100
>> #define DTRACEACT_USTACK (DTRACEACT_PROC + 1)
>> diff --git a/include/dtrace/options_defines.h b/include/dtrace/options_defines.h
>> index 61a23f42..80246be8 100644
>> --- a/include/dtrace/options_defines.h
>> +++ b/include/dtrace/options_defines.h
>> @@ -62,7 +62,8 @@
>> #define DTRACEOPT_BPFLOG 32 /* always output BPF verifier log */
>> #define DTRACEOPT_SCRATCHSIZE 33 /* max scratch size permitted */
>> #define DTRACEOPT_LOCKMEM 34 /* max locked memory */
>> -#define DTRACEOPT_MAX 35 /* number of options */
>> +#define DTRACEOPT_PRINTSIZE 35 /* max # bytes printed by print() action */
>> +#define DTRACEOPT_MAX 36 /* number of options */
>>
>> #define DTRACEOPT_UNSET (dtrace_optval_t)-2 /* unset option */
>>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> index 581ada4e..9c5bb2cf 100644
>> --- a/libdtrace/dt_cg.c
>> +++ b/libdtrace/dt_cg.c
>> @@ -2560,6 +2560,53 @@ dt_cg_act_ustack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>> BPF_NOP());
>> }
>>
>> +static void
>> +dt_cg_act_print(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>> +{
>> + uint_t addr_off, data_off;
>> + dt_node_t *addr = dnp->dn_args;
>> + dt_irlist_t *dlp = &pcb->pcb_ir;
>> + dt_regset_t *drp = pcb->pcb_regs;
>> + dtrace_hdl_t *dtp = pcb->pcb_hdl;
>> + ctf_file_t *fp = addr->dn_ctfp;
>> + ctf_id_t type = addr->dn_type;
>> + char n[DT_TYPE_NAMELEN];
>> + size_t size;
>> +
>> + type = ctf_type_reference(fp, type);
>> + if (type == CTF_ERR)
>> + longjmp(yypcb->pcb_jmpbuf, EDT_CTF);
>> + size = ctf_type_size(fp, type);
>> + if (size == 0)
>> + dnerror(addr, D_PRINT_SIZE,
>> + "print( ) argument #1 reference has type '%s' with size 0; cannot print( ) it.\n",
>> + ctf_type_name(fp, type, n, sizeof(n)));
>> +
>> + /* reserve space for addr/type, data/size */
>> + addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
>> + sizeof(uint64_t), 8, NULL, type);
>> + data_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
>> + size, 8, NULL, size);
>> +
>> + dt_cg_node(addr, &pcb->pcb_ir, drp);
>> + dt_cg_check_ptr_arg(dlp, drp, addr, NULL);
>> +
>> + /* store address for later print()ing */
>> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_9, addr_off, addr->dn_reg));
>> +
>> + if (dt_regset_xalloc_args(drp) == -1)
>> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>> + emit(dlp, BPF_MOV_REG(BPF_REG_3, addr->dn_reg));
>> + dt_regset_free(drp, addr->dn_reg);
>> + emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_9));
>> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, data_off));
>> + emit(dlp, BPF_MOV_IMM(BPF_REG_2, size));
>> + dt_regset_xalloc(drp, BPF_REG_0);
>> + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
>> + dt_regset_free_args(drp);
>> + dt_regset_free(drp, BPF_REG_0);
>> +}
>
> So this (obviously, sensibly) does no pointer chasing, so this is for
> printing a single contiguous block of bytes which happens to be a
> complex aggregate structure? Sounds sensible.
>
>> +static int
>> +dt_print_visit(const char *name, ctf_id_t type, unsigned long offset,
>> + int depth, void *arg)
>> +{
>> + struct dt_visit_arg *dva = arg;
>
> Hm, I should probably implement a _next() variant of ctf_type_visit().
> This is a bit clumsy, but it's the best you can do with the current API.
>
ctf_type_visit was great! the only issue here is in handling closing
parens, but that's a small thing and very specific to this use case.
>> + const void *data = (char *)dva->dv_data + offset/NBBY;
>> + size_t typesize, typelen, asize;
>
> type_size is an ssize_t. All those signed/unsigned warnings are not for
> fun.
>
fixed.
>> + char typename[DT_NAME_LEN] = { 0 };
>
> This limitation is unnecessary.
>
removed, thanks to asprintf() + ctf_type_aname()
>> + char datastr[DT_PRINT_LEN] = { 0 };
>> + const char *membername = NULL;
>> + int64_t intdata = 0;
>> + const char *ename;
>> + ctf_encoding_t e;
>> + ctf_arinfo_t a;
>> + ctf_id_t rtype;
>> + int issigned;
>> + int rkind;
>> + int i;
>> +
>> + /* run out of data to display, or hit depth limit? */
>> + if ((offset/NBBY) > dva->dv_size || depth >= DT_PRINT_DEPTH) {
>> + dt_printf(dva->dv_dtp, dva->dv_fp, "%*s...\n",
>> + dva->dv_startindent + depth, "");
>> + return -1;
>> + }
>> + rtype = ctf_type_resolve(dva->dv_ctfp, type);
>> + if (rtype == CTF_ERR)
>> + rtype = type;
>
> ... no matter what the error is? That's... excitingly risky. I'm not
> sure what sort of error you're even expecting: just about the only
> errors you can get are ECTF_NONREPRESENTABLE (in which case this *is*
> probably the right thing to do, only you should explicitly handle
> printing nonrepresentable types in this case), or ECTF_CORRUPT, in
> which case we should *definitely* emit some sort of indication that
> something is very wrong.
>
> Also, do we really want to resolve away typedefs entirely? Their textual
> name surely provides useful information, but we're not printing it at
> all.
Fixed now; we error out on type resolution failure. In the next version
(forthcoming) we always print the type name and then use the resolved
type to print the associated data; so type resolution is all about
being able to display the data. So for example in an sk_buff we see
.tail = (sk_buff_data_t)1364,
...where the sk_buff_data_t is the typedef, and the type resolution
allows us to print the data.
>
>> + rkind = ctf_type_kind(dva->dv_ctfp, rtype);
>
> This can in theory fail (but in practice it would require corrupt CTF to
> do so, since a disconnected slice is the only plausible cause, so you
> can probably ignore it).
>
>> + /* zeroed data? if so, do not display it. */
>> + if (depth > 0) {
>> + typesize = ctf_type_size(dva->dv_ctfp, rtype);
>
> This can *definitely* fail, though all the failures are unlikely. Error
> check!
>
Fixed.
>> + if (typesize < sizeof(zeros) && memcmp(data, zeros, typesize) == 0)
>> + return 0;
>
> If ctf_type_size had failed, this would lead to fun! (I am surprised,
> astonished and appalled that C has no more efficient way than this to
> check for zeroness/nonzeroness of a memory region, but it doesn't.)
>
>> + ctf_type_name(dva->dv_ctfp, type, typename, sizeof(typename));
>
> It is almost always a mistake to use ctf_type_name for anything, because
> of the horrible static buffers. Use ctf_type_aname(), which returns a
> dynamically allocated string (which always fits) which you can free.
>
Fixed now.
>> + switch (rkind) {
>> + case CTF_K_STRUCT:
>> + case CTF_K_UNION:
>> + snprintf(datastr, sizeof(datastr), "(%s) {", typename);
>
> I twitch violently whenever I see s(n)printf(). This sort of thing is
> what asprintf() is for.
>
Switched to using asprintf() everywhere now.
>> + break;
>> + case CTF_K_ARRAY:
>> + ctf_array_info(dva->dv_ctfp, rtype, &a);
>> + asize = ctf_type_size(dva->dv_ctfp, a.ctr_contents);
>> + snprintf(datastr, sizeof(datastr), "(%s) [%s", typename,
>> + a.ctr_nelems == 0 ? "]," : "");
>
> ctf_array_info and ctf_type_size can both fail. asprintf().
>
Fixed.
>> + /* array member processing will be handled below... */
>> + break;
>> + case CTF_K_INTEGER:
>> + /* type resolution sometimes fails for __u8 bitfields for some reason */
>
> What's the failure? It really shouldn't be failing! The only unusualness
> I can think of is that bitfields, including __u8 bitfields, will
> probably be encoded as slices of the base type, but that should be
> transparent to you: they should appear as suitably-encoded instances of
> the underlying type. If the failures turn out to be a bug I'd like to
> fix it.
>
I think this predates the work Kris did on BTF-to-CTF conversion - I had
a hacky version of same and ended up with some weirdness, likely due to
bugs in my implementation of the conversion process. I tried to
reproduce the problem with the proper BTF-to-CTF conversion and can't
now, so I think this was just a consequence of brokenness I'd introduced
(but that is thankfully not in dtrace). I'll continue to keep an eye out
for it, but no sign thus far.
>> + case CTF_K_TYPEDEF:
>> + ctf_type_encoding(dva->dv_ctfp, rtype, &e);
>
> OK so this is based on rkind, which is the kind of a resolved
> type. We've already chased through typedefs! I don't see how you can get
> a typedef back out. Even a typedef of a slice of an int is going to
> chase to the slice, then return ctf_type_kind() of the kind it is a
> slice of (and ctf_type_encoding() will apply the slice to the base type
> it is a ref of, then return that).>
> How can you ever get a CTF_K_TYPEDEF here?
>
removed now.
>> + issigned = (e.cte_format & CTF_INT_SIGNED) != 0;
>> + switch (e.cte_bits) {
>> + case 8:
>> + if (issigned)
>> + intdata = (int64_t)*(char *)data;
>> + else
>> + intdata = (int64_t)*(unsigned char *)data;
>> + break;
>> + case 16:
>> + if (issigned)
>> + intdata = (int64_t)*(int16_t *)data;
>> + else
>> + intdata = (int64_t)*(uint16_t *)data;
>> + break;
>> + case 32:
>> + if (issigned)
>> + intdata = (int64_t)*(int32_t *)data;
>> + else
>> + intdata = (int64_t)*(uint32_t *)data;
>> + break;
>> + case 64:
>> + if (issigned) {
>> + intdata = (int64_t)*(int64_t *)data;
>> + } else {
>> + /* cannot use intdata (int64_t) to hold uin64_t;
>> + * stringify the cast and data here instead.
>> + */
>> + snprintf(datastr, sizeof(datastr), "(%s)%lu,",
>> + typename, *(uint64_t *)data);
>
> asprintf
>
>> + goto doprint;
>> + }
>> + break;
>> + default:
>> + if (e.cte_bits <= 64) {
>> + unsigned int lshift, rshift;
>> + uint64_t bitdata = *(uint64_t *)((char *)dva->dv_data +
>> + (offset + e.cte_offset)/NBBY);
>> + /* shift bitfield data left then right to
>> + * eliminate unneeded bits.
>> + */
>> + lshift = 63 - ((offset + e.cte_offset) % 64);
>> + rshift = 64 - e.cte_bits;
>> + bitdata = bitdata << lshift;
>> + bitdata = bitdata >> rshift;
>> + intdata = (int64_t)bitdata;
>
> What a cunning way to get around the gross definition of bitfields in C.
> It's almost all implementation-dependent, but this compiler is the same
> implementation :)
>
>> + /* zero-value bitfield; do not display */
>> + if (intdata == 0)
>> + return 0;
>> + } else {
>> + /* > 64 bit integer vals not supported. */
>> + snprintf(datastr, sizeof(datastr),
>> + "(%s)?(%d bit value),",
>> + typename, e.cte_bits);
>
> asprintf
>
>> + goto doprint;
>> + }
>> + break;
>> + }
>> + if ((e.cte_format & CTF_INT_CHAR) != 0 &&
>> + strcmp(typename, "char") == 0) {
>> + snprintf(datastr, sizeof(datastr), "(%s)'%c',",
>> + typename, (char)intdata);
>> + } else {
>> + snprintf(datastr, sizeof(datastr), "(%s)%ld,",
>> + typename, intdata);
>
> asprintf
>
>> + }
>> + break;
>> + case CTF_K_ENUM:
>> + ename = ctf_enum_name(dva->dv_ctfp, rtype, *(int *)data);
>
> (this is, of course, routinely null, with the typedef name providing the
> information we want, but we resolved that away.)
>
>> + if (ename != NULL) {
>> + /* exception to printing 0-valued data here;
>> + * enum values are often meaningful with 0
>> + * value so display the enum value string.
>> + */
>> + snprintf(datastr, sizeof(datastr), "(%s)%s,",
>> + typename, ename);
>> + } else {
>> + snprintf(datastr, sizeof(datastr), "(%s)%d,",
>> + typename, *(int *)data);
>
> asprintf...
>
>> + }
>> + break;
>> + case CTF_K_POINTER:
>> + snprintf(datastr, sizeof(datastr), "(%s)0x%lx,",
>> + typename, (uintptr_t)*(void **)data);
>
> asprintf...
>
>> + if (dt_printf(dva->dv_dtp, dva->dv_fp, "%*s%s%s%s%s\n",
>> + dva->dv_startindent + depth, "",
>> + membername != NULL ? "." : "",
>> + membername != NULL ? membername : "",
>> + membername != NULL ? " = " : "",
>> + datastr) < 0)
>> + return -1;
>> +
>> + /* type visit does not recurse into array elements... */
>
> Mostly because it's so simple to visit them yourself, I think, and
> because the visit function very much assumes it's visiting something
> with elements distinguished via field names, which arrays don't have.
>
yep, makes sense.
>> + if (rkind == CTF_K_ARRAY) {
>> + struct dt_visit_arg dva2;
>> + int ischar = 0;
>> +
>> + dva2.dv_dtp = dva->dv_dtp;
>> + dva2.dv_ctfp = dva->dv_ctfp;
>> + dva2.dv_fp = dva->dv_fp;
>> + dva2.dv_data = dva->dv_data;
>> + dva2.dv_startindent = dva->dv_startindent;
>> + ctf_type_encoding(dva->dv_ctfp, a.ctr_contents, &e);
>
> (error checking)
>
fixed.
Thanks again for the review! I've tried to address all the comments in
the v4, and manual and automated tests all pass.
Alan
More information about the DTrace-devel
mailing list