[PATCH] perf jevents: Fix sys_event_tables to be freed like arch_std_events

From: Like Xu <redacted>

The compiler reports that free_sys_event_tables() is dead code. But
according to the semantics, the "LIST_HEAD(arch_std_events)" should
also be released, just like we do with 'arch_std_events' in the main().

Fixes: e9d32c1bf0cd7a98 ("perf vendor events: Add support for arch standard events")
Signed-off-by: Like Xu <redacted>
---
 tools/perf/pmu-events/jevents.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 6731b3cf0c2f..7c887d37b893 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c

@@ -1285,6 +1285,7 @@ int main(int argc, char *argv[])
 	}
 
 	free_arch_std_events();
+	free_sys_event_tables();
 	free(mapfile);
 	return 0;
 

@@ -1306,6 +1307,7 @@ int main(int argc, char *argv[])
 		create_empty_mapping(output_file);
 err_out:
 	free_arch_std_events();
+	free_sys_event_tables();
 	free(mapfile);
 	return ret;
 }

-- 
2.32.0

Em Tue, Sep 28, 2021 at 06:29:38PM +0800, Like Xu escreveu:

From: Like Xu <redacted>

The compiler reports that free_sys_event_tables() is dead code. But
according to the semantics, the "LIST_HEAD(arch_std_events)" should
also be released, just like we do with 'arch_std_events' in the main().

Thanks, applied.

- Arnaldo

 

-- 

- Arnaldo

On 28/9/2021 7:52 pm, Arnaldo Carvalho de Melo wrote:

Em Tue, Sep 28, 2021 at 06:29:38PM +0800, Like Xu escreveu:

quoted

From: Like Xu <redacted>

The compiler reports that free_sys_event_tables() is dead code. But
according to the semantics, the "LIST_HEAD(arch_std_events)" should

Sorry, s/arch_std_events/sys_event_tables/, please --amend.

quoted

also be released, just like we do with 'arch_std_events' in the main().

Thanks, applied.

- Arnaldo

  

quoted

Fixes: e9d32c1bf0cd7a98 ("perf vendor events: Add support for arch standard events")
Signed-off-by: Like Xu <redacted>
---
  tools/perf/pmu-events/jevents.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 6731b3cf0c2f..7c887d37b893 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c

@@ -1285,6 +1285,7 @@ int main(int argc, char *argv[])
  	}
  
  	free_arch_std_events();
+	free_sys_event_tables();
  	free(mapfile);
  	return 0;
  

@@ -1306,6 +1307,7 @@ int main(int argc, char *argv[])
  		create_empty_mapping(output_file);
  err_out:
  	free_arch_std_events();
+	free_sys_event_tables();
  	free(mapfile);
  	return ret;
  }

-- 
2.32.0

Em Tue, Sep 28, 2021 at 07:53:54PM +0800, Like Xu escreveu:

On 28/9/2021 7:52 pm, Arnaldo Carvalho de Melo wrote:

quoted

Em Tue, Sep 28, 2021 at 06:29:38PM +0800, Like Xu escreveu:

quoted

From: Like Xu <redacted>

The compiler reports that free_sys_event_tables() is dead code. But
according to the semantics, the "LIST_HEAD(arch_std_events)" should

Sorry, s/arch_std_events/sys_event_tables/, please --amend.

sure
 

quoted

quoted

also be released, just like we do with 'arch_std_events' in the main().

Thanks, applied.

- Arnaldo

quoted

Fixes: e9d32c1bf0cd7a98 ("perf vendor events: Add support for arch standard events")
Signed-off-by: Like Xu <redacted>
---
  tools/perf/pmu-events/jevents.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 6731b3cf0c2f..7c887d37b893 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c

@@ -1285,6 +1285,7 @@ int main(int argc, char *argv[])
  	}
  	free_arch_std_events();
+	free_sys_event_tables();
  	free(mapfile);
  	return 0;

@@ -1306,6 +1307,7 @@ int main(int argc, char *argv[])
  		create_empty_mapping(output_file);
  err_out:
  	free_arch_std_events();
+	free_sys_event_tables();
  	free(mapfile);
  	return ret;
  }

-- 
2.32.0

-- 

- Arnaldo

On 28/09/2021 12:52, Arnaldo Carvalho de Melo wrote:

Em Tue, Sep 28, 2021 at 06:29:38PM +0800, Like Xu escreveu:

quoted

From: Like Xu <redacted>

The compiler reports that free_sys_event_tables() is dead code. But
according to the semantics, the "LIST_HEAD(arch_std_events)" should
also be released, just like we do with 'arch_std_events' in the main().

