[DTrace-devel] [PATCH] Fix stack layout definitions to match the new dt_dctx design

Kris Van Hees kris.van.hees at oracle.com
Fri Sep 4 00:18:16 PDT 2020


On Thu, Sep 03, 2020 at 10:25:59PM -0700, Eugene Loh wrote:
> 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:

Yes, they need updating but I need to take a look at where I updated some of
these already.  I might just move the fixes to this patch since it does make
sense to do so.  I'll have a look.  Thanks for pointing them out.

>      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)

No, there is no need to #if 0 that line.  It is harmless to have the define
there.  Note that we excluded the regs from the state before simply because it
was causing us to overrun the available stack space.  That will no longer be
the case with this new design so we can actually re-enable the recording of
the register state.

> 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.

The point is that the print-stack-layout little prog is used by the test but
it exists because it makes it easy to see what the constant values are and it
also provides minimal testing of some of the boundary conditions for variable
offsets.  The output may look rather weird but there is actually a purpose
behind it.  What we're really testing is that the offset of the byte before the
lvar storage and the offset of the byte after the lvar storage yield an invalid
lvar (-1) condition.

Sure, the output could be made more clear or polished but that is hardly an
issue we need to address.

> 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
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel



More information about the DTrace-devel mailing list