[DTrace-devel] [PATCH v4 02/10] cg: fault improvements

[Kris Van Hees]

> Do not use regset allocation in dt_cg_probe_error on the grounds that
> spilling to the stack is pointless when the regs will all be thrown away
> in any case, and will trigger BPF verifier errors if some of the regs
> are not initialized, as is relatively likely in an error condition.
> 
> Also add an argument specifying whether the illegal value is actually
> a register whose value should be printed.  (Cases which don't care
> can just pass 0, 0).

Can you move the DT_ISREG / DT_ISIMM macros I add in [05/10]  (alloca deref)
here (and remove them from that patch) for consistency?  That makes it easier
to read the code in knowing what the argument value is.

So this will cuase a small change to this patch and to [05/10].

> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
>  libdtrace/dt_cg.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index a149c57529f1..80b93c91661b 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -645,39 +645,48 @@ dt_cg_epilogue(dt_pcb_t *pcb)
>  
>  /*
>   * Generate code for a fault condition.  A call is made to dt_probe_error() to
> - * set the fault information.
> + * set the fault information.  ILL may be an absolute value or a register,
> + * depending on the value of ILLISREG.
> + *
> + * Doesn't use the regset code on the grounds that when this is executed we will
> + * never reuse any of the callers' regs in any case.
>   */
>  static void
> -dt_cg_probe_error(dt_pcb_t *pcb, uint32_t fault, uint64_t illval)
> +dt_cg_probe_error(dt_pcb_t *pcb, uint32_t fault, int illisreg, uint64_t ill)
>  {
>  	dt_irlist_t	*dlp = &pcb->pcb_ir;
> -	dt_regset_t	*drp = pcb->pcb_regs;
>  	dt_ident_t	*idp;
>  
>  	/*
>  	 *	dt_probe_error(
> -	 *		dctx,		// lddw %r1, %fp, DT_STK_DCT
> +	 *		dctx,		// lddw %r1, %fp, DT_STK_DCTX
>  	 *		PC,		// mov %r2, PC
>  	 *		fault,		// mov %r3, fault
>  	 *		illval);	// mov %r4, illval
>  	 *				// call dt_probe_error
>  	 *	return;			// exit
>  	 */
> -	if (dt_regset_xalloc_args(drp) == -1)
> -		longjmp(pcb->pcb_jmpbuf, EDT_NOREG);
> +
> +	/*
> +	 * Move the only pre-existing reg we need (ill) into place first,
> +	 * since we don't know which reg it is and it might perfectly well be
> +	 * one we are about to blindly reuse.
> +	 */
> +
> +	if (illisreg && ill != 4)
> +		emit(dlp, BPF_MOV_REG(BPF_REG_4, ill));
> +	else
> +		emit(dlp, BPF_MOV_IMM(BPF_REG_4, ill));
> +
>  	emit(dlp,  BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
>  	idp = dt_dlib_get_var(pcb->pcb_hdl, "PC");
>  	assert(idp != NULL);
>  	emite(dlp, BPF_MOV_IMM(BPF_REG_2, -1), idp);
> -	emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, 4));
> +	emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, 3));
>  	emit(dlp,  BPF_MOV_IMM(BPF_REG_3, fault));
> -	emit(dlp,  BPF_MOV_IMM(BPF_REG_4, illval));
> -	dt_regset_xalloc(drp, BPF_REG_0);
>  	idp = dt_dlib_get_func(pcb->pcb_hdl, "dt_probe_error");
>  	assert(idp != NULL);
>  	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> -	dt_regset_free_args(drp);
> -	dt_regset_free(drp, BPF_REG_0);
>  	emit(dlp,  BPF_RETURN());
>  }
>  
> @@ -691,7 +700,7 @@ 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, DTRACEFLT_BADADDR, 0);
> +	dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, 0, 0);
>  	emitl(dlp, lbl_notnull,
>  		   BPF_NOP());
>  }
> @@ -1652,7 +1661,7 @@ dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>  	dt_regset_free_args(drp);
>  	emit(dlp,  BPF_BRANCH_IMM(BPF_JSGE, BPF_REG_0, 0, lbl_valid));
>  	dt_regset_free(drp, BPF_REG_0);
> -	dt_cg_probe_error(pcb, DTRACEFLT_BADSTACK, 0);
> +	dt_cg_probe_error(pcb, DTRACEFLT_BADSTACK, 0, 0);
>  	emitl(dlp, lbl_valid,
>  		   BPF_NOP());
>  }
> @@ -1822,7 +1831,7 @@ dt_cg_act_ustack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>  	dt_regset_free_args(drp);
>  	emit(dlp,  BPF_BRANCH_IMM(BPF_JSGE, BPF_REG_0, 0, lbl_valid));
>  	dt_regset_free(drp, BPF_REG_0);
> -	dt_cg_probe_error(pcb, DTRACEFLT_BADSTACK, 0);
> +	dt_cg_probe_error(pcb, DTRACEFLT_BADSTACK, 0, 0);
>  	emitl(dlp, lbl_valid,
>  		   BPF_NOP());
>  }
> @@ -2492,7 +2501,7 @@ dt_cg_arglist(dt_ident_t *idp, dt_node_t *args, dt_irlist_t *dlp,
>  			dt_regset_free_args(drp);
>  			emit(dlp,  BPF_BRANCH_IMM(BPF_JSGE, BPF_REG_0, 0, lbl_valid));
>  			dt_regset_free(drp, BPF_REG_0);
> -			dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, 128 + i);
> +			dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, 0, 128 + i);
>  			dt_regset_xalloc(drp, BPF_REG_0);
>  
>  			emitl(dlp, lbl_valid,
> @@ -2514,7 +2523,7 @@ dt_cg_arglist(dt_ident_t *idp, dt_node_t *args, dt_irlist_t *dlp,
>  			emit(dlp,  BPF_CALL_HELPER(BPF_FUNC_probe_read));
>  			dt_regset_free_args(drp);
>  			emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_valid));
> -			dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, 0);
> +			dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, 0, 0);
>  
>  			emitl(dlp, lbl_valid,
>  				   BPF_ALU64_IMM(BPF_ADD, treg, size));
> @@ -2797,7 +2806,7 @@ dt_cg_arithmetic_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>  		/* First ensure we do not perform a division by zero. */
>  		emit(dlp,  BPF_BRANCH_IMM(BPF_JNE, dnp->dn_right->dn_reg, 0,
>  					  lbl_valid));
> -		dt_cg_probe_error(yypcb, DTRACEFLT_DIVZERO, 0);
> +		dt_cg_probe_error(yypcb, DTRACEFLT_DIVZERO, 0, 0);
>  		emitl(dlp, lbl_valid,
>  			   BPF_NOP());
>  
> -- 
> 2.35.1