On Thu, Dec 16, 2021 at 7:59 AM Christy Lee <christyc.y.lee@xxxxxxxxx> wrote:>
Apologies, resending this because the previous email was not plain text.
>
> On Wed, Dec 15, 2021 at 11:02 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>>
>> On Wed, Dec 15, 2021 at 11:22 AM Christy Lee <christylee@xxxxxx> wrote:
>> >
>
> [...]
>>
>>
>> There seems to be quite a lot of jumpin back and forth in terms of
>> 33th (see off by one error below) vs 40th offsets (this is for
>> pyperf50 test):
>>
>> 16: (07) r2 += -8 ; R2_w=fp-8
>> ; Event* event = bpf_map_lookup_elem(&eventmap, &zero);
>> 17: (18) r1 = 0xffff88810d81dc00 ;
>> R1_w=map_ptr(id=0,off=0,ks=4,vs=252,imm=0)
>> 19: (85) call bpf_map_lookup_elem#1 ;
>> R0=map_value_or_null(id=3,off=0,ks=4,vs=252,imm=0)
>> 20: (bf) r7 = r0 ;
>> R0=map_value_or_null(id=3,off=0,ks=4,vs=252,imm=0)
>> R7_w=map_value_or_null(id=3,off=0,ks=4,vs=252,imm=0)
>> ; if (!event)
>> 21: (15) if r7 == 0x0 goto pc+5193 ;
>> R7_w=map_value(id=0,off=0,ks=4,vs=252,imm=0)
>> ; event->pid = pid;
>> 22: (61) r1 = *(u32 *)(r10 -4) ;
>> R1_w=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R10=fp0
>>
>> Maybe let's bump the minimum to 40?
>
> Will do! I'll experiment with the nicest looking offset.
>>
>>
>> >
>> > Signed-off-by: Christy Lee <christylee@xxxxxx>
>
> [...]
>>
>> >
>> > +static u32 vlog_alignment(u32 prev_insn_print_len)
>> > +{
>> > + if (prev_insn_print_len < BPF_LOG_ALIGNMENT_POS)
>> > + return BPF_LOG_ALIGNMENT_POS - prev_insn_print_len + 1;
>>
>> why +1 here?
>
> I wanted the insn_state to be prepended by BPF_LOG_ALIGNMENT_POS
> number of characters, in this case, that would be prepended by 32 and starting
> at 33. This ensures that whether prev_insn_print_len is smaller than
> BPF_LOG_ALIGNMENT_POS or not, the insn_states would be aligned properly,
> there would be an off-by-one error otherwise.
>>
>>
>> > + return round_up(prev_insn_print_len, BPF_LOG_MIN_ALIGNMENT) -
>> > + prev_insn_print_len;
>> > +}
>> > +
>> > +static void print_prev_insn_state(struct bpf_verifier_env *env,
>> > + const struct bpf_func_state *state)
>> > +{
>> > + if (env->prev_log_len == env->log.len_used) {
>> > + if (env->prev_log_len)
>> > + bpf_vlog_reset(&env->log, env->prev_log_len - 1);
>>
>> I don't get this... why do we need this reset? Why just appending
>> alignment spaces below doesn't work?
>
> The insn are printed via print_bpf_insn() which ends the line with a '\n', we need to
> reset one character in order to remove the new line and print insn_state on the same
> line. print_bpf_insn() is defined in kernel/bpf/disasm.h and used by bpf_tool as well, so I
> didn't want to modify it.
>>
>>
>> > + verbose(env, "%*c;", vlog_alignment(env->prev_insn_print_len),
>> > + ' ');
>>
>> nit: keep it on single line
>>
>> > + } else
>> > + verbose(env, "%d:", env->insn_idx);
>>
>> if one branch of if/else has {}, the other one has to have them as
>> well, even if it's a single line statement
>>
>> > + print_verifier_state(env, state);
>> > +}
>> > +
>> > /* copy array src of length n * size bytes to dst. dst is reallocated if it's too
>> > * small to hold src. This is different from krealloc since we don't want to preserve
>> > * the contents of dst.
>>
>> [...]
>>
>> > @@ -9441,8 +9465,10 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>> > insn->dst_reg);
>> > return -EACCES;
>> > }
>> > - if (env->log.level & BPF_LOG_LEVEL)
>> > - print_verifier_state(env, this_branch->frame[this_branch->curframe]);
>> > + if (env->log.level & BPF_LOG_LEVEL) {
>> > + print_prev_insn_state(
>> > + env, this_branch->frame[this_branch->curframe]);
>>
>> nit: keep on a single line. But also is it really a "previous
>> instruction" or still a current instruction? Maybe just
>> "print_insn_state"? Do we have "next_insn" helper anywhere? If not,
>> dropping this "prev_" prefix from helpers and variables would be
>> cleaner, IMO
>>
>> > + }
>> > return 0;
>> > }
>> >
>> > @@ -11310,17 +11336,12 @@ static int do_check(struct bpf_verifier_env *env)
>> > if (need_resched())
>> > cond_resched();
>> >
>> > - if ((env->log.level & BPF_LOG_LEVEL2) ||
>> > - (env->log.level & BPF_LOG_LEVEL && do_print_state)) {
>> > - if (verifier_state_scratched(env) &&
>> > - (env->log.level & BPF_LOG_LEVEL2))
>> > - 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]);
>> > + if (env->log.level & BPF_LOG_LEVEL1 && do_print_state) {
>>
>> () around bit operations
>>
>> > + verbose(env, "\nfrom %d to %d%s:\n", env->prev_insn_idx,
>> > + env->insn_idx,
>> > + env->cur_state->speculative ?
>> > + " (speculative execution)" :
>> > + "");
>>
>> it's ok to go up to 100 characters, please keep the code more readable
>>
>> > do_print_state = false;
>> > }
>> >
>> > @@ -11331,9 +11352,17 @@ static int do_check(struct bpf_verifier_env *env)
>> > .private_data = env,
>> > };
>> >
>> > + if (verifier_state_scratched(env))
>> > + print_prev_insn_state(
>> > + env, state->frame[state->curframe]);
>> > +
>> > verbose_linfo(env, env->insn_idx, "; ");
>> > + env->prev_log_len = env->log.len_used;
>> > verbose(env, "%d: ", env->insn_idx);
>> > print_bpf_insn(&cbs, insn, env->allow_ptr_leaks);
>> > + env->prev_insn_print_len =
>> > + env->log.len_used - env->prev_log_len;
>> > + env->prev_log_len = env->log.len_used;
>> > }
>> >
>> > if (bpf_prog_is_dev_bound(env->prog->aux)) {
>>
>> [...]
