On Thu, 18 Apr 2019 16:57:50 -0700, Alexei Starovoitov wrote:
> > @@ -6371,8 +6406,10 @@ static int propagate_liveness(struct bpf_verifier_env *env,
> > for (i = frame < vstate->curframe ? BPF_REG_6 : 0; i < BPF_REG_FP; i++) {
> > err = propagate_liveness_reg(env, &state_reg[i],
> > &parent_reg[i]);
> > - if (err)
> > + if (err < 0)
> > return err;
> > + if (err & REG_LIVE_READ64)
> > + mark_insn_zext(env, &parent_reg[i]);
>
> I'm not quite following why it's parent_reg here instead of state_reg.
Perhaps we should rename the parameters to something else than parent
here? I always have to do some mental gymnastics looking at this code..
"explored" and "current"?
The current state is parent, the "next" state that pruned the search
is "state". So we check if the reads under state X need 64bit, if so
have to propagate back to writes on current (which is called parent
here, even though it won't become state's parent, ugh.)
> If I understood the code the liveness can have all three states:
> REG_LIVE_READ64 | REG_LIVE_READ32
> REG_LIVE_READ64
> REG_LIVE_READ32
> whereas 2 is a superset of 3, so 1 should never be seen.
>
> If so, why in propagate_liveness we have this dance:
> + u8 parent_bits = parent_reg->live & REG_LIVE_READ;
> + u8 bits = reg->live & REG_LIVE_READ;
> + u8 bits_diff = parent_bits ^ bits;
> + u8 bits_prop = bits_diff & bits;
> int err;
>
> - if (parent_reg->live & REG_LIVE_READ || !(reg->live & REG_LIVE_READ))
> + /* No diff bit comes from "reg". */
> + if (!bits_prop)
>
> I'm struggling to see through all 3 combinations in respect to above diff.
> Shouldn't propagation of REG_LIVE_READ64 remove REG_LIVE_READ32 bit
> and clear subreg_def during mark_reg_read() instead of
> once in propagate_liveness() ?
This reminds me, I'm not entirely clear on the need to propagate the
zext through stack slots... Pointers are guaranteed to be 64bit, we
don't save parentage on scalars (AFAICT), why not pass REG_LIVE_READ
or READ64 to mark_reg_read() from stack_read?
