Re: [PATCH 1/3] perf disasm: Introduce symbol__disassemble_objdump()

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Mon, Nov 11, 2024 at 08:15:53AM -0800, Ian Rogers wrote:
> On Mon, Nov 11, 2024 at 7:17 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> >
> > With the first disassemble method in perf, the parsing of objdump
> > output, just like we have for llvm and capstone.
> >
> > This paves the way to allow the user to specify what disassemblers are
> > preferred and to also to at some point allow building without the
> > objdump method.
> >
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> > Cc: Ian Rogers <irogers@google.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Kan Liang <kan.liang@linux.intel.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Steinar H. Gunderson <sesse@google.com>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Acked-by: Ian Rogers <irogers@google.com>
> 
> Nit below relating to a pre-existing condition in the code.

<SNIP>

> > +#ifdef HAVE_LIBLLVM_SUPPORT
> > +       err = symbol__disassemble_llvm(symfs_filename, sym, args);
> > +       if (err == 0)
> > +               goto out_remove_tmp;
> > +#endif
> > +#ifdef HAVE_LIBCAPSTONE_SUPPORT
> > +       err = symbol__disassemble_capstone(symfs_filename, sym, args);
> > +       if (err == 0)
> > +               goto out_remove_tmp;
> > +#endif
> > +       err = symbol__disassemble_objdump(symfs_filename, sym, args);

> This sure does read like the symbol will be disassembled 3 times if
> those ifdefs are defined. Is there anyway to make the code look more
> intuitive?

This will end up being rewritten, the end result is a loop where the
list of disassemblers to try is iterated and as soon as one succeeeds
(returns zero), the loop is exited.

- Arnaldo