On Fri, Nov 10, 2023 at 10:50 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote: > > On 11/10, Andrii Nakryiko wrote: > > This patch set moves a big chunk of verifier log related code from gigantic > > verifier.c file into more focused kernel/bpf/log.c. This is not essential to > > the rest of functionality in this patch set, so I can undo it, but it felt > > like it's good to start chipping away from 20K+ verifier.c whenever we can. > > > > The main purpose of the patch set, though, is in improving verifier log > > further. > > > > Patches #3-#4 start printing out register state even if that register is > > spilled into stack slot. Previously we'd get only spilled register type, but > > no additional information, like SCALAR_VALUE's ranges. Super limiting during > > debugging. For cases of register spills smaller than 8 bytes, we also print > > out STACK_MISC/STACK_ZERO/STACK_INVALID markers. This, among other things, > > will make it easier to write tests for these mixed spill/misc cases. > > > > Patch #5 prints map name for PTR_TO_MAP_VALUE/PTR_TO_MAP_KEY/CONST_PTR_TO_MAP > > registers. In big production BPF programs, it's important to map assembly to > > actual map, and it's often non-trivial. Having map name helps. > > [..] > > > Patch #6 just removes visual noise in form of ubiquitous imm=0 and off=0. They > > are default values, omit them. > > If you end up with another respin for some reason: > s/verifierl/verifier/ > s/furthre/futher/ > > (in the commit description) thanks, fixed it up locally, but will wait for the other feedback > > > Patch #7 is probably the most controversial, but it reworks how verifier log > > prints numbers. For small valued integers we use decimals, but for large ones > > we switch to hexadecimal. From personal experience this is a much more useful > > convention. We can tune what consitutes "small value", for now it's 16-bit > > range. > > Not sure why not always print in hex, but no strong preference here. for small values decimal is usually much more sane. E.g., if you have some index into `int arr[100];`, seeing that register's range is [0, 396] is much more convenient than [0x0, 0x18c], IMO. So I don't think we want to abandon decimals completely. > > > Patch #8 prints frame number for PTR_TO_CTX registers, if that frame is > > different from the "current" one. This removes ambiguity and confusion, > > especially in complicated cases with multiple subprogs passing around > > pointers. > > > > Andrii Nakryiko (8): > > bpf: move verbose_linfo() into kernel/bpf/log.c > > bpf: move verifier state printing code to kernel/bpf/log.c > > bpf: extract register state printing > > bpf: print spilled register state in stack slot > > bpf: emit map name in register state if applicable and available > > bpf: omit default off=0 and imm=0 in register state log > > bpf: smarter verifier log number printing logic > > bpf: emit frameno for PTR_TO_STACK regs if it differs from current one > > Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx> thanks!
