* [PATCH v4 1/5] usdt: get arg types and xlations into DTrace from the DOF
2024-11-01 15:57 [PATCH v4 0/5] usdt typed args, translators and arg remapping Nick Alcock
@ 2024-11-01 15:57 ` Nick Alcock
2024-11-01 23:46 ` [DTrace-devel] " Kris Van Hees
2024-11-01 15:57 ` [PATCH v4 2/5] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c Nick Alcock
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Nick Alcock @ 2024-11-01 15:57 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..dd3a6cd2b887 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 remapping 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] 13+ messages in thread* Re: [DTrace-devel] [PATCH v4 1/5] usdt: get arg types and xlations into DTrace from the DOF
2024-11-01 15:57 ` [PATCH v4 1/5] usdt: get arg types and xlations into DTrace from the DOF Nick Alcock
@ 2024-11-01 23:46 ` Kris Van Hees
2024-11-02 0:14 ` Kris Van Hees
0 siblings, 1 reply; 13+ messages in thread
From: Kris Van Hees @ 2024-11-01 23:46 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
Two comments below (one tiny thing, and one explanatory comment that I *think*
I have a better suggestion for, but you may want to rephrase it).
On Fri, Nov 01, 2024 at 03:57:08PM +0000, Nick Alcock via DTrace-devel 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>
> ---
> 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..dd3a6cd2b887 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 remapping table. */
remapping -> mapping
> +
> + 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.
This comment is confusing... I think it might be better served by really
spelling out which is which. I.e. each of the xargc elements specifies the
native arg index that the translated arg gets its value from.
> + */
> + 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] 13+ messages in thread* Re: [DTrace-devel] [PATCH v4 1/5] usdt: get arg types and xlations into DTrace from the DOF
2024-11-01 23:46 ` [DTrace-devel] " Kris Van Hees
@ 2024-11-02 0:14 ` Kris Van Hees
2024-11-04 15:26 ` Nick Alcock
0 siblings, 1 reply; 13+ messages in thread
From: Kris Van Hees @ 2024-11-02 0:14 UTC (permalink / raw)
To: Kris Van Hees; +Cc: Nick Alcock, dtrace, dtrace-devel
On Fri, Nov 01, 2024 at 07:46:04PM -0400, Kris Van Hees via DTrace-devel wrote:
> Two comments below (one tiny thing, and one explanatory comment that I *think*
> I have a better suggestion for, but you may want to rephrase it).
Actually, another issue (see below).
> On Fri, Nov 01, 2024 at 03:57:08PM +0000, Nick Alcock via DTrace-devel 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>
> > ---
> > 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..dd3a6cd2b887 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;
> > +}
What if the strtab blob does not contain enough strings? There is no check
here to ensure that you do not start reading past the end of data?
> > +
> > 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);
No validation that there are nargc type strings.
> > + 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);
No validation that there are nargc type strings.
> > + 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 remapping table. */
>
> remapping -> mapping
>
> > +
> > + 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);
Is there any validation anywhere that there are map_size bytes to read from
dhpb->dthpb_args? Is there any validation anywhere here (or in a later patch)
that the entries are valid (between 0 and nargc)?
> > + 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.
>
> This comment is confusing... I think it might be better served by really
> spelling out which is which. I.e. each of the xargc elements specifies the
> native arg index that the translated arg gets its value from.
>
> > + */
> > + 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
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [DTrace-devel] [PATCH v4 1/5] usdt: get arg types and xlations into DTrace from the DOF
2024-11-02 0:14 ` Kris Van Hees
@ 2024-11-04 15:26 ` Nick Alcock
0 siblings, 0 replies; 13+ messages in thread
From: Nick Alcock @ 2024-11-04 15:26 UTC (permalink / raw)
To: Kris Van Hees; +Cc: dtrace, dtrace-devel
On 2 Nov 2024, Kris Van Hees uttered the following:
> On Fri, Nov 01, 2024 at 07:46:04PM -0400, Kris Van Hees via DTrace-devel wrote:
>> Two comments below (one tiny thing, and one explanatory comment that I *think*
>> I have a better suggestion for, but you may want to rephrase it).
>
> Actually, another issue (see below).
>
>> On Fri, Nov 01, 2024 at 03:57:08PM +0000, Nick Alcock via DTrace-devel wrote:
>> > +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;
>> > +}
>
> What if the strtab blob does not contain enough strings? There is no check
> here to ensure that you do not start reading past the end of data?
Then we run off the end! We rely on validation already having been done
by validate_provider() (which checks all this stuff), and if that is
buggy we crash and dtprobed restarts us a few times, gives up, logs a
message and rejects the probes. That's the worst that can happen.
We run in a seccomped jail specifically so we don't need to worry about
things like this :)
>> > + if (dhpb->dthpb_nargc > 0) {
>> > + size_t nargs_size;
>> > +
>> > + nargs_size = strings_len(dhpb->dthpb_ntypes, dhpb->dthpb_nargc);
>
> No validation that there are nargc type strings.
>
>> > + xargs_size = strings_len(dhpb->dthpb_xtypes,
>> > + dhpb->dthpb_xargc);
>
> No validation that there are nargc type strings.
See above.
>> > + 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 remapping table. */
>>
>> remapping -> mapping
... I swear I fixed that. Fixed again.
>> > + msg->size = msg_size;
>> > + msg->type = DIT_ARGS_MAP;
>> > + memcpy(msg->argmap.argmap, dhpb->dthpb_args, map_size);
>
> Is there any validation anywhere that there are map_size bytes to read from
> dhpb->dthpb_args? Is there any validation anywhere here (or in a later patch)
> that the entries are valid (between 0 and nargc)?
Yep! See above: validate_provider checks all of this (you probably
missed it because it's done under another name: see the end of
emit_provider for where we shuffle things into their new homes.)
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 2/5] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c
2024-11-01 15:57 [PATCH v4 0/5] usdt typed args, translators and arg remapping Nick Alcock
2024-11-01 15:57 ` [PATCH v4 1/5] usdt: get arg types and xlations into DTrace from the DOF Nick Alcock
@ 2024-11-01 15:57 ` Nick Alcock
2024-11-01 15:57 ` [PATCH v4 3/5] cg: add argument mapping in the trampoline Nick Alcock
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Nick Alcock @ 2024-11-01 15:57 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] 13+ messages in thread* [PATCH v4 3/5] cg: add argument mapping in the trampoline
2024-11-01 15:57 [PATCH v4 0/5] usdt typed args, translators and arg remapping Nick Alcock
2024-11-01 15:57 ` [PATCH v4 1/5] usdt: get arg types and xlations into DTrace from the DOF Nick Alcock
2024-11-01 15:57 ` [PATCH v4 2/5] dtprobed: stop skipping zero-tracepoint probes in dof_stash.c Nick Alcock
@ 2024-11-01 15:57 ` Nick Alcock
2024-11-02 0:27 ` [DTrace-devel] " Kris Van Hees
2024-11-01 15:57 ` [PATCH v4 4/5] usdt: typed args and arg mapping Nick Alcock
2024-11-01 15:57 ` [PATCH v4 5/5] usdt: fix create_underlying error path Nick Alcock
4 siblings, 1 reply; 13+ messages in thread
From: Nick Alcock @ 2024-11-01 15:57 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 | 36 +++++++++++++++++++++++++++++++++++-
libdtrace/dt_cg.h | 1 +
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 39c27ab0ec0b..b90757bcb5cb 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -831,6 +831,35 @@ 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 overwritten if necessary (but saved). 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;
+
+ /*
+ * Work from saved args so we don't need to worry about overwriting regs
+ * we're about to map into different positions.
+ */
+ dt_cg_tramp_save_args(pcb);
+
+ 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;
@@ -5060,7 +5089,12 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
* array index according to the static argument mapping (if any),
* 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 dt_cg_tramp_map_args is in use, you should apply the mapping at
+ * the probe_info stage, since the effective "native" arg positions will
+ * be changed.
+ *
+ * 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] 13+ messages in thread* Re: [DTrace-devel] [PATCH v4 3/5] cg: add argument mapping in the trampoline
2024-11-01 15:57 ` [PATCH v4 3/5] cg: add argument mapping in the trampoline Nick Alcock
@ 2024-11-02 0:27 ` Kris Van Hees
2024-11-04 16:41 ` Nick Alcock
0 siblings, 1 reply; 13+ messages in thread
From: Kris Van Hees @ 2024-11-02 0:27 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
On Fri, Nov 01, 2024 at 03:57:10PM +0000, Nick Alcock via DTrace-devel 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 | 36 +++++++++++++++++++++++++++++++++++-
> libdtrace/dt_cg.h | 1 +
> 2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 39c27ab0ec0b..b90757bcb5cb 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -831,6 +831,35 @@ 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 overwritten if necessary (but saved). 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;
> +
> + /*
> + * Work from saved args so we don't need to worry about overwriting regs
> + * we're about to map into different positions.
> + */
> + dt_cg_tramp_save_args(pcb);
I still maintain that this should be done in the caller. (See patch 4/5 for
more info on that.) As mentioned before, you can mention in the comment before
this function that the caller must save the arguments, just like you already
mention that %r7 must contain a value set by dt_cg_tramp_prologue*().
> +
> + 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;
> @@ -5060,7 +5089,12 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> * array index according to the static argument mapping (if any),
> * 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.
You dropped the end of a sentence without replacing it with anything?
> + *
> + * If dt_cg_tramp_map_args is in use, you should apply the mapping at
> + * the probe_info stage, since the effective "native" arg positions will
> + * be changed.
This does not belong here. If anything, clauses get compiled separete from
trampolines and a given clause (compiled once) may get used in more than one
BPF program, i.e. it may get associated with more than one trampoline. So
you can't really depend on what a specific trampoline does when generating code
for a clause.
> + *
> + * 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
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [DTrace-devel] [PATCH v4 3/5] cg: add argument mapping in the trampoline
2024-11-02 0:27 ` [DTrace-devel] " Kris Van Hees
@ 2024-11-04 16:41 ` Nick Alcock
0 siblings, 0 replies; 13+ messages in thread
From: Nick Alcock @ 2024-11-04 16:41 UTC (permalink / raw)
To: Kris Van Hees; +Cc: dtrace, dtrace-devel
On 2 Nov 2024, Kris Van Hees uttered the following:
> On Fri, Nov 01, 2024 at 03:57:10PM +0000, Nick Alcock via DTrace-devel wrote:
>> + /*
>> + * Work from saved args so we don't need to worry about overwriting regs
>> + * we're about to map into different positions.
>> + */
>> + dt_cg_tramp_save_args(pcb);
>
> I still maintain that this should be done in the caller. (See patch 4/5 for
> more info on that.) As mentioned before, you can mention in the comment before
> this function that the caller must save the arguments, just like you already
> mention that %r7 must contain a value set by dt_cg_tramp_prologue*().
I think this is gross and just asking for trouble later, but fixed for
the sake of avoiding argument.
>> @@ -5060,7 +5089,12 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>> * array index according to the static argument mapping (if any),
>> * 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.
>
> You dropped the end of a sentence without replacing it with anything?
Argh! Fixed.
>> + *
>> + * If dt_cg_tramp_map_args is in use, you should apply the mapping at
>> + * the probe_info stage, since the effective "native" arg positions will
>> + * be changed.
>
> This does not belong here. If anything, clauses get compiled separete from
> trampolines and a given clause (compiled once) may get used in more than one
> BPF program, i.e. it may get associated with more than one trampoline. So
> you can't really depend on what a specific trampoline does when generating code
> for a clause.
... well, we *do*. All over the place.
But instructions to the caller don't really belong here in any case:
dropped. dt_cg_tramp_map_args's header comment already notes it, and
that's the right place for it.
--
NULL && (void)
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 4/5] usdt: typed args and arg mapping
2024-11-01 15:57 [PATCH v4 0/5] usdt typed args, translators and arg remapping Nick Alcock
` (2 preceding siblings ...)
2024-11-01 15:57 ` [PATCH v4 3/5] cg: add argument mapping in the trampoline Nick Alcock
@ 2024-11-01 15:57 ` Nick Alcock
2024-11-02 1:09 ` Kris Van Hees
2024-11-01 15:57 ` [PATCH v4 5/5] usdt: fix create_underlying error path Nick Alcock
4 siblings, 1 reply; 13+ messages in thread
From: Nick Alcock @ 2024-11-01 15:57 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>
---
libdtrace/dt_pid.c | 1 +
libdtrace/dt_prov_uprobe.c | 159 +++++++++++++++++-
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 +-
14 files changed, 455 insertions(+), 13 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/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
index 71ac1754d343..189b15f61a89 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))
diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
index a8c1ab2761f0..fd7ad1643946 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -57,7 +57,11 @@ 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 in */
+ char *nargvbuf; /* array of native args */
+ char *xargvbuf; /* array of xlated args */
+ dt_list_t probes; /* pid/USDT probes triggered by it */
} dt_uprobe_t;
typedef struct list_probe {
@@ -123,6 +127,9 @@ 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->nargvbuf);
+ dt_free(dtp, upp->xargvbuf);
dt_free(dtp, upp);
}
@@ -516,6 +523,87 @@ static int discover(dtrace_hdl_t *dtp)
return 0;
}
+/*
+ * Populate args for an underlying probe. This really relates to the overlying
+ * probe (all underlying 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 nargvbuf and xargvbuf arrays, which
+ * are straight copies 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.
+ */
+ if (upp->argc == 0 || psp->pps_xargv == NULL || psp->pps_nargv == NULL
+ || psp->pps_xargvlen == 0 || psp->pps_nargvlen == 0)
+ return 0;
+
+ upp->xargvbuf = dt_alloc(dtp, psp->pps_xargvlen);
+ if(upp->xargvbuf == NULL)
+ return -1;
+ memcpy(upp->xargvbuf, psp->pps_xargv, psp->pps_xargvlen);
+ xptr = upp->xargvbuf;
+
+ upp->nargvbuf = dt_alloc(dtp, psp->pps_nargvlen);
+ if(upp->nargvbuf == NULL)
+ return -1;
+ memcpy(upp->nargvbuf, psp->pps_nargv, psp->pps_nargvlen);
+ nptr = upp->nargvbuf;
+
+ 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.
+ */
+
+ for (i = 0; i < upp->argc; i++) {
+ int map_arg = psp->pps_argmap[i];
+
+ if (xptr) {
+ upp->args[i].native = nargv[map_arg];
+ upp->args[i].xlate = xptr;
+ xptr += strlen(xptr) + 1;
+ } else {
+ upp->args[i].native = nptr;
+ nptr += strlen(nptr) + 1;
+ }
+ upp->args[i].mapping = map_arg;
+ upp->args[i].flags = 0;
+ }
+
+ 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 +618,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
@@ -590,8 +678,11 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
if (upp->tp == NULL)
goto fail;
+ if (populate_args(dtp, psp, upp) < 0)
+ goto fail;
+
uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
- upp);
+ upp);
if (uprp == NULL)
goto fail;
} else
@@ -613,6 +704,8 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
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;
}
@@ -787,6 +880,9 @@ static void enable_usdt(dtrace_hdl_t *dtp, dt_probe_t *prp)
*
* The trampoline will first populate a dt_dctx_t struct. It will then emulate
* the firing of all dependent pid* probes and their clauses.
+ *
+ * uprobe trampolines 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 +960,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,13 +969,19 @@ 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->args != NULL)
+ dt_cg_tramp_map_args(pcb, upp->args, upp->argc);
+
/*
* Retrieve the PID of the process that caused the probe to fire.
*/
emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
- /*
+ /*
* Look up in the BPF 'usdt_prids' map. Space for the look-up key
* will be used on the BPF stack:
*
@@ -1083,8 +1185,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 +1295,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 +1320,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] 13+ messages in thread* Re: [PATCH v4 4/5] usdt: typed args and arg mapping
2024-11-01 15:57 ` [PATCH v4 4/5] usdt: typed args and arg mapping Nick Alcock
@ 2024-11-02 1:09 ` Kris Van Hees
2024-11-04 16:38 ` Nick Alcock
0 siblings, 1 reply; 13+ messages in thread
From: Kris Van Hees @ 2024-11-02 1:09 UTC (permalink / raw)
To: Nick Alcock; +Cc: dtrace, dtrace-devel
On Fri, Nov 01, 2024 at 03:57:11PM +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).
>
> 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.
Two comments:
1) DOF is DTrace. If other things generate DOF then they better make sure they
generate DOF that is compatible with DTrace or else it may not work. It is
not an assumption that multiple USDT probes cannot share an underlying
probe. That is an actual fact due to the implementation of USDT probes.
In other words, there is no assumption here, nor a real TODO. The USDT
provider implementation does not support more than 1 USDT probe per
underlying probe, so you can depend on the arg data stored with the
underlying probe to be truly representative for the overlying USDT probe.
2) Why not validate this in dt_prov_uprobe.c, i.e. when creating the underlying
probe? If it was already created for a USDT probe, then we have an issue
because two USDT probes on the same underlying probe is not supported.
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
> libdtrace/dt_pid.c | 1 +
> libdtrace/dt_prov_uprobe.c | 159 +++++++++++++++++-
> 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 +-
> 14 files changed, 455 insertions(+), 13 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/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> index 71ac1754d343..189b15f61a89 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))
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index a8c1ab2761f0..fd7ad1643946 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -57,7 +57,11 @@ 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 in */
> + char *nargvbuf; /* array of native args */
> + char *xargvbuf; /* array of xlated args */
Not really arrays, though. Just a binary blod of concatenated 0-terminated
strings. Since their content should have been validated by now, why not just
store them as a single concatenated blob, e.g. argdata rather than as two
separate pointers? You never end up using them anyway once the args data is
populated based on the strings. So there is no need to keep it separate.
> + dt_list_t probes; /* pid/USDT probes triggered by it */
> } dt_uprobe_t;
>
> typedef struct list_probe {
> @@ -123,6 +127,9 @@ 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->nargvbuf);
> + dt_free(dtp, upp->xargvbuf);
> dt_free(dtp, upp);
> }
>
> @@ -516,6 +523,87 @@ static int discover(dtrace_hdl_t *dtp)
> return 0;
> }
>
> +/*
> + * Populate args for an underlying probe. This really relates to the overlying
> + * probe (all underlying probes associated with a given underlying probe must
The first 'underlying' here should be 'overlying'.
> + * 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.
You need to be more nuanced though because what you say applies to USDT probes
only. Yet, an underlying probe may have an overlying pid probes associated
with it as well. What you say does not apply to pid probes.
> + *
> + * Move it into dt_argdesc_t's for use later on. The char *'s in that
> + * structure are pointers into the nargvbuf and xargvbuf arrays, which
> + * are straight copies of the nargc/xargc in the pid_probespec_t.
As mentioned above, why not combine the two into a single blob?
> + */
> +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.
> + */
> + if (upp->argc == 0 || psp->pps_xargv == NULL || psp->pps_nargv == NULL
> + || psp->pps_xargvlen == 0 || psp->pps_nargvlen == 0)
> + return 0;
> +
> + upp->xargvbuf = dt_alloc(dtp, psp->pps_xargvlen);
> + if(upp->xargvbuf == NULL)
> + return -1;
> + memcpy(upp->xargvbuf, psp->pps_xargv, psp->pps_xargvlen);
> + xptr = upp->xargvbuf;
> +
> + upp->nargvbuf = dt_alloc(dtp, psp->pps_nargvlen);
> + if(upp->nargvbuf == NULL)
> + return -1;
> + memcpy(upp->nargvbuf, psp->pps_nargv, psp->pps_nargvlen);
> + nptr = upp->nargvbuf;
> +
> + 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.
> + */
> +
> + for (i = 0; i < upp->argc; i++) {
> + int map_arg = psp->pps_argmap[i];
> +
> + if (xptr) {
> + upp->args[i].native = nargv[map_arg];
> + upp->args[i].xlate = xptr;
> + xptr += strlen(xptr) + 1;
> + } else {
> + upp->args[i].native = nptr;
> + nptr += strlen(nptr) + 1;
> + }
> + upp->args[i].mapping = map_arg;
> + upp->args[i].flags = 0;
> + }
> +
> + 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 +618,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
> @@ -590,8 +678,11 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> if (upp->tp == NULL)
> goto fail;
>
> + if (populate_args(dtp, psp, upp) < 0)
> + goto fail;
> +
> uprp = dt_probe_insert(dtp, pvp, pd.prv, pd.mod, pd.fun, pd.prb,
> - upp);
> + upp);
> if (uprp == NULL)
> goto fail;
> } else
> @@ -613,6 +704,8 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> 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;
> }
> @@ -787,6 +880,9 @@ static void enable_usdt(dtrace_hdl_t *dtp, dt_probe_t *prp)
> *
> * The trampoline will first populate a dt_dctx_t struct. It will then emulate
> * the firing of all dependent pid* probes and their clauses.
pid* and USDT probes and their clauses.
> + *
> + * uprobe trampolines will also arrange for argument shuffling if the argmap
> + * array calls for it.
That becomes a problem when a uprobe serves both a USDT probes and a pid
probe. We shouldn't be mapping any arguments for pid probes. No need to have
this comment here. If it needs a xomment, put it below where the mapping is
actually done.
> */
> static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> {
> @@ -864,7 +960,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> }
>
> /*
> - * USDT
> + * USDT.
Why?
> */
>
> /* In some cases, we know there are no USDT probes. */ // FIXME: add more checks
> @@ -873,13 +969,19 @@ 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->args != NULL)
> + dt_cg_tramp_map_args(pcb, upp->args, upp->argc);
> +
I would add a dt_cg_tramp_save_args() here, followed by dt_cg_tramp_map_args(),
because the mapping should act on the saved copy of arguments. But if we need
to do anything else with arguments, we want to be able to restore the original
arguments. That is a function of the trampoline, not of the mapping function.
Incidentally, you could consider adding a flag on the dt_uprobe_t to indicate
whether argument mapping is needed. It is more likely that we have argument
type data without mapping than with mapping, so being able to avoid making a
copy of the args (and going through a loop that won't generate any code) is
worth it. Adding PP_HAS_ARGMAP comes to mind?
> /*
> * Retrieve the PID of the process that caused the probe to fire.
> */
> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
>
> - /*
> + /*
Eeks? Indentation issues again?
> * Look up in the BPF 'usdt_prids' map. Space for the look-up key
> * will be used on the BPF stack:
> *
> @@ -1083,8 +1185,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 +1295,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 +1320,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 [flat|nested] 13+ messages in thread* Re: [PATCH v4 4/5] usdt: typed args and arg mapping
2024-11-02 1:09 ` Kris Van Hees
@ 2024-11-04 16:38 ` Nick Alcock
0 siblings, 0 replies; 13+ messages in thread
From: Nick Alcock @ 2024-11-04 16:38 UTC (permalink / raw)
To: Kris Van Hees; +Cc: dtrace, dtrace-devel
On 2 Nov 2024, Kris Van Hees said:
> On Fri, Nov 01, 2024 at 03:57:11PM +0000, Nick Alcock wrote:
>> 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.
>
> Two comments:
> 1) DOF is DTrace. If other things generate DOF then they better make sure they
> generate DOF that is compatible with DTrace or else it may not work. It is
> not an assumption that multiple USDT probes cannot share an underlying
> probe. That is an actual fact due to the implementation of USDT probes.
I was more noting that this is nowhere validated -- and if we don't
validate it, we cannot control what other programs might choose to
provide. You can say "it may not work" but in practice other generators
are not going to be psychic, and if it works in one test they'll
probably say "that's OK" and release it, and then there are binaries in
the wild with embedded DOF that do whatever wrong thing we're not
thinking of yet :(
More generally, we *really* can't control what *hostile attackers*
provide, and we can assume they'll wriggle straight into any ambiguities
we leave and try to compromise our nice running-as-root DTrace with
their unprivileged binary with maliciously twisted DOF in it.
There is no evil bit we can require them to turn on :)
> In other words, there is no assumption here, nor a real TODO. The USDT
> provider implementation does not support more than 1 USDT probe per
> underlying probe, so you can depend on the arg data stored with the
> underlying probe to be truly representative for the overlying USDT probe.
... again, this is assuming that our DTrace is the only possible
generator of DOF. We're not cryptographically signing it so this is
simply not true: it's the only generator *now*. (And even that isn't
true: there are other DTraces around that also generate DOF, and
cross-compilation is a thing so we might be hit with any of their DOFs
too... and God only knows what they look like after all these years of
divergence.)
(The unlikeliness of this scenario is why I haven't fixed this yet, but
left it as a TODO. But see below!)
> 2) Why not validate this in dt_prov_uprobe.c, i.e. when creating the underlying
> probe? If it was already created for a USDT probe, then we have an issue
> because two USDT probes on the same underlying probe is not supported.
Hmmm... in this specific case that might be OK, simply because it's
probably hard to get hostile state past the reduction to dof_parsed_t,
and if that reduction fails we just get a jailed parser child crash. We
can probably draw a distinction between format invalidities (which are
the sort of thing attackers like to play with, and should be detected by
dof_parser.c) and things that are *semantically* unsupported but
format-valid (like overlapping probes: not sure we have much else yet).
I was worrying that we'd have to check arg types and mappings, etc, but
we can make this trivial with a flag on the underlying probe that is
flipped on when a USDT probe is created: if that flag's on and we try
to make a new USDT probe on it, boom!)
Done, I hope, assuming that idea works.
>> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
>> index a8c1ab2761f0..fd7ad1643946 100644
>> --- a/libdtrace/dt_prov_uprobe.c
>> +++ b/libdtrace/dt_prov_uprobe.c
>> @@ -57,7 +57,11 @@ 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 in */
>> + char *nargvbuf; /* array of native args */
>> + char *xargvbuf; /* array of xlated args */
>
> Not really arrays, though. Just a binary blod of concatenated 0-terminated
> strings.
Yeah, bad name, it's a strtab really. Adjusted.
> Since their content should have been validated by now, why not just
> store them as a single concatenated blob, e.g. argdata rather than as two
> separate pointers? You never end up using them anyway once the args data is
> populated based on the strings. So there is no need to keep it separate.
Mostly because that wasn't clear when I was writing it. Adjusting. It's
basically costless and maybe even slightly clearer. I was worried about
overruns but that is of course impossible.
This code is buggy anyway if xptr isn't set, but that just highlights
that the non-xptr code path can never happen any more: dropped.
>> +/*
>> + * Populate args for an underlying probe. This really relates to the overlying
>> + * probe (all underlying probes associated with a given underlying probe must
>
> The first 'underlying' here should be 'overlying'.
Oh yes.
>> + * 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.
>
> You need to be more nuanced though because what you say applies to USDT probes
> only. Yet, an underlying probe may have an overlying pid probes associated
> with it as well. What you say does not apply to pid probes.
Agreed! I wasn't thinking about them :( hence the bug noted above.
>> + *
>> + * Move it into dt_argdesc_t's for use later on. The char *'s in that
>> + * structure are pointers into the nargvbuf and xargvbuf arrays, which
>> + * are straight copies of the nargc/xargc in the pid_probespec_t.
>
> As mentioned above, why not combine the two into a single blob?
Done!
>> * The trampoline will first populate a dt_dctx_t struct. It will then emulate
>> * the firing of all dependent pid* probes and their clauses.
>
> pid* and USDT probes and their clauses.
Ack.
>> + *
>> + * uprobe trampolines will also arrange for argument shuffling if the argmap
>> + * array calls for it.
>
> That becomes a problem when a uprobe serves both a USDT probes and a pid
> probe. We shouldn't be mapping any arguments for pid probes. No need to have
> this comment here. If it needs a xomment, put it below where the mapping is
> actually done.
This comment is simply wrong now -- it does a lot more than just emulate
firing, it copies in things for is-enabled probes, shuffles args etc.
Far from removing... augmented.
>> */
>> static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>> {
>> @@ -864,7 +960,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>> }
>>
>> /*
>> - * USDT
>> + * USDT.
>
> Why?
Because 'pid probes' above has a trailing .
A foolish consistency :)
>> @@ -873,13 +969,19 @@ 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->args != NULL)
>> + dt_cg_tramp_map_args(pcb, upp->args, upp->argc);
>> +
>
> I would add a dt_cg_tramp_save_args() here, followed by dt_cg_tramp_map_args(),
> because the mapping should act on the saved copy of arguments. But if we need
> to do anything else with arguments, we want to be able to restore the original
> arguments. That is a function of the trampoline, not of the mapping function.
... ok? I still think this is wildly less clear and adds unnecessary
coupling between dt_cg_tramp_map_args() and its caller (since
dt_cg_tramp_map_args() now silently misbehaves if you don't save first!
or more likely causes a verifier failure), but since the plan is to get
rid of it eventually, I don't care enough to argue.
> Incidentally, you could consider adding a flag on the dt_uprobe_t to indicate
> whether argument mapping is needed. It is more likely that we have argument
> type data without mapping than with mapping, so being able to avoid making a
> copy of the args (and going through a loop that won't generate any code) is
> worth it. Adding PP_HAS_ARGMAP comes to mind?
I took that out because you said it was unnecessary! Adding back. (I too
thought it was unnecessary because no code was generated, but of course
saving the args generates a bunch of code...)
(To me, this is extra evidence that the right place to do all this is in
dt_cg_tramp_map_args(), since it *already verifies* exactly that
remappings are not identity mappings and this would just need to move up
a bit, but as noted I don't care enough to argue. The optimally
efficient approach would be to save only those args that are in the
from- or to-remappings somewhere, but that's probably ridiculous
over-optimization. It only comes down to one or two lines in the end.)
>> /*
>> * Retrieve the PID of the process that caused the probe to fire.
>> */
>> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
>> emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
>>
>> - /*
>> + /*
>
> Eeks? Indentation issues again?
Ugh! Why didn't git log --check tell me about that? sigh.
--
NULL && (void)
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 5/5] usdt: fix create_underlying error path
2024-11-01 15:57 [PATCH v4 0/5] usdt typed args, translators and arg remapping Nick Alcock
` (3 preceding siblings ...)
2024-11-01 15:57 ` [PATCH v4 4/5] usdt: typed args and arg mapping Nick Alcock
@ 2024-11-01 15:57 ` Nick Alcock
4 siblings, 0 replies; 13+ messages in thread
From: Nick Alcock @ 2024-11-01 15:57 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 fd7ad1643946..6132871a9007 100644
--- a/libdtrace/dt_prov_uprobe.c
+++ b/libdtrace/dt_prov_uprobe.c
@@ -706,7 +706,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] 13+ messages in thread