Re: [DTrace-devel] [PATCH 3/3] cg: fix masking of lower 32 bits

[Kris Van Hees <kris.van.hees@oracle.com>]

On Tue, Sep 23, 2025 at 06:30:08PM -0400, Eugene Loh wrote:
> On 9/23/25 17:05, Kris Van Hees via DTrace-devel wrote:
> 
> > The "and %r0, 0xffffffff' instruction does not actually mask off the
> > lower 32 bits as one would expect because 0xffffffff is interpreted as
> > -1 and then sign-extedned to a 64-bit value, i.e. keeping all bits.
> 
> s/extedned/extended/

Thanks.

> > The "mov32 %r0, %r0" instruxtion does correctly mask off the lwoer 32
> 
> s/instruxtion/instruction/
> s/lwoer/lower/

Thanks.

> I started looking at this a while back and was not convinced that the lower
> 32 bits were what we want.  Do we want them, and do we have a test that
> confirms?  Or do we want the upper 32 bits?  I got distracted and mothballed
> the whole thing, but the test involved spawning a child thread and then
> looking at its ustack()...  maybe after the child had exited but before the
> parent did.  Admittedly, I was confused at the time, and I'm even worse now.

I leave that to you to decide :)  All I do with this patch is accomplish what
the code claimed to do (but failed to do).  But since the consumer only looked
at it as a 32-bit value, one wouldn't notice that the top 32 bits of the 64 bit
word were not 0.  In fact, with this patch, there is not a single change in
test results because the code never would see the higher order bits.  But since
I plan on using those to store other info, I do need this masking to be done
correctly.

So this is a first step towards that.  The code as written was a no-op.  This
patch fixes that.  Without any regressions.

And no, there is no test possible for this because the lack of masking would
only be visible if something else was significantly broken in the consumer.

> > bits because it forced the value in %r0 to be a 32-bit value.
> > 
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > ---
> >   include/bpf_asm.h | 11 +++++++++++
> >   libdtrace/dt_cg.c |  3 ++-
> >   2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/bpf_asm.h b/include/bpf_asm.h
> > index 152d2312..63987e01 100644
> > --- a/include/bpf_asm.h
> > +++ b/include/bpf_asm.h
> > @@ -36,6 +36,15 @@
> >   		.imm = 0						\
> >   	})
> > +#define BPF_ALU32_REG(op, dst, src)					\
> > +	((struct bpf_insn) {						\
> > +		.code = BPF_ALU | (op) | BPF_X,				\
> > +		.dst_reg = (dst),					\
> > +		.src_reg = (src),					\
> > +		.off = 0,						\
> > +		.imm = 0						\
> > +	})
> > +
> >   #define BPF_END_REG(sz, dst, dir)					\
> >   	((struct bpf_insn) {						\
> >   		.code = BPF_ALU | BPF_END | (dir),			\
> > @@ -68,6 +77,8 @@
> >   #define BPF_MOV_REG(dst, src)	BPF_ALU64_REG(BPF_MOV, dst, src)
> >   #define BPF_MOV_IMM(dst, val)	BPF_ALU64_IMM(BPF_MOV, dst, val)
> > +#define BPF_MOV32_REG(dst, src)	BPF_ALU32_REG(BPF_MOV, dst, src)
> > +
> >   #define BPF_LOAD(sz, dst, src, ofs)					\
> >   	((struct bpf_insn) {						\
> >   		.code = BPF_LDX | BPF_MEM | (sz),			\
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index a8f2c9d2..28b7e7c4 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -2757,7 +2757,8 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk
> >   		dt_regset_xalloc(drp, BPF_REG_0);
> >   		emit(dlp,  BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> >   		dt_regset_free_args(drp);
> > -		emit(dlp,  BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffffffff));
> > +		/* mov32 %r0, %r0 effectively masks the lower 32 bits. */
> > +		emit(dlp,  BPF_MOV32_REG(BPF_REG_0, BPF_REG_0));
> >   		emit(dlp,  BPF_STORE(BPF_DW, reg, off, BPF_REG_0));
> >   		dt_regset_free(drp, BPF_REG_0);
> >   	}