[DTrace-devel] [PATCH v2 03/17] cg: fault improvements

[Eugene Loh]

On 3/24/22 4:15 PM, Nick Alcock wrote:

> On 24 Mar 2022, Eugene Loh via DTrace-devel verbalised:
>
>> 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"?
> Much better! Adjusted.
>
>> 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.
> Errr... the NOP ends up being an actual instruction in the stream.
I don't think so.  Not if there is a nontrivial label.  I think the 
magic is due to dt_irlist_append(), which has:
         if (dip->di_label == DT_LBL_NONE || !BPF_IS_NOP(dip->di_instr))
                 dlp->dl_len++;
With a non-NONE label and a NOP instruction, we don't include the 
instruction.  I just tried it and sure enough the NOP does not appear 
(unless I specify DT_LBL_NONE).

> I
> thought we were at least trying to minimize our insn count, at least for
> frequently-used things like the bounds checkers, where the complexity
> cost of doing so is low?
>
>>> 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.)
> DT_LBL_NONE is "unset", isn't it?
I guess.  But to convince myself of that I have to look beyond the 
patch.  Oh well.  Should be moot at this point?
>
>> I suppose "eschew" is fine, but "do not use" is simpler.
> True! Adjusted.
>
>>> 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//
> Adjusted.
>
>>> 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.
> Back when I wrote this dt_cg_probe_error had so many args I couldn't
> remember which was which as it was. It has fewer now, thanks to Kris,
> and this actually lets us simplify a lot of uses of regval, like this:
>
>          if (sizemax < 0)
>                  dt_cg_probe_error(yypcb, lbl_size_err, DTRACEFLT_BADSIZE, size);
>          else
>                  dt_cg_probe_error_regval(yypcb, lbl_size_err, DTRACEFLT_BADSIZE,
>                                           size);
>
> which can now become a simple
>
>          dt_cg_probe_error(yypcb, lbl_size_erro, DTRACEFLT_BADSIZE,
>                            (sizemax >= 0), size);
>
> ("if sizemax >= 0, this is a reg").
>
> Adjusted!
>
>>> 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.
> (Done.)
>
>> 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.
> I wish, but nope! It's broken for me, at least with some kernels (5.15
> was the one I saw it on). It might be specific to the
> using-non-allocated-register thing though, which is pretty specific to
> faulting paths. (Though I didn't think allocating registers did anything
> visible at the BPF layer, so probably not.)
I can well believe %reg=%reg would be considered illegal if %reg is not 
"readable".  E.g. just got back from a function call and %r1-%r5 were 
clobbered.
>> 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.
> True enough!
>
>>>    @@ -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.
> (Fixed.)
>
>>> -	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".
> Yep. Changed to "... possibly at a given label". Keeping the label stuff
> simply because it saves several instructions per bounds check and the
> bounds checks are too damn big already and can't be moved out of line :(
How does it save instructions?  Is this back to the question of whether 
those NOPs appear in the instruction stream?
>>> + * 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.
> Changed to
>
>   * 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.
>
> (some time ago.)
>
>>> + */
>>> +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.
> Yeah, I've stopped using them too now I can read this stuff better. This
> whole function is gone now in favour of an ILLISREG arg to
> dt_cg_probe_error anyway. :)
>