[DTrace-devel] [PATCH] Fix stack layout definitions to match the new dt_dctx design
Eugene Loh
eugene.loh at oracle.com
Thu Sep 3 22:25:59 PDT 2020
I don't know if you want to consider this, but if you're cleaning up
comments on stack layout and mstate, the provider trampoline() functions
have comments that need fixing:
dt_prov_[dtrace|fbt|profile].c (the stray chars before the ']'):
* // stdw [%r7 + DMST_ARG(0)0], %r0
* // stdw [%r7 + DMST_ARG(1)8], %r0
* // stdw [%r7 + DMST_ARG(2)8], %r0
* // stdw [%r7 + DMST_ARG(3)8], %r0
* // stdw [%r7 + DMST_ARG(4)], %r0
* // stdw [%r7 + DMST_ARG(5)], %r0
dt_prov_sdt.c (s/CTX/MST/, s/(0)/(i)/, and missing the ']'):
* // stdw [%r7 + DCTX_ARG(0), 0
dt_prov_syscall.c (s/CTX/MST/, s/(0)/(i)/, and missing the ']'):
* // stdw [%r7 + DCTX_ARG(0), %r0
dt_prov_syscall.c (s/CTX/MST/, missing the ']', and s/%r0/0/):
* // stdw [%r7 + DCTX_ARG(i), %r0
And then,
dt_cg.c dt_cg_tramp_call_clauses() (s/%fp/%r1/):
* // add %fp, DCTX_FP(0)
Argh. Looking at this stuff I also see:
dt_dctx.h (this line should be #if 0'ed out):
#define DMST_REGS offsetof(dt_mstate_t, regs)
Anyhow, if you don't want to include the above changes with this patch,
just let me know and I can prepare another patch.
As far as the test code goes, the lvar stuff is pretty confusing. The .r
file has:
lvar[ -1]: -103 (ID -1)
lvar[ 0]: -104 (ID 0)
lvar[ 1]: -112 (ID 1)
lvar[ 19]: -256 (ID 19)
lvar[ -1]: -257 (ID -1)
Wait, what? lvar[-1] is -103? Or lvar[-1] is -257? Actually, neither
one makes sense. My recommendation is to have the test just do
something like
#define TESTLVARID(off) printf("ID(% 4d) = % 3d\n", off,
DT_LVAR_OFF2ID(off));
TESTLVARID(DT_STK_LVAR(0) + 1)
TESTLVARID(DT_STK_LVAR(0))
TESTLVARID(DT_STK_LVAR(1))
TESTLVARID(DT_STK_LVAR(DT_LVAR_MAX))
TESTLVARID(DT_STK_LVAR(DT_LVAR_MAX) - 1)
That is, do not report the first column (which is really just for
correctness checking, but all we actually do is check against the .r
file) and just focus on going from offset to ID.
On 08/27/2020 11:54 AM, Kris Van Hees wrote:
> The stack layout was changed with the move of the DTrace machine state
> out of the stack. The changes were not properly reflected everywhere,
> both in comments and in code:
>
> - dt_cg.c: dt_cg_tramp_prologue()
>
> The comment describing the BPF code to pass the 2nd argument to the
> bpf_map_lookup_elem() helper was referring to the wrong stack offset
> constant.
>
> The pseudo-code in the comment also incorrectly passed 'mem' rather
> than '&mem'.
>
> - dt_dctx.h:
>
> The definition of DCTX_SIZE now explicitly casts to int16_t to
> avoid compilation warnings in code that uses it in BPF instructions.
>
> The definition of DCTX_FP(off) is now based on DT_STK_DCTX as the
> base offset of the dt_dctx struct (defined in dt_impl.h).
>
> - dt_impl.h:
>
> The comment block describing the stack layout now reflects the
> correct layout in view of dt_dctx.
>
> The definitions of DT_STK_BASE and DT_STK_SLOT_SZ now explicitly
> cast to int16_t to avoid compilation warnings in code that uses
> them in BPF instructions.
>
> The definition of DT_STK_DCTX has been corrected based on its
> actual size (DCTX_SIZE).
>
> - tst.stack_layout.r:
>
> Updated the expected output.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> libdtrace/dt_cg.c | 4 ++--
> libdtrace/dt_dctx.h | 4 ++--
> libdtrace/dt_impl.h | 20 ++++++++---------
> test/unittest/codegen/tst.stack_layout.r | 28 ++++++++++++------------
> 4 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 0d3463c1..8a7a09db 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -64,10 +64,10 @@ dt_cg_tramp_prologue(dt_pcb_t *pcb, uint_t lbl_exit)
>
> /*
> * key = 0; // stw [%fp + DCTX_FP(DCTX_MST)], 0
> - * rc = bpf_map_lookup_elem(mem, &key);
> + * rc = bpf_map_lookup_elem(&mem, &key);
> * // lddw %r1, &mem
> * // mov %r2, %fp
> - * // add %r2, DT_STK_LVA_BASE
> + * // add %r2, DCTX_FP(DCTX_MST)
> * // call bpf_map_lookup_elem
> * // (%r1 ... %r5 clobbered)
> * // (%r0 = 'mem' BPF map value)
> diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> index 79ef7105..b315f522 100644
> --- a/libdtrace/dt_dctx.h
> +++ b/libdtrace/dt_dctx.h
> @@ -36,13 +36,13 @@ typedef struct dt_dctx {
> #define DCTX_CTX offsetof(dt_dctx_t, ctx)
> #define DCTX_MST offsetof(dt_dctx_t, mst)
> #define DCTX_BUF offsetof(dt_dctx_t, buf)
> -#define DCTX_SIZE sizeof(dt_dctx_t)
> +#define DCTX_SIZE (int16_t)sizeof(dt_dctx_t)
>
> /*
> * Macro to determine the (negative) offset from the frame pointer (%fp) for
> * the given offset in dt_dctx_t.
> */
> -#define DCTX_FP(off) (-(ushort_t)DCTX_SIZE + (ushort_t)(off))
> +#define DCTX_FP(off) (DT_STK_DCTX + (int16_t)(off))
>
> #define DMST_EPID offsetof(dt_mstate_t, epid)
> #define DMST_TAG offsetof(dt_mstate_t, tag)
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index fb06032d..0f79b45f 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -494,29 +494,29 @@ struct dtrace_hdl {
> * +----------------+
> * SCRATCH_BASE = -512 | Scratch Memory |
> * +----------------+
> - * LVAR_END = LVAR(n) = -256 | LVAR n | (n = DT_VAR_LOCAL_MAX = 20)
> + * LVAR_END = LVAR(n) = -256 | LVAR n | (n = DT_VAR_LOCAL_MAX = 19)
> * +----------------+
> * | ... |
> * +----------------+
> - * LVAR(1) = -96 | LVAR 1 |
> + * LVAR(1) = -112 | LVAR 1 |
> * +----------------+
> - * LVAR_BASE = LVAR(0) = -88 | LVAR 0 |
> + * LVAR_BASE = LVAR(0) = -104 | LVAR 0 |
> * +----------------+
> - * SPILL(n) = -80 | %r8 | (n = DT_STK_NREGS - 1 = 8)
> + * SPILL(n) = -96 | %r8 | (n = DT_STK_NREGS - 1 = 8)
> * +----------------+
> * | ... |
> * +----------------+
> - * SPILL(1) = -24 | %r1 |
> + * SPILL(1) = -40 | %r1 |
> * +----------------+
> - * SPILL_BASE = SPILL(0) = -16 | %r0 |
> + * SPILL_BASE = SPILL(0) = -32 | %r0 |
> * +----------------+
> - * DCTX = -8 | DTrace Context | -1
> + * DCTX = -24 | DTrace Context | -1
> * +----------------+
> */
> -#define DT_STK_BASE (0)
> -#define DT_STK_SLOT_SZ ((int)sizeof(uint64_t))
> +#define DT_STK_BASE ((int16_t)0)
> +#define DT_STK_SLOT_SZ ((int16_t)sizeof(uint64_t))
>
> -#define DT_STK_DCTX (DT_STK_BASE - DT_STK_SLOT_SZ)
> +#define DT_STK_DCTX (DT_STK_BASE - DCTX_SIZE)
> #define DT_STK_SPILL_BASE (DT_STK_DCTX - DT_STK_SLOT_SZ)
> #define DT_STK_SPILL(n) (DT_STK_SPILL_BASE - (n) * DT_STK_SLOT_SZ)
> #define DT_STK_LVAR_BASE (DT_STK_SPILL(DT_STK_NREGS - 1) - \
> diff --git a/test/unittest/codegen/tst.stack_layout.r b/test/unittest/codegen/tst.stack_layout.r
> index d0081e53..2155e785 100644
> --- a/test/unittest/codegen/tst.stack_layout.r
> +++ b/test/unittest/codegen/tst.stack_layout.r
> @@ -1,17 +1,17 @@
> Base: 0
> -dctx: -8
> -%r0: -16
> -%r1: -24
> -%r2: -32
> -%r3: -40
> -%r4: -48
> -%r5: -56
> -%r6: -64
> -%r7: -72
> -%r8: -80
> -lvar[ -1]: -87 (ID -1)
> -lvar[ 0]: -88 (ID 0)
> -lvar[ 1]: -96 (ID 1)
> -lvar[ 21]: -256 (ID 21)
> +dctx: -24
> +%r0: -32
> +%r1: -40
> +%r2: -48
> +%r3: -56
> +%r4: -64
> +%r5: -72
> +%r6: -80
> +%r7: -88
> +%r8: -96
> +lvar[ -1]: -103 (ID -1)
> +lvar[ 0]: -104 (ID 0)
> +lvar[ 1]: -112 (ID 1)
> +lvar[ 19]: -256 (ID 19)
> lvar[ -1]: -257 (ID -1)
> scratch: -257 .. -512
More information about the DTrace-devel
mailing list