[DTrace-devel] [PATCH v3] Add support for strtok() subroutine
Eugene Loh
eugene.loh at oracle.com
Sat Dec 4 00:47:10 UTC 2021
I'll post v4 in a second. It incorporates your feedback with one
exception below that we can discuss further...
On 12/3/21 5:28 PM, Kris Van Hees wrote:
> On Thu, Dec 02, 2021 at 03:18:08PM -0500, eugene.loh--- via DTrace-devel wrote:
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> @@ -3872,6 +3875,104 @@ dt_cg_subr_strstr(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>> +static void
>> +dt_cg_subr_strtok(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>> +{
>> + dtrace_hdl_t *dtp = yypcb->pcb_hdl;
>> + dt_node_t *str = dnp->dn_args;
>> + dt_node_t *del = str->dn_list;
>> + dt_ident_t *idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_strtok");
>> + uint64_t off;
>> +
>> + assert(idp != NULL);
>> +
>> + TRACE_REGSET(" subr-strtok:Begin");
>> +
>> + /* static check for NULL string */
>> + if (str->dn_op != DT_TOK_INT || str->dn_value != 0) {
>> + /* string is present: copy it to internal state */
>> + dt_cg_node(str, dlp, drp);
> You need a dt_cg_check_not_null() here. Consider the (crazy) case:
> dtrace -n 'BEGIN { a = 1; a--; strtok((char *)a, "a"); exit(0); }'
Yup. And lots of less crazy cases. Anyhow, fixed and new test added.
>> +
>> + if (dt_regset_xalloc_args(drp) == -1)
>> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>> +
>> + /* set flag that the state is initialized */
>> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
>> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_MST));
>> + emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_1, DMST_STRTOK, 1));
>> +
>> + /* the 8-byte prefix is the offset, which we initialize to 0 */
>> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
>> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_MEM));
>> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DMEM_STRTOK));
> Together with accessing the strtok state above to initialize the state, here,
> and further use of the state below, you should be able to allocate a register
> to store the pointer to the strtok state in and use that rather than doing the
> dereferencing and calculation of the pointer multiple times. Since you drop
> registers as soon as you are done with them, I am pretty certain that your code
> here allows the use of an additional register.
This is the exception where I feel hesitation.
First of all, there is no longer any need to initialize the "state"
since that's done automatically when offset is set to 0. So, the
pointer is calculated only twice. And that calculation is only a few
instructions, which you lose back in case a register is spilled/filled.
Most of all, I have nagging concerns about our register management. So,
it just feels "penny wise" to worry about 2 instructions (or whatever)
here... particularly when this is calling strtok.S, which is a beast.
The small savings will never be detectable.
But let me know if you feel strongly about this.
Anyhow, v4 coming up. It differs from v3 in that it adds
test/unittest/funcs/strtok/ tests:
err.D_PROTO_ARG.strtokbad[del|str].[d|r]
err.D_PROTO_LEN.strtoktoo[few|many].[d|r]
tst.strtok_null[del|str|str2].[d|r]
Further, it incorporates your other feedback:
diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
@@ -293,7 +293,8 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
* multiple of 8
* - the greater of:
* - the maximum stack trace size
- * - the size for all tstrings
+ * - DT_TSTRING_SLOTS times the maximum space needed to
+ * store a string
* - size of the internal strtok() state
* - 8 bytes for the offset index
* - the maximum string size
diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
@@ -119,8 +119,6 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb,
dt_activity_t act)
* dctx.mst->prid = PRID; // stw [%r7 + DMST_PRID], PRID
* dctx.mst->syscall_errno = 0;
* // stw [%r7 + DMST_ERRNO], 0
- * dctx.mst->strtok_init = 0;
- * // stw [%r7 + DMST_STRTOK], 0
*/
emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_FP, DCTX_FP(DCTX_MST), 0));
dt_cg_xsetx(dlp, mem, DT_LBL_NONE, BPF_REG_1, mem->di_id);
@@ -132,7 +130,6 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb,
dt_activity_t act)
emit(dlp, BPF_STORE(BPF_DW, BPF_REG_FP, DCTX_FP(DCTX_MST),
BPF_REG_7));
emite(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_7, DMST_PRID, -1), prid);
emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_7, DMST_ERRNO, 0));
- emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_7, DMST_STRTOK, 0));
/*
* buf = rc + roundup(sizeof(dt_mstate_t), 8);
@@ -157,6 +154,12 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb,
dt_activity_t act)
emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0,
roundup(dtp->dt_maxreclen, 8)));
emit(dlp, BPF_STORE(BPF_DW, BPF_REG_FP, DCTX_FP(DCTX_MEM),
BPF_REG_0));
+ /*
+ * Store -1 to the strtok internal-state offset to indicate
+ * that strtok internal state is not yet initialized.
+ */
+ emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_0, DMEM_STRTOK, -1));
+
/*
* Store pointer to BPF map "name" in the DTrace context field
"fld" at
* "offset".
@@ -3875,8 +3878,6 @@ dt_cg_subr_strstr(dt_node_t *dnp, dt_irlist_t
*dlp, dt_regset_t *drp)
TRACE_REGSET(" subr-strstr:End ");
}
-#define DMEM_STRTOK MAX(sizeof(uint64_t) *
dtp->dt_options[DTRACEOPT_MAXFRAMES], \
- DT_TSTRING_SLOTS * roundup(DT_STRLEN_BYTES +
dtp->dt_options[DTRACEOPT_STRSIZE] + 1, 8))
static void
dt_cg_subr_strtok(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
{
@@ -3890,19 +3891,20 @@ dt_cg_subr_strtok(dt_node_t *dnp, dt_irlist_t
*dlp, dt_regset_t *drp)
TRACE_REGSET(" subr-strtok:Begin");
- /* static check for NULL string */
+ /*
+ * Start with a static check for a NULL string.
+ * That is, strtok(NULL, foo) has a special meaning:
+ * continue parsing the previous string. In contrast,
+ * strtok(str, foo) (even if str==NULL) means to parse str.
+ */
if (str->dn_op != DT_TOK_INT || str->dn_value != 0) {
/* string is present: copy it to internal state */
dt_cg_node(str, dlp, drp);
+ dt_cg_check_notnull(dlp, drp, str->dn_reg);
if (dt_regset_xalloc_args(drp) == -1)
longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
- /* set flag that the state is initialized */
- emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
- emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_MST));
- emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_1, DMST_STRTOK, 1));
-
/* the 8-byte prefix is the offset, which we initialize to 0 */
emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_MEM));
@@ -3922,9 +3924,9 @@ dt_cg_subr_strtok(dt_node_t *dnp, dt_irlist_t
*dlp, dt_regset_t *drp)
} else {
/* NULL string: error if internal state is uninitialized */
emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_FP, DT_STK_DCTX));
- emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DCTX_MST));
- emit(dlp, BPF_LOAD(BPF_W, BPF_REG_0, BPF_REG_0, DMST_STRTOK));
- emit(dlp, BPF_ALU64_IMM(BPF_LSH, BPF_REG_0, 32));
+ emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DCTX_MEM));
+ emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DMEM_STRTOK));
+ emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 1));
dt_cg_check_notnull(dlp, drp, BPF_REG_0);
}
@@ -3945,7 +3947,6 @@ dt_cg_subr_strtok(dt_node_t *dnp, dt_irlist_t
*dlp, dt_regset_t *drp)
off = dt_cg_tstring_xalloc(yypcb);
/* function call */
-
if (dt_regset_xalloc_args(drp) == -1)
longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
@@ -3971,7 +3972,6 @@ dt_cg_subr_strtok(dt_node_t *dnp, dt_irlist_t
*dlp, dt_regset_t *drp)
TRACE_REGSET(" subr-strtok:End ");
}
-#undef DMEM_STRTOK
static void
dt_cg_subr_substr(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
@@ -23,7 +23,6 @@ typedef struct dt_mstate {
uint32_t clid; /* Clause ID (unique per probe) */
uint32_t tag; /* Tag (for future use) */
int32_t syscall_errno; /* syscall errno */
- int32_t strtok_init; /* Flag if strtok state inited */
uint64_t fault; /* DTrace fault flags */
uint64_t tstamp; /* cached timestamp value */
dt_pt_regs regs; /* CPU registers */
@@ -56,6 +55,12 @@ typedef struct dt_dctx {
#define DCTX_LVARS offsetof(dt_dctx_t, lvars)
#define DCTX_SIZE ((int16_t)sizeof(dt_dctx_t))
+/*
+ * Macro to determine the offset from mem to the strtok internal state.
+ */
+#define DMEM_STRTOK MAX(sizeof(uint64_t) *
dtp->dt_options[DTRACEOPT_MAXFRAMES], \
+ DT_TSTRING_SLOTS * roundup(DT_STRLEN_BYTES +
dtp->dt_options[DTRACEOPT_STRSIZE] + 1, 8))
+
/*
* Macro to determine the (negative) offset from the frame pointer
(%fp) for
* the given offset in dt_dctx_t.
@@ -67,7 +72,6 @@ typedef struct dt_dctx {
#define DMST_CLID offsetof(dt_mstate_t, clid)
#define DMST_TAG offsetof(dt_mstate_t, tag)
#define DMST_ERRNO offsetof(dt_mstate_t, syscall_errno)
-#define DMST_STRTOK offsetof(dt_mstate_t, strtok_init)
#define DMST_FAULT offsetof(dt_mstate_t, fault)
#define DMST_TSTAMP offsetof(dt_mstate_t, tstamp)
#define DMST_REGS offsetof(dt_mstate_t, regs)
More information about the DTrace-devel
mailing list