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

[Arnaldo Carvalho de Melo]

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@xxxxxxxxxx> wrote:
> >
> > From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Athira Rajeev <atrajeev@xxxxxxxxxxxxxxxxxx>
> > Cc: Ian Rogers <irogers@xxxxxxxxxx>
> > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Cc: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > Cc: Steinar H. Gunderson <sesse@xxxxxxxxxx>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> 
> Acked-by: Ian Rogers <irogers@xxxxxxxxxx>
> 
> 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