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

[Kris Van Hees]

On Tue, Mar 29, 2022 at 12:14:33PM +0100, Nick Alcock 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.

Hm, but that is not reflected in this new patch series?  Did you forget to
update the code prior to sending the new patch series?

I don't think that an isreg argument makes for better code.  If you oppt for
putting it in a single function, then perhaps at least use the argument to
indicate which type of value argument is being passed based on the BPF source
indicator (BPF_X vs BPF_K).  That means you can also use that in emitting BPF
instructions rather than using conditionals.

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

Labelled NOPs are removed by the assembler.