[DTrace-devel] [PATCH v2 03/17] cg: fault improvements
Eugene Loh
eugene.loh at oracle.com
Thu Mar 24 01:05:42 UTC 2022
On 3/14/22 5:30 PM, Nick Alcock wrote:
> Add a label to dt_cg_probe_error, which is dropped on the first
Dropped? That sounds like "omitted". How about "attached to"?
But what is even the point of this? Most of the time, the label is not
needed. For the few times you want it, just insert
emitl(dlp, MYLABEL,
BPF_NOP());
in front of the dt_cg_probe_error() call. The NOP will be optimized
away anyhow and you'll get the same BPF instructions.
> instruction if set; eschew regset allocation on the grounds that
If set? According to this patch, the label is always set. I'd omit "if
set". (Though mostly, as I said, I'd omit the label stuff altogether.)
I suppose "eschew" is fine, but "do not use" is simpler.
> spilling to the stack is pointless when the regs will all be thrown away
> in any case, and can trigger errors if some of the regs are not properly
> initialized, as is relatively likely in an error condition.
Good point. Indeed, I think you can be more emphatic:
s/can trigger errors/will trigger BPF verifier errors/
s/ properly//
> Also add a new dt_cg_probe_error_regval, which is like dt_cg_probe_error
> except the last argument is not an illegal value but a register whose
> value should be emitted.
Actually, I like what you did with dt_cg_trace(). How about just adding
an isreg flag to dt_cg_probe_error() since the differences between the
existing and new functions are otherwise minimal.
> This is a bit trickier: we must move the
> illegal register arg into place first because we don't use the regset
> allocation code, so it might be using one of the regs we're about to
> overwrite; and don't emit a move at all if we're by chance already using
> that reg (the verifier doesn't like mov %r4, %r4 very much).
I'd omit the stuff from "This is a bit tricker" until the end of the
paragraph. It seems premature to get into that detail here. The
comments in the code suffice.
Incidentally, are you sure "mov %r4 %r4" is problematic? If %r4 is
readable, this should be just fine. And someday I can imagine we'll do
code optimization that removes such no-ops.
Also, the issue is not that regset_alloc() is not being used. Even if
you used regset_alloc_args(), you'd still want to write %r4 first.
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
> libdtrace/dt_cg.c | 73 ++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 59 insertions(+), 14 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index b5152bd63caa..ea0af2fbbe7a 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -644,14 +644,17 @@ 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.
> + * Generate code for a fault condition, possibly at a given label. A call is
> + * made to dt_probe_error() to set the fault information.
> + *
> + * 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 off, uint32_t fault, uint64_t illval)
> +dt_cg_probe_error(dt_pcb_t *pcb, int lbl, uint32_t off, uint32_t fault,
> + uint64_t illval)
> {
> dt_irlist_t *dlp = &pcb->pcb_ir;
> - dt_regset_t *drp = pcb->pcb_regs;
> dt_ident_t *idp = dt_dlib_get_func(yypcb->pcb_hdl,
> "dt_probe_error");
>
> @@ -666,16 +669,58 @@ dt_cg_probe_error(dt_pcb_t *pcb, uint32_t off, uint32_t fault, uint64_t illval)
> * // call dt_probe_error
> * return; // exit
> */
Incidentally, the comment block (not completely visible here) has
DT_STK_DCT. That's a pre-existing problem but might as well clean up
that typo while you're in here.
> - if (dt_regset_xalloc_args(drp) == -1)
> - longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> - emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> +
> + emitl(dlp, lbl,
> + BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> emit(dlp, BPF_MOV_IMM(BPF_REG_2, off));
> 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);
> 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());
> +}
> +
> +/*
> + * Generate code for a fault condition with the illval coming from a register.
> + * Drop a label at the start.
Again, I advise against this label stuff, but regardless "drop" sounds
like "omit".
> + * Doesn't use the regset code on the grounds that when this is executed we will
> + * only reuse one caller's reg in a stereotyped manner.
Well, or that we are about to exit the function and so there is no point
in spilling register contents to the stack.
> + */
> +static void
> +dt_cg_probe_error_regval(dt_pcb_t *pcb, int lbl, uint32_t off,
> + uint32_t fault, int illreg)
> +{
> + dt_irlist_t *dlp = &pcb->pcb_ir;
> + dt_ident_t *idp = dt_dlib_get_func(yypcb->pcb_hdl,
> + "dt_probe_error");
> +
> + assert(idp != NULL);
> +
> + /*
> + * dt_probe_error(
> + * dctx, // lddw %r1, %fp, DT_STK_DCTX
> + * off, // mov %r2, off
> + * fault, // mov %r3, fault
> + * illreg); // mov %r4, %illreg
I'm not a fan of these comments with disassembly lines but in any case
here the disassembly instructions in the comment are in a different
order from what one actually does.
> + * // call dt_probe_error
> + * return; // exit
> + */
> +
> + /*
> + * Move the only pre-existing reg we need (illreg) 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 (illreg != 4) {
> + emitl(dlp, lbl, BPF_MOV_REG(BPF_REG_4, illreg));
> + lbl = DT_LBL_NONE;
> + }
> + emitl(dlp, lbl,
> + BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_2, off));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_3, fault));
> + emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> emit(dlp, BPF_RETURN());
> }
>
> @@ -689,7 +734,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, -1, DTRACEFLT_BADADDR, 0);
> + dt_cg_probe_error(yypcb, DT_LBL_NONE, -1, DTRACEFLT_BADADDR, 0);
> emitl(dlp, lbl_notnull,
> BPF_NOP());
> }
> @@ -1677,7 +1722,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, -1, DTRACEFLT_BADSTACK, 0);
> + dt_cg_probe_error(pcb, DT_LBL_NONE, -1, DTRACEFLT_BADSTACK, 0);
> emitl(dlp, lbl_valid,
> BPF_NOP());
> }
> @@ -1847,7 +1892,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, -1, DTRACEFLT_BADSTACK, 0);
> + dt_cg_probe_error(pcb, DT_LBL_NONE, -1, DTRACEFLT_BADSTACK, 0);
> emitl(dlp, lbl_valid,
> BPF_NOP());
> }
> @@ -2612,7 +2657,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, -1, DTRACEFLT_DIVZERO, 0);
> + dt_cg_probe_error(yypcb, DT_LBL_NONE, -1, DTRACEFLT_DIVZERO, 0);
> emitl(dlp, lbl_valid,
> BPF_NOP());
>
More information about the DTrace-devel
mailing list