Re: [PATCH v2 00/27] Constify tool pointers

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Jun 26, 2024 at 01:36:02PM -0700, Ian Rogers wrote:
> struct perf_tool provides a set of function pointers that are called
> through when processing perf data. To make filling the pointers less
> cumbersome, if they are NULL perf_tools__fill_defaults will add
> default do nothing implementations.
> 
> This change refactors struct perf_tool to have an init function that
> provides the default implementation. The special use of NULL and
> perf_tools__fill_defaults are removed. As a consequence the tool
> pointers can then all be made const, which better reflects the
> behavior a particular perf command would expect of the tool and to
> some extent can reduce the cognitive load on someone working on a
> command.

I thought you actually wanted to make the tool const (rodata) but it
seems you leave it as is but treat it as const.

I'm curious if we can change the event delivery code something like:

  if (tool->func)
      tool->func(...);
  else
      stub_func(...);

Then probably we don't need to touch the tool and make it const.
Thoughts?

Thanks,
Namhyung

> 
> v2: Remove dummy tool initialization [Adrian] and make zero sized. Add
>     cs-etm fix for address sanitizer build, found necessary when
>     testing dummy tool change.
> 
> Ian Rogers (27):
>   perf auxevent: Zero size dummy tool
>   perf cs-etm: Fix address sanitizer dso build failure
>   perf tool: Constify tool pointers
>   perf tool: Move fill defaults into tool.c
>   perf tool: Add perf_tool__init
>   perf kmem: Use perf_tool__init
>   perf buildid-list: Use perf_tool__init
>   perf kvm: Use perf_tool__init
>   perf lock: Use perf_tool__init
>   perf evlist: Use perf_tool__init
>   perf record: Use perf_tool__init
>   perf c2c: Use perf_tool__init
>   perf script: Use perf_tool__init
>   perf inject: Use perf_tool__init
>   perf report: Use perf_tool__init
>   perf stat: Use perf_tool__init
>   perf annotate: Use perf_tool__init
>   perf sched: Use perf_tool__init
>   perf mem: Use perf_tool__init
>   perf timechart: Use perf_tool__init
>   perf diff: Use perf_tool__init
>   perf data convert json: Use perf_tool__init
>   perf data convert ctf: Use perf_tool__init
>   perf test event_update: Ensure tools is initialized
>   perf kwork: Use perf_tool__init
>   perf tool: Remove perf_tool__fill_defaults
>   perf session: Constify tool
> 
>  tools/perf/arch/x86/util/event.c    |   4 +-
>  tools/perf/bench/synthesize.c       |   2 +-
>  tools/perf/builtin-annotate.c       |  44 ++--
>  tools/perf/builtin-buildid-list.c   |  10 +
>  tools/perf/builtin-c2c.c            |  33 ++-
>  tools/perf/builtin-diff.c           |  30 ++-
>  tools/perf/builtin-evlist.c         |  10 +-
>  tools/perf/builtin-inject.c         | 159 +++++++------
>  tools/perf/builtin-kmem.c           |  20 +-
>  tools/perf/builtin-kvm.c            |  19 +-
>  tools/perf/builtin-kwork.c          |  33 ++-
>  tools/perf/builtin-lock.c           |  41 ++--
>  tools/perf/builtin-mem.c            |  37 +--
>  tools/perf/builtin-record.c         |  47 ++--
>  tools/perf/builtin-report.c         |  67 +++---
>  tools/perf/builtin-sched.c          |  50 ++--
>  tools/perf/builtin-script.c         | 106 ++++-----
>  tools/perf/builtin-stat.c           |  26 +--
>  tools/perf/builtin-timechart.c      |  25 +-
>  tools/perf/builtin-top.c            |   2 +-
>  tools/perf/builtin-trace.c          |   4 +-
>  tools/perf/tests/cpumap.c           |   6 +-
>  tools/perf/tests/dlfilter-test.c    |   2 +-
>  tools/perf/tests/dwarf-unwind.c     |   2 +-
>  tools/perf/tests/event_update.c     |   9 +-
>  tools/perf/tests/stat.c             |   6 +-
>  tools/perf/tests/thread-map.c       |   2 +-
>  tools/perf/util/Build               |   1 +
>  tools/perf/util/arm-spe.c           |  14 +-
>  tools/perf/util/auxtrace.c          |  12 +-
>  tools/perf/util/auxtrace.h          |  20 +-
>  tools/perf/util/bpf-event.c         |   4 +-
>  tools/perf/util/build-id.c          |  34 +--
>  tools/perf/util/build-id.h          |   8 +-
>  tools/perf/util/cs-etm.c            |  24 +-
>  tools/perf/util/data-convert-bt.c   |  34 ++-
>  tools/perf/util/data-convert-json.c |  47 ++--
>  tools/perf/util/dso.h               |  10 +
>  tools/perf/util/event.c             |  54 +++--
>  tools/perf/util/event.h             |  38 ++--
>  tools/perf/util/header.c            |   6 +-
>  tools/perf/util/header.h            |   4 +-
>  tools/perf/util/hisi-ptt.c          |   6 +-
>  tools/perf/util/intel-bts.c         |  14 +-
>  tools/perf/util/intel-pt.c          |  15 +-
>  tools/perf/util/jitdump.c           |   4 +-
>  tools/perf/util/s390-cpumsf.c       |  11 +-
>  tools/perf/util/session.c           | 342 ++--------------------------
>  tools/perf/util/session.h           |   6 +-
>  tools/perf/util/synthetic-events.c  |  80 +++----
>  tools/perf/util/synthetic-events.h  |  70 +++---
>  tools/perf/util/tool.c              | 294 ++++++++++++++++++++++++
>  tools/perf/util/tool.h              |  18 +-
>  tools/perf/util/tsc.c               |   2 +-
>  54 files changed, 967 insertions(+), 1001 deletions(-)
>  create mode 100644 tools/perf/util/tool.c
> 
> -- 
> 2.45.2.741.gdbec12cfda-goog
>