> 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? > >> >>> 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, ")"); >>> } >>> } >> >> [...]
