> On Feb 19, 2022, at 5:10 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Feb 18, 2022 at 4:36 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>
>> ---
>
> Few nitpicks below, but this is a great improvement!
Thanks a lot for the super quick review!
>
>> kernel/bpf/verifier.c | 72 +++---
>> .../testing/selftests/bpf/prog_tests/align.c | 218 +++++++++---------
>> .../selftests/bpf/prog_tests/log_buf.c | 4 +-
>> 3 files changed, 156 insertions(+), 138 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index d7473fee247c..a43bb0cf4c46 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",
>> @@ -666,6 +666,15 @@ static void scrub_spilled_slot(u8 *stype)
>> *stype = STACK_MISC;
>> }
>>
>> +#define verbose_append(fmt, ...) \
>> +({ \
>> + if (is_first_item) \
>> + is_first_item = false; \
>> + else \
>> + verbose(env, ","); \
>> + verbose(env, fmt, __VA_ARGS__); \
>> +})
>> +
>
> let's move this inside print_verifier_state() and #undef it at the end
> of that function, given it should only be used inside it.
>
> I don't know if it's better (it sucks either way that we need to
> define extra macro for this, but alternative approach would be to
> define separator:
>
> const char *sep = "";
>
> #define verbose_append(fmt, ...) ({ verbose(env, "%s" fmt, sep,
> __VA_ARGS__); sep = ","; })
I will use your approach as it is more compact than mine
>
>> static void print_verifier_state(struct bpf_verifier_env *env,
>> const struct bpf_func_state *state,
>> bool print_all)
>> @@ -693,65 +702,74 @@ 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 {
>> + bool is_first_item = true;
>> +
>> 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, "(");
>> +
>> + if (reg->id) {
>> + verbose(env, "id=%d", reg->id);
>> + is_first_item = false;
>
> should be also verbose_append?
yes
>
>> + }
>> +
>> + if (reg_type_may_be_refcounted_or_null(t) && reg->ref_obj_id)
>> + verbose_append("ref_obj_id=%d", reg->ref_obj_id);
>> if (t != SCALAR_VALUE)
>> - verbose(env, ",off=%d", reg->off);
>> + verbose_append("off=%d", reg->off);
>> if (type_is_pkt_pointer(t))
>> - verbose(env, ",r=%d", reg->range);
>> + verbose_append("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_append("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_append("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_append("smin=%lld",
>> + (long long)reg->smin_value);
>
> a bunch of these should fit within 100 character single line, given
> the code churn anyways, let's "straighten" those wrapped lines where
> possible? if we go with something shorter than "verbose_append" even
> more lines would fit (verbose_add, don't know, naming is hard).
sounds good
>
>> if (reg->smax_value != reg->umax_value &&
>> reg->smax_value != S64_MAX)
>> - verbose(env, ",smax_value=%lld",
>> - (long long)reg->smax_value);
>> + verbose_append("smax=%lld",
>> + (long long)reg->smax_value);
>> if (reg->umin_value != 0)
>> - verbose(env, ",umin_value=%llu",
>> - (unsigned long long)reg->umin_value);
>> + verbose_append("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_append("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_append("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_append("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_append("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_append("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_append("u32_max=%d",
>> + (int)(reg->u32_max_value));
>> }
>> verbose(env, ")");
>> }
>
> […]
