On Wed, Feb 23, 2022 at 3:25 PM Mykola Lysenko <mykolal@xxxxxx> wrote: > > > > On Feb 23, 2022, at 1:52 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > > > 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 > > Great catch! Thanks. > > Reviewing failed test logs I realized that while print_verifier_state works as expected and makes output tidier, my change did broke error messages. > > For example, my change turned > ______________________ > R2 invalid mem access ‘inv’ > ______________________ > into > ______________________ > R2 invalid mem access ‘’ > ______________________ > > Removing inv in this case does not make sense. We should either leave inv here, or substitute it with something more obvious, like ’scalar’. > > Thoughts? Yeah, scalar here would make most sense in this context. Would it be possible to use "scalar" in this error message, but still have empty string output in register state? > > > > > >> > >>> 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, ")"); > >>> } > >>> } > >> > >> [...] >