Thanks, applied.

- Arnaldo

If not too late:
Reviewed-by: John Garry <redacted>

I think that it could be a good idea to raise gcc warning level to 
detect unused static functions, like this was

thanks

quoted

Fixes: e9d32c1bf0cd7a98 ("perf vendor events: Add support for arch standard events")
Signed-off-by: Like Xu <redacted>
---
  tools/perf/pmu-events/jevents.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 6731b3cf0c2f..7c887d37b893 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c

@@ -1285,6 +1285,7 @@ int main(int argc, char *argv[])
  	}
  
  	free_arch_std_events();
+	free_sys_event_tables();
  	free(mapfile);
  	return 0;
  

@@ -1306,6 +1307,7 @@ int main(int argc, char *argv[])
  		create_empty_mapping(output_file);
  err_out:
  	free_arch_std_events();
+	free_sys_event_tables();
  	free(mapfile);
  	return ret;
  }

-- 
2.32.0

Em Tue, Sep 28, 2021 at 01:49:20PM +0100, John Garry escreveu:

On 28/09/2021 12:52, Arnaldo Carvalho de Melo wrote:

quoted

Em Tue, Sep 28, 2021 at 06:29:38PM +0800, Like Xu escreveu:

quoted

From: Like Xu <redacted>

The compiler reports that free_sys_event_tables() is dead code. But
according to the semantics, the "LIST_HEAD(arch_std_events)" should
also be released, just like we do with 'arch_std_events' in the main().

Thanks, applied.

- Arnaldo

If not too late:
Reviewed-by: John Garry <redacted>

Not too late, collected.
 

I think that it could be a good idea to raise gcc warning level to detect
unused static functions, like this was

Agreed, but we already have:

CORE_CFLAGS += -Wall
CORE_CFLAGS += -Wextra

We can se it for this specific case with:

$ make V=1 -k BUILD_BPF_SKEL=1 CORESIGHT=1 PYTHON=python3 O=/tmp/build/perf -C tools/perf install-bin | grep jevents
make -f /var/home/acme/git/perf/tools/build/Makefile.build dir=pmu-events obj=jevents
  gcc -Wp,-MD,/tmp/build/perf/pmu-events/.jevents.o.d -Wp,-MT,/tmp/build/perf/pmu-events/jevents.o  -D"BUILD_STR(s)=#s" -I/var/home/acme/git/perf/tools/include  -c -o /tmp/build/perf/pmu-events/jevents.o pmu-events/jevents.c
   ld -r -o /tmp/build/perf/pmu-events/jevents-in.o  /tmp/build/perf/pmu-events/json.o /tmp/build/perf/pmu-events/jsmn.o /tmp/build/perf/pmu-events/jevents.o
gcc /tmp/build/perf/pmu-events/jevents-in.o -o /tmp/build/perf/pmu-events/jevents
/tmp/build/perf/pmu-events/jevents x86 pmu-events/arch /tmp/build/perf/pmu-events/pmu-events.c 1
jevents: Processing mapfile pmu-events/arch/x86/mapfile.csv

Humm... no "-Wall -Wextra" there... lemme try to fix it

- Arnaldo
 

thanks

quoted

quoted

Fixes: e9d32c1bf0cd7a98 ("perf vendor events: Add support for arch standard events")
Signed-off-by: Like Xu <redacted>
---
  tools/perf/pmu-events/jevents.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 6731b3cf0c2f..7c887d37b893 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c

@@ -1285,6 +1285,7 @@ int main(int argc, char *argv[])
  	}
  	free_arch_std_events();
+	free_sys_event_tables();
  	free(mapfile);
  	return 0;

@@ -1306,6 +1307,7 @@ int main(int argc, char *argv[])
  		create_empty_mapping(output_file);
  err_out:
  	free_arch_std_events();
+	free_sys_event_tables();
  	free(mapfile);
  	return ret;
  }

-- 
2.32.0

-- 

- Arnaldo

Em Tue, Sep 28, 2021 at 10:16:48AM -0300, Arnaldo Carvalho de Melo escreveu:

Em Tue, Sep 28, 2021 at 01:49:20PM +0100, John Garry escreveu:

quoted

On 28/09/2021 12:52, Arnaldo Carvalho de Melo wrote:

quoted

Em Tue, Sep 28, 2021 at 06:29:38PM +0800, Like Xu escreveu:

quoted

From: Like Xu <redacted>

