Re: [PATCH 2/3] perf disasm: Define stubs for the LLVM and capstone disassemblers

[Arnaldo Carvalho de Melo]

On Mon, Nov 11, 2024 at 08:23:49AM -0800, Ian Rogers wrote:
> On Mon, Nov 11, 2024 at 7:18 AM Arnaldo Carvalho de Melo
> <acme@xxxxxxxxxx> wrote:
> >
> > From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> >
> > This reduces the number of ifdefs in the main symbol__disassemble()
> > method and paves the way for allowing the user to configure the
> > disassemblers of preference.
> >
> > 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>

Thanks

> Style nit below.
> 
> > ---
> >  tools/perf/util/disasm.c | 40 ++++++++++++++++++++++++++++++++--------
> >  1 file changed, 32 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> > index 36cf61602c17fe69..83df1da20a7b16cd 100644
> > --- a/tools/perf/util/disasm.c
> > +++ b/tools/perf/util/disasm.c
> > @@ -1422,6 +1422,13 @@ read_symbol(const char *filename, struct map *map, struct symbol *sym,
> >         free(buf);
> >         return NULL;
> >  }
> > +#else // defined(HAVE_LIBCAPSTONE_SUPPORT) || defined(HAVE_LIBLLVM_SUPPORT)
> > +static void symbol__disassembler_missing(const char *disassembler, const char *filename,
> > +                                        struct symbol *sym)
> > +{
> > +       pr_debug("The %s disassembler isn't linked in for %s in %s\n",
> > +                disassembler, sym->name, filename);
> > +}
> 
> I can see why you're doing this but it seems overkill to have a
> function just for the sake of 1 pr_debug. The #ifdefs make this look
> like it could be doing more. Perhaps the style:

Well, I wrote it like that, then had to cut'n't paste that thing to use
it, ended up with two callers, three when we make objdump a build time
selection, unsure if we will end up with more disassemblers.

- Arnaldo

> ```
> static int symbol__disassemble_capstone(char *filename, struct symbol
> *sym, struct annotate_args *args)
> {
> #ifndef HAVE_LIBCAPSTONE_SUPPORT
>     pr_debug("The capstone disassembler isn't linked in for %s in
> %s\n", sym->name, filename);
>     return -1;
> #else
>     ...
> #endif
> }
> ```
> Would be preferable as the #ifdef's scope is more limited, you don't
> need to hunt around for 1 of 3 functions, etc.
> 
> Thanks,
> Ian
> 
> >  #endif
> >
> >  #ifdef HAVE_LIBCAPSTONE_SUPPORT
> > @@ -1715,7 +1722,20 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
> >         count = -1;
> >         goto out;
> >  }
> > -#endif
> > +#else // HAVE_LIBCAPSTONE_SUPPORT
> > +static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
> > +                                       struct annotate_args *args)
> > +       symbol__disassembler_missing("capstone", filename, sym);
> > +       return -1;
> > +}
> > +
> > +static int symbol__disassemble_capstone_powerpc(char *filename, struct symbol *sym,
> > +                                               struct annotate_args *args __maybe_unused)
> > +{
> > +       symbol__disassembler_missing("capstone powerpc", filename, sym);
> > +       return -1;
> > +}
> > +#endif // HAVE_LIBCAPSTONE_SUPPORT
> >
> >  static int symbol__disassemble_raw(char *filename, struct symbol *sym,
> >                                         struct annotate_args *args)
> > @@ -1983,7 +2003,14 @@ static int symbol__disassemble_llvm(char *filename, struct symbol *sym,
> >         free(line_storage);
> >         return ret;
> >  }
> > -#endif
> > +#else // HAVE_LIBLLVM_SUPPORT
> > +static int symbol__disassemble_llvm(char *filename, struct symbol *sym,
> > +                                   struct annotate_args *args __maybe_unused)
> > +{
> > +       symbol__disassembler_missing("LLVM", filename, sym);
> > +       return -1;
> > +}
> > +#endif // HAVE_LIBLLVM_SUPPORT
> >
> >  /*
> >   * Possibly create a new version of line with tabs expanded. Returns the
> > @@ -2242,24 +2269,21 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
> >                         err = symbol__disassemble_raw(symfs_filename, sym, args);
> >                         if (err == 0)
> >                                 goto out_remove_tmp;
> > -#ifdef HAVE_LIBCAPSTONE_SUPPORT
> > +
> >                         err = symbol__disassemble_capstone_powerpc(symfs_filename, sym, args);
> >                         if (err == 0)
> >                                 goto out_remove_tmp;
> > -#endif
> >                 }
> >         }
> >
> > -#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);
> >
> >  out_remove_tmp:
> > --
> > 2.47.0
> >