[DTrace-devel] [PATCH v2 16/17] cg: support casts of pointers to integers

[Eugene Loh]

First of all, how about a quick return:
         if (!dt_node_is_scalar(dst))
                 return;
Thereafter, no more dst-is-scalar checks.

Also, there should be test cases.

E.g., consider:
test1: trace((int)alloca(8)); printf("success\n"); exit(0)}
test2: trace((unsigned long)alloca(8)); printf("success\n"); exit(0)}

With Solaris I get:
test1: -1965031400success
test2: -103600976035808success

With legacy DT on Linux I get:
test1: -1001582568success
test2: -111296403427296success

With DT on BPF without this patch I get:
test1: BPF: 198: (67) r7 <<= 32    BPF: R7 pointer arithmetic with <<= 
operator prohibited
test2: 18446694915642859384success

With DT on BPF with this patch I get:
test1: BPF: 198: (67) r7 <<= 32    BPF: R7 pointer arithmetic with <<= 
operator prohibited
test2: 18446694915643383672success

As far as these tests go, the patch makes no difference.  So, I think 
you should add tests like these but you also need a test that's 
sensitive to what you are trying to do with this patch.

And why does that BPF verifier problem occur even with your patch?  I 
think because the code tries type casting with the <<, which you cannot 
use on a pointer.  So maybe you have to switch the logic up some.  First 
check if the src is a pointer. (With the quick return, we already know 
the dst is a scalar.)  If src is a pointer, do the scalarize operation.  
Then, if necessary, also do the shift left/right magic.

On 3/14/22 5:30 PM, Nick Alcock wrote:
> When we cast a pointer to an integer, we want to scalarize it,
> i.e. ensure that it is no longer a map_value, so that the user can
> compare it freely to other integers.
>
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
>   libdtrace/dt_cg.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 1f622d365464..c940ce7b8bc6 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -3009,6 +3009,8 @@ out:
>    * a scalar type is being narrowed or changing signed-ness.  We first shift the
>    * desired bits high (losing excess bits if narrowing) and then shift them down
>    * using logical shift (unsigned result) or arithmetic shift (signed result).
> + *
> + * We also need to scalarize pointers if we are casting them to an integral type.
>    */
>   static void
>   dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
> @@ -3035,6 +3037,18 @@ dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
>   		emit(dlp, BPF_MOV_REG(dst->dn_reg, src->dn_reg));
>   		emit(dlp, BPF_ALU64_IMM(BPF_LSH, dst->dn_reg, n));
>   		emit(dlp, BPF_ALU64_IMM((dst->dn_flags & DT_NF_SIGNED) ? BPF_ARSH : BPF_RSH, dst->dn_reg, n));
> +	} else if (dt_node_is_scalar(dst) && dt_node_is_pointer(src)) {
> +		int mst;
> +
> +		if ((mst = dt_regset_alloc(drp)) == -1)
> +			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> +		emit(dlp,  BPF_LOAD(BPF_DW, mst, BPF_REG_FP, DT_STK_DCTX));
> +		emit(dlp,  BPF_LOAD(BPF_DW, mst, mst, DCTX_MST));
> +		emit(dlp,  BPF_STORE(BPF_DW, mst, DMST_SCALARIZER, src->dn_reg));
> +		emit(dlp,  BPF_LOAD(BPF_DW, dst->dn_reg, mst, DMST_SCALARIZER));
> +
> +		dt_regset_free(drp, mst);
>   	}
>   }
>