Hi Daniel,
> On Mar 1, 2022, at 5:33 AM, Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> Hi Mykola,
>
> On 2/24/22 1:37 AM, Mykola Lysenko wrote:
>> In particular:
>> 1) remove output of inv for scalars in print_verifier_state
>> 2) replace inv with scalar in verifier error messages
>> 3) remove _value suffixes for umin/umax/s32_min/etc (except map_value)
>> 4) remove output of id=0
>> 5) remove output of ref_obj_id=0
>> Signed-off-by: Mykola Lysenko <mykolal@xxxxxx>
>
> Thanks for helping to improve the verifier output. Small comment below:
Thanks for the review!
>
> [...]
>> diff --git a/tools/testing/selftests/bpf/prog_tests/align.c b/tools/testing/selftests/bpf/prog_tests/align.c
>> index 0ee29e11eaee..210dc6b4a169 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/align.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/align.c
>> @@ -39,13 +39,13 @@ static struct bpf_align_test tests[] = {
>> },
>> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>> .matches = {
>> - {0, "R1=ctx(id=0,off=0,imm=0)"},
>> + {0, "R1=ctx(off=0,imm=0)"},
>> {0, "R10=fp0"},
>> - {0, "R3_w=inv2"},
>> - {1, "R3_w=inv4"},
>> - {2, "R3_w=inv8"},
>> - {3, "R3_w=inv16"},
>> - {4, "R3_w=inv32"},
>> + {0, "R3_w=2"},
>> + {1, "R3_w=4"},
>> + {2, "R3_w=8"},
>> + {3, "R3_w=16"},
>> + {4, "R3_w=32"},
>
> Ack, definitely better compared to the state today. :)
>
> [...]
>> @@ -161,19 +161,19 @@ static struct bpf_align_test tests[] = {
>> },
>> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>> .matches = {
>> - {6, "R0_w=pkt(id=0,off=8,r=8,imm=0)"},
>> - {6, "R3_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
>> - {7, "R3_w=inv(id=0,umax_value=510,var_off=(0x0; 0x1fe))"},
>> - {8, "R3_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
>> - {9, "R3_w=inv(id=0,umax_value=2040,var_off=(0x0; 0x7f8))"},
>> - {10, "R3_w=inv(id=0,umax_value=4080,var_off=(0x0; 0xff0))"},
>> - {12, "R3_w=pkt_end(id=0,off=0,imm=0)"},
>> - {17, "R4_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
>> - {18, "R4_w=inv(id=0,umax_value=8160,var_off=(0x0; 0x1fe0))"},
>> - {19, "R4_w=inv(id=0,umax_value=4080,var_off=(0x0; 0xff0))"},
>> - {20, "R4_w=inv(id=0,umax_value=2040,var_off=(0x0; 0x7f8))"},
>> - {21, "R4_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
>> - {22, "R4_w=inv(id=0,umax_value=510,var_off=(0x0; 0x1fe))"},
>> + {6, "R0_w=pkt(off=8,r=8,imm=0)"},
>> + {6, "R3_w=(umax=255,var_off=(0x0; 0xff))"},
>> + {7, "R3_w=(umax=510,var_off=(0x0; 0x1fe))"},
>> + {8, "R3_w=(umax=1020,var_off=(0x0; 0x3fc))"},
>> + {9, "R3_w=(umax=2040,var_off=(0x0; 0x7f8))"},
>> + {10, "R3_w=(umax=4080,var_off=(0x0; 0xff0))"},
>> + {12, "R3_w=pkt_end(off=0,imm=0)"},
>> + {17, "R4_w=(umax=255,var_off=(0x0; 0xff))"},
>> + {18, "R4_w=(umax=8160,var_off=(0x0; 0x1fe0))"},
>> + {19, "R4_w=(umax=4080,var_off=(0x0; 0xff0))"},
>> + {20, "R4_w=(umax=2040,var_off=(0x0; 0x7f8))"},
>> + {21, "R4_w=(umax=1020,var_off=(0x0; 0x3fc))"},
>> + {22, "R4_w=(umax=510,var_off=(0x0; 0x1fe))"},
>> },
>> },
>> {
>
> However, not printing any type info here is imho more confusing. For debugging /
> troubleshooting knowing that the register type is inv or scalar would be helpful.
> Fwiw, scalar is probably a better fit, although longer..
So, just to confirm. You are proposing to leave cases like "R3_w=8” as is, but change cases like "R3_w=(umax=255,var_off=(0x0; 0xff))” to “R3_w=scalar(umax=255,var_off=(0x0; 0xff))”, correct?
>
> Thanks,
> Daniel
Regards,
Mykola
