Re: [PATCH v4 1/3] print() action: identify ctf object used for print

[Kris Van Hees <kris.van.hees@oracle.com>]

Small question below...

On Mon, Jul 08, 2024 at 03:54:02PM +0100, Alan Maguire wrote:
> when generating code for print() action we need to identify source
> of CTF; is it shared CTF, cdefs or ddefs or another module?
> Rework the print() action to be a printf()like function under the
> hood using the format specifier as module name.  We can then
> do away with dt_print_print(), calling dt_print_type() directly
> from consume.
> 
> Suggested-by: Kris Van Hees <kris.van.hees@oracle.com>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  libdtrace/dt_cg.c      |  8 +++++++-
>  libdtrace/dt_consume.c | 28 +++-------------------------
>  libdtrace/dt_impl.h    |  1 +
>  libdtrace/dt_printf.c  | 28 ++++++++++++++++++++++++----
>  libdtrace/dt_printf.h  |  6 ++++--
>  5 files changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index a1c24e37..2eb2df9c 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -20,6 +20,7 @@
>  #include <dt_dctx.h>
>  #include <dt_cg.h>
>  #include <dt_grammar.h>
> +#include <dt_module.h>
>  #include <dt_parser.h>
>  #include <dt_printf.h>
>  #include <dt_provider.h>
> @@ -2838,8 +2839,11 @@ dt_cg_act_print(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>  	ctf_file_t	*fp = addr->dn_ctfp;
>  	ctf_id_t	type = addr->dn_type;
>  	char		n[DT_TYPE_NAMELEN];
> +	dt_module_t	*dmp;
> +	dt_pfargv_t	*pfp;
>  	size_t		size;
>  
> +	dmp = dt_module_lookup_by_ctf(dtp, fp);
>  	type = ctf_type_reference(fp, type);
>  	if (type == CTF_ERR)
>  		longjmp(yypcb->pcb_jmpbuf, EDT_CTF);
> @@ -2849,9 +2853,11 @@ dt_cg_act_print(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>  			"print( ) argument #1 reference has type '%s' with size 0; cannot print( ) it.\n",
>  			ctf_type_name(fp, type, n, sizeof(n)));
>  
> +	pfp = dt_printf_create(dtp, dmp->dm_name);
> +
>  	/* 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);
> +			      sizeof(uint64_t), 8, pfp, type);
>  	data_off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_PRINT,
>  			      size, 8, NULL, size);

Why pass 'size' as record argument when you are already passing it as the true
size of the record?  That's unnecessary.  We can clearly get the size of the
record from the 2nd record's dtrd_size.

> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index dec2314b..e6880703 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -507,7 +507,7 @@ dt_nullrec()
>  	return DTRACE_CONSUME_NEXT;
>  }
>  
> -static int
> +int
>  dt_read_scalar(caddr_t addr, const dtrace_recdesc_t *rec, uint64_t *valp)
>  {
>  	addr += rec->dtrd_offset;
> @@ -1983,25 +1983,6 @@ dt_print_trace(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
>  	return dt_print_rawbytes(dtp, fp, data, rec->dtrd_size);
>  }
>  
> -static int
> -dt_print_print(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
> -	       const caddr_t buf)
> -{
> -	dtrace_recdesc_t *data_rec = rec + 1;
> -	size_t max_size = dtp->dt_options[DTRACEOPT_PRINTSIZE];
> -	size_t size = (size_t)data_rec->dtrd_arg;
> -	uint64_t printaddr;
> -
> -	if (size > max_size)
> -		size = max_size;
> -
> -	if (dt_read_scalar(buf, rec, &printaddr) < 0)
> -		return dt_set_errno(dtp, EDT_PRINT);
> -
> -	return dt_print_type(dtp, fp, printaddr, (ctf_id_t)rec->dtrd_arg,
> -			     (caddr_t)buf + data_rec->dtrd_offset, size);
> -}
> -
>  /*
>   * The lifecycle of speculation buffers is as follows:
>   *
> @@ -2562,11 +2543,8 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
>  			func = dtrace_freopen;
>  			break;
>  		case DTRACEACT_PRINT:
> -			n = dt_print_print(dtp, fp, rec, data);
> -			if (n < 0)
> -				return -1;
> -			i += n - 1;
> -			continue;
> +			func = dt_print_type;
> +			break;
>  		default:
>  			break;
>  		}
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 01313ff3..88b31ce0 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -789,6 +789,7 @@ extern int dt_buffered_flush(dtrace_hdl_t *, dtrace_probedata_t *,
>      const dtrace_recdesc_t *, const dtrace_aggdata_t *, uint32_t flags);
>  extern void dt_buffered_disable(dtrace_hdl_t *);
>  extern void dt_buffered_destroy(dtrace_hdl_t *);
> +extern int dt_read_scalar(caddr_t, const dtrace_recdesc_t *, uint64_t *);
>  
>  extern uint64_t dt_stddev(uint64_t *, uint64_t);
>  
> diff --git a/libdtrace/dt_printf.c b/libdtrace/dt_printf.c
> index 50842216..d794dd47 100644
> --- a/libdtrace/dt_printf.c
> +++ b/libdtrace/dt_printf.c
> @@ -14,6 +14,7 @@
>  #include <limits.h>
>  #include <port.h>
>  
> +#include <dt_module.h>
>  #include <dt_printf.h>
>  #include <dt_string.h>
>  #include <dt_impl.h>
> @@ -2235,15 +2236,34 @@ err:
>  }
>  
>  int
> -dt_print_type(dtrace_hdl_t *dtp, FILE *fp, uint64_t printaddr,
> -	      ctf_id_t type, caddr_t data, size_t size)
> +dt_print_type(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
> +	      const dtrace_probedata_t *data, const dtrace_recdesc_t *recs,
> +	      uint_t nrecs, const void *buf, size_t len)
>  {
> +	const char *modname = ((dt_pfargv_t *)fmtdata)->pfv_format;
> +	size_t max_size = dtp->dt_options[DTRACEOPT_PRINTSIZE];
> +	ctf_id_t type = (ctf_id_t)recs->dtrd_arg;
> +	const dtrace_recdesc_t *data_rec = recs + 1;
> +	size_t size = (size_t)data_rec->dtrd_arg;

Use data_rec->dtrd_size.  No need to use dtrd_arg with a cast.

> +	caddr_t addr_data = (caddr_t)buf;
>  	struct dt_visit_arg dva;
> +	uint64_t printaddr;
> +	dt_module_t *dmp;
>  
> +	if (size > max_size)
> +		size = max_size;
> +
> +	if (dt_read_scalar(addr_data, recs, &printaddr) < 0)
> +		return dt_set_errno(dtp, EDT_PRINT);
> +
> +	dmp = dt_module_lookup_by_name(dtp, modname);
> +	if (!dmp) {
> +		return dt_set_errno(dtp, EDT_PRINT);
> +	}
>  	dva.dv_dtp = dtp;
>  	dva.dv_fp = fp;
> -	dva.dv_ctfp = dtp->dt_ddefs->dm_ctfp;
> -	dva.dv_data = data;
> +	dva.dv_ctfp = dmp->dm_ctfp;
> +	dva.dv_data = addr_data + data_rec->dtrd_offset;
>  	dva.dv_size = size;
>  	dva.dv_last_depth = 0;
>  	dva.dv_startindent = DT_PRINT_STARTINDENT;
> diff --git a/libdtrace/dt_printf.h b/libdtrace/dt_printf.h
> index 9771c4a8..4f4386a5 100644
> --- a/libdtrace/dt_printf.h
> +++ b/libdtrace/dt_printf.h
> @@ -108,8 +108,10 @@ extern int dt_print_ustack(dtrace_hdl_t *, FILE *,
>      const char *, caddr_t, uint64_t);
>  extern int dt_print_mod(dtrace_hdl_t *, FILE *, const char *, caddr_t);
>  extern int dt_print_umod(dtrace_hdl_t *, FILE *, const char *, caddr_t);
> -extern int dt_print_type(dtrace_hdl_t *, FILE *, uint64_t, ctf_id_t,
> -			 caddr_t, size_t);
> +extern int dt_print_type(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
> +			 const dtrace_probedata_t *data,
> +			 const dtrace_recdesc_t *recs,
> +			 uint_t nrecs, const void *buf, size_t len);
>  
>  #ifdef	__cplusplus
>  }
> -- 
> 2.43.5