[PATCH 0/4] perf tools: Assorted random updates and fixes

[PATCH 0/4] perf tools: Assorted random updates and fixes

[Namhyung Kim]

Hello,

I found some misc updates and fixes are still remaining.  Just sending out
together.  Please take a look!

Thanks,
Namhyung


Namhyung Kim (4):
  perf report: Fix condition in sort__sym_cmp()
  perf symbol: Simplify kernel module checking
  perf tools: Fix a compiler warning of NULL pointer
  perf mem: Fix a segfault with NULL event->name

 tools/perf/util/dsos.c       | 3 +++
 tools/perf/util/mem-events.c | 2 +-
 tools/perf/util/sort.c       | 2 +-
 tools/perf/util/symbol.c     | 5 +----
 4 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.45.2.741.gdbec12cfda-goog

[PATCH 1/4] perf report: Fix condition in sort__sym_cmp()

[Namhyung Kim]

It's expected that both hist entries are in the same hists when
comparing two.  But the current code in the function checks one without
dso sort key and other with the key.  This would make the condition true
in any case.

I guess the intention of the original commit was to add '!' for the
right side too.  But as it should be the same, let's just remove it.

Fixes: 69849fc5d2119 ("perf hists: Move sort__has_dso into struct perf_hpp_list")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index cd39ea972193..ab7c7ff35f9b 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -334,7 +334,7 @@ sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
 	 * comparing symbol address alone is not enough since it's a
 	 * relative address within a dso.
 	 */
