[DTrace-devel] [PATCH] Unify NULL pointer checking in dt_cg_check_notnull()

[Kris Van Hees]

On Fri, Jun 18, 2021 at 02:26:32PM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> 
> What will be the ordering of patches?  If this goes in first, I can 
> tweak dt_cg_act_[u]stack() to use it.  If those patches go in first, 
> this patch should include those two functions.

This patch is planned to go before your stack related patches.  So, yes, you
make make use of it.

> What about DIVZERO in dt_cg_arithmetic_op()?  How about extending the 
> new function to allow for use with DIVZERO?

Different thing.  There are less places where DIVZERO checking is being done,
but putting the check in a cg function to be used in the places that need it
makes sense.  Not in this patch though.
> 
> On 6/18/21 2:07 PM, Kris Van Hees wrote:
> > We need to generate code in multiple places to validate that a pointer
> > value is not NULL.  This patch introduces the dt_cg_check_notnull()
> > function to do just that,  Existing checks are replaced by a call to
> > this function.
> >
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   libdtrace/dt_cg.c | 40 +++++++++++++++++-----------------------
> >   1 file changed, 17 insertions(+), 23 deletions(-)
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 3d705a11..22f97404 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -612,6 +612,21 @@ dt_cg_probe_error(dt_pcb_t *pcb, uint32_t off, uint32_t fault, uint64_t illval)
> >   	emit(dlp,  BPF_RETURN());
> >   }
> >   
> > +/*
> > + * Generate code to validate that the value in the given register 'reg' is not
> > + * the NULL pointer.
> > + */
> > +static void
> > +dt_cg_check_notnull(dt_irlist_t *dlp, dt_regset_t *drp, int reg)
> > +{
> > +	uint_t	lbl_notnull = dt_irlist_label(dlp);
> > +
> > +	emit(dlp,  BPF_BRANCH_IMM(BPF_JNE, reg, 0, lbl_notnull));
> > +	dt_cg_probe_error(yypcb, -1, DTRACEFLT_BADADDR, 0);
> > +	emitl(dlp, lbl_notnull,
> > +		   BPF_NOP());
> > +}
> > +
> >   /*
> >    * Check whether mst->fault indicates a fault was triggered.  If so, abort the
> >    * current clause by means of a straight jump to the exit labal.
> > @@ -3191,7 +3206,6 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >   
> >   		if (!(dnp->dn_flags & DT_NF_REF)) {
> >   			uint_t	ubit;
> > -			uint_t	lbl_valid = dt_irlist_label(dlp);
> >   
> >   			/*
> >   			 * Save and restore DT_NF_USERLAND across dt_cg_load():
> > @@ -3202,12 +3216,7 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >   			dnp->dn_flags |=
> >   			    (dnp->dn_child->dn_flags & DT_NF_USERLAND);
> >   
> > -			/* if NULL pointer, report BADARR */
> > -			emit(dlp,  BPF_BRANCH_IMM(BPF_JNE, dnp->dn_reg, 0,
> > -						  lbl_valid));
> > -			dt_cg_probe_error(yypcb, -1, DTRACEFLT_BADADDR, 0);
> > -			emitl(dlp, lbl_valid,
> > -				   BPF_NOP());
> > +			dt_cg_check_notnull(dlp, drp, dnp->dn_reg);
> >   
> >   			/* FIXME: Does not handled signed or userland */
> >   			emit(dlp, BPF_LOAD(dt_cg_load(dnp, ctfp, dnp->dn_type), dnp->dn_reg, dnp->dn_reg, 0));
> > @@ -3293,25 +3302,10 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >   
> >   	case DT_TOK_PTR:
> >   	case DT_TOK_DOT: {
> > -		uint_t	lbl_valid = dt_irlist_label(dlp);
> >   
> >   		assert(dnp->dn_right->dn_kind == DT_NODE_IDENT);
> >   		dt_cg_node(dnp->dn_left, dlp, drp);
> > -
> > -		/*
> > -		 * If the lvalue is the NULL pointer, we must report a BADARR
> > -		 * fault.
> > -		 *
> > -		 *	if (left != 0)		// jne %lreg, 0, valid
> > -		 *		goto valid;
> > -		 *				//     (report BADADDR fault)
> > -		 * valid:
> > -		 */
> > -		emit(dlp,  BPF_BRANCH_IMM(BPF_JNE, dnp->dn_left->dn_reg, 0,
> > -					  lbl_valid));
> > -		dt_cg_probe_error(yypcb, -1, DTRACEFLT_BADADDR, 0);
> > -		emitl(dlp, lbl_valid,
> > -			   BPF_NOP());
> > +		dt_cg_check_notnull(dlp, drp, dnp->dn_left->dn_reg);
> >   
> >   		/*
> >   		 * If the left-hand side of PTR or DOT is a dynamic variable,
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel