[DTrace-devel] [PATCH v3 03/19] cg: fault improvements

[Nick Alcock]

On 29 Mar 2022, Kris Van Hees spake thusly:

> 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 already changed this after Eugene's suggestions, to a single
dt_cg_probe_error() which takes an isreg arg. It makes a number of
failing code paths easier to follow.

> 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.

... I thought we were trying to save instructions? Are we not any more?
I was trying to keep the number of pointless NOPs down...