Re: [PATCH bpf-next v7] selftests/bpf: add BPF program dump in veristat

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2025-09-05 at 12:14 -0700, Andrii Nakryiko wrote:
> On Fri, Sep 5, 2025 at 12:00 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Fri, 2025-09-05 at 15:08 +0100, Mykyta Yatsenko wrote:
> > > From: Mykyta Yatsenko <yatsenko@meta.com>
> > > 
> > > Add the ability to dump BPF program instructions directly from veristat.
> > > Previously, inspecting a program required separate bpftool invocations:
> > > one to load and another to dump it, which meant running multiple
> > > commands.
> > > During active development, it's common for developers to use veristat
> > > for testing verification. Integrating instruction dumping into veristat
> > > reduces the need to switch tools and simplifies the workflow.
> > > By making this information more readily accessible, this change aims
> > > to streamline the BPF development cycle and improve usability for
> > > developers.
> > > This implementation leverages bpftool, by running it directly via popen
> > > to avoid any code duplication and keep veristat simple.
> > > 
> > > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> > > ---
> > 
> > Lgtm with a small nit.
> > 
> > Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> > 
> > > @@ -1554,6 +1573,35 @@ static int parse_rvalue(const char *val, struct rvalue *rvalue)
> > >       return 0;
> > >  }
> > > 
> > > +static void dump(__u32 prog_id, enum dump_mode mode, const char *file_name, const char *prog_name)
> > > +{
> > > +     char command[64], buf[4096];
> > > +     FILE *fp;
> > > +     int status;
> > > +
> > > +     status = system("which bpftool > /dev/null 2>&1");
> > 
> > Fun fact: if you do a minimal Fedora install (dnf group install core)
> >           "which" is not installed by default o.O
> >           (not suggesting any changes).
> 
> I switched to `command -v bpftool` for now, is there any gotcha with
> that one as well?

Should be fine, I guess:

  $ rpm -qf /usr/sbin/command
  bash-5.2.37-1.fc42.x86_64

> > 
> > > +     if (status != 0) {
> > > +             fprintf(stderr, "bpftool is not available, can't print program dump\n");
> > > +             return;
> > > +     }
> > 
> > [...]
> > 
> > > @@ -1630,8 +1678,13 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
> > > 
> > >       memset(&info, 0, info_len);
> > >       fd = bpf_program__fd(prog);
> > > -     if (fd > 0 && bpf_prog_get_info_by_fd(fd, &info, &info_len) == 0)
> > > +     if (fd > 0 && bpf_prog_get_info_by_fd(fd, &info, &info_len) == 0) {
> > >               stats->stats[JITED_SIZE] = info.jited_prog_len;
> > > +             if (env.dump_mode & DUMP_JITED)
> > > +                     dump(info.id, DUMP_JITED, base_filename, prog_name);
> > > +             if (env.dump_mode & DUMP_XLATED)
> > > +                     dump(info.id, DUMP_XLATED, base_filename, prog_name);
> > 
> > Nit: if you do `./veristat --dump=jited iters.bpf.o` there would be an empty line
> >      after dump for each program, but not for --dump=xlated.
> > 
> 
> Yeah, bpftool's output isn't consistent. I just added an extra empty
> line, that makes dump a bit more clean (and I didn't mind two empty
> lines, whatever).

+1

> 
> I was also finding it hard to notice where the dump for a given
> program starts, so I reformatted header a bit. Overall, applied the
> following changes and pushed to bpf-next, thanks for a useful feature!

Yeap, nice little feature.
I was doing bogus __xlated("foo") before in tests,
just to see how assembly looks like.