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

Arnaldo Carvalho de Melo
2021-09-28T13:22:41Z

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:
> > On 28/09/2021 12:52, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Sep 28, 2021 at 06:29:38PM +0800, Like Xu escreveu:
> > > > From: Like Xu <likexu@tencent.com>
> > > >
> > > > 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 <john.garry@huawei.com>
>
> 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

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