[DTrace-devel] [PATCH] Built-in variable "timestamp": semantics should agree with v1

[eugene.loh at oracle.com]

From: Eugene Loh <eugene.loh at oracle.com>

The semantics of the built-in variable "timestamp" are not 100% clear from
the documentation.  Specifically, should it record a fresh time stamp each
time it is called?  Or should it be measured once upon probe (or clause)
entry and then reused?

Regardless, "legacy" DTrace seems to record a fresh timestamp for each
clause, reusing the same value throughout the clause.  Consider:

    # dtrace -qn 'BEGIN {trace(timestamp);trace(timestamp);}
                  BEGIN {trace(timestamp);trace(timestamp);exit(0)};'
    3823604430297094
    3823604430297094
    3823604430297975
    3823604430297975

In contrast, DTv2 appears to use a fresh timestamp each instance,
even within the same clause:

    # dtrace -qn 'BEGIN {trace(timestamp);trace(timestamp); exit(0);}'
    1645711727876826
    1645711727876873

The timestamp value should be consistent throughout an entire clause
and its predicate.  E.g., with DTv1:

    # dtrace -qn 'BEGIN
                  /(t = timestamp) == timestamp/
                  { trace(t);
                    trace(timestamp);
                    trace(timestamp); exit(0);
                  }'

trace() reports the same value three times, while with DTv2 the
predicate never even evaluates to true at all and the test hangs.

Fix the DTv2 version to match the legacy behavior, including in
dt_cg_prologue() moving mstate initialization before predicate evaluation.
Also, free the register used for any predicate, fixing a register leak.

Orabug: 31632630, 31638431
Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
---
 bpf/get_bvar.c      |  4 +++-
 libdtrace/dt_cg.c   | 32 ++++++++++++++++++--------------
 libdtrace/dt_dctx.h |  4 +++-
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/bpf/get_bvar.c b/bpf/get_bvar.c
index 40045fb0..1d0f3e46 100644
--- a/bpf/get_bvar.c
+++ b/bpf/get_bvar.c
@@ -21,7 +21,9 @@ noinline uint64_t dt_get_bvar(dt_mstate_t *mst, uint32_t id)
 	case DIF_VAR_CURTHREAD:
 		return bpf_get_current_task();
 	case DIF_VAR_TIMESTAMP:
-		return bpf_ktime_get_ns();
+		if (mst->tstamp == 0)
+			mst->tstamp = bpf_ktime_get_ns();
+		return mst->tstamp;
 	case DIF_VAR_EPID:
 		return mst->epid;
 	case DIF_VAR_ARG0: case DIF_VAR_ARG1: case DIF_VAR_ARG2:
diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 2435b84d..2746ccd7 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -181,20 +181,6 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
 	instr = BPF_STORE(BPF_DW, BPF_REG_FP, DT_STK_DCTX, BPF_REG_1);
 	dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
 
-	/*
-	 * If there is a predicate:
-	 *
-	 *	rc = predicate;		//     (evaluate predicate into %rX)
-	 *	if (rc == 0)		// jeq %rX, 0, pcb->pcb_exitlbl
-	 *		goto exit;
-	 */
-	if (pred != NULL) {
-		dt_cg_node(pred, &pcb->pcb_ir, pcb->pcb_regs);
-		instr = BPF_BRANCH_IMM(BPF_JEQ, pred->dn_reg, 0,
-				       pcb->pcb_exitlbl);
-		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
-	}
-
 	/*
 	 *	buf = dctx->buf;	// lddw %r0, [%fp + DT_STK_DCTX]
 	 *				// lddw %r9, [%r0 + DCTX_BUF]
@@ -207,6 +193,7 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
 	/*
 	 *	dctx->mst->fault = 0;	// lddw %r0, [%r0 + DCTX_MST]
 	 *				// stdw [%r0 + DMST_FAULT], 0
+	 *	dctx->mst->tstamp = 0;	// stdw [%r0 + DMST_TSTAMP], 0
 	 *	dctx->mst->epid = EPID;	// stw [%r0 + DMST_EPID], EPID
 	 *	*((uint32_t *)&buf[0]) = EPID;
 	 *				// stw [%r9 + 0], EPID
@@ -215,6 +202,8 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
 	dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
 	instr = BPF_STORE_IMM(BPF_DW, BPF_REG_0, DMST_FAULT, 0);
 	dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
+	instr = BPF_STORE_IMM(BPF_DW, BPF_REG_0, DMST_TSTAMP, 0);
+	dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
 	instr = BPF_STORE_IMM(BPF_W, BPF_REG_0, DMST_EPID, -1);
 	dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
 	dlp->dl_last->di_extern = epid;
@@ -232,6 +221,21 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
 	instr = BPF_STORE_IMM(BPF_W, BPF_REG_9, 4, 0);
 	dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
 
+	/*
+	 * If there is a predicate:
+	 *
+	 *	rc = predicate;		//     (evaluate predicate into %rX)
+	 *	if (rc == 0)		// jeq %rX, 0, pcb->pcb_exitlbl
+	 *		goto exit;
+	 */
+	if (pred != NULL) {
+		dt_cg_node(pred, &pcb->pcb_ir, pcb->pcb_regs);
+		instr = BPF_BRANCH_IMM(BPF_JEQ, pred->dn_reg, 0,
+				       pcb->pcb_exitlbl);
+		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
+		dt_regset_free(pcb->pcb_regs, pred->dn_reg);
+	}
+
 	/*
 	 * Account for 32-bit epid (at offset 0) and 32-bit tag (at offset 4).
 	 */
diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
index 647d181a..df4ff6fa 100644
--- a/libdtrace/dt_dctx.h
+++ b/libdtrace/dt_dctx.h
@@ -11,12 +11,13 @@
 #include <dt_pt_regs.h>
 
 /*
- * The DTrace machien state.
+ * The DTrace machine state.
  */
 typedef struct dt_mstate {
 	uint32_t	epid;		/* Enabled probe ID */
 	uint32_t	tag;		/* Tag (for future use) */
 	uint64_t	fault;		/* DTrace fault flags */
+	uint64_t	tstamp;		/* timestamp built-in variable */
 #if 0
 	dt_pt_regs	regs;		/* CPU registers */
 #endif
@@ -46,6 +47,7 @@ typedef struct dt_dctx {
 #define DMST_EPID	offsetof(dt_mstate_t, epid)
 #define DMST_TAG	offsetof(dt_mstate_t, tag)
 #define DMST_FAULT	offsetof(dt_mstate_t, fault)
+#define DMST_TSTAMP	offsetof(dt_mstate_t, tstamp)
 #define DMST_REGS	offsetof(dt_mstate_t, regs)
 #define DMST_ARG(n)	offsetof(dt_mstate_t, argv[n])
 
-- 
2.18.2