The compiler reports that free_sys_event_tables() is dead code. But
according to the semantics, the "LIST_HEAD(arch_std_events)" should
also be released, just like we do with 'arch_std_events' in the main().

Thanks, applied.

- Arnaldo

If not too late:
Reviewed-by: John Garry <redacted>

Not too late, collected.
 

quoted

I think that it could be a good idea to raise gcc warning level to detect
unused static functions, like this was

Agreed, but we already have:

CORE_CFLAGS += -Wall
CORE_CFLAGS += -Wextra

We can se it for this specific case with:

$ make V=1 -k BUILD_BPF_SKEL=1 CORESIGHT=1 PYTHON=python3 O=/tmp/build/perf -C tools/perf install-bin | grep jevents
make -f /var/home/acme/git/perf/tools/build/Makefile.build dir=pmu-events obj=jevents
  gcc -Wp,-MD,/tmp/build/perf/pmu-events/.jevents.o.d -Wp,-MT,/tmp/build/perf/pmu-events/jevents.o  -D"BUILD_STR(s)=#s" -I/var/home/acme/git/perf/tools/include  -c -o /tmp/build/perf/pmu-events/jevents.o pmu-events/jevents.c
   ld -r -o /tmp/build/perf/pmu-events/jevents-in.o  /tmp/build/perf/pmu-events/json.o /tmp/build/perf/pmu-events/jsmn.o /tmp/build/perf/pmu-events/jevents.o
gcc /tmp/build/perf/pmu-events/jevents-in.o -o /tmp/build/perf/pmu-events/jevents
/tmp/build/perf/pmu-events/jevents x86 pmu-events/arch /tmp/build/perf/pmu-events/pmu-events.c 1
jevents: Processing mapfile pmu-events/arch/x86/mapfile.csv

Humm... no "-Wall -Wextra" there... lemme try to fix it

With this:

⬢[acme@toolbox perf]$ git diff

diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build
index a055dee6a46af77e..ea7107630bf4327f 100644
--- a/tools/perf/pmu-events/Build
+++ b/tools/perf/pmu-events/Build

@@ -1,7 +1,7 @@
 hostprogs := jevents

 jevents-y      += json.o jsmn.o jevents.o
-HOSTCFLAGS_jevents.o   = -I$(srctree)/tools/include
+HOSTCFLAGS_jevents.o   = -I$(srctree)/tools/include -Wall -Wextra
 pmu-events-y   += pmu-events.o
 JDIR           =  pmu-events/arch/$(SRCARCH)
 JSON           =  $(shell [ -d $(JDIR) ] &&                            \

⬢[acme@toolbox perf]$

I get this before applying Xu's patch:

  LINK    /tmp/build/perf/libbpf.a
pmu-events/jevents.c: In function ‘save_arch_std_events’:
pmu-events/jevents.c:473:39: warning: unused parameter ‘data’ [-Wunused-parameter]
  473 | static int save_arch_std_events(void *data, struct json_event *je)
      |                                 ~~~~~~^~~~
At top level:
pmu-events/jevents.c:93:13: warning: ‘free_sys_event_tables’ defined but not used [-Wunused-function]
   93 | static void free_sys_event_tables(void)
      |             ^~~~~~~~~~~~~~~~~~~~~


-------------------------------------

I'll add this to perf/core, as this isn't a strict fix, so can wait for
v5.16.

- Arnaldo

On 28/09/2021 14:22, Arnaldo Carvalho de Melo wrote:

  jevents-y      += json.o jsmn.o jevents.o
-HOSTCFLAGS_jevents.o   = -I$(srctree)/tools/include
+HOSTCFLAGS_jevents.o   = -I$(srctree)/tools/include -Wall -Wextra
  pmu-events-y   += pmu-events.o
  JDIR           =  pmu-events/arch/$(SRCARCH)
  JSON           =  $(shell [ -d $(JDIR) ] &&                            \
⬢[acme@toolbox perf]$

I get this before applying Xu's patch:

   LINK    /tmp/build/perf/libbpf.a
pmu-events/jevents.c: In function ‘save_arch_std_events’:
pmu-events/jevents.c:473:39: warning: unused parameter ‘data’ [-Wunused-parameter]
   473 | static int save_arch_std_events(void *data, struct json_event *je)
       |                                 ~~~~~~^~~~
At top level:
pmu-events/jevents.c:93:13: warning: ‘free_sys_event_tables’ defined but not used [-Wunused-function]
    93 | static void free_sys_event_tables(void)
       |             ^~~~~~~~~~~~~~~~~~~~~


-------------------------------------

I'll add this to perf/core, as this isn't a strict fix, so can wait for
v5.16.

Hi Arnaldo,

OK, would you also consider reusing CFLAGS:

--- a/tools/perf/pmu-events/Build
+++ b/tools/perf/pmu-events/Build

@@ -9,10 +9,12 @@ JSON          =  $(shell [ -d $(JDIR) ] && 
                \

JDIR_TEST      =  pmu-events/arch/test
JSON_TEST      =  $(shell [ -d $(JDIR_TEST) ] &&                       \
                        find $(JDIR_TEST) -name '*.json')
-
+HOSTCFLAGS_jevents += $(CFLAGS)

I tried it, and there are more things to fix for jevents.o. Let me know 
your preference and if any help required to fix any errors up.

Thanks!

Em Tue, Sep 28, 2021 at 02:32:02PM +0100, John Garry escreveu:

Humm, we have to check if CFLAGS doesn't come with cross-build options,
i.e. IIRC we have to use HOSTCFLAGS instead. Unsure if there is some
*CFLAGS variable that gets the common part, where these -Wall and
-Wextra, -Werror could go.
 

I tried it, and there are more things to fix for jevents.o. Let me know your
preference and if any help required to fix any errors up.

I fixed the one I found, see below, I'll test build what I have in
perf/core and push it, then you can continue from there, after checking
this HOSTCFLAGS/CFLAGS issue.

- Arnaldo

From 0e46c8307574a8e2dac8d7ba97e0f6f4bbee67a5 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <redacted>
Date: Tue, 28 Sep 2021 14:15:01 -0300
Subject: [PATCH 1/1] perf jevents: Add __maybe_unused attribute to unused
 function arg

The tools/perf/pmu-events/jevents.c file isn't being compiled with
-Werror and -Wextra, which will be the case soon, so before we turn
those compiler flags on, fix what it would flag.

Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <redacted>
Cc: Like Xu <redacted>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Arnaldo Carvalho de Melo <redacted>
To: John Garry <redacted>
---
 tools/perf/pmu-events/jevents.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 6731b3cf0c2fc9b7..323e1dfe2436c049 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c

@@ -45,6 +45,7 @@
 #include <sys/resource.h>		/* getrlimit */
 #include <ftw.h>
 #include <sys/stat.h>
+#include <linux/compiler.h>
 #include <linux/list.h>
 #include "jsmn.h"
 #include "json.h"

@@ -470,7 +471,7 @@ static void free_arch_std_events(void)
 	}
 }
 
-static int save_arch_std_events(void *data, struct json_event *je)
+static int save_arch_std_events(void *data __maybe_unused, struct json_event *je)
 {
 	struct event_struct *es;
 

-- 
2.31.1

On 28/09/2021 18:56, Arnaldo Carvalho de Melo wrote:

quoted

quoted

  ‘save_arch_std_events’:
pmu-events/jevents.c:473:39: warning: unused parameter ‘data’ [-Wunused-parameter]
    473 | static int save_arch_std_events(void *data, struct json_event *je)
        |                                 ~~~~~~^~~~
At top level:
pmu-events/jevents.c:93:13: warning: ‘free_sys_event_tables’ defined but not used [-Wunused-function]
     93 | static void free_sys_event_tables(void)
        |             ^~~~~~~~~~~~~~~~~~~~~


-------------------------------------

I'll add this to perf/core, as this isn't a strict fix, so can wait for
v5.16.

Hi Arnaldo,

OK, would you also consider reusing CFLAGS:

--- a/tools/perf/pmu-events/Build
+++ b/tools/perf/pmu-events/Build

@@ -9,10 +9,12 @@ JSON          =  $(shell [ -d $(JDIR) ] &&

\
JDIR_TEST      =  pmu-events/arch/test
JSON_TEST      =  $(shell [ -d $(JDIR_TEST) ] &&                       \
                        find $(JDIR_TEST) -name '*.json')
-
+HOSTCFLAGS_jevents += $(CFLAGS)

Humm, we have to check if CFLAGS doesn't come with cross-build options,
i.e. IIRC we have to use HOSTCFLAGS instead. Unsure if there is some
*CFLAGS variable that gets the common part, where these -Wall and
-Wextra, -Werror could go.

not sure. As I see, the bulk of flags we have in CFLAGS comes from 
EXTRA_WARNINGS in scripts/Makefile.include; but CFLAGS seems to also 
include EXTRA_CLAGS, which are for cross-builds (see perf/Makefile.config)

So maybe we just need to use EXTRA_WARNINGS for HOST_CFLAGS

quoted

I tried it, and there are more things to fix for jevents.o. Let me know your
preference and if any help required to fix any errors up.

I fixed the one I found, see below, I'll test build what I have in
perf/core and push it, then you can continue from there, after checking
this HOSTCFLAGS/CFLAGS issue.

- Arnaldo

quoted

From 0e46c8307574a8e2dac8d7ba97e0f6f4bbee67a5 Mon Sep 17 00:00:00 2001

From: Arnaldo Carvalho de Melo<redacted>
Date: Tue, 28 Sep 2021 14:15:01 -0300
Subject: [PATCH 1/1] perf jevents: Add __maybe_unused attribute to unused
  function arg

The tools/perf/pmu-events/jevents.c file isn't being compiled with
-Werror and -Wextra, which will be the case soon, so before we turn
those compiler flags on, fix what it would flag.

Cc: Alexander Shishkin<alexander.shishkin@linux.intel.com>
Cc: Ingo Molnar<mingo@redhat.com>
Cc: Jiri Olsa<redacted>
Cc: Like Xu<redacted>
Cc: Mark Rutland<mark.rutland@arm.com>
Cc: Namhyung Kim<namhyung@kernel.org>
Cc: Peter Zijlstra<peterz@infradead.org>
Signed-off-by: Arnaldo Carvalho de Melo<redacted>
To: John Garry<redacted>

thanks.

Reviewed-by: John Garry <redacted>

On 28/09/2021 21:30, John Garry wrote:

quoted

quoted

OK, would you also consider reusing CFLAGS:

--- a/tools/perf/pmu-events/Build
+++ b/tools/perf/pmu-events/Build

@@ -9,10 +9,12 @@ JSON          =  $(shell [ -d $(JDIR) ] &&

\
JDIR_TEST      =  pmu-events/arch/test
JSON_TEST      =  $(shell [ -d $(JDIR_TEST) ] &&                       \
                        find $(JDIR_TEST) -name '*.json')
-
+HOSTCFLAGS_jevents += $(CFLAGS)

Humm, we have to check if CFLAGS doesn't come with cross-build options,
i.e. IIRC we have to use HOSTCFLAGS instead. Unsure if there is some
*CFLAGS variable that gets the common part, where these -Wall and
-Wextra, -Werror could go.

not sure. As I see, the bulk of flags we have in CFLAGS comes from 
EXTRA_WARNINGS in scripts/Makefile.include; but CFLAGS seems to also 
include EXTRA_CLAGS, which are for cross-builds (see perf/Makefile.config)

Hi Arnaldo,

I'm just looking at enabling warning cflags for jevents again.

So how about this:

--->8----


Subject: [PATCH] perf pmu-events: Enable jevents warnings through HOSTCFLAGS

diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index 0ae2e3d8b832..65934984f032 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config

@@ -17,6 +17,7 @@ detected     = $(shell echo "$(1)=y"       >> 

$(OUTPUT).config-detected)
  detected_var = $(shell echo "$(1)=$($(1))" >> $(OUTPUT).config-detected)

  CFLAGS := $(EXTRA_CFLAGS) $(filter-out -Wnested-externs,$(EXTRA_WARNINGS))
+HOSTCFLAGS := $(filter-out -Wnested-externs,$(EXTRA_WARNINGS))

  include $(srctree)/tools/scripts/Makefile.arch

diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 7df13e74450c..118bcdc70bb4 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf

@@ -226,7 +226,7 @@ else
  endif

  export srctree OUTPUT RM CC CXX LD AR CFLAGS CXXFLAGS V BISON FLEX AWK
-export HOSTCC HOSTLD HOSTAR
+export HOSTCC HOSTLD HOSTAR HOSTCFLAGS

  include $(srctree)/tools/build/Makefile.include

diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build
index a055dee6a46a..d5c287f069a2 100644
--- a/tools/perf/pmu-events/Build
+++ b/tools/perf/pmu-events/Build

@@ -1,7 +1,7 @@
  hostprogs := jevents

  jevents-y	+= json.o jsmn.o jevents.o
-HOSTCFLAGS_jevents.o	= -I$(srctree)/tools/include
+HOSTCFLAGS_jevents.o	= -I$(srctree)/tools/include $(HOSTCFLAGS)
  pmu-events-y	+= pmu-events.o
  JDIR		=  pmu-events/arch/$(SRCARCH)
  JSON		=  $(shell [ -d $(JDIR) ] &&				\

-- 
2.26.2

---8<---

The newly generated warnings in jevents.c are pretty straightforward to 
tidy up.

Thanks,
John