[DTrace-devel] [PATCH 1/2] Add support for the print() action

[Alan Maguire]

On 17/11/2023 18:52, Eugene Loh via DTrace-devel wrote:
> Incidentally, we've been adding version numbers to the PATCH label. 
> E.g., "PATCH v2 1/2".  No biggie.
> 

sure will do for the next round.

> Adding the address to the output is fine.  It's not Solaris
> functionality, but... who cares!  I am puzzled by the "0xffff1234 = *"
> output format.  The asterisk confuses me;  if anything, I might imagine
> "* 0xffff1234 = etc.".  But maybe I'm just looking at it wrong.
> 

The aim is to say that the argument is a pointer to the type rather than
the type itself; i.e. a pointer to an int rather than an int:

	0xfeedfacefeedface = *
	(int)2

"pointer is a pointer to an int with value 2".

If there's a clearer way to signify this, we can change it,
or indeed drop it completely, but I do think having the
pointer address is definitely useful.

> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> subject to fixing the apparent reg leak below...
> 
thanks! will fix

> On 11/17/23 06:48, Alan Maguire via DTrace-devel wrote:
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> +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    basetype, type = addr->dn_type;
>> +    char        n[DT_TYPE_NAMELEN];
>> +    size_t        size;
>> +
>> +    if (dt_node_is_pointer(addr) == 0) {
>> +        dnerror(addr, D_PRINT_ADDR,
>> +            "print( ) argument #1 is incompatible with "
>> +            "prototype:\n\tprototype: pointer\n"
>> +            "\t argument: %s\n",
>> +            dt_node_type_name(addr, n, sizeof(n)));
>> +    }
>> +    basetype = ctf_type_reference(fp, type);
>> +    if (basetype == CTF_ERR)
>> +        longjmp(yypcb->pcb_jmpbuf, EDT_CTF);
>> +    size = ctf_type_size(fp, basetype);
>> +    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, basetype, n, sizeof(n)));
>> +    if (size > dtp->dt_options[DTRACEOPT_PRINTSIZE])
>> +        size = dtp->dt_options[DTRACEOPT_PRINTSIZE];
>> +
>> +    /* reserve space for addr/type, data/size */
>> +    addr_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
>> +                  sizeof(uint64_t), 8, NULL, basetype);
>> +    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 addrress for later print()ing */
> 
> addrress
> ->
> address
>

oops, will fix

>> +    emit(dlp, BPF_STORE(BPF_DW, BPF_REG_9, addr_off, addr->dn_reg));
>> +
>> +    /* store addr, size, type and bpf_probe_read(data_off, size,
>> addr); */
> 
> Strike "store addr, size, type and".
> 

missed that last time, will remove

>> +    if (dt_regset_xalloc_args(drp) == -1)
>> +        longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>> +    emit(dlp, BPF_MOV_REG(BPF_REG_3, addr->dn_reg));
> 
> Great, but need to free the addr->dn_reg reg?  Like "dt_regset_free(drp,
> addr->dn_reg)"?  E.g. what if you run one of your tests, but you replace
> "print(t);" with "print(t); print(t); print(t); print(t); print(t);
> print(t); [...for a total of 20x...] print(t);"?  Okay, or run out of regs?
>

yep free needed here. i'll add it next time.

>> +    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);
>> +}
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel