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

[Eugene Loh]

On 3/29/22 7:14 AM, Nick Alcock via DTrace-devel wrote:

> 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...
I thought we discussed this already.  Labeled NOPs do not end up in the 
instruction stream.  Check dt_irlist_append().  There is a check:

         if (dip->di_label == DT_LBL_NONE || !BPF_IS_NOP(dip->di_instr))
                 dlp->dl_len++;

So if you have a label and the instruction is a NOP, you don't keep the 
instruction.