On Tue, Feb 22, 2022 at 8:23 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Tue, Feb 22, 2022 at 6:07 PM Mykola Lysenko <mykolal@xxxxxx> wrote: > > > > In particular: > > 1) remove output of inv for scalars > > 2) remove _value suffixes for umin/umax/s32_min/etc (except map_value) > > 3) remove output of id=0 > > 4) remove output of ref_obj_id=0 > > > > Signed-off-by: Mykola Lysenko <mykolal@xxxxxx> > > --- > > LGTM, thanks. > > Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx> Actually seems like you missed updating some tests, please take a look: [0] https://github.com/kernel-patches/bpf/runs/5297754845?check_suite_focus=true > > > kernel/bpf/verifier.c | 59 ++--- > > .../testing/selftests/bpf/prog_tests/align.c | 218 +++++++++--------- > > .../selftests/bpf/prog_tests/log_buf.c | 4 +- > > 3 files changed, 143 insertions(+), 138 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index d7473fee247c..91154806715d 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -539,7 +539,7 @@ static const char *reg_type_str(struct bpf_verifier_env *env, > > char postfix[16] = {0}, prefix[32] = {0}; > > static const char * const str[] = { > > [NOT_INIT] = "?", > > - [SCALAR_VALUE] = "inv", > > + [SCALAR_VALUE] = "", > > [PTR_TO_CTX] = "ctx", > > [CONST_PTR_TO_MAP] = "map_ptr", > > [PTR_TO_MAP_VALUE] = "map_value", > > @@ -693,66 +693,71 @@ static void print_verifier_state(struct bpf_verifier_env *env, > > /* reg->off should be 0 for SCALAR_VALUE */ > > verbose(env, "%lld", reg->var_off.value + reg->off); > > } else { > > + const char *sep = ""; > > + > > if (base_type(t) == PTR_TO_BTF_ID || > > base_type(t) == PTR_TO_PERCPU_BTF_ID) > > verbose(env, "%s", kernel_type_name(reg->btf, reg->btf_id)); > > - verbose(env, "(id=%d", reg->id); > > - if (reg_type_may_be_refcounted_or_null(t)) > > - verbose(env, ",ref_obj_id=%d", reg->ref_obj_id); > > + verbose(env, "("); > > + > > +/* > > + * _a stands for append, was shortened to avoid multiline statements below. this macro is used to > > + * output a comma separated list of attributes > > + */ > > +#define verbose_a(fmt, ...) ({ verbose(env, "%s" fmt, sep, __VA_ARGS__); sep = ","; }) > > it's a very local macro so it probably doesn't matter all that much, > but a bit more readable name could be verbose_attr() or even just > log_attr(). I'll leave it up to Alexei and Daniel to decide if they'd > like to change it. > > > + > > + if (reg->id) > > + verbose_a("id=%d", reg->id); > > + if (reg_type_may_be_refcounted_or_null(t) && reg->ref_obj_id) > > + verbose_a("ref_obj_id=%d", reg->ref_obj_id); > > if (t != SCALAR_VALUE) > > - verbose(env, ",off=%d", reg->off); > > + verbose_a("off=%d", reg->off); > > if (type_is_pkt_pointer(t)) > > - verbose(env, ",r=%d", reg->range); > > + verbose_a("r=%d", reg->range); > > else if (base_type(t) == CONST_PTR_TO_MAP || > > base_type(t) == PTR_TO_MAP_KEY || > > base_type(t) == PTR_TO_MAP_VALUE) > > - verbose(env, ",ks=%d,vs=%d", > > - reg->map_ptr->key_size, > > - reg->map_ptr->value_size); > > + verbose_a("ks=%d,vs=%d", > > + reg->map_ptr->key_size, > > + reg->map_ptr->value_size); > > if (tnum_is_const(reg->var_off)) { > > /* Typically an immediate SCALAR_VALUE, but > > * could be a pointer whose offset is too big > > * for reg->off > > */ > > - verbose(env, ",imm=%llx", reg->var_off.value); > > + verbose_a("imm=%llx", reg->var_off.value); > > } else { > > if (reg->smin_value != reg->umin_value && > > reg->smin_value != S64_MIN) > > - verbose(env, ",smin_value=%lld", > > - (long long)reg->smin_value); > > + verbose_a("smin=%lld", (long long)reg->smin_value); > > if (reg->smax_value != reg->umax_value && > > reg->smax_value != S64_MAX) > > - verbose(env, ",smax_value=%lld", > > - (long long)reg->smax_value); > > + verbose_a("smax=%lld", (long long)reg->smax_value); > > if (reg->umin_value != 0) > > - verbose(env, ",umin_value=%llu", > > - (unsigned long long)reg->umin_value); > > + verbose_a("umin=%llu", (unsigned long long)reg->umin_value); > > if (reg->umax_value != U64_MAX) > > - verbose(env, ",umax_value=%llu", > > - (unsigned long long)reg->umax_value); > > + verbose_a("umax=%llu", (unsigned long long)reg->umax_value); > > if (!tnum_is_unknown(reg->var_off)) { > > char tn_buf[48]; > > > > tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off); > > - verbose(env, ",var_off=%s", tn_buf); > > + verbose_a("var_off=%s", tn_buf); > > } > > if (reg->s32_min_value != reg->smin_value && > > reg->s32_min_value != S32_MIN) > > - verbose(env, ",s32_min_value=%d", > > - (int)(reg->s32_min_value)); > > + verbose_a("s32_min=%d", (int)(reg->s32_min_value)); > > if (reg->s32_max_value != reg->smax_value && > > reg->s32_max_value != S32_MAX) > > - verbose(env, ",s32_max_value=%d", > > - (int)(reg->s32_max_value)); > > + verbose_a("s32_max=%d", (int)(reg->s32_max_value)); > > if (reg->u32_min_value != reg->umin_value && > > reg->u32_min_value != U32_MIN) > > - verbose(env, ",u32_min_value=%d", > > - (int)(reg->u32_min_value)); > > + verbose_a("u32_min=%d", (int)(reg->u32_min_value)); > > if (reg->u32_max_value != reg->umax_value && > > reg->u32_max_value != U32_MAX) > > - verbose(env, ",u32_max_value=%d", > > - (int)(reg->u32_max_value)); > > + verbose_a("u32_max=%d", (int)(reg->u32_max_value)); > > } > > +#undef verbose_a > > + > > verbose(env, ")"); > > } > > } > > [...]
