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

[Eugene Loh]

Incidentally, we've been adding version numbers to the PATCH label.  
E.g., "PATCH v2 1/2".  No biggie.

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.

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

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

> +	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".

> +	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?

> +	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);
> +}