On Thu, Dec 16, 2021 at 1:34 PM Christy Lee <christylee@xxxxxx> wrote:
> @@ -11256,16 +11306,18 @@ static int do_check(struct bpf_verifier_env *env)
> if (need_resched())
> cond_resched();
>
> - if (env->log.level & BPF_LOG_LEVEL2 ||
> + if ((env->log.level & BPF_LOG_LEVEL2) ||
Why add redundant () ?
> (env->log.level & BPF_LOG_LEVEL && do_print_state)) {
> - if (env->log.level & BPF_LOG_LEVEL2)
> + if (verifier_state_scratched(env) &&
> + (env->log.level & BPF_LOG_LEVEL2))
redundant ()
> verbose(env, "%d:", env->insn_idx);
> else
> verbose(env, "\nfrom %d to %d%s:",
> env->prev_insn_idx, env->insn_idx,
> env->cur_state->speculative ?
> " (speculative execution)" : "");
Also the addition of "verifier_state_scratched(env) &&"
changes the logic.
Instead of printing "%d:" it prints "from %d to %d" from time to time
which looks really weird.
The "from %d to %d" used to be printed only when
log_level == 1 && do_print_state==true.
It's doing this to indicate that the state was popped from the stack.
It's a branch processing.
When "from %d to %d" appears in the middle of the basic block it's
confusing.
When vlog_reset logic went in the "from %d to %d" became
irrelevant, since they would never be seen anymore,
but for folks who've seen the logs earlier it's odd.
I think it meant to be:
if (env->log.level & BPF_LOG_LEVEL2 ||
(env->log.level & BPF_LOG_LEVEL && do_print_state)) {
if (env->log.level & BPF_LOG_LEVEL2) {
if (verifier_state_scratched(env))
verbose(env, "%d:", env->insn_idx);
} else {
verbose(env, "\nfrom %d to %d%s:",
env->prev_insn_idx, env->insn_idx,
env->cur_state->speculative ?
" (speculative execution)" : "");
}
print_verifier_state(env, state->frame[state->curframe],
false);
do_print_state = false;
}
It's all minor. I fixed it all up while applying.
