[DTrace-devel] [PATCH v3 03/19] cg: fault improvements
Kris Van Hees
kris.van.hees at oracle.com
Tue Mar 29 05:43:20 UTC 2022
On Thu, Mar 24, 2022 at 06:24:29PM +0000, Nick Alcock via DTrace-devel wrote:
> Add a label to dt_cg_probe_error, which is dropped on the first
> instruction if set; eschew regset allocation on the grounds that
> 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.
>
> 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. 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).
How about renaming dt_cg_probe_error_regval() to be dt_cg_probe_error_reg()
and rename dt_cg_probe_error() to be dt_cg_probe_error_imm(), to match the
BPF instructions (*_IMM vs *_REG). That emphesises the distinct even better.
I would also implement dt_cg_probe_error_imm() as moving the immediate value
into a register (BPF_REG_4 would make the most sense), and then simply calling
dt_cg_probe_error_reg() to generate the rest of the code.
Further, I am not sure that there is enough value to adding a label argument to
the dt_cg_probe_error_*() calls. There are only 4 such calls in the remainder
of this patch series, and those are 2 sets of conditional-controlled pairs of
calls. As such, you could as easily place a BPF_NOP() with a label before
each conditional clause. I think that would also make those uses more clear
in the calling functions.
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
> libdtrace/dt_cg.c | 76 +++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 60 insertions(+), 16 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 5f86c34d7be4..ffbe5e95670f 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -647,14 +647,16 @@ 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 fault, uint64_t illval)
> +dt_cg_probe_error(dt_pcb_t *pcb, int lbl, uint32_t fault, uint64_t illval)
> {
> dt_irlist_t *dlp = &pcb->pcb_ir;
> - dt_regset_t *drp = pcb->pcb_regs;
> dt_ident_t *idp;
>
> /*
> @@ -666,21 +668,63 @@ dt_cg_probe_error(dt_pcb_t *pcb, uint32_t fault, uint64_t illval)
> * // call dt_probe_error
> * return; // exit
> */
> - if (dt_regset_xalloc_args(drp) == -1)
> - longjmp(pcb->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));
> 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_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());
> +}
> +
> +/*
> + * Generate code for a fault condition with the illval coming from a register.
> + * Drop a label at the start.
> + *
> + * 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.
> + */
> +static void
> +dt_cg_probe_error_regval(dt_pcb_t *pcb, int lbl, uint32_t fault, int illreg)
> +{
> + dt_irlist_t *dlp = &pcb->pcb_ir;
> + dt_ident_t *idp;
> +
> + /*
> + * dt_probe_error(
> + * dctx, // lddw %r1, %fp, DT_STK_DCTX
> + * PC, // mov %r2, PC
> + * fault, // mov %r3, fault
> + * illreg); // mov %r4, %illreg
> + * // 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));
> + 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_MOV_IMM(BPF_REG_3, fault));
> + idp = dt_dlib_get_func(pcb->pcb_hdl, "dt_probe_error");
> + assert(idp != NULL);
> + emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> emit(dlp, BPF_RETURN());
> }
>
> @@ -694,7 +738,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, DT_LBL_NONE, DTRACEFLT_BADADDR, 0);
> emitl(dlp, lbl_notnull,
> BPF_NOP());
> }
> @@ -1682,7 +1726,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, DT_LBL_NONE, DTRACEFLT_BADSTACK, 0);
> emitl(dlp, lbl_valid,
> BPF_NOP());
> }
> @@ -1852,7 +1896,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, DT_LBL_NONE, DTRACEFLT_BADSTACK, 0);
> emitl(dlp, lbl_valid,
> BPF_NOP());
> }
> @@ -2522,7 +2566,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, DT_LBL_NONE, DTRACEFLT_BADADDR, 128 + i);
> dt_regset_xalloc(drp, BPF_REG_0);
>
> emitl(dlp, lbl_valid,
> @@ -2544,7 +2588,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, DT_LBL_NONE, DTRACEFLT_BADADDR, 0);
>
> emitl(dlp, lbl_valid,
> BPF_ALU64_IMM(BPF_ADD, treg, size));
> @@ -2827,7 +2871,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, DT_LBL_NONE, DTRACEFLT_DIVZERO, 0);
> emitl(dlp, lbl_valid,
> BPF_NOP());
>
> --
> 2.35.1.261.g8402f930ba.dirty
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list