From: Arnaldo Carvalho de Melo <acme@redhat.com>
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@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>
---
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);
+}
#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
> On 11 Nov 2024, at 8:47 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> 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@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>
> ---
> 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);
> +}
> #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;
> +}
Hi Arnaldo
I was testing this change in powerpc setup I see below compilation error
CC util/disasm.o
util/disasm.c: In function ‘symbol__disassemble_capstone’:
util/disasm.c:1728:9: error: expected declaration specifiers before ‘symbol__disassembler_missing’
1728 | symbol__disassembler_missing("capstone", filename, sym);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
util/disasm.c:1729:9: error: expected declaration specifiers before ‘return’
1729 | return -1;
| ^~~~~~
util/disasm.c:1730:1: error: expected declaration specifiers before ‘}’ token
1730 | }
Below patch fixes the issue
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index df6c172c9c7f..da22b6ab9ecf 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1724,7 +1724,8 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
}
#else // HAVE_LIBCAPSTONE_SUPPORT
static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
- struct annotate_args *args)
+ struct annotate_args *args __maybe_unused)
+{
symbol__disassembler_missing("capstone", filename, sym);
return -1;
}
I tried with tmp.perf-tools-next , I have tested the above patch fixes the compilation error
Thanks
Aditya Bodkhe
> +
> +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
>
>
On Wed, Nov 13, 2024 at 03:24:08PM +0000, Aditya Bodkhe wrote:
> Hi Arnaldo
> I was testing this change in powerpc setup I see below compilation error
>
> CC util/disasm.o
> util/disasm.c: In function ‘symbol__disassemble_capstone’:
> util/disasm.c:1728:9: error: expected declaration specifiers before ‘symbol__disassembler_missing’
> 1728 | symbol__disassembler_missing("capstone", filename, sym);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> util/disasm.c:1729:9: error: expected declaration specifiers before ‘return’
> 1729 | return -1;
> | ^~~~~~
> util/disasm.c:1730:1: error: expected declaration specifiers before ‘}’ token
> 1730 | }
>
> Below patch fixes the issue
Thanks, I've folded your fix into the patch since it is still in the
tmp.perf-tools-next branch, added a note to the patch to give you
and Masami credit, both sent fixes that I had to slightly adjust:
----
perf disasm: Define stubs for the LLVM and capstone disassemblers
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.
Acked-by: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Aditya Bodkhe <Aditya.Bodkhe1@ibm.com>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Steinar H. Gunderson <sesse@google.com>
Link: https://lore.kernel.org/r/20241111151734.1018476-3-acme@kernel.org
[ Applied fixes from Masami Hiramatsu and Aditya Bodkhe for when capstone devel files are not available ]
Link: https://lore.kernel.org/r/B78FB6DF-24E9-4A3C-91C9-535765EC0E2A@ibm.com
Link: https://lore.kernel.org/r/173145729034.2747044.453926054000880254.stgit@mhiramat.roam.corp.google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
----
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index df6c172c9c7f..da22b6ab9ecf 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1724,7 +1724,8 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
> }
> #else // HAVE_LIBCAPSTONE_SUPPORT
> static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
> - struct annotate_args *args)
> + struct annotate_args *args __maybe_unused)
> +{
> symbol__disassembler_missing("capstone", filename, sym);
> return -1;
> }
>
> I tried with tmp.perf-tools-next , I have tested the above patch fixes the compilation error
>
> Thanks
> Aditya Bodkhe
> > +
> > +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
> >
> >
>
> On 14 Nov 2024, at 12:57 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> On Wed, Nov 13, 2024 at 03:24:08PM +0000, Aditya Bodkhe wrote:
>> Hi Arnaldo
>> I was testing this change in powerpc setup I see below compilation error
>>
>> CC util/disasm.o
>> util/disasm.c: In function ‘symbol__disassemble_capstone’:
>> util/disasm.c:1728:9: error: expected declaration specifiers before ‘symbol__disassembler_missing’
>> 1728 | symbol__disassembler_missing("capstone", filename, sym);
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> util/disasm.c:1729:9: error: expected declaration specifiers before ‘return’
>> 1729 | return -1;
>> | ^~~~~~
>> util/disasm.c:1730:1: error: expected declaration specifiers before ‘}’ token
>> 1730 | }
>>
>> Below patch fixes the issue
>
> Thanks, I've folded your fix into the patch since it is still in the
> tmp.perf-tools-next branch, added a note to the patch to give you
> and Masami credit, both sent fixes that I had to slightly adjust:
>
> ----
> perf disasm: Define stubs for the LLVM and capstone disassemblers
>
> 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.
>
Hi Arnaldo
Thanks for adding the fixes. I tested with tmp.perf-tools-next and it compiles fine
Tested-by: Aditya Bodkhe <adityab1@linux.ibm.com <mailto:adityab1@linux.ibm.com>>
> Acked-by: Ian Rogers <irogers@google.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Aditya Bodkhe <Aditya.Bodkhe1@ibm.com>
> Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Steinar H. Gunderson <sesse@google.com>
> Link: https://lore.kernel.org/r/20241111151734.1018476-3-acme@kernel.org
> [ Applied fixes from Masami Hiramatsu and Aditya Bodkhe for when capstone devel files are not available ]
> Link: https://lore.kernel.org/r/B78FB6DF-24E9-4A3C-91C9-535765EC0E2A@ibm.com
> Link: https://lore.kernel.org/r/173145729034.2747044.453926054000880254.stgit@mhiramat.roam.corp.google.com
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ----
>
>> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
>> index df6c172c9c7f..da22b6ab9ecf 100644
>> --- a/tools/perf/util/disasm.c
>> +++ b/tools/perf/util/disasm.c
>> @@ -1724,7 +1724,8 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
>> }
>> #else // HAVE_LIBCAPSTONE_SUPPORT
>> static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
>> - struct annotate_args *args)
>> + struct annotate_args *args __maybe_unused)
>> +{
>> symbol__disassembler_missing("capstone", filename, sym);
>> return -1;
>> }
>>
>> I tried with tmp.perf-tools-next , I have tested the above patch fixes the compilation error
>>
>> Thanks
>> Aditya Bodkhe
>>> +
>>> +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
On Mon, Nov 11, 2024 at 7:18 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> 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@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>
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:
```
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
>
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@kernel.org> wrote:
> >
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> >
> > 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@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>
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
> >
© 2016 - 2024 Red Hat, Inc.