-	if (!hists__has(left->hists, dso) || hists__has(right->hists, dso)) {
+	if (!hists__has(left->hists, dso)) {
 		ret = sort__dso_cmp(left, right);
 		if (ret != 0)
 			return ret;
-- 
2.45.2.741.gdbec12cfda-goog

[PATCH 2/4] perf symbol: Simplify kernel module checking

[Namhyung Kim]

In dso__load(), it checks if the dso is a kernel module by looking the
symtab type.  Actually dso has 'is_kmod' field to check that easily and
dso__set_module_info() set the symtab type and the is_kmod bit.  So it
should have the same result to check the is_kmod bit.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 8e2da48eeaf0..25dfad79b5d7 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1816,10 +1816,7 @@ int dso__load(struct dso *dso, struct map *map)
 		goto out;
 	}
 
-	kmod = dso__symtab_type(dso) == DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE ||
-		dso__symtab_type(dso) == DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE_COMP ||
-		dso__symtab_type(dso) == DSO_BINARY_TYPE__GUEST_KMODULE ||
-		dso__symtab_type(dso) == DSO_BINARY_TYPE__GUEST_KMODULE_COMP;
+	kmod = dso__is_kmod(dso);
 
 	if (dso__kernel(dso) && !kmod) {
 		if (dso__kernel(dso) == DSO_SPACE__KERNEL)
-- 
2.45.2.741.gdbec12cfda-goog

[PATCH 3/4] perf tools: Fix a compiler warning of NULL pointer

[Namhyung Kim]

A compiler warning on the second argument of bsearch() should not be
NULL, but there's a case we might pass it.  Let's return early if we
don't have any DSOs to search in __dsos__find_by_longname_id().

  util/dsos.c:184:8: runtime error: null pointer passed as argument 2, which is declared to never be null

Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202406180932.84be448c-oliver.sang@intel.com
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dsos.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index ab3d0c01dd63..d5d78bdc56b2 100644
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -164,6 +164,9 @@ static struct dso *__dsos__find_by_longname_id(struct dsos *dsos,
 	};
 	struct dso **res;
 
+	if (dsos->dsos == NULL)
+		return NULL;
+
 	if (!dsos->sorted) {
 		if (!write_locked) {
 			struct dso *dso;
-- 
2.45.2.741.gdbec12cfda-goog

[PATCH 4/4] perf mem: Fix a segfault with NULL event->name

[Namhyung Kim]

Guilherme reported a crash in perf mem record.  It's because the
perf_mem_event->name was NULL on his machine.  It should just return
a NULL string when it has no format string in the name.

But I'm not sure why it returns TRUE if it doesn't have event_name in
perf_pmu__mem_events_supported().

The backtrace at the crash is below:

  Program received signal SIGSEGV, Segmentation fault.
  __strchrnul_avx2 () at ../sysdeps/x86_64/multiarch/strchr-avx2.S:67
  67              vmovdqu (%rdi), %ymm2
  (gdb) bt
  #0  __strchrnul_avx2 () at ../sysdeps/x86_64/multiarch/strchr-avx2.S:67
  #1  0x00007ffff6c982de in __find_specmb (format=0x0) at printf-parse.h:82
  #2  __printf_buffer (buf=buf@entry=0x7fffffffc760, format=format@entry=0x0, ap=ap@entry=0x7fffffffc880,
      mode_flags=mode_flags@entry=0) at vfprintf-internal.c:649
  #3  0x00007ffff6cb7840 in __vsnprintf_internal (string=<optimized out>, maxlen=<optimized out>, format=0x0,
      args=0x7fffffffc880, mode_flags=mode_flags@entry=0) at vsnprintf.c:96
  #4  0x00007ffff6cb787f in ___vsnprintf (string=<optimized out>, maxlen=<optimized out>, format=<optimized out>,
      args=<optimized out>) at vsnprintf.c:103
  #5  0x00005555557b9391 in scnprintf (buf=0x555555fe9320 <mem_loads_name> "", size=100, fmt=0x0)
      at ../lib/vsprintf.c:21
  #6  0x00005555557b74c3 in perf_pmu__mem_events_name (i=0, pmu=0x555556832180) at util/mem-events.c:106
  #7  0x00005555557b7ab9 in perf_mem_events__record_args (rec_argv=0x55555684c000, argv_nr=0x7fffffffca20)
      at util/mem-events.c:252
  #8  0x00005555555e370d in __cmd_record (argc=3, argv=0x7fffffffd760, mem=0x7fffffffcd80) at builtin-mem.c:156
  #9  0x00005555555e49c4 in cmd_mem (argc=4, argv=0x7fffffffd760) at builtin-mem.c:514
  #10 0x000055555569716c in run_builtin (p=0x555555fcde80 <commands+672>, argc=8, argv=0x7fffffffd760) at perf.c:349
  #11 0x0000555555697402 in handle_internal_command (argc=8, argv=0x7fffffffd760) at perf.c:402
  #12 0x0000555555697560 in run_argv (argcp=0x7fffffffd59c, argv=0x7fffffffd590) at perf.c:446
  #13 0x00005555556978a6 in main (argc=8, argv=0x7fffffffd760) at perf.c:562

Reported-by: Guilherme Amadio <amadio@cern.ch>
Closes: https://lore.kernel.org/linux-perf-users/Zlns_o_IE5L28168@cern.ch
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/mem-events.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index 6dda47bb774f..429079329e48 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -86,7 +86,7 @@ static const char *perf_pmu__mem_events_name(int i, struct perf_pmu *pmu)
 		return NULL;
 
 	e = &pmu->mem_events[i];
-	if (!e)
+	if (!e || !e->name)
 		return NULL;
 
 	if (i == PERF_MEM_EVENTS__LOAD || i == PERF_MEM_EVENTS__LOAD_STORE) {
-- 
2.45.2.741.gdbec12cfda-goog

Re: [PATCH 4/4] perf mem: Fix a segfault with NULL event->name

[Liang, Kan]

On 2024-06-21 1:05 p.m., Namhyung Kim wrote:
> Guilherme reported a crash in perf mem record.  It's because the
> perf_mem_event->name was NULL on his machine.  It should just return
> a NULL string when it has no format string in the name.
> 
> But I'm not sure why it returns TRUE if it doesn't have event_name in
> perf_pmu__mem_events_supported().

AMD doesn't have the event_name.

struct perf_mem_event perf_mem_events_amd[PERF_MEM_EVENTS__MAX] = {
	E(NULL,		NULL,		NULL,	false,	0),
	E(NULL,		NULL,		NULL,	false,	0),
	E("mem-ldst",	"%s//",		NULL,	false,	0),
};

It looks like as long as it's a "ibs_op" PMU, the mem event is always
available. The "ibs_op" check has been moved into perf_pmu__arch_init().
So the event_name is empty.

The "e->tag" in the perf_pmu__mem_events_init() can help to skip the
case  E(NULL,		NULL,		NULL,	false,	0).
So the perf_pmu__mem_events_supported() returns TRUE for the !event_name.

Thanks,
Kan
> 
> The backtrace at the crash is below:
> 
>   Program received signal SIGSEGV, Segmentation fault.
>   __strchrnul_avx2 () at ../sysdeps/x86_64/multiarch/strchr-avx2.S:67
>   67              vmovdqu (%rdi), %ymm2
>   (gdb) bt
>   #0  __strchrnul_avx2 () at ../sysdeps/x86_64/multiarch/strchr-avx2.S:67
>   #1  0x00007ffff6c982de in __find_specmb (format=0x0) at printf-parse.h:82
>   #2  __printf_buffer (buf=buf@entry=0x7fffffffc760, format=format@entry=0x0, ap=ap@entry=0x7fffffffc880,
>       mode_flags=mode_flags@entry=0) at vfprintf-internal.c:649
>   #3  0x00007ffff6cb7840 in __vsnprintf_internal (string=<optimized out>, maxlen=<optimized out>, format=0x0,
>       args=0x7fffffffc880, mode_flags=mode_flags@entry=0) at vsnprintf.c:96
>   #4  0x00007ffff6cb787f in ___vsnprintf (string=<optimized out>, maxlen=<optimized out>, format=<optimized out>,
>       args=<optimized out>) at vsnprintf.c:103
>   #5  0x00005555557b9391 in scnprintf (buf=0x555555fe9320 <mem_loads_name> "", size=100, fmt=0x0)
>       at ../lib/vsprintf.c:21
>   #6  0x00005555557b74c3 in perf_pmu__mem_events_name (i=0, pmu=0x555556832180) at util/mem-events.c:106
>   #7  0x00005555557b7ab9 in perf_mem_events__record_args (rec_argv=0x55555684c000, argv_nr=0x7fffffffca20)
>       at util/mem-events.c:252
>   #8  0x00005555555e370d in __cmd_record (argc=3, argv=0x7fffffffd760, mem=0x7fffffffcd80) at builtin-mem.c:156
>   #9  0x00005555555e49c4 in cmd_mem (argc=4, argv=0x7fffffffd760) at builtin-mem.c:514
>   #10 0x000055555569716c in run_builtin (p=0x555555fcde80 <commands+672>, argc=8, argv=0x7fffffffd760) at perf.c:349
>   #11 0x0000555555697402 in handle_internal_command (argc=8, argv=0x7fffffffd760) at perf.c:402
>   #12 0x0000555555697560 in run_argv (argcp=0x7fffffffd59c, argv=0x7fffffffd590) at perf.c:446
>   #13 0x00005555556978a6 in main (argc=8, argv=0x7fffffffd760) at perf.c:562
> 
> Reported-by: Guilherme Amadio <amadio@cern.ch>
> Closes: https://lore.kernel.org/linux-perf-users/Zlns_o_IE5L28168@cern.ch
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/mem-events.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index 6dda47bb774f..429079329e48 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -86,7 +86,7 @@ static const char *perf_pmu__mem_events_name(int i, struct perf_pmu *pmu)
>  		return NULL;
>  
>  	e = &pmu->mem_events[i];
> -	if (!e)
> +	if (!e || !e->name)
>  		return NULL;
>  
>  	if (i == PERF_MEM_EVENTS__LOAD || i == PERF_MEM_EVENTS__LOAD_STORE) {

Re: [PATCH 4/4] perf mem: Fix a segfault with NULL event->name

[Namhyung Kim]

Hi Kan,

On Fri, Jun 21, 2024 at 1:11 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-06-21 1:05 p.m., Namhyung Kim wrote:
> > Guilherme reported a crash in perf mem record.  It's because the
> > perf_mem_event->name was NULL on his machine.  It should just return
> > a NULL string when it has no format string in the name.
> >
> > But I'm not sure why it returns TRUE if it doesn't have event_name in
> > perf_pmu__mem_events_supported().
>
> AMD doesn't have the event_name.
>
> struct perf_mem_event perf_mem_events_amd[PERF_MEM_EVENTS__MAX] = {
>         E(NULL,         NULL,           NULL,   false,  0),
>         E(NULL,         NULL,           NULL,   false,  0),
>         E("mem-ldst",   "%s//",         NULL,   false,  0),
> };
>
> It looks like as long as it's a "ibs_op" PMU, the mem event is always
> available. The "ibs_op" check has been moved into perf_pmu__arch_init().
> So the event_name is empty.
>
> The "e->tag" in the perf_pmu__mem_events_init() can help to skip the
> case  E(NULL,           NULL,           NULL,   false,  0).
> So the perf_pmu__mem_events_supported() returns TRUE for the !event_name.

Ok, thanks for the explanation!

Namhyung

Re: [PATCH 0/4] perf tools: Assorted random updates and fixes

[Liang, Kan]

On 2024-06-21 1:05 p.m., Namhyung Kim wrote:
> Hello,
> 
> I found some misc updates and fixes are still remaining.  Just sending out
> together.  Please take a look!
> 
> Thanks,
> Namhyung
> 
> 
> Namhyung Kim (4):
>   perf report: Fix condition in sort__sym_cmp()
>   perf symbol: Simplify kernel module checking
>   perf tools: Fix a compiler warning of NULL pointer
>   perf mem: Fix a segfault with NULL event->name

Looks good to me.

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

> 
>  tools/perf/util/dsos.c       | 3 +++
>  tools/perf/util/mem-events.c | 2 +-
>  tools/perf/util/sort.c       | 2 +-
>  tools/perf/util/symbol.c     | 5 +----
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 

Re: [PATCH 0/4] perf tools: Assorted random updates and fixes

[Namhyung Kim]

On Fri, 21 Jun 2024 10:05:24 -0700, Namhyung Kim wrote:

> I found some misc updates and fixes are still remaining.  Just sending out
> together.  Please take a look!
> 
> Thanks,
> Namhyung
> 
> 
> [...]

Applied to perf-tools-next, thanks!

Best regards,
Namhyung