* [PATCH v5 1/6] usdt: get arg types and xlations into DTrace from the DOF
2024-11-05 0:06 [PATCH v5 0/6] usdt typed args, translators and arg mapping Nick Alcock
@ 2024-11-05 0:06 ` Nick Alcock
2024-11-05 16:56 ` Kris Van Hees
2024-11-05 0:06 ` [PATCH v5 2/6] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c Nick Alcock
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Nick Alcock @ 2024-11-05 0:06 UTC (permalink / raw)
To: dtrace, dtrace-devel
This change propagates native and xlated arg types and mapping info all
the way from the DOF, through the parser and dtprobed and the DOF stash,
into DTrace, where they end up in the pid_probespec_t for USDT probes.
We don't do anything with them once they get there in this commit: no
user-visible impacts expected.
We bump the DOF_PARSED_VERSION since we add three new types of record to the
dof_parser_t, all optional, covering native and xlated args and arg mapping
tables; to make life easier for consumers we emit them in a defined order
(documented in dof_parser.h), and add arg counts to the DIT_PROBE record
that precedes them indicating which will be present and how many args are in
them. This means we retain the property that you can always tell which
records within a provider are coming next purely from records you already
have: there's no need to make decisions once the records turn up. The DOF
stash hardly changes: all that happens is that the parsed data written to
each per-probe file gains some extra types of record (it can have
DIT_ARGS_NATIVE, DIT_ARGS_XLAT and DIT_ARGS_MAP entries following the
DIT_PROBE record now).
As usual with DOF_PARSED_VERSION bumps, DTraces that cross this change will
refuse to read probes added by dtprobeds that are on the other side of it.
(Restarting dtprobed will reparse all the probes in the stash to match the
new layout, and newly-started DTraces will pick that up -- so this only
affects running DTraces picking up new probes, which DTrace cannot do yet:
so no visible effects are expected.)
The code is a bit repetitive, with nargs, xargs and arg mapping all handled
separately even though the code is very similar. In future maybe we could
unify them (but my attempts led to code that was much harder to read than
the original).
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
dtprobed/dof_stash.c | 15 +++--
dtprobed/dtprobed.c | 10 ++-
include/dtrace/pid.h | 7 ++
libcommon/dof_parser.c | 150 +++++++++++++++++++++++++++++++----------
libcommon/dof_parser.h | 64 ++++++++++++++++--
libdtrace/dt_pid.c | 59 ++++++++++++++++
6 files changed, 256 insertions(+), 49 deletions(-)
diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
index 0e639076f655..8bf465678217 100644
--- a/dtprobed/dof_stash.c
+++ b/dtprobed/dof_stash.c
@@ -59,8 +59,10 @@
* startup if the dof_parsed_t structure changed. The file consists of a
* uint64_t version number (DOF_PARSED_VERSION), then a stream of
* dof_parsed_t records, the first of type DIT_PROVIDER, the second
- * DIT_PROBE, then as many struct dof_parsed_t's of type DIT_TRACEPOINT as
- * the DIT_PROBE record specifies.
+ * DIT_PROBE, then optionally some DIT_*ARGS records (if the count of native
+ * args in the probe is >0, you get a DIT_NATIVE_ARGS: if the count of xlated
+ * args is >0, you get DIT_XLAT_ARGS and DIT_MAP_ARGS), then as many struct
+ * dof_parsed_t's of type DIT_TRACEPOINT as the DIT_PROBE record specifies.
*
* /run/dtrace/probes/: Per-probe info, written by dtprobed, read by DTrace.
*
@@ -640,9 +642,14 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
break;
}
- /* Tracepoint: add to already-open file. */
+ /* Args info or tracepoint: add to already-open file. */
+ case DIT_ARGS_NATIVE:
+ case DIT_ARGS_XLAT:
+ case DIT_ARGS_MAP:
case DIT_TRACEPOINT:
- assert(state == DIT_PROBE || state == DIT_TRACEPOINT);
+ assert(state == DIT_PROBE || state == DIT_ARGS_NATIVE ||
+ state == DIT_ARGS_XLAT || state == DIT_ARGS_MAP ||
+ state == DIT_TRACEPOINT);
state = accump->parsed->type;
if (write_chunk(parsed_fd, accump->parsed, accump->parsed->size) < 0)
diff --git a/dtprobed/dtprobed.c b/dtprobed/dtprobed.c
index f4c6be1e045d..2ca39b26f88a 100644
--- a/dtprobed/dtprobed.c
+++ b/dtprobed/dtprobed.c
@@ -791,16 +791,20 @@ process_dof(pid_t pid, int out, int in, dev_t dev, ino_t inum, dev_t exec_dev,
if (dof_stash_push_parsed(&accum, probe) < 0)
goto oom;
- for (j = 0; j < probe->probe.ntp; j++) {
+ j = 0;
+ do {
dof_parsed_t *tp = dof_read(pid, in);
errmsg = "no tracepoints in a probe, or parse state corrupt";
- if (!tp || tp->type != DIT_TRACEPOINT)
+ if (!tp || tp->type == DIT_PROVIDER || tp->type == DIT_PROBE)
goto err;
if (dof_stash_push_parsed(&accum, tp) < 0)
goto oom;
- }
+
+ if (tp->type == DIT_TRACEPOINT)
+ j++;
+ } while (j < probe->probe.ntp);
}
if (!reparsing)
diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
index 0129674adf0c..25bfacfdbfe2 100644
--- a/include/dtrace/pid.h
+++ b/include/dtrace/pid.h
@@ -35,6 +35,13 @@ typedef struct pid_probespec {
ino_t pps_inum; /* object inode */
char *pps_fn; /* object full filename */
uint64_t pps_off; /* probe offset (in object) */
+ int pps_nargc; /* number of native args */
+ int pps_xargc; /* number of xlated and mapped args */
+ char *pps_nargv; /* array of native args */
+ size_t pps_nargvlen; /* (high estimate of) length of array */
+ char *pps_xargv; /* array of xlated args */
+ size_t pps_xargvlen; /* (high estimate of) length of array */
+ int8_t *pps_argmap; /* mapped arg indexes */
/*
* Fields below this point do not apply to underlying probes.
diff --git a/libcommon/dof_parser.c b/libcommon/dof_parser.c
index d6631a1afdcb..1792a8bfcc84 100644
--- a/libcommon/dof_parser.c
+++ b/libcommon/dof_parser.c
@@ -807,10 +807,11 @@ validate_provider(int out, dof_hdr_t *dof, dof_sec_t *sec)
}
dt_dbg_dof(" Probe %d %s:%s:%s:%s with %d offsets, "
- "%d is-enabled offsets\n", j,
+ "%d is-enabled offsets, %i args, %i nargs, argidx %i\n", j,
strtab + prov->dofpv_name, "",
strtab + prb->dofpr_func, strtab + prb->dofpr_name,
- prb->dofpr_noffs, prb->dofpr_nenoffs);
+ prb->dofpr_noffs, prb->dofpr_nenoffs,
+ prb->dofpr_xargc, prb->dofpr_nargc, prb->dofpr_argidx);
}
return 0;
@@ -879,12 +880,26 @@ validate_probe(int out, dtrace_helper_probedesc_t *dhpb)
return 0;
}
+static size_t
+strings_len(const char *strtab, size_t count)
+{
+ size_t len = 0;
+
+ for (; count > 0; count--) {
+ size_t this_len = strlen(strtab) + 1;
+
+ len += this_len;
+ strtab += this_len;
+ }
+ return len;
+}
+
static void
emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
{
int i;
- dof_parsed_t *probe_msg;
- size_t probe_msg_size;
+ dof_parsed_t *msg;
+ size_t msg_size;
char *ptr;
dt_dbg_dof(" Creating probe %s:%s:%s:%s\n", dhpb->dthpb_prov,
@@ -893,35 +908,106 @@ emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
if (validate_probe(out, dhpb) != 0)
return;
- probe_msg_size = offsetof(dof_parsed_t, probe.name) +
- strlen(dhpb->dthpb_mod) + 1 + strlen(dhpb->dthpb_func) + 1 +
- strlen(dhpb->dthpb_name) + 1;
+ /*
+ * Compute the size of all the optional elements first, to fill out the
+ * flags.
+ */
- probe_msg = malloc(probe_msg_size);
- if (!probe_msg) {
- dof_error(out, ENOMEM, "Out of memory allocating probe");
- return;
- }
+ msg_size = offsetof(dof_parsed_t, probe.name) +
+ strlen(dhpb->dthpb_mod) + 1 +
+ strlen(dhpb->dthpb_func) + 1 +
+ strlen(dhpb->dthpb_name) + 1;
- memset(probe_msg, 0, probe_msg_size);
+ msg = malloc(msg_size);
+ if (!msg)
+ goto oom;
- probe_msg->size = probe_msg_size;
- probe_msg->type = DIT_PROBE;
- probe_msg->probe.ntp = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
- ptr = stpcpy(probe_msg->probe.name, dhpb->dthpb_mod);
+ memset(msg, 0, msg_size);
+
+ msg->size = msg_size;
+ msg->type = DIT_PROBE;
+ msg->probe.ntp = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
+ msg->probe.nargc = dhpb->dthpb_nargc;
+ msg->probe.xargc = dhpb->dthpb_xargc;
+
+ ptr = stpcpy(msg->probe.name, dhpb->dthpb_mod);
ptr++;
ptr = stpcpy(ptr, dhpb->dthpb_func);
ptr++;
strcpy(ptr, dhpb->dthpb_name);
- dof_parser_write_one(out, probe_msg, probe_msg_size);
+ dof_parser_write_one(out, msg, msg_size);
- free(probe_msg);
+ free(msg);
- /* XXX TODO translated args
- pp->ftp_nargs = dhpb->dthpb_xargc;
- pp->ftp_xtypes = dhpb->dthpb_xtypes;
- pp->ftp_ntypes = dhpb->dthpb_ntypes;
- */
+ /*
+ * Emit info on all native and translated args in turn.
+ *
+ * FIXME: this code is a bit repetitious.
+ *
+ * First native args (if any).
+ */
+
+ if (dhpb->dthpb_nargc > 0) {
+ size_t nargs_size;
+
+ nargs_size = strings_len(dhpb->dthpb_ntypes, dhpb->dthpb_nargc);
+ msg_size = offsetof(dof_parsed_t, nargs.args) + nargs_size;
+
+ msg = malloc(msg_size);
+ if (!msg)
+ goto oom;
+
+ memset(msg, 0, msg_size);
+
+ msg->size = msg_size;
+ msg->type = DIT_ARGS_NATIVE;
+ memcpy(msg->nargs.args, dhpb->dthpb_ntypes, nargs_size);
+ dof_parser_write_one(out, msg, msg_size);
+
+ free(msg);
+
+ /* Then translated args. */
+
+ if (dhpb->dthpb_xargc > 0) {
+ size_t xargs_size, map_size;
+
+ xargs_size = strings_len(dhpb->dthpb_xtypes,
+ dhpb->dthpb_xargc);
+ msg_size = offsetof(dof_parsed_t, xargs.args) +
+ xargs_size;
+
+ msg = malloc(msg_size);
+ if (!msg)
+ goto oom;
+
+ memset(msg, 0, msg_size);
+
+ msg->size = msg_size;
+ msg->type = DIT_ARGS_XLAT;
+ memcpy(msg->xargs.args, dhpb->dthpb_xtypes, xargs_size);
+ dof_parser_write_one(out, msg, msg_size);
+
+ free(msg);
+
+ /* Then the mapping table. */
+
+ map_size = dhpb->dthpb_xargc * sizeof(int8_t);
+ msg_size = offsetof(dof_parsed_t, argmap.argmap) +
+ map_size;
+
+ msg = malloc(msg_size);
+ if (!msg)
+ goto oom;
+
+ memset(msg, 0, msg_size);
+
+ msg->size = msg_size;
+ msg->type = DIT_ARGS_MAP;
+ memcpy(msg->argmap.argmap, dhpb->dthpb_args, map_size);
+ dof_parser_write_one(out, msg, msg_size);
+ free(msg);
+ }
+ }
/*
* Emit info on each tracepoint in turn.
@@ -938,18 +1024,10 @@ emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
for (i = 0; i < dhpb->dthpb_nenoffs; i++)
emit_tp(out, dhpb->dthpb_base, dhpb->dthpb_enoffs[i], 1);
- /*
- * XXX later:
- * If the arguments are shuffled around we set the argument remapping
- * table. Later, when the probe fires, we only remap the arguments
- * if the table is non-NULL.
- *
- for (i = 0; i < dhpb->dthpb_xargc; i++) {
- if (dhpb->dthpb_args[i] != i) {
- pp->ftp_argmap = dhpb->dthpb_args;
- break;
- }
- } */
+ return;
+ oom:
+ dof_error(out, ENOMEM, "Out of memory allocating %zi bytes for probe",
+ msg_size);
}
static void
diff --git a/libcommon/dof_parser.h b/libcommon/dof_parser.h
index d3a6836f15fd..8f42d00551e3 100644
--- a/libcommon/dof_parser.h
+++ b/libcommon/dof_parser.h
@@ -15,10 +15,19 @@
#include <dtrace/helpers.h>
/*
- * Result of DOF probe parsing. We receive a provider info structure, followed
- * by N probe info structures each of which is followed by possibly many
- * tracepoint info structures, all tagged. Things not yet used for probe
- * creation (like args, translated types, etc) are not returned.
+ * Result of DOF probe parsing. The order of elements in the parsed stream
+ * is:
+ *
+ * DIT_PROVIDER (at least 1, which contains...)
+ * DIT_PROBE (at least 1, each of which has...)
+ * DIT_ARGS_NATIVE (1, optional)
+ * DIT_ARGS_XLAT (1, optional)
+ * DIT_ARGS_MAP (1, optional)
+ * DIT_TRACEPOINT (any number >= 1)
+ *
+ * The dof_parsed.provider.flags word indicates the presence of the
+ * various optional args records in the following stream (you can rely on
+ * them if it simplifies things, but you don't have to).
*
* On error, a DIT_ERR structure is returned with an error message.
*/
@@ -27,7 +36,10 @@ typedef enum dof_parsed_info {
DIT_PROVIDER = 0,
DIT_PROBE = 1,
DIT_TRACEPOINT = 2,
- DIT_ERR = 3
+ DIT_ERR = 3,
+ DIT_ARGS_NATIVE = 4,
+ DIT_ARGS_XLAT = 5,
+ DIT_ARGS_MAP = 6,
} dof_parsed_info_t;
/*
@@ -37,7 +49,7 @@ typedef enum dof_parsed_info {
* start which is the version of the dof_parseds within it. The data flowing
* over the stream from the seccomped parser has no such prefix.
*/
-#define DOF_PARSED_VERSION 1
+#define DOF_PARSED_VERSION 2
typedef struct dof_parsed {
/*
@@ -59,18 +71,58 @@ typedef struct dof_parsed {
*/
char name[1];
} provider;
+
struct dpi_probe_info {
/*
* Number of tracepoints that follow.
*/
size_t ntp;
+ /*
+ * Number of native arguments that follow (if > 0, a
+ * DIT_ARGS_NATIVE will be received).
+ */
+ size_t nargc;
+
+ /*
+ * Number of xlated arguments that follow (if > 0, a
+ * DIT_ARGS_XLAT and DIT_ARGS_MAP will be received).
+ */
+ size_t xargc;
+
/*
* Probe module, function, and name (\0-separated).
*/
char name[1];
} probe;
+ /* V2+ only. */
+ struct dpi_probe_args_native_info {
+ /*
+ * Array of native args. nargc in length.
+ */
+ char args[1];
+ } nargs;
+
+ /* V2+ only. */
+ struct dpi_probe_args_xlat_info {
+ /*
+ * Array of translated args. xargc in length.
+ */
+ char args[1];
+ } xargs;
+
+ /*
+ * V2+ only.
+ */
+ struct dpi_probe_args_map_info {
+ /*
+ * Mapping from native arg index to xlated arg index.
+ * xargc in length.
+ */
+ int8_t argmap[1];
+ } argmap;
+
struct dpi_tracepoint_info {
/*
* Offset of this tracepoint.
diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
index 41a87955ec1b..71ac1754d343 100644
--- a/libdtrace/dt_pid.c
+++ b/libdtrace/dt_pid.c
@@ -866,6 +866,9 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
uint64_t *dof_version;
char *prv, *mod, *fun, *prb;
dof_parsed_t *provider, *probe;
+ ssize_t nargvlen = 0, xargvlen = 0;
+ char *nargv = NULL, *xargv = NULL;
+ int8_t *argmap = NULL;
/*
* Regular files only: in particular, skip . and ..,
@@ -917,6 +920,47 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
p += probe->size;
seen_size += probe->size;
+ /*
+ * Assume the order given in dof_parser.h, for simplicity.
+ */
+ if (probe->probe.nargc > 0) {
+ dof_parsed_t *args = (dof_parsed_t *) p;
+
+ if (!validate_dof_record(path, args, DIT_ARGS_NATIVE,
+ dof_buf_size, seen_size))
+ goto parse_err;
+
+ nargv = args->nargs.args;
+ nargvlen = args->size - offsetof(dof_parsed_t, nargs.args);
+ assert(nargvlen >= 0);
+
+ p += args->size;
+ seen_size += args->size;
+ }
+ if (probe->probe.xargc > 0) {
+ dof_parsed_t *args = (dof_parsed_t *) p;
+
+ if (!validate_dof_record(path, args, DIT_ARGS_XLAT,
+ dof_buf_size, seen_size))
+ goto parse_err;
+
+ xargv = args->xargs.args;
+ xargvlen = args->size - offsetof(dof_parsed_t, xargs.args);
+ assert(xargvlen >= 0);
+
+ p += args->size;
+ seen_size += args->size;
+
+ if (!validate_dof_record(path, args, DIT_ARGS_MAP,
+ dof_buf_size, seen_size))
+ goto parse_err;
+
+ argmap = args->argmap.argmap;
+
+ p += args->size;
+ seen_size += args->size;
+ }
+
/*
* Now the parsed DOF for this probe's tracepoints.
*/
@@ -967,6 +1011,21 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
psp.pps_off = tp->tracepoint.addr - pmp->pr_file->first_segment->pr_vaddr;
psp.pps_nameoff = 0;
+ if (nargv) {
+ psp.pps_nargc = probe->probe.nargc;
+ psp.pps_nargvlen = nargvlen;
+ psp.pps_nargv = nargv;
+ }
+
+ if (xargv) {
+ psp.pps_xargc = probe->probe.xargc;
+ psp.pps_xargvlen = xargvlen;
+ psp.pps_xargv = xargv;
+ }
+
+ if (argmap)
+ psp.pps_argmap = argmap;
+
dt_dprintf("providing %s:%s:%s:%s for pid %d\n", psp.pps_prv,
psp.pps_mod, psp.pps_fun, psp.pps_prb, psp.pps_pid);
if (pvp->impl->provide_probe(dtp, &psp) < 0) {
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v5 1/6] usdt: get arg types and xlations into DTrace from the DOF
2024-11-05 0:06 ` [PATCH v5 1/6] usdt: get arg types and xlations into DTrace from the DOF Nick Alcock
@ 2024-11-05 16:56 ` Kris Van Hees
2024-11-05 18:01 ` [DTrace-devel] " Kris Van Hees
0 siblings, 1 reply; 15+ messages in thread
From: Kris Van Hees @ 2024-11-05 16:56 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
On Tue, Nov 05, 2024 at 12:06:03AM +0000, Nick Alcock wrote:
> This change propagates native and xlated arg types and mapping info all
> the way from the DOF, through the parser and dtprobed and the DOF stash,
> into DTrace, where they end up in the pid_probespec_t for USDT probes.
> We don't do anything with them once they get there in this commit: no
> user-visible impacts expected.
>
> We bump the DOF_PARSED_VERSION since we add three new types of record to the
> dof_parser_t, all optional, covering native and xlated args and arg mapping
> tables; to make life easier for consumers we emit them in a defined order
> (documented in dof_parser.h), and add arg counts to the DIT_PROBE record
> that precedes them indicating which will be present and how many args are in
> them. This means we retain the property that you can always tell which
> records within a provider are coming next purely from records you already
> have: there's no need to make decisions once the records turn up. The DOF
> stash hardly changes: all that happens is that the parsed data written to
> each per-probe file gains some extra types of record (it can have
> DIT_ARGS_NATIVE, DIT_ARGS_XLAT and DIT_ARGS_MAP entries following the
> DIT_PROBE record now).
>
> As usual with DOF_PARSED_VERSION bumps, DTraces that cross this change will
> refuse to read probes added by dtprobeds that are on the other side of it.
> (Restarting dtprobed will reparse all the probes in the stash to match the
> new layout, and newly-started DTraces will pick that up -- so this only
> affects running DTraces picking up new probes, which DTrace cannot do yet:
> so no visible effects are expected.)
>
> The code is a bit repetitive, with nargs, xargs and arg mapping all handled
> separately even though the code is very similar. In future maybe we could
> unify them (but my attempts led to code that was much harder to read than
> the original).
>
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
> dtprobed/dof_stash.c | 15 +++--
> dtprobed/dtprobed.c | 10 ++-
> include/dtrace/pid.h | 7 ++
> libcommon/dof_parser.c | 150 +++++++++++++++++++++++++++++++----------
> libcommon/dof_parser.h | 64 ++++++++++++++++--
> libdtrace/dt_pid.c | 59 ++++++++++++++++
> 6 files changed, 256 insertions(+), 49 deletions(-)
>
> diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
> index 0e639076f655..8bf465678217 100644
> --- a/dtprobed/dof_stash.c
> +++ b/dtprobed/dof_stash.c
> @@ -59,8 +59,10 @@
> * startup if the dof_parsed_t structure changed. The file consists of a
> * uint64_t version number (DOF_PARSED_VERSION), then a stream of
> * dof_parsed_t records, the first of type DIT_PROVIDER, the second
> - * DIT_PROBE, then as many struct dof_parsed_t's of type DIT_TRACEPOINT as
> - * the DIT_PROBE record specifies.
> + * DIT_PROBE, then optionally some DIT_*ARGS records (if the count of native
> + * args in the probe is >0, you get a DIT_NATIVE_ARGS: if the count of xlated
> + * args is >0, you get DIT_XLAT_ARGS and DIT_MAP_ARGS), then as many struct
> + * dof_parsed_t's of type DIT_TRACEPOINT as the DIT_PROBE record specifies.
> *
> * /run/dtrace/probes/: Per-probe info, written by dtprobed, read by DTrace.
> *
> @@ -640,9 +642,14 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
> break;
> }
>
> - /* Tracepoint: add to already-open file. */
> + /* Args info or tracepoint: add to already-open file. */
> + case DIT_ARGS_NATIVE:
> + case DIT_ARGS_XLAT:
> + case DIT_ARGS_MAP:
> case DIT_TRACEPOINT:
> - assert(state == DIT_PROBE || state == DIT_TRACEPOINT);
> + assert(state == DIT_PROBE || state == DIT_ARGS_NATIVE ||
> + state == DIT_ARGS_XLAT || state == DIT_ARGS_MAP ||
> + state == DIT_TRACEPOINT);
> state = accump->parsed->type;
>
> if (write_chunk(parsed_fd, accump->parsed, accump->parsed->size) < 0)
> diff --git a/dtprobed/dtprobed.c b/dtprobed/dtprobed.c
> index f4c6be1e045d..2ca39b26f88a 100644
> --- a/dtprobed/dtprobed.c
> +++ b/dtprobed/dtprobed.c
> @@ -791,16 +791,20 @@ process_dof(pid_t pid, int out, int in, dev_t dev, ino_t inum, dev_t exec_dev,
> if (dof_stash_push_parsed(&accum, probe) < 0)
> goto oom;
>
> - for (j = 0; j < probe->probe.ntp; j++) {
> + j = 0;
> + do {
> dof_parsed_t *tp = dof_read(pid, in);
>
> errmsg = "no tracepoints in a probe, or parse state corrupt";
> - if (!tp || tp->type != DIT_TRACEPOINT)
> + if (!tp || tp->type == DIT_PROVIDER || tp->type == DIT_PROBE)
> goto err;
>
> if (dof_stash_push_parsed(&accum, tp) < 0)
> goto oom;
> - }
> +
> + if (tp->type == DIT_TRACEPOINT)
> + j++;
> + } while (j < probe->probe.ntp);
> }
>
> if (!reparsing)
> diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
> index 0129674adf0c..25bfacfdbfe2 100644
> --- a/include/dtrace/pid.h
> +++ b/include/dtrace/pid.h
> @@ -35,6 +35,13 @@ typedef struct pid_probespec {
> ino_t pps_inum; /* object inode */
> char *pps_fn; /* object full filename */
> uint64_t pps_off; /* probe offset (in object) */
> + int pps_nargc; /* number of native args */
> + int pps_xargc; /* number of xlated and mapped args */
> + char *pps_nargv; /* array of native args */
> + size_t pps_nargvlen; /* (high estimate of) length of array */
> + char *pps_xargv; /* array of xlated args */
> + size_t pps_xargvlen; /* (high estimate of) length of array */
> + int8_t *pps_argmap; /* mapped arg indexes */
>
> /*
> * Fields below this point do not apply to underlying probes.
> diff --git a/libcommon/dof_parser.c b/libcommon/dof_parser.c
> index d6631a1afdcb..1792a8bfcc84 100644
> --- a/libcommon/dof_parser.c
> +++ b/libcommon/dof_parser.c
> @@ -807,10 +807,11 @@ validate_provider(int out, dof_hdr_t *dof, dof_sec_t *sec)
> }
>
> dt_dbg_dof(" Probe %d %s:%s:%s:%s with %d offsets, "
> - "%d is-enabled offsets\n", j,
> + "%d is-enabled offsets, %i args, %i nargs, argidx %i\n", j,
> strtab + prov->dofpv_name, "",
> strtab + prb->dofpr_func, strtab + prb->dofpr_name,
> - prb->dofpr_noffs, prb->dofpr_nenoffs);
> + prb->dofpr_noffs, prb->dofpr_nenoffs,
> + prb->dofpr_xargc, prb->dofpr_nargc, prb->dofpr_argidx);
> }
>
> return 0;
> @@ -879,12 +880,26 @@ validate_probe(int out, dtrace_helper_probedesc_t *dhpb)
> return 0;
> }
>
> +static size_t
> +strings_len(const char *strtab, size_t count)
> +{
> + size_t len = 0;
> +
> + for (; count > 0; count--) {
> + size_t this_len = strlen(strtab) + 1;
> +
> + len += this_len;
> + strtab += this_len;
> + }
> + return len;
> +}
> +
> static void
> emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
> {
> int i;
> - dof_parsed_t *probe_msg;
> - size_t probe_msg_size;
> + dof_parsed_t *msg;
> + size_t msg_size;
> char *ptr;
>
> dt_dbg_dof(" Creating probe %s:%s:%s:%s\n", dhpb->dthpb_prov,
> @@ -893,35 +908,106 @@ emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
> if (validate_probe(out, dhpb) != 0)
> return;
>
> - probe_msg_size = offsetof(dof_parsed_t, probe.name) +
> - strlen(dhpb->dthpb_mod) + 1 + strlen(dhpb->dthpb_func) + 1 +
> - strlen(dhpb->dthpb_name) + 1;
> + /*
> + * Compute the size of all the optional elements first, to fill out the
> + * flags.
> + */
>
> - probe_msg = malloc(probe_msg_size);
> - if (!probe_msg) {
> - dof_error(out, ENOMEM, "Out of memory allocating probe");
> - return;
> - }
> + msg_size = offsetof(dof_parsed_t, probe.name) +
> + strlen(dhpb->dthpb_mod) + 1 +
> + strlen(dhpb->dthpb_func) + 1 +
> + strlen(dhpb->dthpb_name) + 1;
>
> - memset(probe_msg, 0, probe_msg_size);
> + msg = malloc(msg_size);
> + if (!msg)
> + goto oom;
>
> - probe_msg->size = probe_msg_size;
> - probe_msg->type = DIT_PROBE;
> - probe_msg->probe.ntp = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
> - ptr = stpcpy(probe_msg->probe.name, dhpb->dthpb_mod);
> + memset(msg, 0, msg_size);
> +
> + msg->size = msg_size;
> + msg->type = DIT_PROBE;
> + msg->probe.ntp = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
> + msg->probe.nargc = dhpb->dthpb_nargc;
> + msg->probe.xargc = dhpb->dthpb_xargc;
> +
> + ptr = stpcpy(msg->probe.name, dhpb->dthpb_mod);
> ptr++;
> ptr = stpcpy(ptr, dhpb->dthpb_func);
> ptr++;
> strcpy(ptr, dhpb->dthpb_name);
> - dof_parser_write_one(out, probe_msg, probe_msg_size);
> + dof_parser_write_one(out, msg, msg_size);
>
> - free(probe_msg);
> + free(msg);
>
> - /* XXX TODO translated args
> - pp->ftp_nargs = dhpb->dthpb_xargc;
> - pp->ftp_xtypes = dhpb->dthpb_xtypes;
> - pp->ftp_ntypes = dhpb->dthpb_ntypes;
> - */
> + /*
> + * Emit info on all native and translated args in turn.
> + *
> + * FIXME: this code is a bit repetitious.
> + *
> + * First native args (if any).
> + */
> +
> + if (dhpb->dthpb_nargc > 0) {
> + size_t nargs_size;
> +
> + nargs_size = strings_len(dhpb->dthpb_ntypes, dhpb->dthpb_nargc);
> + msg_size = offsetof(dof_parsed_t, nargs.args) + nargs_size;
> +
> + msg = malloc(msg_size);
> + if (!msg)
> + goto oom;
> +
> + memset(msg, 0, msg_size);
> +
> + msg->size = msg_size;
> + msg->type = DIT_ARGS_NATIVE;
> + memcpy(msg->nargs.args, dhpb->dthpb_ntypes, nargs_size);
> + dof_parser_write_one(out, msg, msg_size);
> +
> + free(msg);
> +
> + /* Then translated args. */
> +
> + if (dhpb->dthpb_xargc > 0) {
> + size_t xargs_size, map_size;
> +
> + xargs_size = strings_len(dhpb->dthpb_xtypes,
> + dhpb->dthpb_xargc);
> + msg_size = offsetof(dof_parsed_t, xargs.args) +
> + xargs_size;
> +
> + msg = malloc(msg_size);
> + if (!msg)
> + goto oom;
> +
> + memset(msg, 0, msg_size);
> +
> + msg->size = msg_size;
> + msg->type = DIT_ARGS_XLAT;
> + memcpy(msg->xargs.args, dhpb->dthpb_xtypes, xargs_size);
> + dof_parser_write_one(out, msg, msg_size);
> +
> + free(msg);
> +
> + /* Then the mapping table. */
> +
> + map_size = dhpb->dthpb_xargc * sizeof(int8_t);
> + msg_size = offsetof(dof_parsed_t, argmap.argmap) +
> + map_size;
> +
> + msg = malloc(msg_size);
> + if (!msg)
> + goto oom;
> +
> + memset(msg, 0, msg_size);
> +
> + msg->size = msg_size;
> + msg->type = DIT_ARGS_MAP;
> + memcpy(msg->argmap.argmap, dhpb->dthpb_args, map_size);
> + dof_parser_write_one(out, msg, msg_size);
> + free(msg);
> + }
> + }
>
> /*
> * Emit info on each tracepoint in turn.
> @@ -938,18 +1024,10 @@ emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
> for (i = 0; i < dhpb->dthpb_nenoffs; i++)
> emit_tp(out, dhpb->dthpb_base, dhpb->dthpb_enoffs[i], 1);
>
> - /*
> - * XXX later:
> - * If the arguments are shuffled around we set the argument remapping
> - * table. Later, when the probe fires, we only remap the arguments
> - * if the table is non-NULL.
> - *
> - for (i = 0; i < dhpb->dthpb_xargc; i++) {
> - if (dhpb->dthpb_args[i] != i) {
> - pp->ftp_argmap = dhpb->dthpb_args;
> - break;
> - }
> - } */
> + return;
> + oom:
> + dof_error(out, ENOMEM, "Out of memory allocating %zi bytes for probe",
> + msg_size);
> }
>
> static void
> diff --git a/libcommon/dof_parser.h b/libcommon/dof_parser.h
> index d3a6836f15fd..8f42d00551e3 100644
> --- a/libcommon/dof_parser.h
> +++ b/libcommon/dof_parser.h
> @@ -15,10 +15,19 @@
> #include <dtrace/helpers.h>
>
> /*
> - * Result of DOF probe parsing. We receive a provider info structure, followed
> - * by N probe info structures each of which is followed by possibly many
> - * tracepoint info structures, all tagged. Things not yet used for probe
> - * creation (like args, translated types, etc) are not returned.
> + * Result of DOF probe parsing. The order of elements in the parsed stream
> + * is:
> + *
> + * DIT_PROVIDER (at least 1, which contains...)
> + * DIT_PROBE (at least 1, each of which has...)
> + * DIT_ARGS_NATIVE (1, optional)
> + * DIT_ARGS_XLAT (1, optional)
> + * DIT_ARGS_MAP (1, optional)
> + * DIT_TRACEPOINT (any number >= 1)
> + *
> + * The dof_parsed.provider.flags word indicates the presence of the
> + * various optional args records in the following stream (you can rely on
> + * them if it simplifies things, but you don't have to).
> *
> * On error, a DIT_ERR structure is returned with an error message.
> */
> @@ -27,7 +36,10 @@ typedef enum dof_parsed_info {
> DIT_PROVIDER = 0,
> DIT_PROBE = 1,
> DIT_TRACEPOINT = 2,
> - DIT_ERR = 3
> + DIT_ERR = 3,
> + DIT_ARGS_NATIVE = 4,
> + DIT_ARGS_XLAT = 5,
> + DIT_ARGS_MAP = 6,
> } dof_parsed_info_t;
>
> /*
> @@ -37,7 +49,7 @@ typedef enum dof_parsed_info {
> * start which is the version of the dof_parseds within it. The data flowing
> * over the stream from the seccomped parser has no such prefix.
> */
> -#define DOF_PARSED_VERSION 1
> +#define DOF_PARSED_VERSION 2
>
> typedef struct dof_parsed {
> /*
> @@ -59,18 +71,58 @@ typedef struct dof_parsed {
> */
> char name[1];
> } provider;
> +
> struct dpi_probe_info {
> /*
> * Number of tracepoints that follow.
> */
> size_t ntp;
>
> + /*
> + * Number of native arguments that follow (if > 0, a
> + * DIT_ARGS_NATIVE will be received).
> + */
> + size_t nargc;
> +
> + /*
> + * Number of xlated arguments that follow (if > 0, a
> + * DIT_ARGS_XLAT and DIT_ARGS_MAP will be received).
> + */
> + size_t xargc;
> +
> /*
> * Probe module, function, and name (\0-separated).
> */
> char name[1];
> } probe;
>
> + /* V2+ only. */
> + struct dpi_probe_args_native_info {
> + /*
> + * Array of native args. nargc in length.
> + */
> + char args[1];
> + } nargs;
> +
> + /* V2+ only. */
> + struct dpi_probe_args_xlat_info {
> + /*
> + * Array of translated args. xargc in length.
> + */
> + char args[1];
> + } xargs;
> +
> + /*
> + * V2+ only.
> + */
> + struct dpi_probe_args_map_info {
> + /*
> + * Mapping from native arg index to xlated arg index.
> + * xargc in length.
> + */
> + int8_t argmap[1];
> + } argmap;
> +
> struct dpi_tracepoint_info {
> /*
> * Offset of this tracepoint.
> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> index 41a87955ec1b..71ac1754d343 100644
> --- a/libdtrace/dt_pid.c
> +++ b/libdtrace/dt_pid.c
> @@ -866,6 +866,9 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> uint64_t *dof_version;
> char *prv, *mod, *fun, *prb;
> dof_parsed_t *provider, *probe;
> + ssize_t nargvlen = 0, xargvlen = 0;
> + char *nargv = NULL, *xargv = NULL;
> + int8_t *argmap = NULL;
>
> /*
> * Regular files only: in particular, skip . and ..,
> @@ -917,6 +920,47 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> p += probe->size;
> seen_size += probe->size;
>
> + /*
> + * Assume the order given in dof_parser.h, for simplicity.
> + */
> + if (probe->probe.nargc > 0) {
> + dof_parsed_t *args = (dof_parsed_t *) p;
> +
> + if (!validate_dof_record(path, args, DIT_ARGS_NATIVE,
> + dof_buf_size, seen_size))
> + goto parse_err;
> +
> + nargv = args->nargs.args;
> + nargvlen = args->size - offsetof(dof_parsed_t, nargs.args);
> + assert(nargvlen >= 0);
> +
> + p += args->size;
> + seen_size += args->size;
> + }
> + if (probe->probe.xargc > 0) {
> + dof_parsed_t *args = (dof_parsed_t *) p;
> +
> + if (!validate_dof_record(path, args, DIT_ARGS_XLAT,
> + dof_buf_size, seen_size))
> + goto parse_err;
> +
> + xargv = args->xargs.args;
> + xargvlen = args->size - offsetof(dof_parsed_t, xargs.args);
> + assert(xargvlen >= 0);
> +
> + p += args->size;
> + seen_size += args->size;
> +
> + if (!validate_dof_record(path, args, DIT_ARGS_MAP,
> + dof_buf_size, seen_size))
> + goto parse_err;
> +
> + argmap = args->argmap.argmap;
> +
> + p += args->size;
> + seen_size += args->size;
> + }
> +
> /*
> * Now the parsed DOF for this probe's tracepoints.
> */
> @@ -967,6 +1011,21 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> psp.pps_off = tp->tracepoint.addr - pmp->pr_file->first_segment->pr_vaddr;
> psp.pps_nameoff = 0;
>
> + if (nargv) {
> + psp.pps_nargc = probe->probe.nargc;
> + psp.pps_nargvlen = nargvlen;
> + psp.pps_nargv = nargv;
> + }
> +
> + if (xargv) {
> + psp.pps_xargc = probe->probe.xargc;
> + psp.pps_xargvlen = xargvlen;
> + psp.pps_xargv = xargv;
> + }
> +
> + if (argmap)
> + psp.pps_argmap = argmap;
> +
> dt_dprintf("providing %s:%s:%s:%s for pid %d\n", psp.pps_prv,
> psp.pps_mod, psp.pps_fun, psp.pps_prb, psp.pps_pid);
> if (pvp->impl->provide_probe(dtp, &psp) < 0) {
> --
> 2.46.0.278.g36e3a12567
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [DTrace-devel] [PATCH v5 1/6] usdt: get arg types and xlations into DTrace from the DOF
2024-11-05 16:56 ` Kris Van Hees
@ 2024-11-05 18:01 ` Kris Van Hees
2024-11-05 19:53 ` Nick Alcock
0 siblings, 1 reply; 15+ messages in thread
From: Kris Van Hees @ 2024-11-05 18:01 UTC (permalink / raw)
To: Kris Van Hees; +Cc: Nick Alcock, dtrace, dtrace-devel
Reviewed-by withdrawn - see my comments on patch 4/6 (that one contains a diff
that belongs in this patch).
On Tue, Nov 05, 2024 at 11:56:33AM -0500, Kris Van Hees via DTrace-devel wrote:
> On Tue, Nov 05, 2024 at 12:06:03AM +0000, Nick Alcock wrote:
> > This change propagates native and xlated arg types and mapping info all
> > the way from the DOF, through the parser and dtprobed and the DOF stash,
> > into DTrace, where they end up in the pid_probespec_t for USDT probes.
> > We don't do anything with them once they get there in this commit: no
> > user-visible impacts expected.
> >
> > We bump the DOF_PARSED_VERSION since we add three new types of record to the
> > dof_parser_t, all optional, covering native and xlated args and arg mapping
> > tables; to make life easier for consumers we emit them in a defined order
> > (documented in dof_parser.h), and add arg counts to the DIT_PROBE record
> > that precedes them indicating which will be present and how many args are in
> > them. This means we retain the property that you can always tell which
> > records within a provider are coming next purely from records you already
> > have: there's no need to make decisions once the records turn up. The DOF
> > stash hardly changes: all that happens is that the parsed data written to
> > each per-probe file gains some extra types of record (it can have
> > DIT_ARGS_NATIVE, DIT_ARGS_XLAT and DIT_ARGS_MAP entries following the
> > DIT_PROBE record now).
> >
> > As usual with DOF_PARSED_VERSION bumps, DTraces that cross this change will
> > refuse to read probes added by dtprobeds that are on the other side of it.
> > (Restarting dtprobed will reparse all the probes in the stash to match the
> > new layout, and newly-started DTraces will pick that up -- so this only
> > affects running DTraces picking up new probes, which DTrace cannot do yet:
> > so no visible effects are expected.)
> >
> > The code is a bit repetitive, with nargs, xargs and arg mapping all handled
> > separately even though the code is very similar. In future maybe we could
> > unify them (but my attempts led to code that was much harder to read than
> > the original).
> >
> > Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
>
> Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
>
> > ---
> > dtprobed/dof_stash.c | 15 +++--
> > dtprobed/dtprobed.c | 10 ++-
> > include/dtrace/pid.h | 7 ++
> > libcommon/dof_parser.c | 150 +++++++++++++++++++++++++++++++----------
> > libcommon/dof_parser.h | 64 ++++++++++++++++--
> > libdtrace/dt_pid.c | 59 ++++++++++++++++
> > 6 files changed, 256 insertions(+), 49 deletions(-)
> >
> > diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
> > index 0e639076f655..8bf465678217 100644
> > --- a/dtprobed/dof_stash.c
> > +++ b/dtprobed/dof_stash.c
> > @@ -59,8 +59,10 @@
> > * startup if the dof_parsed_t structure changed. The file consists of a
> > * uint64_t version number (DOF_PARSED_VERSION), then a stream of
> > * dof_parsed_t records, the first of type DIT_PROVIDER, the second
> > - * DIT_PROBE, then as many struct dof_parsed_t's of type DIT_TRACEPOINT as
> > - * the DIT_PROBE record specifies.
> > + * DIT_PROBE, then optionally some DIT_*ARGS records (if the count of native
> > + * args in the probe is >0, you get a DIT_NATIVE_ARGS: if the count of xlated
> > + * args is >0, you get DIT_XLAT_ARGS and DIT_MAP_ARGS), then as many struct
> > + * dof_parsed_t's of type DIT_TRACEPOINT as the DIT_PROBE record specifies.
> > *
> > * /run/dtrace/probes/: Per-probe info, written by dtprobed, read by DTrace.
> > *
> > @@ -640,9 +642,14 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
> > break;
> > }
> >
> > - /* Tracepoint: add to already-open file. */
> > + /* Args info or tracepoint: add to already-open file. */
> > + case DIT_ARGS_NATIVE:
> > + case DIT_ARGS_XLAT:
> > + case DIT_ARGS_MAP:
> > case DIT_TRACEPOINT:
> > - assert(state == DIT_PROBE || state == DIT_TRACEPOINT);
> > + assert(state == DIT_PROBE || state == DIT_ARGS_NATIVE ||
> > + state == DIT_ARGS_XLAT || state == DIT_ARGS_MAP ||
> > + state == DIT_TRACEPOINT);
> > state = accump->parsed->type;
> >
> > if (write_chunk(parsed_fd, accump->parsed, accump->parsed->size) < 0)
> > diff --git a/dtprobed/dtprobed.c b/dtprobed/dtprobed.c
> > index f4c6be1e045d..2ca39b26f88a 100644
> > --- a/dtprobed/dtprobed.c
> > +++ b/dtprobed/dtprobed.c
> > @@ -791,16 +791,20 @@ process_dof(pid_t pid, int out, int in, dev_t dev, ino_t inum, dev_t exec_dev,
> > if (dof_stash_push_parsed(&accum, probe) < 0)
> > goto oom;
> >
> > - for (j = 0; j < probe->probe.ntp; j++) {
> > + j = 0;
> > + do {
> > dof_parsed_t *tp = dof_read(pid, in);
> >
> > errmsg = "no tracepoints in a probe, or parse state corrupt";
> > - if (!tp || tp->type != DIT_TRACEPOINT)
> > + if (!tp || tp->type == DIT_PROVIDER || tp->type == DIT_PROBE)
> > goto err;
> >
> > if (dof_stash_push_parsed(&accum, tp) < 0)
> > goto oom;
> > - }
> > +
> > + if (tp->type == DIT_TRACEPOINT)
> > + j++;
> > + } while (j < probe->probe.ntp);
> > }
> >
> > if (!reparsing)
> > diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
> > index 0129674adf0c..25bfacfdbfe2 100644
> > --- a/include/dtrace/pid.h
> > +++ b/include/dtrace/pid.h
> > @@ -35,6 +35,13 @@ typedef struct pid_probespec {
> > ino_t pps_inum; /* object inode */
> > char *pps_fn; /* object full filename */
> > uint64_t pps_off; /* probe offset (in object) */
> > + int pps_nargc; /* number of native args */
> > + int pps_xargc; /* number of xlated and mapped args */
> > + char *pps_nargv; /* array of native args */
> > + size_t pps_nargvlen; /* (high estimate of) length of array */
> > + char *pps_xargv; /* array of xlated args */
> > + size_t pps_xargvlen; /* (high estimate of) length of array */
> > + int8_t *pps_argmap; /* mapped arg indexes */
> >
> > /*
> > * Fields below this point do not apply to underlying probes.
> > diff --git a/libcommon/dof_parser.c b/libcommon/dof_parser.c
> > index d6631a1afdcb..1792a8bfcc84 100644
> > --- a/libcommon/dof_parser.c
> > +++ b/libcommon/dof_parser.c
> > @@ -807,10 +807,11 @@ validate_provider(int out, dof_hdr_t *dof, dof_sec_t *sec)
> > }
> >
> > dt_dbg_dof(" Probe %d %s:%s:%s:%s with %d offsets, "
> > - "%d is-enabled offsets\n", j,
> > + "%d is-enabled offsets, %i args, %i nargs, argidx %i\n", j,
> > strtab + prov->dofpv_name, "",
> > strtab + prb->dofpr_func, strtab + prb->dofpr_name,
> > - prb->dofpr_noffs, prb->dofpr_nenoffs);
> > + prb->dofpr_noffs, prb->dofpr_nenoffs,
> > + prb->dofpr_xargc, prb->dofpr_nargc, prb->dofpr_argidx);
> > }
> >
> > return 0;
> > @@ -879,12 +880,26 @@ validate_probe(int out, dtrace_helper_probedesc_t *dhpb)
> > return 0;
> > }
> >
> > +static size_t
> > +strings_len(const char *strtab, size_t count)
> > +{
> > + size_t len = 0;
> > +
> > + for (; count > 0; count--) {
> > + size_t this_len = strlen(strtab) + 1;
> > +
> > + len += this_len;
> > + strtab += this_len;
> > + }
> > + return len;
> > +}
> > +
> > static void
> > emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
> > {
> > int i;
> > - dof_parsed_t *probe_msg;
> > - size_t probe_msg_size;
> > + dof_parsed_t *msg;
> > + size_t msg_size;
> > char *ptr;
> >
> > dt_dbg_dof(" Creating probe %s:%s:%s:%s\n", dhpb->dthpb_prov,
> > @@ -893,35 +908,106 @@ emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
> > if (validate_probe(out, dhpb) != 0)
> > return;
> >
> > - probe_msg_size = offsetof(dof_parsed_t, probe.name) +
> > - strlen(dhpb->dthpb_mod) + 1 + strlen(dhpb->dthpb_func) + 1 +
> > - strlen(dhpb->dthpb_name) + 1;
> > + /*
> > + * Compute the size of all the optional elements first, to fill out the
> > + * flags.
> > + */
> >
> > - probe_msg = malloc(probe_msg_size);
> > - if (!probe_msg) {
> > - dof_error(out, ENOMEM, "Out of memory allocating probe");
> > - return;
> > - }
> > + msg_size = offsetof(dof_parsed_t, probe.name) +
> > + strlen(dhpb->dthpb_mod) + 1 +
> > + strlen(dhpb->dthpb_func) + 1 +
> > + strlen(dhpb->dthpb_name) + 1;
> >
> > - memset(probe_msg, 0, probe_msg_size);
> > + msg = malloc(msg_size);
> > + if (!msg)
> > + goto oom;
> >
> > - probe_msg->size = probe_msg_size;
> > - probe_msg->type = DIT_PROBE;
> > - probe_msg->probe.ntp = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
> > - ptr = stpcpy(probe_msg->probe.name, dhpb->dthpb_mod);
> > + memset(msg, 0, msg_size);
> > +
> > + msg->size = msg_size;
> > + msg->type = DIT_PROBE;
> > + msg->probe.ntp = dhpb->dthpb_noffs + dhpb->dthpb_nenoffs;
> > + msg->probe.nargc = dhpb->dthpb_nargc;
> > + msg->probe.xargc = dhpb->dthpb_xargc;
> > +
> > + ptr = stpcpy(msg->probe.name, dhpb->dthpb_mod);
> > ptr++;
> > ptr = stpcpy(ptr, dhpb->dthpb_func);
> > ptr++;
> > strcpy(ptr, dhpb->dthpb_name);
> > - dof_parser_write_one(out, probe_msg, probe_msg_size);
> > + dof_parser_write_one(out, msg, msg_size);
> >
> > - free(probe_msg);
> > + free(msg);
> >
> > - /* XXX TODO translated args
> > - pp->ftp_nargs = dhpb->dthpb_xargc;
> > - pp->ftp_xtypes = dhpb->dthpb_xtypes;
> > - pp->ftp_ntypes = dhpb->dthpb_ntypes;
> > - */
> > + /*
> > + * Emit info on all native and translated args in turn.
> > + *
> > + * FIXME: this code is a bit repetitious.
> > + *
> > + * First native args (if any).
> > + */
> > +
> > + if (dhpb->dthpb_nargc > 0) {
> > + size_t nargs_size;
> > +
> > + nargs_size = strings_len(dhpb->dthpb_ntypes, dhpb->dthpb_nargc);
> > + msg_size = offsetof(dof_parsed_t, nargs.args) + nargs_size;
> > +
> > + msg = malloc(msg_size);
> > + if (!msg)
> > + goto oom;
> > +
> > + memset(msg, 0, msg_size);
> > +
> > + msg->size = msg_size;
> > + msg->type = DIT_ARGS_NATIVE;
> > + memcpy(msg->nargs.args, dhpb->dthpb_ntypes, nargs_size);
> > + dof_parser_write_one(out, msg, msg_size);
> > +
> > + free(msg);
> > +
> > + /* Then translated args. */
> > +
> > + if (dhpb->dthpb_xargc > 0) {
> > + size_t xargs_size, map_size;
> > +
> > + xargs_size = strings_len(dhpb->dthpb_xtypes,
> > + dhpb->dthpb_xargc);
> > + msg_size = offsetof(dof_parsed_t, xargs.args) +
> > + xargs_size;
> > +
> > + msg = malloc(msg_size);
> > + if (!msg)
> > + goto oom;
> > +
> > + memset(msg, 0, msg_size);
> > +
> > + msg->size = msg_size;
> > + msg->type = DIT_ARGS_XLAT;
> > + memcpy(msg->xargs.args, dhpb->dthpb_xtypes, xargs_size);
> > + dof_parser_write_one(out, msg, msg_size);
> > +
> > + free(msg);
> > +
> > + /* Then the mapping table. */
> > +
> > + map_size = dhpb->dthpb_xargc * sizeof(int8_t);
> > + msg_size = offsetof(dof_parsed_t, argmap.argmap) +
> > + map_size;
> > +
> > + msg = malloc(msg_size);
> > + if (!msg)
> > + goto oom;
> > +
> > + memset(msg, 0, msg_size);
> > +
> > + msg->size = msg_size;
> > + msg->type = DIT_ARGS_MAP;
> > + memcpy(msg->argmap.argmap, dhpb->dthpb_args, map_size);
> > + dof_parser_write_one(out, msg, msg_size);
> > + free(msg);
> > + }
> > + }
> >
> > /*
> > * Emit info on each tracepoint in turn.
> > @@ -938,18 +1024,10 @@ emit_probe(int out, dtrace_helper_probedesc_t *dhpb)
> > for (i = 0; i < dhpb->dthpb_nenoffs; i++)
> > emit_tp(out, dhpb->dthpb_base, dhpb->dthpb_enoffs[i], 1);
> >
> > - /*
> > - * XXX later:
> > - * If the arguments are shuffled around we set the argument remapping
> > - * table. Later, when the probe fires, we only remap the arguments
> > - * if the table is non-NULL.
> > - *
> > - for (i = 0; i < dhpb->dthpb_xargc; i++) {
> > - if (dhpb->dthpb_args[i] != i) {
> > - pp->ftp_argmap = dhpb->dthpb_args;
> > - break;
> > - }
> > - } */
> > + return;
> > + oom:
> > + dof_error(out, ENOMEM, "Out of memory allocating %zi bytes for probe",
> > + msg_size);
> > }
> >
> > static void
> > diff --git a/libcommon/dof_parser.h b/libcommon/dof_parser.h
> > index d3a6836f15fd..8f42d00551e3 100644
> > --- a/libcommon/dof_parser.h
> > +++ b/libcommon/dof_parser.h
> > @@ -15,10 +15,19 @@
> > #include <dtrace/helpers.h>
> >
> > /*
> > - * Result of DOF probe parsing. We receive a provider info structure, followed
> > - * by N probe info structures each of which is followed by possibly many
> > - * tracepoint info structures, all tagged. Things not yet used for probe
> > - * creation (like args, translated types, etc) are not returned.
> > + * Result of DOF probe parsing. The order of elements in the parsed stream
> > + * is:
> > + *
> > + * DIT_PROVIDER (at least 1, which contains...)
> > + * DIT_PROBE (at least 1, each of which has...)
> > + * DIT_ARGS_NATIVE (1, optional)
> > + * DIT_ARGS_XLAT (1, optional)
> > + * DIT_ARGS_MAP (1, optional)
> > + * DIT_TRACEPOINT (any number >= 1)
> > + *
> > + * The dof_parsed.provider.flags word indicates the presence of the
> > + * various optional args records in the following stream (you can rely on
> > + * them if it simplifies things, but you don't have to).
> > *
> > * On error, a DIT_ERR structure is returned with an error message.
> > */
> > @@ -27,7 +36,10 @@ typedef enum dof_parsed_info {
> > DIT_PROVIDER = 0,
> > DIT_PROBE = 1,
> > DIT_TRACEPOINT = 2,
> > - DIT_ERR = 3
> > + DIT_ERR = 3,
> > + DIT_ARGS_NATIVE = 4,
> > + DIT_ARGS_XLAT = 5,
> > + DIT_ARGS_MAP = 6,
> > } dof_parsed_info_t;
> >
> > /*
> > @@ -37,7 +49,7 @@ typedef enum dof_parsed_info {
> > * start which is the version of the dof_parseds within it. The data flowing
> > * over the stream from the seccomped parser has no such prefix.
> > */
> > -#define DOF_PARSED_VERSION 1
> > +#define DOF_PARSED_VERSION 2
> >
> > typedef struct dof_parsed {
> > /*
> > @@ -59,18 +71,58 @@ typedef struct dof_parsed {
> > */
> > char name[1];
> > } provider;
> > +
> > struct dpi_probe_info {
> > /*
> > * Number of tracepoints that follow.
> > */
> > size_t ntp;
> >
> > + /*
> > + * Number of native arguments that follow (if > 0, a
> > + * DIT_ARGS_NATIVE will be received).
> > + */
> > + size_t nargc;
> > +
> > + /*
> > + * Number of xlated arguments that follow (if > 0, a
> > + * DIT_ARGS_XLAT and DIT_ARGS_MAP will be received).
> > + */
> > + size_t xargc;
> > +
> > /*
> > * Probe module, function, and name (\0-separated).
> > */
> > char name[1];
> > } probe;
> >
> > + /* V2+ only. */
> > + struct dpi_probe_args_native_info {
> > + /*
> > + * Array of native args. nargc in length.
> > + */
> > + char args[1];
> > + } nargs;
> > +
> > + /* V2+ only. */
> > + struct dpi_probe_args_xlat_info {
> > + /*
> > + * Array of translated args. xargc in length.
> > + */
> > + char args[1];
> > + } xargs;
> > +
> > + /*
> > + * V2+ only.
> > + */
> > + struct dpi_probe_args_map_info {
> > + /*
> > + * Mapping from native arg index to xlated arg index.
> > + * xargc in length.
> > + */
> > + int8_t argmap[1];
> > + } argmap;
> > +
> > struct dpi_tracepoint_info {
> > /*
> > * Offset of this tracepoint.
> > diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> > index 41a87955ec1b..71ac1754d343 100644
> > --- a/libdtrace/dt_pid.c
> > +++ b/libdtrace/dt_pid.c
> > @@ -866,6 +866,9 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> > uint64_t *dof_version;
> > char *prv, *mod, *fun, *prb;
> > dof_parsed_t *provider, *probe;
> > + ssize_t nargvlen = 0, xargvlen = 0;
> > + char *nargv = NULL, *xargv = NULL;
> > + int8_t *argmap = NULL;
> >
> > /*
> > * Regular files only: in particular, skip . and ..,
> > @@ -917,6 +920,47 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> > p += probe->size;
> > seen_size += probe->size;
> >
> > + /*
> > + * Assume the order given in dof_parser.h, for simplicity.
> > + */
> > + if (probe->probe.nargc > 0) {
> > + dof_parsed_t *args = (dof_parsed_t *) p;
> > +
> > + if (!validate_dof_record(path, args, DIT_ARGS_NATIVE,
> > + dof_buf_size, seen_size))
> > + goto parse_err;
> > +
> > + nargv = args->nargs.args;
> > + nargvlen = args->size - offsetof(dof_parsed_t, nargs.args);
> > + assert(nargvlen >= 0);
> > +
> > + p += args->size;
> > + seen_size += args->size;
> > + }
> > + if (probe->probe.xargc > 0) {
> > + dof_parsed_t *args = (dof_parsed_t *) p;
> > +
> > + if (!validate_dof_record(path, args, DIT_ARGS_XLAT,
> > + dof_buf_size, seen_size))
> > + goto parse_err;
> > +
> > + xargv = args->xargs.args;
> > + xargvlen = args->size - offsetof(dof_parsed_t, xargs.args);
> > + assert(xargvlen >= 0);
> > +
> > + p += args->size;
> > + seen_size += args->size;
> > +
> > + if (!validate_dof_record(path, args, DIT_ARGS_MAP,
> > + dof_buf_size, seen_size))
> > + goto parse_err;
> > +
> > + argmap = args->argmap.argmap;
> > +
> > + p += args->size;
> > + seen_size += args->size;
> > + }
> > +
> > /*
> > * Now the parsed DOF for this probe's tracepoints.
> > */
> > @@ -967,6 +1011,21 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> > psp.pps_off = tp->tracepoint.addr - pmp->pr_file->first_segment->pr_vaddr;
> > psp.pps_nameoff = 0;
> >
> > + if (nargv) {
> > + psp.pps_nargc = probe->probe.nargc;
> > + psp.pps_nargvlen = nargvlen;
> > + psp.pps_nargv = nargv;
> > + }
> > +
> > + if (xargv) {
> > + psp.pps_xargc = probe->probe.xargc;
> > + psp.pps_xargvlen = xargvlen;
> > + psp.pps_xargv = xargv;
> > + }
> > +
> > + if (argmap)
> > + psp.pps_argmap = argmap;
> > +
> > dt_dprintf("providing %s:%s:%s:%s for pid %d\n", psp.pps_prv,
> > psp.pps_mod, psp.pps_fun, psp.pps_prb, psp.pps_pid);
> > if (pvp->impl->provide_probe(dtp, &psp) < 0) {
> > --
> > 2.46.0.278.g36e3a12567
> >
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 2/6] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c
2024-11-05 0:06 [PATCH v5 0/6] usdt typed args, translators and arg mapping Nick Alcock
2024-11-05 0:06 ` [PATCH v5 1/6] usdt: get arg types and xlations into DTrace from the DOF Nick Alcock
@ 2024-11-05 0:06 ` Nick Alcock
2024-11-05 0:06 ` [PATCH v5 3/6] cg: add argument mapping in the trampoline Nick Alcock
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Nick Alcock @ 2024-11-05 0:06 UTC (permalink / raw)
To: dtrace, dtrace-devel; +Cc: Kris Van Hees
We already validate that they don't exist in dof_parser.c, so no such probes
can ever get here. Handling this impossible, untestable case has just got
harder, because we'd have to skip DIT_ARGS_{NATIVE,XLAT,MAP} records too.
Just stop considering it.
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
---
dtprobed/dof_stash.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
index 8bf465678217..82fdd3174759 100644
--- a/dtprobed/dof_stash.c
+++ b/dtprobed/dof_stash.c
@@ -586,12 +586,6 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
if (err != 0)
goto err_provider;
- /*
- * Skip probes with zero tracepoints entirely.
- */
- if (accump->parsed->probe.ntp == 0)
- break;
-
mod = accump->parsed->probe.name;
assert(accump->parsed->size > (mod - (char *) accump->parsed));
fun = mod + strlen(mod) + 1;
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v5 3/6] cg: add argument mapping in the trampoline
2024-11-05 0:06 [PATCH v5 0/6] usdt typed args, translators and arg mapping Nick Alcock
2024-11-05 0:06 ` [PATCH v5 1/6] usdt: get arg types and xlations into DTrace from the DOF Nick Alcock
2024-11-05 0:06 ` [PATCH v5 2/6] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c Nick Alcock
@ 2024-11-05 0:06 ` Nick Alcock
2024-11-05 17:29 ` Kris Van Hees
2024-11-05 0:06 ` [PATCH v5 4/6] usdt: typed args and arg mapping Nick Alcock
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Nick Alcock @ 2024-11-05 0:06 UTC (permalink / raw)
To: dtrace, dtrace-devel
Before now, argument mapping was restricted to the args[] array, and was
implemented in dt_cg_array_op in the same place where we decide whether
to automatically copyin() userspace strings.
But existing DTrace testcases suggest that arg mapping is also applied to
argN for USDT, even though this is inconsistent with SDT and arguably less
flexible than having argN be the unmapped arguments in all cases. Add a new
function dt_cg_tramp_map_args(), which can be called from trampolines to
apply mappings (destructively, but it keeps a copy of the old args in the
save area so you can unapply them if you need to). No existing providers do
any mapping, so this is not yet called, but it's about to be.
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
libdtrace/dt_cg.c | 26 ++++++++++++++++++++++++++
libdtrace/dt_cg.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 39c27ab0ec0b..464ff781d008 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -831,6 +831,30 @@ dt_cg_tramp_restore_args(dt_pcb_t *pcb)
}
}
+/*
+ * Move arguments according to the mappings in the array of arguments given.
+ * The original arguments are obtained from the saved regs, and overwritten, so
+ * the regs must be saved in advance of this call. In effect, this changes the
+ * native arguments: the caller should adjust the mappings array accordingly,
+ * and shuffle the native arg types to match.
+ *
+ * The caller must ensure that %r7 contains the value set by the
+ * dt_cg_tramp_prologue*() functions.
+ */
+void
+dt_cg_tramp_map_args(dt_pcb_t *pcb, dt_argdesc_t *args, size_t nargs)
+{
+ dt_irlist_t *dlp = &pcb->pcb_ir;
+ int i;
+
+ for (i = 0; i < nargs; i++) {
+ if (args[i].mapping != i) {
+ emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(args[i].mapping)));
+ emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
+ }
+ }
+}
+
typedef struct {
dt_irlist_t *dlp;
dt_activity_t act;
@@ -5061,6 +5085,8 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
* unless the argument reference is provided by a dynamic translator.
* If we're using a dynamic translator for args[], then just set dn_reg
* to an invalid reg and return: DIF_OP_XLARG will fetch the arg later.
+ *
+ * If this is a userland variable, note that we need to copy it in.
*/
if (idp->di_id == DIF_VAR_ARGS) {
if ((idp->di_kind == DT_IDENT_XLPTR ||
diff --git a/libdtrace/dt_cg.h b/libdtrace/dt_cg.h
index 0a7c7ba6a8c5..fb26c125adbc 100644
--- a/libdtrace/dt_cg.h
+++ b/libdtrace/dt_cg.h
@@ -34,6 +34,7 @@ extern void dt_cg_tramp_get_var(dt_pcb_t *pcb, const char *name, int isstore,
extern void dt_cg_tramp_del_var(dt_pcb_t *pcb, const char *name);
extern void dt_cg_tramp_save_args(dt_pcb_t *pcb);
extern void dt_cg_tramp_restore_args(dt_pcb_t *pcb);
+extern void dt_cg_tramp_map_args(dt_pcb_t *pcb, dt_argdesc_t *args, size_t nargs);
extern void dt_cg_tramp_call_clauses(dt_pcb_t *pcb, const dt_probe_t *prp,
dt_activity_t act);
extern void dt_cg_tramp_return(dt_pcb_t *pcb);
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v5 3/6] cg: add argument mapping in the trampoline
2024-11-05 0:06 ` [PATCH v5 3/6] cg: add argument mapping in the trampoline Nick Alcock
@ 2024-11-05 17:29 ` Kris Van Hees
2024-11-05 19:58 ` Nick Alcock
0 siblings, 1 reply; 15+ messages in thread
From: Kris Van Hees @ 2024-11-05 17:29 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
On Tue, Nov 05, 2024 at 12:06:05AM +0000, Nick Alcock wrote:
> Before now, argument mapping was restricted to the args[] array, and was
> implemented in dt_cg_array_op in the same place where we decide whether
> to automatically copyin() userspace strings.
>
> But existing DTrace testcases suggest that arg mapping is also applied to
> argN for USDT, even though this is inconsistent with SDT and arguably less
> flexible than having argN be the unmapped arguments in all cases. Add a new
> function dt_cg_tramp_map_args(), which can be called from trampolines to
> apply mappings (destructively, but it keeps a copy of the old args in the
> save area so you can unapply them if you need to). No existing providers do
> any mapping, so this is not yet called, but it's about to be.
>
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
> libdtrace/dt_cg.c | 26 ++++++++++++++++++++++++++
> libdtrace/dt_cg.h | 1 +
> 2 files changed, 27 insertions(+)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 39c27ab0ec0b..464ff781d008 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -831,6 +831,30 @@ dt_cg_tramp_restore_args(dt_pcb_t *pcb)
> }
> }
>
> +/*
> + * Move arguments according to the mappings in the array of arguments given.
> + * The original arguments are obtained from the saved regs, and overwritten, so
> + * the regs must be saved in advance of this call. In effect, this changes the
> + * native arguments: the caller should adjust the mappings array accordingly,
> + * and shuffle the native arg types to match.
This is confusing... How about;
"Populate the probe arguments based on the provided dt_argdesc_t array.
The caller must save the arguments because argument mapping copies values from
the saved arguments to the current arguments. After this function returns,
the caller should adjust the mapping to reflect that shuffling has been done."
There is no need to talk about shuffling types because the dt_argdesc_t array
already lists the correct types anyway.
> + *
> + * The caller must ensure that %r7 contains the value set by the
> + * dt_cg_tramp_prologue*() functions.
> + */
> +void
> +dt_cg_tramp_map_args(dt_pcb_t *pcb, dt_argdesc_t *args, size_t nargs)
> +{
> + dt_irlist_t *dlp = &pcb->pcb_ir;
> + int i;
> +
> + for (i = 0; i < nargs; i++) {
> + if (args[i].mapping != i) {
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(args[i].mapping)));
> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> + }
> + }
> +}
> +
> typedef struct {
> dt_irlist_t *dlp;
> dt_activity_t act;
> @@ -5061,6 +5085,8 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> * unless the argument reference is provided by a dynamic translator.
> * If we're using a dynamic translator for args[], then just set dn_reg
> * to an invalid reg and return: DIF_OP_XLARG will fetch the arg later.
> + *
> + * If this is a userland variable, note that we need to copy it in.
> */
> if (idp->di_id == DIF_VAR_ARGS) {
> if ((idp->di_kind == DT_IDENT_XLPTR ||
> diff --git a/libdtrace/dt_cg.h b/libdtrace/dt_cg.h
> index 0a7c7ba6a8c5..fb26c125adbc 100644
> --- a/libdtrace/dt_cg.h
> +++ b/libdtrace/dt_cg.h
> @@ -34,6 +34,7 @@ extern void dt_cg_tramp_get_var(dt_pcb_t *pcb, const char *name, int isstore,
> extern void dt_cg_tramp_del_var(dt_pcb_t *pcb, const char *name);
> extern void dt_cg_tramp_save_args(dt_pcb_t *pcb);
> extern void dt_cg_tramp_restore_args(dt_pcb_t *pcb);
> +extern void dt_cg_tramp_map_args(dt_pcb_t *pcb, dt_argdesc_t *args, size_t nargs);
> extern void dt_cg_tramp_call_clauses(dt_pcb_t *pcb, const dt_probe_t *prp,
> dt_activity_t act);
> extern void dt_cg_tramp_return(dt_pcb_t *pcb);
> --
> 2.46.0.278.g36e3a12567
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v5 3/6] cg: add argument mapping in the trampoline
2024-11-05 17:29 ` Kris Van Hees
@ 2024-11-05 19:58 ` Nick Alcock
0 siblings, 0 replies; 15+ messages in thread
From: Nick Alcock @ 2024-11-05 19:58 UTC (permalink / raw)
To: Kris Van Hees; +Cc: dtrace, dtrace-devel
On 5 Nov 2024, Kris Van Hees told this:
> On Tue, Nov 05, 2024 at 12:06:05AM +0000, Nick Alcock wrote:
>> +/*
>> + * Move arguments according to the mappings in the array of arguments given.
>> + * The original arguments are obtained from the saved regs, and overwritten, so
>> + * the regs must be saved in advance of this call. In effect, this changes the
>> + * native arguments: the caller should adjust the mappings array accordingly,
>> + * and shuffle the native arg types to match.
>
> This is confusing... How about;
>
> "Populate the probe arguments based on the provided dt_argdesc_t array.
> The caller must save the arguments because argument mapping copies values from
> the saved arguments to the current arguments. After this function returns,
> the caller should adjust the mapping to reflect that shuffling has been done."
>
> There is no need to talk about shuffling types because the dt_argdesc_t array
> already lists the correct types anyway.
Ah yeah, historical wart -- when that comment was written, it didn't.
Adjusted as you suggest.
--
NULL && (void)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 4/6] usdt: typed args and arg mapping
2024-11-05 0:06 [PATCH v5 0/6] usdt typed args, translators and arg mapping Nick Alcock
` (2 preceding siblings ...)
2024-11-05 0:06 ` [PATCH v5 3/6] cg: add argument mapping in the trampoline Nick Alcock
@ 2024-11-05 0:06 ` Nick Alcock
2024-11-05 18:53 ` Kris Van Hees
2024-11-05 0:06 ` [PATCH v5 5/6] usdt: new tests for USDT arg sanity with overlapping pid probes Nick Alcock
2024-11-05 0:06 ` [PATCH v5 6/6] usdt: fix create_underlying error path Nick Alcock
5 siblings, 1 reply; 15+ messages in thread
From: Nick Alcock @ 2024-11-05 0:06 UTC (permalink / raw)
To: dtrace, dtrace-devel
This change propagates the arg type and mapping info we just got into the
pid_probespec_t onward into the underlying dt_uprobe_t, shuffles the native
types before handing off a 1:1 mapping to the dt_probe_t, and calls
dt_cg_tramp_map_args() to apply the requested mappings. With this, USDT
probes should now have visible types which obey the :-notation translators
specified in their .d fils (this is now tested).
The probe info has been moved from the underlying probe to the overlying one
as part of this: underlying probes don't have any user-visible args to
get info for, and their probe_info functions are never called. We do all
the arg reshuffling in a function ultimately called from USDT probe
discovery via provide_underlying, so it predates trampoline setup let alone
probe_info on the overlying probe and all the args info is present before
it is needed. (We intentionally do not shuffle the args in
dt_cg_tramp_map_args specifically so that we are not sensitive to the
relative call order of trampoline() versus probe_info(), but there's no
escaping the requirement for USDT probe discovery to predate all of this:
but this was already a requirement in any case.)
We make one assumption for simplicity, noted by a TODO in the source.
We assume that overlying probes all have the same native types and mappings,
so we don't need to look at more than one underlying probe to figure out what
they are. DTrace always generates such a thing, but DOF specifying multiple
overlapping probes seems perfectly possible for other programs to generate.
This should probably be validated in dof_parser.c in future.
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
include/dtrace/pid.h | 1 +
libdtrace/dt_pid.c | 3 +-
libdtrace/dt_prov_uprobe.c | 185 ++++++++++++++++--
test/triggers/usdt-tst-argmap-prov.d | 5 +-
test/triggers/usdt-tst-argmap.c | 5 +-
.../dtrace-util/tst.ListProbesArgsUSDT.r | 34 ++++
.../dtrace-util/tst.ListProbesArgsUSDT.r.p | 2 +
.../dtrace-util/tst.ListProbesArgsUSDT.sh | 83 ++++++++
test/unittest/usdt/err.argmap-null.d | 41 ++++
test/unittest/usdt/err.argmap-null.r | 2 +
test/unittest/usdt/err.argmap-null.r.p | 2 +
test/unittest/usdt/tst.argmap-null.d | 32 +++
test/unittest/usdt/tst.argmap-typed-partial.d | 49 +++++
test/unittest/usdt/tst.argmap-typed.d | 48 +++++
test/unittest/usdt/tst.argmap.d | 5 +-
15 files changed, 478 insertions(+), 19 deletions(-)
create mode 100644 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
create mode 100755 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
create mode 100755 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
create mode 100644 test/unittest/usdt/err.argmap-null.d
create mode 100644 test/unittest/usdt/err.argmap-null.r
create mode 100755 test/unittest/usdt/err.argmap-null.r.p
create mode 100644 test/unittest/usdt/tst.argmap-null.d
create mode 100644 test/unittest/usdt/tst.argmap-typed-partial.d
create mode 100644 test/unittest/usdt/tst.argmap-typed.d
diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
index 25bfacfdbfe2..c53e600475df 100644
--- a/include/dtrace/pid.h
+++ b/include/dtrace/pid.h
@@ -22,6 +22,7 @@ typedef enum pid_probetype {
DTPPT_ENTRY,
DTPPT_RETURN,
DTPPT_OFFSETS,
+ DTPPT_USDT,
DTPPT_IS_ENABLED
} pid_probetype_t;
diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
index 71ac1754d343..a0b3d892fd23 100644
--- a/libdtrace/dt_pid.c
+++ b/libdtrace/dt_pid.c
@@ -950,6 +950,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
p += args->size;
seen_size += args->size;
+ args = (dof_parsed_t *) p;
if (!validate_dof_record(path, args, DIT_ARGS_MAP,
dof_buf_size, seen_size))
@@ -1000,7 +1001,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
goto oom;
}
- psp.pps_type = tp->tracepoint.is_enabled ? DTPPT_IS_ENABLED : DTPPT_OFFSETS;
+ psp.pps_type = tp->tracepoint.is_enabled ? DTPPT_IS_ENABLED : DTPPT_USDT;
psp.pps_prv = prv;
psp.pps_mod = mod;
psp.pps_fun = fun;
diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index a8c1ab2761f0..28f5351be21a 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -46,9 +46,11 @@
/* Provider name for the underlying probes. */
static const char prvname[] = "uprobe";
-#define PP_IS_RETURN 1
-#define PP_IS_FUNCALL 2
-#define PP_IS_ENABLED 4
+#define PP_IS_RETURN 0x1
+#define PP_IS_FUNCALL 0x2
+#define PP_IS_ENABLED 0x4
+#define PP_IS_USDT 0x8
+#define PP_IS_MAPPED 0x10
typedef struct dt_uprobe {
dev_t dev;
@@ -57,7 +59,10 @@ typedef struct dt_uprobe {
uint64_t off;
int flags;
tp_probe_t *tp;
- dt_list_t probes; /* pid/USDT probes triggered by it */
+ int argc; /* number of args */
+ dt_argdesc_t *args; /* args array (points into argvbuf) */
+ char *argvbuf; /* arg strtab */
+ dt_list_t probes; /* pid/USDT probes triggered by it */
} dt_uprobe_t;
typedef struct list_probe {
@@ -123,6 +128,8 @@ static void probe_destroy_underlying(dtrace_hdl_t *dtp, void *datap)
dt_tp_destroy(dtp, tpp);
free_probe_list(dtp, dt_list_next(&upp->probes));
dt_free(dtp, upp->fn);
+ dt_free(dtp, upp->args);
+ dt_free(dtp, upp->argvbuf);
dt_free(dtp, upp);
}
@@ -516,6 +523,83 @@ static int discover(dtrace_hdl_t *dtp)
return 0;
}
+/*
+ * Populate args for an underlying probe. This really relates to an overlying
+ * USDT probe (all overlying probes associated with a given underlying probe
+ * must have the same args), but the overlying probe doesn't exist at the point
+ * this is populated, so we must put it in the underlying probe instead and pull
+ * it out when the overlying probe info is called for.
+ *
+ * Move it into dt_argdesc_t's for use later on. The char *'s in that structure
+ * are pointers into the argvbuf array, which is a straight concatenated copy of
+ * the nargc/xargc in the pid_probespec_t.
+ */
+static int populate_args(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
+ dt_uprobe_t *upp)
+{
+ char **nargv = NULL;
+ char *nptr = NULL, *xptr = NULL;
+ size_t i;
+
+ upp->argc = psp->pps_xargc;
+
+ /*
+ * If we have a nonzero number of args, we always have at least one narg
+ * and at least one xarg. Double-check to be sure. (These are not
+ * populated, and thus left 0/NULL, for non-USDT probes.)
+ */
+ if (upp->argc == 0 || psp->pps_xargv == NULL || psp->pps_nargv == NULL
+ || psp->pps_xargvlen == 0 || psp->pps_nargvlen == 0)
+ return 0;
+
+ upp->argvbuf = dt_alloc(dtp, psp->pps_xargvlen + psp->pps_nargvlen);
+ if(upp->argvbuf == NULL)
+ return -1;
+ memcpy(upp->argvbuf, psp->pps_xargv, psp->pps_xargvlen);
+ xptr = upp->argvbuf;
+
+ memcpy(upp->argvbuf + psp->pps_xargvlen, psp->pps_nargv, psp->pps_nargvlen);
+ nptr = upp->argvbuf + psp->pps_xargvlen;
+
+ upp->args = dt_calloc(dtp, upp->argc, sizeof(dt_argdesc_t));
+ if (upp->args == NULL)
+ return -1;
+
+ /*
+ * Construct an array to allow accessing native args by index.
+ */
+
+ if ((nargv = dt_calloc(dtp, psp->pps_nargc, sizeof (char *))) == NULL)
+ goto fail;
+
+ for (i = 0; i < psp->pps_nargc; i++, nptr += strlen(nptr) + 1)
+ nargv[i] = nptr;
+
+ /*
+ * Fill up the upp->args array based on xargs. If this indicates that
+ * mapping is needed, note as much.
+ */
+
+ for (i = 0; i < upp->argc; i++, xptr += strlen(xptr) + 1) {
+ int map_arg = psp->pps_argmap[i];
+
+ upp->args[i].native = nargv[map_arg];
+ upp->args[i].xlate = xptr;
+ upp->args[i].mapping = map_arg;
+ upp->args[i].flags = 0;
+
+ if (i != map_arg)
+ upp->flags |= PP_IS_MAPPED;
+ }
+
+ free(nargv);
+ return 0;
+
+ fail:
+ free(nargv);
+ return -1;
+}
+
/*
* Look up or create an underlying (real) probe, corresponding directly to a
* uprobe. Since multiple pid and USDT probes may all map onto the same
@@ -530,7 +614,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
char prb[DTRACE_NAMELEN];
dtrace_probedesc_t pd;
dt_probe_t *uprp;
- dt_uprobe_t *upp;
+ dt_uprobe_t *upp = NULL;
/*
* The underlying probes (uprobes) represent the tracepoints that pid
@@ -555,6 +639,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
case DTPPT_IS_ENABLED:
case DTPPT_ENTRY:
case DTPPT_OFFSETS:
+ case DTPPT_USDT:
snprintf(prb, sizeof(prb), "%lx", psp->pps_off);
break;
default:
@@ -568,6 +653,8 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
pd.fun = psp->pps_fun;
pd.prb = prb;
+ dt_dprintf("Providing underlying probe %s:%s:%s:%s @ %lx\n", psp->pps_prv,
+ psp->pps_mod, psp->pps_fn, psp->pps_prb, psp->pps_off);
uprp = dt_probe_lookup(dtp, &pd);
if (uprp == NULL) {
dt_provider_t *pvp;
@@ -591,12 +678,24 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
goto fail;
uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
- upp);
+ upp);
if (uprp == NULL)
goto fail;
} else
upp = uprp->prv_data;
+ /*
+ * Only one USDT probe can correspond to each underlying probe.
+ */
+ if (psp->pps_type == DTPPT_USDT && upp->flags == PP_IS_USDT) {
+ dt_dprintf("Found overlapping USDT probe at %lx/%lx/%lx/%s\n",
+ upp->dev, upp->inum, upp->off, upp->fn);
+ goto fail;
+ }
+
+ if (populate_args(dtp, psp, upp) < 0)
+ goto fail;
+
switch (psp->pps_type) {
case DTPPT_RETURN:
upp->flags |= PP_IS_RETURN;
@@ -604,15 +703,19 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
case DTPPT_IS_ENABLED:
upp->flags |= PP_IS_ENABLED;
break;
+ case DTPPT_USDT:
+ upp->flags |= PP_IS_USDT;
+ break;
default: ;
/*
* No flags needed for other types.
*/
}
-
return uprp;
fail:
+ dt_dprintf("Failed to instantiate %s:%s:%s:%s\n", psp->pps_prv,
+ psp->pps_mod, psp->pps_fn, psp->pps_prb);
probe_destroy(dtp, upp);
return NULL;
}
@@ -732,7 +835,7 @@ static int provide_pid_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp)
static int provide_usdt_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp)
{
- if (psp->pps_type != DTPPT_OFFSETS &&
+ if (psp->pps_type != DTPPT_USDT &&
psp->pps_type != DTPPT_IS_ENABLED) {
dt_dprintf("pid: unknown USDT probe type %i\n", psp->pps_type);
return -1;
@@ -786,7 +889,12 @@ static void enable_usdt(dtrace_hdl_t *dtp, dt_probe_t *prp)
* int dt_uprobe(dt_pt_regs *regs)
*
* The trampoline will first populate a dt_dctx_t struct. It will then emulate
- * the firing of all dependent pid* probes and their clauses.
+ * the firing of all dependent pid* and USDT probes and their clauses, or (in
+ * the case of is-enabled probes), do the necessary copying (is-enabled probes
+ * have no associated clauses and their behaviour is hardwired).
+ *
+ * Trampolines associated with USDT probes will also arrange for argument
+ * shuffling if the argmap array calls for it.
*/
static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
{
@@ -864,7 +972,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
}
/*
- * USDT
+ * USDT.
*/
/* In some cases, we know there are no USDT probes. */ // FIXME: add more checks
@@ -873,6 +981,16 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
dt_cg_tramp_copy_args_from_regs(pcb, 0);
+ /*
+ * Apply arg mappings, if needed.
+ */
+ if (upp->flags & PP_IS_MAPPED) {
+
+ /* dt_cg_tramp_map_args() works from the saved args. */
+ dt_cg_tramp_save_args(pcb);
+ dt_cg_tramp_map_args(pcb, upp->args, upp->argc);
+ }
+
/*
* Retrieve the PID of the process that caused the probe to fire.
*/
@@ -1083,8 +1201,49 @@ attach_bpf:
static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
int *argcp, dt_argdesc_t **argvp)
{
- *argcp = 0; /* no known arguments */
- *argvp = NULL;
+ size_t i;
+ list_probe_t *pup = prp->prv_data;
+ dt_uprobe_t *upp;
+ size_t argc = 0;
+ dt_argdesc_t *argv = NULL;
+
+ /* No underlying probes? No args. */
+ if (!pup)
+ goto done;
+
+ upp = pup->probe->prv_data;
+ if (!upp || upp->args == NULL)
+ goto done;
+
+ argc = upp->argc;
+
+ argv = dt_calloc(dtp, argc, sizeof(dt_argdesc_t));
+ if (argv == NULL)
+ return dt_set_errno(dtp, EDT_NOMEM);
+
+ for (i = 0; i < argc; i++) {
+ argv[i].native = strdup(upp->args[i].native);
+ if (upp->args[i].xlate)
+ argv[i].xlate = strdup(upp->args[i].xlate);
+ argv[i].mapping = i;
+
+ if (argv[i].native == NULL ||
+ (upp->args[i].xlate != NULL && argv[i].xlate == NULL)) {
+ size_t j;
+
+ for (j = 0; j <= i; j++) {
+ free((char *) argv[i].native);
+ free((char *) argv[i].xlate);
+ }
+
+ dt_free(dtp, argv);
+ return dt_set_errno(dtp, EDT_NOMEM);
+ }
+ }
+
+done:
+ *argcp = argc;
+ *argvp = argv;
return 0;
}
@@ -1152,7 +1311,6 @@ dt_provimpl_t dt_uprobe = {
.load_prog = &dt_bpf_prog_load,
.trampoline = &trampoline,
.attach = &attach,
- .probe_info = &probe_info,
.detach = &detach,
.probe_destroy = &probe_destroy_underlying,
.add_probe = &add_probe_uprobe,
@@ -1178,6 +1336,7 @@ dt_provimpl_t dt_usdt = {
.populate = &populate_usdt,
.provide_probe = &provide_usdt_probe,
.enable = &enable_usdt,
+ .probe_info = &probe_info,
.probe_destroy = &probe_destroy,
.discover = &discover,
.add_probe = &add_probe_usdt,
diff --git a/test/triggers/usdt-tst-argmap-prov.d b/test/triggers/usdt-tst-argmap-prov.d
index 28134baa6fa3..d8c3e88c4616 100644
--- a/test/triggers/usdt-tst-argmap-prov.d
+++ b/test/triggers/usdt-tst-argmap-prov.d
@@ -1,10 +1,13 @@
/*
* Oracle Linux DTrace.
- * Copyright (c) 2006, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2006, 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.
*/
provider test_prov {
probe place(int i, int j) : (int j, int i, int i, int j);
+ probe place2(int i, char *j) : (char *j, int i, int i, char *j);
+ probe place3(int i, char *j) : (char *j);
+ probe place4(int i, char *j) : ();
};
diff --git a/test/triggers/usdt-tst-argmap.c b/test/triggers/usdt-tst-argmap.c
index 89a0a53fc1d5..9244092514ff 100644
--- a/test/triggers/usdt-tst-argmap.c
+++ b/test/triggers/usdt-tst-argmap.c
@@ -1,6 +1,6 @@
/*
* Oracle Linux DTrace.
- * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2006, 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.
*/
@@ -12,6 +12,9 @@ main(int argc, char **argv)
{
for (;;) {
DTRACE_PROBE2(test_prov, place, 10, 4);
+ DTRACE_PROBE(test_prov, place2, 255, "foo");
+ DTRACE_PROBE(test_prov, place3, 126, "bar");
+ DTRACE_PROBE(test_prov, place4, 204, "baz");
}
return 0;
diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
new file mode 100644
index 000000000000..8bcfa92ff6b0
--- /dev/null
+++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
@@ -0,0 +1,34 @@
+ ID PROVIDER MODULE FUNCTION NAME
+ XX test_provXXXX test main go
+
+ Probe Description Attributes
+ Identifier Names: Private
+ Data Semantics: Private
+ Dependency Class: Unknown
+
+ Argument Attributes
+ Identifier Names: Private
+ Data Semantics: Private
+ Dependency Class: Unknown
+
+ Argument Types
+ args[0]: char *
+ args[1]: int
+
+ ID PROVIDER MODULE FUNCTION NAME
+ XX test_provXXXX test main go
+
+ Probe Description Attributes
+ Identifier Names: Private
+ Data Semantics: Private
+ Dependency Class: Unknown
+
+ Argument Attributes
+ Identifier Names: Private
+ Data Semantics: Private
+ Dependency Class: Unknown
+
+ Argument Types
+ args[0]: char *
+ args[1]: int
+
diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
new file mode 100755
index 000000000000..c575983adf65
--- /dev/null
+++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
@@ -0,0 +1,2 @@
+#!/bin/sed -rf
+s,test_prov[0-9]*,test_provXXXX,g; s,^ *[0-9]+, XX,g;
diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
new file mode 100755
index 000000000000..ad8dc2e7c267
--- /dev/null
+++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
@@ -0,0 +1,83 @@
+#!/bin/bash
+#
+# Oracle Linux DTrace.
+# Copyright (c) 2013, 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.
+
+##
+#
+# ASSERTION:
+# Testing -lvn option with USDT probes with a valid probe name.
+#
+# SECTION: dtrace Utility/-ln Option
+#
+##
+
+if [ $# != 1 ]; then
+ echo expected one argument: '<'dtrace-path'>'
+ exit 2
+fi
+
+dtrace=$1
+CC=/usr/bin/gcc
+CFLAGS=
+
+DIRNAME="$tmpdir/list-probes-args-usdt.$$.$RANDOM"
+mkdir -p $DIRNAME
+cd $DIRNAME
+
+cat > prov.d <<EOF
+provider test_prov {
+ probe go(int a, char *b) : (char *b, int a);
+};
+EOF
+
+$dtrace -h -s prov.d
+if [ $? -ne 0 ]; then
+ echo "failed to generate header file" >& 2
+ exit 1
+fi
+
+cat > test.c <<EOF
+#include <sys/types.h>
+#include "prov.h"
+
+int
+main(int argc, char **argv)
+{
+ TEST_PROV_GO(666, "foo");
+ sleep(10);
+}
+EOF
+
+${CC} ${CFLAGS} -c test.c
+if [ $? -ne 0 ]; then
+ echo "failed to compile test.c" >& 2
+ exit 1
+fi
+$dtrace -G -s prov.d test.o
+if [ $? -ne 0 ]; then
+ echo "failed to create DOF" >& 2
+ exit 1
+fi
+${CC} ${CFLAGS} -o test test.o prov.o
+if [ $? -ne 0 ]; then
+ echo "failed to link final executable" >& 2
+ exit 1
+fi
+
+script()
+{
+ $dtrace -c ./test -lvn 'test_prov$target:::go'
+ ./test &
+ PID=$!
+ disown %+
+ $dtrace -p $PID -lvn 'test_prov$target:::go'
+ kill -9 $PID
+}
+
+script
+status=$?
+
+exit $status
diff --git a/test/unittest/usdt/err.argmap-null.d b/test/unittest/usdt/err.argmap-null.d
new file mode 100644
index 000000000000..ba765bea7a04
--- /dev/null
+++ b/test/unittest/usdt/err.argmap-null.d
@@ -0,0 +1,41 @@
+/*
+ * 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.
+ */
+
+/* @@trigger: usdt-tst-argmap */
+/* @@trigger-timing: before */
+/* @@runtest-opts: $_pid */
+
+/*
+ * ASSERTION: Verify that a probe left with no args at all due to mapping
+ * has no args.
+ */
+
+BEGIN
+{
+ /* Timeout after 5 seconds */
+ timeout = timestamp + 5000000000;
+}
+
+test_prov$1:::place4
+/args[0] != 204 || args[0] == 204/
+{
+ printf("this should never happen");
+ exit(1);
+}
+
+test_prov$1:::place4
+{
+ printf("nor should this");
+ exit(1);
+}
+
+profile:::tick-1
+/timestamp > timeout/
+{
+ trace("test timed out");
+ exit(1);
+}
diff --git a/test/unittest/usdt/err.argmap-null.r b/test/unittest/usdt/err.argmap-null.r
new file mode 100644
index 000000000000..215475e39b48
--- /dev/null
+++ b/test/unittest/usdt/err.argmap-null.r
@@ -0,0 +1,2 @@
+-- @@stderr --
+dtrace: failed to compile script test/unittest/usdt/err.argmap-null.d: line 24: index 0 is out of range for test_provXXXX:::place4 args[ ]
diff --git a/test/unittest/usdt/err.argmap-null.r.p b/test/unittest/usdt/err.argmap-null.r.p
new file mode 100755
index 000000000000..c575983adf65
--- /dev/null
+++ b/test/unittest/usdt/err.argmap-null.r.p
@@ -0,0 +1,2 @@
+#!/bin/sed -rf
+s,test_prov[0-9]*,test_provXXXX,g; s,^ *[0-9]+, XX,g;
diff --git a/test/unittest/usdt/tst.argmap-null.d b/test/unittest/usdt/tst.argmap-null.d
new file mode 100644
index 000000000000..dacf4c4f569a
--- /dev/null
+++ b/test/unittest/usdt/tst.argmap-null.d
@@ -0,0 +1,32 @@
+/*
+ * 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.
+ */
+
+/* @@trigger: usdt-tst-argmap */
+/* @@trigger-timing: before */
+/* @@runtest-opts: $_pid */
+
+/*
+ * ASSERTION: Verify that you can map to no args at all.
+ */
+
+BEGIN
+{
+ /* Timeout after 5 seconds */
+ timeout = timestamp + 5000000000;
+}
+
+test_prov$1:::place4
+{
+ exit(0);
+}
+
+profile:::tick-1
+/timestamp > timeout/
+{
+ trace("test timed out");
+ exit(1);
+}
diff --git a/test/unittest/usdt/tst.argmap-typed-partial.d b/test/unittest/usdt/tst.argmap-typed-partial.d
new file mode 100644
index 000000000000..8c4d7273c0ab
--- /dev/null
+++ b/test/unittest/usdt/tst.argmap-typed-partial.d
@@ -0,0 +1,49 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2007, 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.
+ */
+
+/* @@trigger: usdt-tst-argmap */
+/* @@trigger-timing: before */
+/* @@runtest-opts: $_pid */
+
+/*
+ * ASSERTION: Verify that args[N] variables are properly typed when mapped,
+ * even if some args are unused.
+ */
+
+BEGIN
+{
+ /* Timeout after 5 seconds */
+ timeout = timestamp + 5000000000;
+}
+
+test_prov$1:::place3
+/stringof(args[0]) != "bar"/
+{
+ printf("arg is %s; should be \"bar\"",
+ stringof(args[0]));
+ exit(1);
+}
+
+test_prov$1:::place3
+/stringof(copyinstr(arg0)) != "bar"/
+{
+ printf("arg is %s; should be \"bar\"",
+ stringof(copyinstr(arg0)));
+ exit(1);
+}
+
+test_prov$1:::place3
+{
+ exit(0);
+}
+
+profile:::tick-1
+/timestamp > timeout/
+{
+ trace("test timed out");
+ exit(1);
+}
diff --git a/test/unittest/usdt/tst.argmap-typed.d b/test/unittest/usdt/tst.argmap-typed.d
new file mode 100644
index 000000000000..1243e059b8ae
--- /dev/null
+++ b/test/unittest/usdt/tst.argmap-typed.d
@@ -0,0 +1,48 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2007, 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.
+ */
+
+/* @@trigger: usdt-tst-argmap */
+/* @@trigger-timing: before */
+/* @@runtest-opts: $_pid */
+
+/*
+ * ASSERTION: Verify that args[N] variables are properly typed when mapped.
+ */
+
+BEGIN
+{
+ /* Timeout after 5 seconds */
+ timeout = timestamp + 5000000000;
+}
+
+test_prov$1:::place2
+/stringof(args[0]) != "foo" || args[1] != 255 || args[2] != 255 || stringof(args[3]) != "foo"/
+{
+ printf("args are %s, %d, %d, %s; should be \"foo\", 255, 255, \"foo\"",
+ stringof(args[0]), args[1], args[2], stringof(args[3]));
+ exit(1);
+}
+
+test_prov$1:::place2
+/stringof(copyinstr(arg0)) != "foo" || arg1 != 255 || arg2 != 255 || stringof(copyinstr(arg3)) != "foo"/
+{
+ printf("args are %s, %d, %d, %s; should be \"foo\", 255, 255, \"foo\"",
+ stringof(copyinstr(arg0)), arg1, arg2, stringof(copyinstr(arg3)));
+ exit(1);
+}
+
+test_prov$1:::place2
+{
+ exit(0);
+}
+
+profile:::tick-1
+/timestamp > timeout/
+{
+ trace("test timed out");
+ exit(1);
+}
diff --git a/test/unittest/usdt/tst.argmap.d b/test/unittest/usdt/tst.argmap.d
index 7896dc07e0e2..a7d68da3dfbc 100644
--- a/test/unittest/usdt/tst.argmap.d
+++ b/test/unittest/usdt/tst.argmap.d
@@ -1,17 +1,16 @@
/*
* Oracle Linux DTrace.
- * Copyright (c) 2007, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2007, 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.
*/
-/* @@xfail: dtv2 */
/* @@trigger: usdt-tst-argmap */
/* @@trigger-timing: before */
/* @@runtest-opts: $_pid */
/*
- * ASSERTION: Verify that argN and args[N] variables are properly remapped.
+ * ASSERTION: Verify that argN and args[N] variables are properly mapped.
*/
BEGIN
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v5 4/6] usdt: typed args and arg mapping
2024-11-05 0:06 ` [PATCH v5 4/6] usdt: typed args and arg mapping Nick Alcock
@ 2024-11-05 18:53 ` Kris Van Hees
2024-11-06 11:16 ` Nick Alcock
0 siblings, 1 reply; 15+ messages in thread
From: Kris Van Hees @ 2024-11-05 18:53 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
On Tue, Nov 05, 2024 at 12:06:06AM +0000, Nick Alcock wrote:
> This change propagates the arg type and mapping info we just got into the
> pid_probespec_t onward into the underlying dt_uprobe_t, shuffles the native
> types before handing off a 1:1 mapping to the dt_probe_t, and calls
> dt_cg_tramp_map_args() to apply the requested mappings. With this, USDT
> probes should now have visible types which obey the :-notation translators
> specified in their .d fils (this is now tested).
Not sure why I didn't catch this earlier but this is confusing (or possibly
wrong?)... Perhaps the problem is that you are trying to put too much into a
single summary sentence.
There is no shuffling of native types.
What happens (based on my reading of the code) is that:
- arg data in pid_probespec_t is propagated into the underlying dt_uprobe_t
where it is stored as a dt_argdesc_t array
- the trampoline shuffles the argument values according to the mapping data
in the underlying probe
- calls to probe_info() retrieve arg data as a dt_argdesc_t array with 1:1
mapping (because the trace program can only access arguments after they
were already mapped)
Also.... "fils" -> "file"
> The probe info has been moved from the underlying probe to the overlying one
"probe info" -> "probe info hook"
> as part of this: underlying probes don't have any user-visible args to
> get info for, and their probe_info functions are never called. We do all
> the arg reshuffling in a function ultimately called from USDT probe
There is no arg reshuffling or shuffling... all that happens is that the
dt_argdesc_t array is populated.
> discovery via provide_underlying, so it predates trampoline setup let alone
> probe_info on the overlying probe and all the args info is present before
> it is needed. (We intentionally do not shuffle the args in
> dt_cg_tramp_map_args specifically so that we are not sensitive to the
Actually, dt_cg_tramp_map_args() specifically *does* the arg shuffling (the
mapping of args). That is its sole purpose.
> relative call order of trampoline() versus probe_info(), but there's no
> escaping the requirement for USDT probe discovery to predate all of this:
> but this was already a requirement in any case.)
There is no call order relation in any way between trampoline and probe_info.
In fact, except for some special providers that do not have a low-level probe
implementation component, trampolines are not generated until all olauses have
been copiled.
>
> We make one assumption for simplicity, noted by a TODO in the source.
> We assume that overlying probes all have the same native types and mappings,
> so we don't need to look at more than one underlying probe to figure out what
> they are. DTrace always generates such a thing, but DOF specifying multiple
> overlapping probes seems perfectly possible for other programs to generate.
> This should probably be validated in dof_parser.c in future.
Since you added validation that rejects multiple USDT probes using the same
tracepoint (underlying probe), this is a moot point. There is no assumption
anymore in any case. Any underlying probe can only be associated with a single
overlying probe, so storing the arg data in the underlying probe is valid.
If you feel the need to mention that other DOF generators could potentially
create DOF that associates multiple overying probes with the same underlying
probe. that's fine, but since your code will not accept that, I don't think
there is a need to mention it.
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
> include/dtrace/pid.h | 1 +
> libdtrace/dt_pid.c | 3 +-
> libdtrace/dt_prov_uprobe.c | 185 ++++++++++++++++--
> test/triggers/usdt-tst-argmap-prov.d | 5 +-
> test/triggers/usdt-tst-argmap.c | 5 +-
> .../dtrace-util/tst.ListProbesArgsUSDT.r | 34 ++++
> .../dtrace-util/tst.ListProbesArgsUSDT.r.p | 2 +
> .../dtrace-util/tst.ListProbesArgsUSDT.sh | 83 ++++++++
> test/unittest/usdt/err.argmap-null.d | 41 ++++
> test/unittest/usdt/err.argmap-null.r | 2 +
> test/unittest/usdt/err.argmap-null.r.p | 2 +
> test/unittest/usdt/tst.argmap-null.d | 32 +++
> test/unittest/usdt/tst.argmap-typed-partial.d | 49 +++++
> test/unittest/usdt/tst.argmap-typed.d | 48 +++++
> test/unittest/usdt/tst.argmap.d | 5 +-
> 15 files changed, 478 insertions(+), 19 deletions(-)
> create mode 100644 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
> create mode 100755 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
> create mode 100755 test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
> create mode 100644 test/unittest/usdt/err.argmap-null.d
> create mode 100644 test/unittest/usdt/err.argmap-null.r
> create mode 100755 test/unittest/usdt/err.argmap-null.r.p
> create mode 100644 test/unittest/usdt/tst.argmap-null.d
> create mode 100644 test/unittest/usdt/tst.argmap-typed-partial.d
> create mode 100644 test/unittest/usdt/tst.argmap-typed.d
>
> diff --git a/include/dtrace/pid.h b/include/dtrace/pid.h
> index 25bfacfdbfe2..c53e600475df 100644
> --- a/include/dtrace/pid.h
> +++ b/include/dtrace/pid.h
> @@ -22,6 +22,7 @@ typedef enum pid_probetype {
> DTPPT_ENTRY,
> DTPPT_RETURN,
> DTPPT_OFFSETS,
> + DTPPT_USDT,
> DTPPT_IS_ENABLED
> } pid_probetype_t;
>
> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> index 71ac1754d343..a0b3d892fd23 100644
> --- a/libdtrace/dt_pid.c
> +++ b/libdtrace/dt_pid.c
> @@ -950,6 +950,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
>
> p += args->size;
> seen_size += args->size;
> + args = (dof_parsed_t *) p;
This change belongs in patch 1/6... without it that 1/6 patch is validating
the wrong data.
>
> if (!validate_dof_record(path, args, DIT_ARGS_MAP,
> dof_buf_size, seen_size))
> @@ -1000,7 +1001,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> goto oom;
> }
>
> - psp.pps_type = tp->tracepoint.is_enabled ? DTPPT_IS_ENABLED : DTPPT_OFFSETS;
> + psp.pps_type = tp->tracepoint.is_enabled ? DTPPT_IS_ENABLED : DTPPT_USDT;
> psp.pps_prv = prv;
> psp.pps_mod = mod;
> psp.pps_fun = fun;
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index a8c1ab2761f0..28f5351be21a 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -46,9 +46,11 @@
> /* Provider name for the underlying probes. */
> static const char prvname[] = "uprobe";
>
> -#define PP_IS_RETURN 1
> -#define PP_IS_FUNCALL 2
> -#define PP_IS_ENABLED 4
> +#define PP_IS_RETURN 0x1
> +#define PP_IS_FUNCALL 0x2
> +#define PP_IS_ENABLED 0x4
> +#define PP_IS_USDT 0x8
> +#define PP_IS_MAPPED 0x10
>
> typedef struct dt_uprobe {
> dev_t dev;
> @@ -57,7 +59,10 @@ typedef struct dt_uprobe {
> uint64_t off;
> int flags;
> tp_probe_t *tp;
> - dt_list_t probes; /* pid/USDT probes triggered by it */
> + int argc; /* number of args */
> + dt_argdesc_t *args; /* args array (points into argvbuf) */
> + char *argvbuf; /* arg strtab */
> + dt_list_t probes; /* pid/USDT probes triggered by it */
> } dt_uprobe_t;
>
> typedef struct list_probe {
> @@ -123,6 +128,8 @@ static void probe_destroy_underlying(dtrace_hdl_t *dtp, void *datap)
> dt_tp_destroy(dtp, tpp);
> free_probe_list(dtp, dt_list_next(&upp->probes));
> dt_free(dtp, upp->fn);
> + dt_free(dtp, upp->args);
> + dt_free(dtp, upp->argvbuf);
> dt_free(dtp, upp);
> }
>
> @@ -516,6 +523,83 @@ static int discover(dtrace_hdl_t *dtp)
> return 0;
> }
>
> +/*
> + * Populate args for an underlying probe. This really relates to an overlying
> + * USDT probe (all overlying probes associated with a given underlying probe
> + * must have the same args), but the overlying probe doesn't exist at the point
> + * this is populated, so we must put it in the underlying probe instead and pull
> + * it out when the overlying probe info is called for.
"all overlying probes associated with a given underlying probe" makes little
sense when it is already established (ad code guarantees) that there will
only be a single overlying USDT probe for any underlying probe - and any other
overlying probe is therefore a pid probe and does not have defined arguments.
Pehaps better would be:
/*
* Populate args for an underlying probe for use by the overlying USDT probe.
* The overlying probe does not exist yet at this point, so the arg data is
* stored in the underlying probe instead and will be accessed when probe_info
* is called in the overlying probe.
> + *
> + * Move it into dt_argdesc_t's for use later on. The char *'s in that structure
> + * are pointers into the argvbuf array, which is a straight concatenated copy of
> + * the nargc/xargc in the pid_probespec_t.
nargc/xargc -> nargv/xargv
> + */
> +static int populate_args(dtrace_hdl_t *dtp, const pid_probespec_t *psp,
> + dt_uprobe_t *upp)
> +{
> + char **nargv = NULL;
> + char *nptr = NULL, *xptr = NULL;
> + size_t i;
> +
> + upp->argc = psp->pps_xargc;
> +
> + /*
> + * If we have a nonzero number of args, we always have at least one narg
> + * and at least one xarg. Double-check to be sure. (These are not
> + * populated, and thus left 0/NULL, for non-USDT probes.)
> + */
> + if (upp->argc == 0 || psp->pps_xargv == NULL || psp->pps_nargv == NULL
> + || psp->pps_xargvlen == 0 || psp->pps_nargvlen == 0)
> + return 0;
> +
> + upp->argvbuf = dt_alloc(dtp, psp->pps_xargvlen + psp->pps_nargvlen);
> + if(upp->argvbuf == NULL)
> + return -1;
> + memcpy(upp->argvbuf, psp->pps_xargv, psp->pps_xargvlen);
> + xptr = upp->argvbuf;
> +
> + memcpy(upp->argvbuf + psp->pps_xargvlen, psp->pps_nargv, psp->pps_nargvlen);
> + nptr = upp->argvbuf + psp->pps_xargvlen;
> +
> + upp->args = dt_calloc(dtp, upp->argc, sizeof(dt_argdesc_t));
> + if (upp->args == NULL)
> + return -1;
> +
> + /*
> + * Construct an array to allow accessing native args by index.
> + */
> +
Remove blank line.
> + if ((nargv = dt_calloc(dtp, psp->pps_nargc, sizeof (char *))) == NULL)
> + goto fail;
> +
> + for (i = 0; i < psp->pps_nargc; i++, nptr += strlen(nptr) + 1)
> + nargv[i] = nptr;
> +
> + /*
> + * Fill up the upp->args array based on xargs. If this indicates that
> + * mapping is needed, note as much.
> + */
> +
Remove blank line.
> + for (i = 0; i < upp->argc; i++, xptr += strlen(xptr) + 1) {
> + int map_arg = psp->pps_argmap[i];
> +
> + upp->args[i].native = nargv[map_arg];
> + upp->args[i].xlate = xptr;
> + upp->args[i].mapping = map_arg;
> + upp->args[i].flags = 0;
> +
> + if (i != map_arg)
> + upp->flags |= PP_IS_MAPPED;
> + }
> +
> + free(nargv);
> + return 0;
> +
> + fail:
> + free(nargv);
> + return -1;
> +}
> +
> /*
> * Look up or create an underlying (real) probe, corresponding directly to a
> * uprobe. Since multiple pid and USDT probes may all map onto the same
> @@ -530,7 +614,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> char prb[DTRACE_NAMELEN];
> dtrace_probedesc_t pd;
> dt_probe_t *uprp;
> - dt_uprobe_t *upp;
> + dt_uprobe_t *upp = NULL;
>
> /*
> * The underlying probes (uprobes) represent the tracepoints that pid
> @@ -555,6 +639,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> case DTPPT_IS_ENABLED:
> case DTPPT_ENTRY:
> case DTPPT_OFFSETS:
> + case DTPPT_USDT:
> snprintf(prb, sizeof(prb), "%lx", psp->pps_off);
> break;
> default:
> @@ -568,6 +653,8 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> pd.fun = psp->pps_fun;
> pd.prb = prb;
>
> + dt_dprintf("Providing underlying probe %s:%s:%s:%s @ %lx\n", psp->pps_prv,
> + psp->pps_mod, psp->pps_fn, psp->pps_prb, psp->pps_off);
> uprp = dt_probe_lookup(dtp, &pd);
> if (uprp == NULL) {
> dt_provider_t *pvp;
> @@ -591,12 +678,24 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> goto fail;
>
> uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
> - upp);
> + upp);
> if (uprp == NULL)
> goto fail;
> } else
> upp = uprp->prv_data;
>
> + /*
> + * Only one USDT probe can correspond to each underlying probe.
> + */
> + if (psp->pps_type == DTPPT_USDT && upp->flags == PP_IS_USDT) {
> + dt_dprintf("Found overlapping USDT probe at %lx/%lx/%lx/%s\n",
> + upp->dev, upp->inum, upp->off, upp->fn);
> + goto fail;
> + }
> +
> + if (populate_args(dtp, psp, upp) < 0)
> + goto fail;
Why bother calling this for a non-USDT probe?
I would use:
if ( psp->pps_type == DTPPT_USD && populate_args(dtp, psp, upp) < 0)
> +
> switch (psp->pps_type) {
> case DTPPT_RETURN:
> upp->flags |= PP_IS_RETURN;
> @@ -604,15 +703,19 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> case DTPPT_IS_ENABLED:
> upp->flags |= PP_IS_ENABLED;
> break;
> + case DTPPT_USDT:
> + upp->flags |= PP_IS_USDT;
> + break;
> default: ;
> /*
> * No flags needed for other types.
> */
> }
> -
I would leave the blank line.
> return uprp;
>
> fail:
> + dt_dprintf("Failed to instantiate %s:%s:%s:%s\n", psp->pps_prv,
> + psp->pps_mod, psp->pps_fn, psp->pps_prb);
> probe_destroy(dtp, upp);
> return NULL;
> }
> @@ -732,7 +835,7 @@ static int provide_pid_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp)
>
> static int provide_usdt_probe(dtrace_hdl_t *dtp, const pid_probespec_t *psp)
> {
> - if (psp->pps_type != DTPPT_OFFSETS &&
> + if (psp->pps_type != DTPPT_USDT &&
> psp->pps_type != DTPPT_IS_ENABLED) {
> dt_dprintf("pid: unknown USDT probe type %i\n", psp->pps_type);
> return -1;
> @@ -786,7 +889,12 @@ static void enable_usdt(dtrace_hdl_t *dtp, dt_probe_t *prp)
> * int dt_uprobe(dt_pt_regs *regs)
> *
> * The trampoline will first populate a dt_dctx_t struct. It will then emulate
> - * the firing of all dependent pid* probes and their clauses.
> + * the firing of all dependent pid* and USDT probes and their clauses, or (in
> + * the case of is-enabled probes), do the necessary copying (is-enabled probes
> + * have no associated clauses and their behaviour is hardwired).
> + *
> + * Trampolines associated with USDT probes will also arrange for argument
> + * shuffling if the argmap array calls for it.
I would drop this since you mention the napping in a comment below already.
Here is is potentially confusing because trampolines are associated with the
underlying probe, and that probe might have both pid probes and a USDT probe
associated with it.
> */
> static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> {
> @@ -864,7 +972,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> }
>
> /*
> - * USDT
> + * USDT.
> */
>
> /* In some cases, we know there are no USDT probes. */ // FIXME: add more checks
> @@ -873,6 +981,16 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>
> dt_cg_tramp_copy_args_from_regs(pcb, 0);
>
> + /*
> + * Apply arg mappings, if needed.
> + */
> + if (upp->flags & PP_IS_MAPPED) {
> +
> + /* dt_cg_tramp_map_args() works from the saved args. */
> + dt_cg_tramp_save_args(pcb);
> + dt_cg_tramp_map_args(pcb, upp->args, upp->argc);
> + }
> +
> /*
> * Retrieve the PID of the process that caused the probe to fire.
> */
> @@ -1083,8 +1201,49 @@ attach_bpf:
> static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> int *argcp, dt_argdesc_t **argvp)
> {
> - *argcp = 0; /* no known arguments */
> - *argvp = NULL;
> + size_t i;
> + list_probe_t *pup = prp->prv_data;
> + dt_uprobe_t *upp;
> + size_t argc = 0;
> + dt_argdesc_t *argv = NULL;
> +
> + /* No underlying probes? No args. */
> + if (!pup)
> + goto done;
> +
> + upp = pup->probe->prv_data;
> + if (!upp || upp->args == NULL)
> + goto done;
> +
> + argc = upp->argc;
> +
Remove blank line.
> + argv = dt_calloc(dtp, argc, sizeof(dt_argdesc_t));
> + if (argv == NULL)
> + return dt_set_errno(dtp, EDT_NOMEM);
> +
> + for (i = 0; i < argc; i++) {
> + argv[i].native = strdup(upp->args[i].native);
> + if (upp->args[i].xlate)
> + argv[i].xlate = strdup(upp->args[i].xlate);
> + argv[i].mapping = i;
> +
Perhaps add a comment here that this is code to handle an allocation failure?
Or move this to the end of the function with a fail: label, and goto to that
if allocation fails (since that makes it more obvious that this is code to
handle an allocation failure).
> + if (argv[i].native == NULL ||
> + (upp->args[i].xlate != NULL && argv[i].xlate == NULL)) {
> + size_t j;
> +
> + for (j = 0; j <= i; j++) {
> + free((char *) argv[i].native);
> + free((char *) argv[i].xlate);
> + }
> +
> + dt_free(dtp, argv);
> + return dt_set_errno(dtp, EDT_NOMEM);
> + }
> + }
> +
> +done:
> + *argcp = argc;
> + *argvp = argv;
>
> return 0;
> }
> @@ -1152,7 +1311,6 @@ dt_provimpl_t dt_uprobe = {
> .load_prog = &dt_bpf_prog_load,
> .trampoline = &trampoline,
> .attach = &attach,
> - .probe_info = &probe_info,
> .detach = &detach,
> .probe_destroy = &probe_destroy_underlying,
> .add_probe = &add_probe_uprobe,
> @@ -1178,6 +1336,7 @@ dt_provimpl_t dt_usdt = {
> .populate = &populate_usdt,
> .provide_probe = &provide_usdt_probe,
> .enable = &enable_usdt,
> + .probe_info = &probe_info,
> .probe_destroy = &probe_destroy,
> .discover = &discover,
> .add_probe = &add_probe_usdt,
> diff --git a/test/triggers/usdt-tst-argmap-prov.d b/test/triggers/usdt-tst-argmap-prov.d
> index 28134baa6fa3..d8c3e88c4616 100644
> --- a/test/triggers/usdt-tst-argmap-prov.d
> +++ b/test/triggers/usdt-tst-argmap-prov.d
> @@ -1,10 +1,13 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 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.
> */
>
> provider test_prov {
> probe place(int i, int j) : (int j, int i, int i, int j);
> + probe place2(int i, char *j) : (char *j, int i, int i, char *j);
> + probe place3(int i, char *j) : (char *j);
> + probe place4(int i, char *j) : ();
It might be worth adding tests for -lvn for all these cases, as an extra
check that probe info is being provided correctly, even in the case where we
do not actually trace.
> };
> diff --git a/test/triggers/usdt-tst-argmap.c b/test/triggers/usdt-tst-argmap.c
> index 89a0a53fc1d5..9244092514ff 100644
> --- a/test/triggers/usdt-tst-argmap.c
> +++ b/test/triggers/usdt-tst-argmap.c
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 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.
> */
> @@ -12,6 +12,9 @@ main(int argc, char **argv)
> {
> for (;;) {
> DTRACE_PROBE2(test_prov, place, 10, 4);
> + DTRACE_PROBE(test_prov, place2, 255, "foo");
> + DTRACE_PROBE(test_prov, place3, 126, "bar");
> + DTRACE_PROBE(test_prov, place4, 204, "baz");
> }
>
> return 0;
> diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
> new file mode 100644
> index 000000000000..8bcfa92ff6b0
> --- /dev/null
> +++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r
> @@ -0,0 +1,34 @@
> + ID PROVIDER MODULE FUNCTION NAME
> + XX test_provXXXX test main go
> +
> + Probe Description Attributes
> + Identifier Names: Private
> + Data Semantics: Private
> + Dependency Class: Unknown
> +
> + Argument Attributes
> + Identifier Names: Private
> + Data Semantics: Private
> + Dependency Class: Unknown
> +
> + Argument Types
> + args[0]: char *
> + args[1]: int
> +
> + ID PROVIDER MODULE FUNCTION NAME
> + XX test_provXXXX test main go
> +
> + Probe Description Attributes
> + Identifier Names: Private
> + Data Semantics: Private
> + Dependency Class: Unknown
> +
> + Argument Attributes
> + Identifier Names: Private
> + Data Semantics: Private
> + Dependency Class: Unknown
> +
> + Argument Types
> + args[0]: char *
> + args[1]: int
> +
> diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
> new file mode 100755
> index 000000000000..c575983adf65
> --- /dev/null
> +++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.r.p
> @@ -0,0 +1,2 @@
> +#!/bin/sed -rf
> +s,test_prov[0-9]*,test_provXXXX,g; s,^ *[0-9]+, XX,g;
> diff --git a/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
> new file mode 100755
> index 000000000000..ad8dc2e7c267
> --- /dev/null
> +++ b/test/unittest/dtrace-util/tst.ListProbesArgsUSDT.sh
> @@ -0,0 +1,83 @@
> +#!/bin/bash
> +#
> +# Oracle Linux DTrace.
> +# Copyright (c) 2013, 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.
> +
> +##
> +#
> +# ASSERTION:
> +# Testing -lvn option with USDT probes with a valid probe name.
> +#
> +# SECTION: dtrace Utility/-ln Option
> +#
> +##
> +
> +if [ $# != 1 ]; then
> + echo expected one argument: '<'dtrace-path'>'
> + exit 2
> +fi
> +
> +dtrace=$1
> +CC=/usr/bin/gcc
> +CFLAGS=
> +
> +DIRNAME="$tmpdir/list-probes-args-usdt.$$.$RANDOM"
> +mkdir -p $DIRNAME
> +cd $DIRNAME
> +
> +cat > prov.d <<EOF
> +provider test_prov {
> + probe go(int a, char *b) : (char *b, int a);
> +};
> +EOF
> +
> +$dtrace -h -s prov.d
> +if [ $? -ne 0 ]; then
> + echo "failed to generate header file" >& 2
> + exit 1
> +fi
> +
> +cat > test.c <<EOF
> +#include <sys/types.h>
> +#include "prov.h"
> +
> +int
> +main(int argc, char **argv)
> +{
> + TEST_PROV_GO(666, "foo");
> + sleep(10);
> +}
> +EOF
> +
> +${CC} ${CFLAGS} -c test.c
> +if [ $? -ne 0 ]; then
> + echo "failed to compile test.c" >& 2
> + exit 1
> +fi
> +$dtrace -G -s prov.d test.o
> +if [ $? -ne 0 ]; then
> + echo "failed to create DOF" >& 2
> + exit 1
> +fi
> +${CC} ${CFLAGS} -o test test.o prov.o
> +if [ $? -ne 0 ]; then
> + echo "failed to link final executable" >& 2
> + exit 1
> +fi
> +
> +script()
> +{
> + $dtrace -c ./test -lvn 'test_prov$target:::go'
> + ./test &
> + PID=$!
> + disown %+
> + $dtrace -p $PID -lvn 'test_prov$target:::go'
> + kill -9 $PID
> +}
> +
> +script
> +status=$?
> +
> +exit $status
> diff --git a/test/unittest/usdt/err.argmap-null.d b/test/unittest/usdt/err.argmap-null.d
> new file mode 100644
> index 000000000000..ba765bea7a04
> --- /dev/null
> +++ b/test/unittest/usdt/err.argmap-null.d
> @@ -0,0 +1,41 @@
> +/*
> + * 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.
> + */
> +
> +/* @@trigger: usdt-tst-argmap */
> +/* @@trigger-timing: before */
> +/* @@runtest-opts: $_pid */
> +
> +/*
> + * ASSERTION: Verify that a probe left with no args at all due to mapping
> + * has no args.
> + */
> +
> +BEGIN
> +{
> + /* Timeout after 5 seconds */
> + timeout = timestamp + 5000000000;
> +}
> +
> +test_prov$1:::place4
> +/args[0] != 204 || args[0] == 204/
> +{
> + printf("this should never happen");
> + exit(1);
> +}
> +
> +test_prov$1:::place4
> +{
> + printf("nor should this");
> + exit(1);
> +}
> +
> +profile:::tick-1
> +/timestamp > timeout/
> +{
> + trace("test timed out");
> + exit(1);
> +}
> diff --git a/test/unittest/usdt/err.argmap-null.r b/test/unittest/usdt/err.argmap-null.r
> new file mode 100644
> index 000000000000..215475e39b48
> --- /dev/null
> +++ b/test/unittest/usdt/err.argmap-null.r
> @@ -0,0 +1,2 @@
> +-- @@stderr --
> +dtrace: failed to compile script test/unittest/usdt/err.argmap-null.d: line 24: index 0 is out of range for test_provXXXX:::place4 args[ ]
> diff --git a/test/unittest/usdt/err.argmap-null.r.p b/test/unittest/usdt/err.argmap-null.r.p
> new file mode 100755
> index 000000000000..c575983adf65
> --- /dev/null
> +++ b/test/unittest/usdt/err.argmap-null.r.p
> @@ -0,0 +1,2 @@
> +#!/bin/sed -rf
> +s,test_prov[0-9]*,test_provXXXX,g; s,^ *[0-9]+, XX,g;
> diff --git a/test/unittest/usdt/tst.argmap-null.d b/test/unittest/usdt/tst.argmap-null.d
> new file mode 100644
> index 000000000000..dacf4c4f569a
> --- /dev/null
> +++ b/test/unittest/usdt/tst.argmap-null.d
> @@ -0,0 +1,32 @@
> +/*
> + * 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.
> + */
> +
> +/* @@trigger: usdt-tst-argmap */
> +/* @@trigger-timing: before */
> +/* @@runtest-opts: $_pid */
> +
> +/*
> + * ASSERTION: Verify that you can map to no args at all.
> + */
> +
> +BEGIN
> +{
> + /* Timeout after 5 seconds */
> + timeout = timestamp + 5000000000;
> +}
> +
> +test_prov$1:::place4
> +{
> + exit(0);
> +}
> +
> +profile:::tick-1
> +/timestamp > timeout/
> +{
> + trace("test timed out");
> + exit(1);
> +}
> diff --git a/test/unittest/usdt/tst.argmap-typed-partial.d b/test/unittest/usdt/tst.argmap-typed-partial.d
> new file mode 100644
> index 000000000000..8c4d7273c0ab
> --- /dev/null
> +++ b/test/unittest/usdt/tst.argmap-typed-partial.d
> @@ -0,0 +1,49 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2007, 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.
> + */
> +
> +/* @@trigger: usdt-tst-argmap */
> +/* @@trigger-timing: before */
> +/* @@runtest-opts: $_pid */
> +
> +/*
> + * ASSERTION: Verify that args[N] variables are properly typed when mapped,
> + * even if some args are unused.
> + */
> +
> +BEGIN
> +{
> + /* Timeout after 5 seconds */
> + timeout = timestamp + 5000000000;
> +}
> +
> +test_prov$1:::place3
> +/stringof(args[0]) != "bar"/
> +{
> + printf("arg is %s; should be \"bar\"",
> + stringof(args[0]));
> + exit(1);
> +}
> +
> +test_prov$1:::place3
> +/stringof(copyinstr(arg0)) != "bar"/
> +{
> + printf("arg is %s; should be \"bar\"",
> + stringof(copyinstr(arg0)));
> + exit(1);
> +}
> +
> +test_prov$1:::place3
> +{
> + exit(0);
> +}
> +
> +profile:::tick-1
> +/timestamp > timeout/
> +{
> + trace("test timed out");
> + exit(1);
> +}
> diff --git a/test/unittest/usdt/tst.argmap-typed.d b/test/unittest/usdt/tst.argmap-typed.d
> new file mode 100644
> index 000000000000..1243e059b8ae
> --- /dev/null
> +++ b/test/unittest/usdt/tst.argmap-typed.d
> @@ -0,0 +1,48 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2007, 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.
> + */
> +
> +/* @@trigger: usdt-tst-argmap */
> +/* @@trigger-timing: before */
> +/* @@runtest-opts: $_pid */
> +
> +/*
> + * ASSERTION: Verify that args[N] variables are properly typed when mapped.
> + */
> +
> +BEGIN
> +{
> + /* Timeout after 5 seconds */
> + timeout = timestamp + 5000000000;
> +}
> +
> +test_prov$1:::place2
> +/stringof(args[0]) != "foo" || args[1] != 255 || args[2] != 255 || stringof(args[3]) != "foo"/
> +{
> + printf("args are %s, %d, %d, %s; should be \"foo\", 255, 255, \"foo\"",
> + stringof(args[0]), args[1], args[2], stringof(args[3]));
> + exit(1);
> +}
> +
> +test_prov$1:::place2
> +/stringof(copyinstr(arg0)) != "foo" || arg1 != 255 || arg2 != 255 || stringof(copyinstr(arg3)) != "foo"/
> +{
> + printf("args are %s, %d, %d, %s; should be \"foo\", 255, 255, \"foo\"",
> + stringof(copyinstr(arg0)), arg1, arg2, stringof(copyinstr(arg3)));
> + exit(1);
> +}
> +
> +test_prov$1:::place2
> +{
> + exit(0);
> +}
> +
> +profile:::tick-1
> +/timestamp > timeout/
> +{
> + trace("test timed out");
> + exit(1);
> +}
> diff --git a/test/unittest/usdt/tst.argmap.d b/test/unittest/usdt/tst.argmap.d
> index 7896dc07e0e2..a7d68da3dfbc 100644
> --- a/test/unittest/usdt/tst.argmap.d
> +++ b/test/unittest/usdt/tst.argmap.d
> @@ -1,17 +1,16 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2007, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2007, 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.
> */
>
> -/* @@xfail: dtv2 */
> /* @@trigger: usdt-tst-argmap */
> /* @@trigger-timing: before */
> /* @@runtest-opts: $_pid */
>
> /*
> - * ASSERTION: Verify that argN and args[N] variables are properly remapped.
> + * ASSERTION: Verify that argN and args[N] variables are properly mapped.
> */
>
> BEGIN
> --
> 2.46.0.278.g36e3a12567
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v5 4/6] usdt: typed args and arg mapping
2024-11-05 18:53 ` Kris Van Hees
@ 2024-11-06 11:16 ` Nick Alcock
2024-11-06 22:18 ` Kris Van Hees
0 siblings, 1 reply; 15+ messages in thread
From: Nick Alcock @ 2024-11-06 11:16 UTC (permalink / raw)
To: Kris Van Hees; +Cc: Nick Alcock, dtrace, dtrace-devel
[Forgot to send this last night, sorry. Probably not much use now.]
On 5 Nov 2024, Kris Van Hees verbalised:
> On Tue, Nov 05, 2024 at 12:06:06AM +0000, Nick Alcock wrote:
>> This change propagates the arg type and mapping info we just got into the
>> pid_probespec_t onward into the underlying dt_uprobe_t, shuffles the native
>> types before handing off a 1:1 mapping to the dt_probe_t, and calls
>> dt_cg_tramp_map_args() to apply the requested mappings. With this, USDT
>> probes should now have visible types which obey the :-notation translators
>> specified in their .d fils (this is now tested).
>
> Not sure why I didn't catch this earlier but this is confusing (or possibly
> wrong?)... Perhaps the problem is that you are trying to put too much into a
> single summary sentence.
There's no chance that I'd do something like that! (ha ha yes there is).
> There is no shuffling of native types.
Indeed not. It shuffles the *arguments*, but the types are not shuffled.
Nice catch.
> What happens (based on my reading of the code) is that:
>
> - arg data in pid_probespec_t is propagated into the underlying dt_uprobe_t
> where it is stored as a dt_argdesc_t array
> - the trampoline shuffles the argument values according to the mapping data
> in the underlying probe
> - calls to probe_info() retrieve arg data as a dt_argdesc_t array with 1:1
> mapping (because the trace program can only access arguments after they
> were already mapped)
>
> Also.... "fils" -> "file"
Rephrased thusly:
> This change propagates the arg type and mapping info we just got into the
> pid_probespec_t onward into the underlying dt_uprobe_t and shuffles the
> argument values by calling dt_cg_tramp_map_args(). With this, USDT probes
> should now have visible types which obey the :-notation translators
> specified in their .d file (this is now tested).
which is much simpler.
>> The probe info has been moved from the underlying probe to the overlying one
>
> "probe info" -> "probe info hook"
probe_info hook, and yes.
>> as part of this: underlying probes don't have any user-visible args to
>> get info for, and their probe_info functions are never called. We do all
>> the arg reshuffling in a function ultimately called from USDT probe
>
> There is no arg reshuffling or shuffling... all that happens is that the
> dt_argdesc_t array is populated.
It used to be shuffled and I forgot it wasn't when I rephrased bits.
>> discovery via provide_underlying, so it predates trampoline setup let alone
>> probe_info on the overlying probe and all the args info is present before
>> it is needed. (We intentionally do not shuffle the args in
>> dt_cg_tramp_map_args specifically so that we are not sensitive to the
>
> Actually, dt_cg_tramp_map_args() specifically *does* the arg shuffling (the
> mapping of args). That is its sole purpose.
Yep. I was thinking in terms of mapping modifications, but of course
that is done by probe_info... sigh.
>> relative call order of trampoline() versus probe_info(), but there's no
>> escaping the requirement for USDT probe discovery to predate all of this:
>> but this was already a requirement in any case.)
>
> There is no call order relation in any way between trampoline and probe_info.
> In fact, except for some special providers that do not have a low-level probe
> implementation component, trampolines are not generated until all olauses have
> been copiled.
Yeah, I've just dropped this whole parenthetical, it's a load of
rubbish. (And we no longer care what relative order they're called in
anyway: indeed we can even cope with create_underlying being called
later nowadays, as long as create_underlying for a given USDT probe is
called before probe_info for that probe, but we necessarily always
depended on that *anyway* so there's no point mentioning that either.)
>> We make one assumption for simplicity, noted by a TODO in the source.
>> We assume that overlying probes all have the same native types and mappings,
>> so we don't need to look at more than one underlying probe to figure out what
>> they are. DTrace always generates such a thing, but DOF specifying multiple
>> overlapping probes seems perfectly possible for other programs to generate.
>> This should probably be validated in dof_parser.c in future.
>
> Since you added validation that rejects multiple USDT probes using the same
I meant to take this out :( :( it's gone now. I did the squashing-back
late last night, clearly too late...
> If you feel the need to mention that other DOF generators could potentially
> create DOF that associates multiple overying probes with the same underlying
> probe. that's fine, but since your code will not accept that, I don't think
> there is a need to mention it.
It's gone gone gone. Anyone who does that gets a failure from DTrace and
will immediately realise they've done something wrong.
>> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
>> index 71ac1754d343..a0b3d892fd23 100644
>> --- a/libdtrace/dt_pid.c
>> +++ b/libdtrace/dt_pid.c
>> @@ -950,6 +950,7 @@ dt_pid_create_usdt_probes_proc(dtrace_hdl_t *dtp, dt_proc_t *dpr,
>>
>> p += args->size;
>> seen_size += args->size;
>> + args = (dof_parsed_t *) p;
>
> This change belongs in patch 1/6... without it that 1/6 patch is validating
> the wrong data.
Moved.
>> +/*
>> + * Populate args for an underlying probe. This really relates to an overlying
>> + * USDT probe (all overlying probes associated with a given underlying probe
>> + * must have the same args), but the overlying probe doesn't exist at the point
>> + * this is populated, so we must put it in the underlying probe instead and pull
>> + * it out when the overlying probe info is called for.
>
> "all overlying probes associated with a given underlying probe" makes little
> sense when it is already established (ad code guarantees) that there will
> only be a single overlying USDT probe for any underlying probe - and any other
> overlying probe is therefore a pid probe and does not have defined arguments.
True!
> Pehaps better would be:
>
> /*
> * Populate args for an underlying probe for use by the overlying USDT probe.
> * The overlying probe does not exist yet at this point, so the arg data is
> * stored in the underlying probe instead and will be accessed when probe_info
> * is called in the overlying probe.
Much better.
>> + *
>> + * Move it into dt_argdesc_t's for use later on. The char *'s in that structure
>> + * are pointers into the argvbuf array, which is a straight concatenated copy of
>> + * the nargc/xargc in the pid_probespec_t.
>
> nargc/xargc -> nargv/xargv
gah! I don't *believe* how many times I made that mistake. Almost every
reference was backwards.
>> + /*
>> + * Construct an array to allow accessing native args by index.
>> + */
>> +
>
> Remove blank line.
Ew, seriously?
... it looks like that is in fact the formatting. Adjusted.
>> + /*
>> + * Only one USDT probe can correspond to each underlying probe.
>> + */
>> + if (psp->pps_type == DTPPT_USDT && upp->flags == PP_IS_USDT) {
>> + dt_dprintf("Found overlapping USDT probe at %lx/%lx/%lx/%s\n",
>> + upp->dev, upp->inum, upp->off, upp->fn);
>> + goto fail;
>> + }
>> +
>> + if (populate_args(dtp, psp, upp) < 0)
>> + goto fail;
>
> Why bother calling this for a non-USDT probe?
populate_args() doesn't care if a probe is a USDT probe. It populates
args if the probe has them. It so happens that only USDT probes do, but
populate_args() is not sensitive to that and will work regardless.
Almost the first line of populate_args() is
/*
* If we have a nonzero number of args, we always have at least one narg
* and at least one xarg. Double-check to be sure. (These are not
* populated, and thus left 0/NULL, for non-USDT probes.)
*/
if (upp->argc == 0 || psp->pps_xargv == NULL || psp->pps_nargv == NULL
|| psp->pps_xargvlen == 0 || psp->pps_nargvlen == 0)
return 0;
which will always return unless the probe has args, no matter what type
it is. populate_args() doesn't care.
> I would use:
> if ( psp->pps_type == DTPPT_USD && populate_args(dtp, psp, upp) < 0)
I don't see any way in which that is clearer, but I could do that if you
actually wanted. Should future probe types turn up that have args that
need population, that would rust and need replacing, so that's a (tiny)
disadvantage and as far as I can tell no advantages.
>> +
>> switch (psp->pps_type) {
>> case DTPPT_RETURN:
>> upp->flags |= PP_IS_RETURN;
>> @@ -604,15 +703,19 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
>> case DTPPT_IS_ENABLED:
>> upp->flags |= PP_IS_ENABLED;
>> break;
>> + case DTPPT_USDT:
>> + upp->flags |= PP_IS_USDT;
>> + break;
>> default: ;
>> /*
>> * No flags needed for other types.
>> */
>> }
>> -
>
> I would leave the blank line.
Accidental deletion :(
>> - * the firing of all dependent pid* probes and their clauses.
>> + * the firing of all dependent pid* and USDT probes and their clauses, or (in
>> + * the case of is-enabled probes), do the necessary copying (is-enabled probes
>> + * have no associated clauses and their behaviour is hardwired).
>> + *
>> + * Trampolines associated with USDT probes will also arrange for argument
>> + * shuffling if the argmap array calls for it.
>
> I would drop this since you mention the napping in a comment below already.
I was trying to describe what the function does, rather than only one
thing it does which happens to be the first thing that was implemented
(firing the dependent probes and their clauses). This is one of the
things it does.
> Here is is potentially confusing because trampolines are associated with the
> underlying probe, and that probe might have both pid probes and a USDT probe
> associated with it.
So if I add "... for argument shuffling before the USDT clauses are
invoked" in the appropriate place, is that OK? (I assumed that was obvious.)
>> + for (i = 0; i < argc; i++) {
>> + argv[i].native = strdup(upp->args[i].native);
>> + if (upp->args[i].xlate)
>> + argv[i].xlate = strdup(upp->args[i].xlate);
>> + argv[i].mapping = i;
>> +
>
> Perhaps add a comment here that this is code to handle an allocation failure?
... er, the == NULL right after an allocation makes that sort of obvious?
> Or move this to the end of the function with a fail: label, and goto to that
> if allocation fails (since that makes it more obvious that this is code to
> handle an allocation failure).
That's much clearer, though you need to initialize i to zero to avoid
compiler warnings in that case. Adjusted.
>> provider test_prov {
>> probe place(int i, int j) : (int j, int i, int i, int j);
>> + probe place2(int i, char *j) : (char *j, int i, int i, char *j);
>> + probe place3(int i, char *j) : (char *j);
>> + probe place4(int i, char *j) : ();
>
> It might be worth adding tests for -lvn for all these cases, as an extra
> check that probe info is being provided correctly, even in the case where we
> do not actually trace.
Good point! Added some.
--
NULL && (void)
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v5 4/6] usdt: typed args and arg mapping
2024-11-06 11:16 ` Nick Alcock
@ 2024-11-06 22:18 ` Kris Van Hees
0 siblings, 0 replies; 15+ messages in thread
From: Kris Van Hees @ 2024-11-06 22:18 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
I plan to merge this patch with my r-b with changes outlined below if you are
OK with it. No need for a new series.
On Wed, Nov 06, 2024 at 11:16:41AM +0000, Nick Alcock wrote:
> >> - * the firing of all dependent pid* probes and their clauses.
> >> + * the firing of all dependent pid* and USDT probes and their clauses, or (in
> >> + * the case of is-enabled probes), do the necessary copying (is-enabled probes
> >> + * have no associated clauses and their behaviour is hardwired).
> >> + *
> >> + * Trampolines associated with USDT probes will also arrange for argument
> >> + * shuffling if the argmap array calls for it.
> >
> > I would drop this since you mention the napping in a comment below already.
>
> I was trying to describe what the function does, rather than only one
> thing it does which happens to be the first thing that was implemented
> (firing the dependent probes and their clauses). This is one of the
> things it does.
>
> > Here is is potentially confusing because trampolines are associated with the
> > underlying probe, and that probe might have both pid probes and a USDT probe
> > associated with it.
>
> So if I add "... for argument shuffling before the USDT clauses are
> invoked" in the appropriate place, is that OK? (I assumed that was obvious.)
No, honestly, because you already made the case that adding a conditional to
only populate args for USDT is not wanted because other types of userspace
probes might have arguments also. And that means they may also have argument
mapping data, and thus this is again not USDT-specific. The comment in the
code itself is better.
> >> + for (i = 0; i < argc; i++) {
> >> + argv[i].native = strdup(upp->args[i].native);
> >> + if (upp->args[i].xlate)
> >> + argv[i].xlate = strdup(upp->args[i].xlate);
> >> + argv[i].mapping = i;
> >> +
> >
> > Perhaps add a comment here that this is code to handle an allocation failure?
>
> ... er, the == NULL right after an allocation makes that sort of obvious?
>
> > Or move this to the end of the function with a fail: label, and goto to that
> > if allocation fails (since that makes it more obvious that this is code to
> > handle an allocation failure).
>
> That's much clearer, though you need to initialize i to zero to avoid
> compiler warnings in that case. Adjusted.
Hm, why would you need to init i to 0? The compiler should be able to see
that you cannot reach the oom label except after i is already initialized by
the for loop.
Which compiler version flagged a warning on this when i is not initialized.
Also, you put a size_t j; declaration at the oom: label rather than at the
beginning of a code block (beginning of the function in this case). I'll
change that in my copy before merging.
Also, in your patch you added a space before the label (I removed it because
we put labels at the left margin), and you introduced space indentation where
there should be a tab before the dt_free().
> >> provider test_prov {
> >> probe place(int i, int j) : (int j, int i, int i, int j);
> >> + probe place2(int i, char *j) : (char *j, int i, int i, char *j);
> >> + probe place3(int i, char *j) : (char *j);
> >> + probe place4(int i, char *j) : ();
> >
> > It might be worth adding tests for -lvn for all these cases, as an extra
> > check that probe info is being provided correctly, even in the case where we
> > do not actually trace.
>
> Good point! Added some.
>
> --
> NULL && (void)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 5/6] usdt: new tests for USDT arg sanity with overlapping pid probes
2024-11-05 0:06 [PATCH v5 0/6] usdt typed args, translators and arg mapping Nick Alcock
` (3 preceding siblings ...)
2024-11-05 0:06 ` [PATCH v5 4/6] usdt: typed args and arg mapping Nick Alcock
@ 2024-11-05 0:06 ` Nick Alcock
2024-11-05 0:06 ` [PATCH v5 6/6] usdt: fix create_underlying error path Nick Alcock
5 siblings, 0 replies; 15+ messages in thread
From: Nick Alcock @ 2024-11-05 0:06 UTC (permalink / raw)
To: dtrace, dtrace-devel
This generalizes the existing tst.pidprobes.sh to check the args
reported by the probe both with and without arg mapping in place.
Everything seems to work.
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
---
.../usdt/{tst.pidprobes.sh => pidprobes.sh} | 54 +++-
test/unittest/usdt/tst.pidargmap.sh | 11 +
test/unittest/usdt/tst.pidargs.sh | 11 +
test/unittest/usdt/tst.pidprobes.sh | 284 +-----------------
4 files changed, 67 insertions(+), 293 deletions(-)
copy test/unittest/usdt/{tst.pidprobes.sh => pidprobes.sh} (83%)
create mode 100755 test/unittest/usdt/tst.pidargmap.sh
create mode 100755 test/unittest/usdt/tst.pidargs.sh
diff --git a/test/unittest/usdt/tst.pidprobes.sh b/test/unittest/usdt/pidprobes.sh
similarity index 83%
copy from test/unittest/usdt/tst.pidprobes.sh
copy to test/unittest/usdt/pidprobes.sh
index 8d18c89a4aec..c9769fc318e7 100755
--- a/test/unittest/usdt/tst.pidprobes.sh
+++ b/test/unittest/usdt/pidprobes.sh
@@ -5,9 +5,12 @@
# Licensed under the Universal Permissive License v 1.0 as shown at
# http://oss.oracle.com/licenses/upl.
#
-# This test verifies that USDT and pid probes can share underlying probes.
+# This test verifies various properties of USDT and pid probes sharing
+# underlying probes.
dtrace=$1
+usdt=$2
+mapping=$3
# Set up test directory.
@@ -17,13 +20,21 @@ cd $DIRNAME
# Create test source files.
-cat > prov.d <<EOF
+if [[ -z $mapping ]]; then
+ cat > prov.d <<EOF
provider pyramid {
probe entry(int, char, int, int);
};
EOF
+else
+ cat > prov.d <<EOF
+provider pyramid {
+ probe entry(int a, char b, int c, int d) : (int c, int d, int a, char b);
+};
+EOF
+fi
-cat > main.c <<EOF
+cat > main.c <<'EOF'
#include <stdio.h>
#include "prov.h"
@@ -48,7 +59,7 @@ EOF
# Build the test program.
-$dtrace -h -s prov.d
+$dtrace $dt_flags -h -s prov.d
if [ $? -ne 0 ]; then
echo "failed to generate header file" >&2
exit 1
@@ -61,12 +72,12 @@ fi
if [[ `uname -m` = "aarch64" ]]; then
objdump -d main.o > disasm_foo.txt.before
fi
-$dtrace -G -64 -s prov.d main.o
+$dtrace $dt_flags -G -64 -s prov.d main.o
if [ $? -ne 0 ]; then
echo "failed to create DOF" >&2
exit 1
fi
-cc $test_cppflags -o main main.o prov.o
+cc $test_ldflags -o main main.o prov.o
if [ $? -ne 0 ]; then
echo "failed to link final executable" >&2
exit 1
@@ -75,7 +86,7 @@ fi
# Check that the program output is 0 when the USDT probe is not enabled.
# That is, the PYRAMID_ENTRY_ENABLED() is-enabled checks should not pass.
-./main > main.out
+./main standalone > main.out
echo "my result: 0" > main.out.expected
if ! diff -q main.out main.out.expected > /dev/null; then
echo '"my result"' looks wrong when not using DTrace
@@ -88,11 +99,25 @@ fi
# Run dtrace.
-$dtrace $dt_flags -q -c ./main -o dtrace.out -n '
+cat >> pidprobes.d <<'EOF'
p*d$target::foo:
{
printf("%d %s:%s:%s:%s %x\n", pid, probeprov, probemod, probefunc, probename, uregs[R_PC]);
-}' > main.out2
+}
+EOF
+
+if [[ -n $usdt ]]; then
+ echo 'pyramid$target::foo: {' >> pidprobes.d
+
+ if [[ -n $mapping ]]; then
+ echo 'printf("%d %s:%s:%s:%s %i %i %i %c\n", pid, probeprov, probemod, probefunc, probename, args[0], args[1], args[2], args[3]);' >> pidprobes.d
+ else
+ echo 'printf("%d %s:%s:%s:%s %i %c %i %i\n", pid, probeprov, probemod, probefunc, probename, args[0], args[1], args[2], args[3]);' >> pidprobes.d
+ fi
+ echo '}' >> pidprobes.d
+fi
+
+$dtrace $dt_flags -q -c ./main -o dtrace.out -s pidprobes.d > main.out2
if [ $? -ne 0 ]; then
echo "failed to run dtrace" >&2
cat main.out2
@@ -271,8 +296,17 @@ for pc in $pcs; do
done
echo $usdt_pcs | awk '{printf("'$pid' pyramid'$pid':main:foo:entry %x\n", $1);}' >> dtrace.out.expected
echo $usdt_pcs | awk '{printf("'$pid' pyramid'$pid':main:foo:entry %x\n", $2);}' >> dtrace.out.expected
+if [[ -n $usdt ]]; then
+ if [[ -z $mapping ]]; then
+ echo "$pid pyramid$pid:main:foo:entry 2 a 16 128" >> dtrace.out.expected
+ echo "$pid pyramid$pid:main:foo:entry 4 b 32 256" >> dtrace.out.expected
+ else
+ echo "$pid pyramid$pid:main:foo:entry 16 128 2 a" >> dtrace.out.expected
+ echo "$pid pyramid$pid:main:foo:entry 32 256 4 b" >> dtrace.out.expected
+ fi
+fi
-# Sort and check.
+# Sort and check (dropping any wake-up firings from deferred probing).
sort dtrace.out > dtrace.out.sorted
sort dtrace.out.expected > dtrace.out.expected.sorted
diff --git a/test/unittest/usdt/tst.pidargmap.sh b/test/unittest/usdt/tst.pidargmap.sh
new file mode 100755
index 000000000000..0c83f8703539
--- /dev/null
+++ b/test/unittest/usdt/tst.pidargmap.sh
@@ -0,0 +1,11 @@
+#!/bin/bash
+#
+# 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.
+#
+# This test verifies that USDT and pid probes that share underlying probes
+# do not apply arg mappings (incorrectly) to the pid probes.
+
+exec $(dirname $_test)/pidprobes.sh $1 t t
diff --git a/test/unittest/usdt/tst.pidargs.sh b/test/unittest/usdt/tst.pidargs.sh
new file mode 100755
index 000000000000..53cba7c39624
--- /dev/null
+++ b/test/unittest/usdt/tst.pidargs.sh
@@ -0,0 +1,11 @@
+#!/bin/bash
+#
+# 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.
+#
+# This test verifies that USDT and pid probes that share underlying probes
+# get the arguments correct for the USDT probes.
+
+exec $(dirname $_test)/pidprobes.sh $1 t ""
diff --git a/test/unittest/usdt/tst.pidprobes.sh b/test/unittest/usdt/tst.pidprobes.sh
index 8d18c89a4aec..e63d9f5dc096 100755
--- a/test/unittest/usdt/tst.pidprobes.sh
+++ b/test/unittest/usdt/tst.pidprobes.sh
@@ -7,286 +7,4 @@
#
# This test verifies that USDT and pid probes can share underlying probes.
-dtrace=$1
-
-# Set up test directory.
-
-DIRNAME=$tmpdir/pidprobes.$$.$RANDOM
-mkdir -p $DIRNAME
-cd $DIRNAME
-
-# Create test source files.
-
-cat > prov.d <<EOF
-provider pyramid {
- probe entry(int, char, int, int);
-};
-EOF
-
-cat > main.c <<EOF
-#include <stdio.h>
-#include "prov.h"
-
-void foo() {
- int n = 0;
-
- PYRAMID_ENTRY(2, 'a', 16, 128);
- if (PYRAMID_ENTRY_ENABLED())
- n += 2;
- PYRAMID_ENTRY(4, 'b', 32, 256);
- if (PYRAMID_ENTRY_ENABLED())
- n += 8;
- printf("my result: %d\n", n);
-}
-
-int
-main(int argc, char **argv)
-{
- foo();
-}
-EOF
-
-# Build the test program.
-
-$dtrace -h -s prov.d
-if [ $? -ne 0 ]; then
- echo "failed to generate header file" >&2
- exit 1
-fi
-cc $test_cppflags -c main.c
-if [ $? -ne 0 ]; then
- echo "failed to compile test" >&2
- exit 1
-fi
-if [[ `uname -m` = "aarch64" ]]; then
- objdump -d main.o > disasm_foo.txt.before
-fi
-$dtrace -G -64 -s prov.d main.o
-if [ $? -ne 0 ]; then
- echo "failed to create DOF" >&2
- exit 1
-fi
-cc $test_cppflags -o main main.o prov.o
-if [ $? -ne 0 ]; then
- echo "failed to link final executable" >&2
- exit 1
-fi
-
-# Check that the program output is 0 when the USDT probe is not enabled.
-# That is, the PYRAMID_ENTRY_ENABLED() is-enabled checks should not pass.
-
-./main > main.out
-echo "my result: 0" > main.out.expected
-if ! diff -q main.out main.out.expected > /dev/null; then
- echo '"my result"' looks wrong when not using DTrace
- echo === got ===
- cat main.out
- echo === expected ===
- cat main.out.expected
- exit 1
-fi
-
-# Run dtrace.
-
-$dtrace $dt_flags -q -c ./main -o dtrace.out -n '
-p*d$target::foo:
-{
- printf("%d %s:%s:%s:%s %x\n", pid, probeprov, probemod, probefunc, probename, uregs[R_PC]);
-}' > main.out2
-if [ $? -ne 0 ]; then
- echo "failed to run dtrace" >&2
- cat main.out2
- cat dtrace.out
- exit 1
-fi
-echo "my result: 10" > main.out2.expected
-if ! diff -q main.out2 main.out2.expected > /dev/null; then
- echo '"my result"' looks wrong when using DTrace
- echo === got ===
- cat main.out2
- echo === expected ===
- cat main.out2.expected
- exit 1
-fi
-
-# Check that the program output is 10 when the USDT probe is enabled.
-# That is, the PYRAMID_ENTRY_ENABLED() is-enabled checks should pass.
-
-echo "my result: 10" > main.out2.expected
-
-if ! diff -q main.out2 main.out2.expected > /dev/null; then
- echo '"my result"' looks wrong
- echo === got ===
- cat main.out2
- echo === expected ===
- cat main.out2.expected
- exit 1
-fi
-
-# Get the reported pid.
-
-if [ `awk 'NF != 0 { print $1 }' dtrace.out | uniq | wc -l` -ne 1 ]; then
- echo no unique pid
- cat dtrace.out
- exit 1
-fi
-pid=`awk 'NF != 0 { print $1 }' dtrace.out | uniq`
-
-# Disassemble foo().
-
-objdump -d main | awk '
-BEGIN { use = 0 } # start by not printing lines
-use == 1 && NF == 0 { exit } # if printing lines but hit a blank, then exit
-use == 1 { print } # print lines
-/<foo>:/ { use = 1 } # turn on printing when we hit "<foo>:" (without printing this line itself)
-' > disasm_foo.txt
-
-# From the disassembly, get the PCs for foo()'s instructions.
-
-pcs=`awk '{print strtonum("0x"$1)}' disasm_foo.txt`
-pc0=`echo $pcs | awk '{print $1}'`
-
-# From the disassembly, get the PCs for USDT probes.
-# Check libdtrace/dt_link.c's arch-dependent dt_modtext() to see
-# what sequence of instructions signal a USDT probe.
-
-if [[ `uname -m` = "x86_64" ]]; then
-
- # It is the first of five nop instructions in a row.
- # So track pc[-6], pc[-5], pc[-4], pc[-3], pc[-2], pc[-1], pc[0]
- # as well as whether they are nop.
-
- usdt_pcs_all=`awk '
- BEGIN {
- pc6 = -1; is_nop6 = 0;
- pc5 = -1; is_nop5 = 0;
- pc4 = -1; is_nop4 = 0;
- pc3 = -1; is_nop3 = 0;
- pc2 = -1; is_nop2 = 0;
- pc1 = -1; is_nop1 = 0;
- }
- {
- # pc0 is current instruction
- pc0 = strtonum("0x"$1);
-
- # decide whether it is a nop
- is_nop0 = 0;
- if (NF == 3 &&
- $2 == "90" &&
- $3 == "nop")
- is_nop0 = 1;
-
- # report if pc[-5] is a USDT instruction
- if (is_nop6 == 0 &&
- is_nop5 == 1 &&
- is_nop4 == 1 &&
- is_nop3 == 1 &&
- is_nop2 == 1 &&
- is_nop1 == 1 &&
- is_nop0 == 0)
- print pc5;
-
- # prepare advance to next instruction
- pc6 = pc5; is_nop6 = is_nop5;
- pc5 = pc4; is_nop5 = is_nop4;
- pc4 = pc3; is_nop4 = is_nop3;
- pc3 = pc2; is_nop3 = is_nop2;
- pc2 = pc1; is_nop2 = is_nop1;
- pc1 = pc0; is_nop1 = is_nop0;
- }' disasm_foo.txt`
-
- # We expect 4 USDT probes (2 USDT and 2 is-enabled).
- if [ `echo $usdt_pcs_all | awk '{print NF}'` -ne 4 ]; then
- echo ERROR: expected 4 USDT probes but got $usdt_pcs_all
- cat disasm_foo.txt
- exit 1
- fi
-
- # Separate them into regular and is-enabled PCs.
- # We assume they alternate.
- usdt_pcs=`echo $usdt_pcs_all | awk '{ print $1, $3 }'`
- usdt_pcs_isenabled=`echo $usdt_pcs_all | awk '{ print $2, $4 }'`
-
-elif [[ `uname -m` = "aarch64" ]]; then
-
- # The initial compilation of foo() makes it obvious where the
- # USDT probes are. We just have to add the function offset in.
- usdt_pcs=`awk '/<__dtrace_pyramid___entry>/ { print strtonum("0x"$1) + '$pc0' }' disasm_foo.txt.before`
- usdt_pcs_isenabled=`awk '/<__dtraceenabled_pyramid___entry>/ { print strtonum("0x"$1) + '$pc0' }' disasm_foo.txt.before`
-
- # We expect 4 USDT probes (2 USDT and 2 is-enabled).
- if [ `echo $usdt_pcs | awk '{print NF}'` -ne 2 -o \
- `echo $usdt_pcs_isenabled | awk '{print NF}'` -ne 2 ]; then
- echo ERROR: expected 4 USDT probes but got $usdt_pcs and $usdt_pcs_isenabled
- cat disasm_foo.txt.before
- exit 1
- fi
-
-else
- echo ERROR unrecognized machine hardware name
- exit 1
-fi
-
-# We expect all of the USDT probe PCs to be among the PCs in objdump output.
-
-for pc in $usdt_pcs $usdt_pcs_isenabled; do
- if echo $pcs | grep -q -vw $pc ; then
- echo ERROR: cannot find USDT PC $pc in $pcs
- exit 1
- fi
-done
-
-# Get the PC for the pid return probe. (Just keep it in hex.)
-
-pc_return=`awk '/'$pid' pid'$pid':main:foo:return/ { print $NF }' dtrace.out`
-
-objdump -d main | awk '
-/^[0-9a-f]* <.*>:$/ { myfunc = $NF } # enter a new function
-/^ *'$pc_return'/ { print myfunc; exit(0) } # report the function $pc_return is in
-' > return_func.out
-
-echo "<main>:" > return_func.out.expected # since we use uretprobe for pid return probes, the PC will be in the caller
-
-if ! diff -q return_func.out return_func.out.expected > /dev/null; then
- echo ERROR: return PC looks to be in the wrong function
- echo === got ===
- cat return_func.out
- echo === expected ===
- cat return_func.out.expected
- exit 1
-fi
-
-# Build up a list of expected dtrace output:
-# - a blank line
-# - pid entry
-# - pid return
-# - pid offset
-# - two USDT probes (ignore is-enabled probes)
-
-echo > dtrace.out.expected
-printf "$pid pid$pid:main:foo:entry %x\n" $pc0 >> dtrace.out.expected
-echo "$pid pid$pid:main:foo:return $pc_return" >> dtrace.out.expected
-for pc in $pcs; do
- printf "$pid pid$pid:main:foo:%x %x\n" $(($pc - $pc0)) $pc >> dtrace.out.expected
-done
-echo $usdt_pcs | awk '{printf("'$pid' pyramid'$pid':main:foo:entry %x\n", $1);}' >> dtrace.out.expected
-echo $usdt_pcs | awk '{printf("'$pid' pyramid'$pid':main:foo:entry %x\n", $2);}' >> dtrace.out.expected
-
-# Sort and check.
-
-sort dtrace.out > dtrace.out.sorted
-sort dtrace.out.expected > dtrace.out.expected.sorted
-
-if ! diff -q dtrace.out.sorted dtrace.out.expected.sorted ; then
- echo ERROR: dtrace output looks wrong
- echo === got ===
- cat dtrace.out.sorted
- echo === expected ===
- cat dtrace.out.expected.sorted
- echo === diff ===
- diff dtrace.out.sorted dtrace.out.expected.sorted
- exit 1
-fi
-
-echo success
-exit 0
+exec $(dirname $_test)/pidprobes.sh $1 "" ""
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v5 6/6] usdt: fix create_underlying error path
2024-11-05 0:06 [PATCH v5 0/6] usdt typed args, translators and arg mapping Nick Alcock
` (4 preceding siblings ...)
2024-11-05 0:06 ` [PATCH v5 5/6] usdt: new tests for USDT arg sanity with overlapping pid probes Nick Alcock
@ 2024-11-05 0:06 ` Nick Alcock
5 siblings, 0 replies; 15+ messages in thread
From: Nick Alcock @ 2024-11-05 0:06 UTC (permalink / raw)
To: dtrace, dtrace-devel; +Cc: Kris Van Hees
On error, we were destroying the underlying probe using the wrong call,
as if it were an overlying probe.
Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
---
libdtrace/dt_prov_uprobe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index 28f5351be21a..5882644e03e9 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -716,7 +716,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
fail:
dt_dprintf("Failed to instantiate %s:%s:%s:%s\n", psp->pps_prv,
psp->pps_mod, psp->pps_fn, psp->pps_prb);
- probe_destroy(dtp, upp);
+ probe_destroy_underlying(dtp, upp);
return NULL;
}
--
2.46.0.278.g36e3a12567
^ permalink raw reply related [flat|nested] 15+ messages in thread