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

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

[Alan Maguire]

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

[PATCH v4 2/3] libdtrace: BTF->CTF conversion for kernel modules

[Alan Maguire]

We had logic in place to collect BTF in modules and convert vmlinux
BTF to CTF, but were not doing the same for module BTF.  Add support
to do this, and explicitly set parent dict to "shared_ctf".  With
this in place (along with the support to find the appropriate module
CTF) we can successfully print module-specific types such as:

$ dtrace -n 'fbt::ieee80211_rx_napi:entry { print((struct ieee80211_hw *)arg0); }'
dtrace: description 'fbt::ieee80211_rx_napi:entry ' matched 1 probe
CPU     ID                    FUNCTION:NAME
  6 117489          ieee80211_rx_napi:entry 0xffff9fabc2e688e0 = *
                                            (struct ieee80211_hw) {
                                             .conf = (struct ieee80211_conf) {
                                              .listen_interval = (u16)10,
                                              .long_frame_max_tx_count = (u8)4,
                                              .short_frame_max_tx_count = (u8)7,
                                             },
                                             .wiphy = (struct wiphy *)0xffff9fabc2e683c0,
                                             .rate_control_algorithm = (const char *)0xffffffffc1346ae5,
...

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 libdtrace/dt_btf.c    | 3 ++-
 libdtrace/dt_cg.c     | 1 -
 libdtrace/dt_module.c | 7 ++++++-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/libdtrace/dt_btf.c b/libdtrace/dt_btf.c
index 9e67fd9d..4e1ed9f6 100644
--- a/libdtrace/dt_btf.c
+++ b/libdtrace/dt_btf.c
@@ -715,6 +715,7 @@ dt_btf_to_ctf(dtrace_hdl_t *dtp, dt_module_t *dmp, dt_btf_t *btf)
 
 		if (strcmp(dmp->dm_name, "vmlinux") == 0)
 			goto out;
+		ctf_parent_name_set(ctf, "shared_ctf");
 	}
 
 	if (!btf)
@@ -867,7 +868,7 @@ dt_btf_lookup_name_kind(dtrace_hdl_t *dtp, dt_btf_t *btf, const char *name,
 	 * shared one.
 	 */
 	 if (!dtp->dt_shared_btf) {
-		  dt_btf_load_module(dtp, dtp->dt_exec);
+		  btf = dt_btf_load_module(dtp, dtp->dt_exec);
 
 		  if (!btf)
 			   btf = dtp->dt_shared_btf;
diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 2eb2df9c..4206235b 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -2852,7 +2852,6 @@ dt_cg_act_print(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
 		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)));
-
 	pfp = dt_printf_create(dtp, dmp->dm_name);
 
 	/* reserve space for addr/type, data/size */
diff --git a/libdtrace/dt_module.c b/libdtrace/dt_module.c
index 5e608446..d8a539af 100644
--- a/libdtrace/dt_module.c
+++ b/libdtrace/dt_module.c
@@ -962,8 +962,8 @@ dt_kern_module_find_ctf(dtrace_hdl_t *dtp, dt_module_t *dmp)
 			dt_dprintf("Cannot open CTF archive %s: %s; "
 				   "looking for in-module CTF instead.\n",
 				   ctfa_name, ctf_errmsg(dtp->dt_ctferr));
-#endif
 			return;
+#endif
 		}
 
 		if (dtp->dt_ctfa_path == NULL)
@@ -982,6 +982,11 @@ dt_kern_module_find_ctf(dtrace_hdl_t *dtp, dt_module_t *dmp)
 			    "archive: %s; looking for out-of-tree module.\n",
 			    dmp->dm_name, ctf_errmsg(dtp->dt_ctferr));
 		}
+	} else if (dtp->dt_shared_ctf) {
+#ifdef HAVE_LIBCTF
+		/* Have shared CTF via vmlinux BTF; now do BTF->CTF for mod. */
+		dt_kern_module_ctf_from_btf(dtp, dmp);
+#endif
 	}
 
 	if (dmp->dm_ctfp != NULL) {
-- 
2.43.5

[PATCH v4 3/3] unittest/print: add test covering module-defined type

[Alan Maguire]

print() action tests do not cover module-defined types.  Add a test
using a module-defined type that has not changed recently.

struct tun_page was chosen since it is identical in UEK6U3 and upstream
and declared in tun.c which is a module on most distros.

Added tun to test/modules as suggested by Eugene.

Suggested-by: Nick Alcock <nick.alcock@oracle.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
---
 test/modules                            |  1 +
 test/unittest/print/tst.print.modtype.d | 18 ++++++++++++++++++
 test/unittest/print/tst.print.modtype.r |  6 ++++++
 3 files changed, 25 insertions(+)
 create mode 100644 test/unittest/print/tst.print.modtype.d
 create mode 100644 test/unittest/print/tst.print.modtype.r

diff --git a/test/modules b/test/modules
index 53dc5544..0f01d6e0 100644
--- a/test/modules
+++ b/test/modules
@@ -2,3 +2,4 @@ ext4
 isofs
 nfs
 rds
+tun
diff --git a/test/unittest/print/tst.print.modtype.d b/test/unittest/print/tst.print.modtype.d
new file mode 100644
index 00000000..ed68b492
--- /dev/null
+++ b/test/unittest/print/tst.print.modtype.d
@@ -0,0 +1,18 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * Licensed under the Universal Permissive License v 1.0 as shown at
+ * http://oss.oracle.com/licenses/upl.
+ */
+/* @@nosort */
+
+#pragma D option quiet
+
+BEGIN
+{
+	tun_page = (struct tun_page *)alloca(sizeof (struct tun_page));
+	tun_page->page = (struct page *)0xfeedfacefeedface;
+	tun_page->count = 123;
+	print(tun_page);
+	exit(0);
+}
diff --git a/test/unittest/print/tst.print.modtype.r b/test/unittest/print/tst.print.modtype.r
new file mode 100644
index 00000000..b9736607
--- /dev/null
+++ b/test/unittest/print/tst.print.modtype.r
@@ -0,0 +1,6 @@
+{ptr} = *
+                                            (struct tun_page) {
+                                             .page = (struct page *){ptr},
+                                             .count = (int)123,
+                                            }
+
-- 
2.43.5

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

[Kris Van Hees]

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

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

[Alan Maguire]

On 01/08/2024 17:50, Kris Van Hees wrote:
> 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.
>

yep, good point. I've sent a v5 which doesn't pass it as an arg and uses
record size below as suggested. Thanks!

Alan

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