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

[Nick Alcock]

On 29 Mar 2022, Eugene Loh via DTrace-devel verbalised:

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

Oh! No, I had no idea about this. In that case I'm happy to tear the
labelled-fault stuff out.

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

Nice!

On 30 Mar 2022, Kris Van Hees said:

> On Tue, Mar 29, 2022 at 12:14:33PM +0100, Nick Alcock wrote:
>> 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 only did those changes the day after sending this series, and you
asked me not to resend subsequently so you didn't have to keep
re-reviewing (which is only sensible!).

See nix/wip/alloca for the ongoing changes (incorporating your review
requests as well).

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

That's only clear if you're used to writing direct BPF a lot -- but the
macros and GNU as completely hide this detail, so nobody working
predominantly in the bpf code generator *or* in bpf/*.S is going to be
among that set. (I had to read the docs to remember which was which, and
I forgot it before I got to the end of this paragraph.)

I'll let you and Eugene decide which alternative you prefer here, since
I made this change in response to a review request and I'd rather not
keep changing this one way and then back the other. (We have to use
conditionals on the cg level anyway to avoid a mov %reg, %reg pattern in
the isreg case, since the verifier seems to hate it.)

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

Excellent reason not to need all this labelled fault stuff: taking it
out again :)

-- 
NULL && (void)