The perf tool has a class of memory problems where reference counts
are used incorrectly. Memory/address sanitizers and valgrind don't
provide useful ways to debug these problems, you see a memory leak
where the only pertinent information is the original allocation
site. What would be more useful is knowing where a get fails to have a
corresponding put, where there are double puts, etc.

This work was motivated by the roll-back of:
https://lore.kernel.org/linux-perf-users/20211118193714.2293728-1-irogers@google.com/
where fixing a missed put resulted in a use-after-free in a different
context. There was a sense in fixing the issue that a game of
wac-a-mole had been embarked upon in adding missed gets and puts.

The basic approach of the change is to add a level of indirection at
the get and put calls. Get allocates a level of indirection that, if
no corresponding put is called, becomes a memory leak (and associated
stack trace) that leak sanitizer can report. Similarly if two puts are
called for the same get, then a double free can be detected by address
sanitizer. This can also detect the use after put, which should also
yield a segv without a sanitizer.

Adding reference count checking to cpu map was done as a proof of
concept, it yielded little other than a location where the use of get
could be cleaner by using its result. Reference count checking on
nsinfo identified a double free of the indirection layer and the
related threads, thereby identifying a data race as discussed here:
 https://lore.kernel.org/linux-perf-users/CAP-5=fWZH20L4kv-BwVtGLwR=Em3AOOT+Q4QGivvQuYn5AsPRg@mail.gmail.com/
Accordingly the dso->lock was extended and use to cover the race.

The v3 version addresses problems in v2, in particular using macros to
avoid #ifdefs. The v3 version applies the reference count checking
approach to two more data structures, maps and map. While maps was
straightforward, struct map showed a problem where reference counted
thing can be on lists and rb-trees that are oblivious to the
reference count. To sanitize this, struct map is changed so that it is
referenced by either a list or rb-tree node and not part of it. This
simplifies the reference count and the patches have caught and fixed a
number of missed or mismatched reference counts relating to struct
map.

The patches are arranged so that API refactors and bug fixes appear
first, then the reference count checker itself appears. This allows
for the refactor and fixes to be applied upstream first, as has
already happened with cpumap.

A wider discussion of the approach is on the mailing list:
 https://lore.kernel.org/linux-perf-users/YffqnynWcc5oFkI5@kernel.org/T/#mf25ccd7a2e03de92cec29d36e2999a8ab5ec7f88
Comparing it to a past approach:
 https://lore.kernel.org/all/20151209021047.10245.8918.stgit@localhost.localdomain/
and to ref_tracker:
 https://lwn.net/Articles/877603/

v5. rebase removing 5 merged changes. Add map_list_node__new to the
    1st patch (perf map: Move map list node into symbol) as suggested
    by Arnaldo. Remove unnecessary map__puts from patch 12 (perf map:
    Changes to reference counting) as suggested by Adrian. A summary
    of the sizes of the remaining patches is:
74fd7ffafdd0 perf map: Add reference count checking
 12 files changed, 136 insertions(+), 114 deletions(-)
4719196db8d3 perf maps: Add reference count checking.
 8 files changed, 64 insertions(+), 56 deletions(-)
03943e7594cf perf namespaces: Add reference count checking
 7 files changed, 83 insertions(+), 62 deletions(-)
0bb382cc52d7 perf cpumap: Add reference count checking
 6 files changed, 81 insertions(+), 71 deletions(-)
ef39f550c40d libperf: Add reference count checking macros.
 1 file changed, 94 insertions(+)
d9ac37c750e0 perf map: Changes to reference counting
 11 files changed, 112 insertions(+), 44 deletions(-)
476014bc9b55 perf maps: Modify maps_by_name to hold a reference to a map
 2 files changed, 33 insertions(+), 18 deletions(-)
91384676fddd perf test: Add extra diagnostics to maps test
 1 file changed, 36 insertions(+), 15 deletions(-)
fdc30434f826 perf map: Add accessors for pgoff and reloc
 9 files changed, 33 insertions(+), 23 deletions(-)
368fe015adb2 perf map: Add accessors for prot, priv and flags
 6 files changed, 28 insertions(+), 12 deletions(-)
2c6a8169826a perf map: Add helper for map_ip and unmap_ip
 23 files changed, 80 insertions(+), 65 deletions(-)
929e59d49f4b perf map: Rename map_ip and unmap_ip
 6 files changed, 13 insertions(+), 13 deletions(-)
4a38194aaaf5 perf map: Add accessor for start and end
 24 files changed, 114 insertions(+), 103 deletions(-)
02b63e5c415e perf map: Add accessor for dso
 48 files changed, 404 insertions(+), 293 deletions(-)
9324af6ccf42 perf maps: Add functions to access maps
 20 files changed, 175 insertions(+), 111 deletions(-)
5c590d36a308 perf maps: Remove rb_node from struct map
 16 files changed, 291 insertions(+), 184 deletions(-)
af1d142eb777 perf map: Move map list node into symbol
 2 files changed, 63 insertions(+), 35 deletions(-)
 
v4. rebases on to acme's perf-tools-next, fixes more issues with
    map/maps and breaks apart the accessor functions to reduce
    individual patch sizes. The accessor functions are mechanical
    changes where the single biggest one is refactoring use of
    map->dso to be map__dso(map).

The v3 change is available here:
https://lore.kernel.org/lkml/20220211103415.2737789-1-irogers@google.com/

Ian Rogers (17):
  perf map: Move map list node into symbol
  perf maps: Remove rb_node from struct map
  perf maps: Add functions to access maps
  perf map: Add accessor for dso
  perf map: Add accessor for start and end
  perf map: Rename map_ip and unmap_ip
  perf map: Add helper for map_ip and unmap_ip
  perf map: Add accessors for prot, priv and flags
  perf map: Add accessors for pgoff and reloc
  perf test: Add extra diagnostics to maps test
  perf maps: Modify maps_by_name to hold a reference to a map
  perf map: Changes to reference counting
  libperf: Add reference count checking macros.
  perf cpumap: Add reference count checking
  perf namespaces: Add reference count checking
  perf maps: Add reference count checking.
  perf map: Add reference count checking

 tools/lib/perf/Makefile                       |   2 +-
 tools/lib/perf/cpumap.c                       |  94 ++---
 tools/lib/perf/include/internal/cpumap.h      |   4 +-
 tools/lib/perf/include/internal/rc_check.h    |  94 +++++
 tools/perf/arch/s390/annotate/instructions.c  |   4 +-
 tools/perf/arch/x86/tests/dwarf-unwind.c      |   2 +-
 tools/perf/arch/x86/util/event.c              |  13 +-
 tools/perf/builtin-annotate.c                 |  11 +-
 tools/perf/builtin-buildid-list.c             |   4 +-
 tools/perf/builtin-inject.c                   |  12 +-
 tools/perf/builtin-kallsyms.c                 |   6 +-
 tools/perf/builtin-kmem.c                     |   4 +-
 tools/perf/builtin-lock.c                     |   4 +-
 tools/perf/builtin-mem.c                      |  10 +-
 tools/perf/builtin-report.c                   |  26 +-
 tools/perf/builtin-script.c                   |  27 +-
 tools/perf/builtin-top.c                      |  17 +-
 tools/perf/builtin-trace.c                    |   2 +-
 .../scripts/python/Perf-Trace-Util/Context.c  |  13 +-
 tools/perf/tests/code-reading.c               |  37 +-
 tools/perf/tests/cpumap.c                     |   4 +-
 tools/perf/tests/hists_common.c               |   8 +-
 tools/perf/tests/hists_cumulate.c             |  14 +-
 tools/perf/tests/hists_filter.c               |  14 +-
 tools/perf/tests/hists_link.c                 |  18 +-
 tools/perf/tests/hists_output.c               |  12 +-
 tools/perf/tests/maps.c                       |  69 ++--
 tools/perf/tests/mmap-thread-lookup.c         |   3 +-
 tools/perf/tests/symbols.c                    |   6 +-
 tools/perf/tests/thread-maps-share.c          |  29 +-
 tools/perf/tests/vmlinux-kallsyms.c           |  54 +--
 tools/perf/ui/browsers/annotate.c             |   9 +-
 tools/perf/ui/browsers/hists.c                |  19 +-
 tools/perf/ui/browsers/map.c                  |   4 +-
 tools/perf/util/annotate.c                    |  40 ++-
 tools/perf/util/auxtrace.c                    |   2 +-
 tools/perf/util/block-info.c                  |   4 +-
 tools/perf/util/bpf-event.c                   |  10 +-
 tools/perf/util/bpf_lock_contention.c         |   6 +-
 tools/perf/util/build-id.c                    |   2 +-
 tools/perf/util/callchain.c                   |  24 +-
 tools/perf/util/cpumap.c                      |  40 ++-
 tools/perf/util/data-convert-json.c           |  10 +-
 tools/perf/util/db-export.c                   |  16 +-
 tools/perf/util/dlfilter.c                    |  28 +-
 tools/perf/util/dso.c                         |   8 +-
 tools/perf/util/dsos.c                        |   2 +-
 tools/perf/util/event.c                       |  27 +-
 tools/perf/util/evsel_fprintf.c               |   4 +-
 tools/perf/util/hist.c                        |  22 +-
 tools/perf/util/intel-pt.c                    |  63 ++--
 tools/perf/util/machine.c                     | 252 ++++++++------
 tools/perf/util/map.c                         | 217 ++++++------
 tools/perf/util/map.h                         |  74 +++-
 tools/perf/util/maps.c                        | 318 ++++++++++-------
 tools/perf/util/maps.h                        |  67 +++-
 tools/perf/util/namespaces.c                  | 132 +++++---
 tools/perf/util/namespaces.h                  |   3 +-
 tools/perf/util/pmu.c                         |   8 +-
 tools/perf/util/probe-event.c                 |  62 ++--
 .../util/scripting-engines/trace-event-perl.c |  10 +-
 .../scripting-engines/trace-event-python.c    |  26 +-
 tools/perf/util/sort.c                        |  67 ++--
 tools/perf/util/symbol-elf.c                  |  41 ++-
 tools/perf/util/symbol.c                      | 320 +++++++++++-------
 tools/perf/util/symbol_fprintf.c              |   2 +-
 tools/perf/util/synthetic-events.c            |  34 +-
 tools/perf/util/thread-stack.c                |   4 +-
 tools/perf/util/thread.c                      |  39 +--
 tools/perf/util/unwind-libdw.c                |  20 +-
 tools/perf/util/unwind-libunwind-local.c      |  16 +-
 tools/perf/util/unwind-libunwind.c            |  33 +-
 tools/perf/util/vdso.c                        |   7 +-
 73 files changed, 1665 insertions(+), 1044 deletions(-)
 create mode 100644 tools/lib/perf/include/internal/rc_check.h

-- 
2.40.0.rc1.284.g88254d51c5-goog

Ping. It would be nice to have this landed or at least the first 10
patches that refactor the map API and are the bulk of the
lines-of-code changed. Having those landed would make it easier to
rebase in the future, but I also think the whole series is ready to
go.

Thanks,
Ian

On Mon, Mar 20, 2023 at 2:23 PM Ian Rogers <irogers@google.com> wrote:
>
> The perf tool has a class of memory problems where reference counts
> are used incorrectly. Memory/address sanitizers and valgrind don't
> provide useful ways to debug these problems, you see a memory leak
> where the only pertinent information is the original allocation
> site. What would be more useful is knowing where a get fails to have a
> corresponding put, where there are double puts, etc.
>
> This work was motivated by the roll-back of:
> https://lore.kernel.org/linux-perf-users/20211118193714.2293728-1-irogers@google.com/
> where fixing a missed put resulted in a use-after-free in a different
> context. There was a sense in fixing the issue that a game of
> wac-a-mole had been embarked upon in adding missed gets and puts.
>
> The basic approach of the change is to add a level of indirection at
> the get and put calls. Get allocates a level of indirection that, if
> no corresponding put is called, becomes a memory leak (and associated
> stack trace) that leak sanitizer can report. Similarly if two puts are
> called for the same get, then a double free can be detected by address
> sanitizer. This can also detect the use after put, which should also
> yield a segv without a sanitizer.
>
> Adding reference count checking to cpu map was done as a proof of
> concept, it yielded little other than a location where the use of get
> could be cleaner by using its result. Reference count checking on
> nsinfo identified a double free of the indirection layer and the
> related threads, thereby identifying a data race as discussed here:
>  https://lore.kernel.org/linux-perf-users/CAP-5=fWZH20L4kv-BwVtGLwR=Em3AOOT+Q4QGivvQuYn5AsPRg@mail.gmail.com/
> Accordingly the dso->lock was extended and use to cover the race.
>
> The v3 version addresses problems in v2, in particular using macros to
> avoid #ifdefs. The v3 version applies the reference count checking
> approach to two more data structures, maps and map. While maps was
> straightforward, struct map showed a problem where reference counted
> thing can be on lists and rb-trees that are oblivious to the
> reference count. To sanitize this, struct map is changed so that it is
> referenced by either a list or rb-tree node and not part of it. This
> simplifies the reference count and the patches have caught and fixed a
> number of missed or mismatched reference counts relating to struct
> map.
>
> The patches are arranged so that API refactors and bug fixes appear
> first, then the reference count checker itself appears. This allows
> for the refactor and fixes to be applied upstream first, as has
> already happened with cpumap.
>
> A wider discussion of the approach is on the mailing list:
>  https://lore.kernel.org/linux-perf-users/YffqnynWcc5oFkI5@kernel.org/T/#mf25ccd7a2e03de92cec29d36e2999a8ab5ec7f88
> Comparing it to a past approach:
>  https://lore.kernel.org/all/20151209021047.10245.8918.stgit@localhost.localdomain/
> and to ref_tracker:
>  https://lwn.net/Articles/877603/
>
> v5. rebase removing 5 merged changes. Add map_list_node__new to the
>     1st patch (perf map: Move map list node into symbol) as suggested
>     by Arnaldo. Remove unnecessary map__puts from patch 12 (perf map:
>     Changes to reference counting) as suggested by Adrian. A summary
>     of the sizes of the remaining patches is:
> 74fd7ffafdd0 perf map: Add reference count checking
>  12 files changed, 136 insertions(+), 114 deletions(-)
> 4719196db8d3 perf maps: Add reference count checking.
>  8 files changed, 64 insertions(+), 56 deletions(-)
> 03943e7594cf perf namespaces: Add reference count checking
>  7 files changed, 83 insertions(+), 62 deletions(-)
> 0bb382cc52d7 perf cpumap: Add reference count checking
>  6 files changed, 81 insertions(+), 71 deletions(-)
> ef39f550c40d libperf: Add reference count checking macros.
>  1 file changed, 94 insertions(+)
> d9ac37c750e0 perf map: Changes to reference counting
>  11 files changed, 112 insertions(+), 44 deletions(-)
> 476014bc9b55 perf maps: Modify maps_by_name to hold a reference to a map
>  2 files changed, 33 insertions(+), 18 deletions(-)
> 91384676fddd perf test: Add extra diagnostics to maps test
>  1 file changed, 36 insertions(+), 15 deletions(-)
> fdc30434f826 perf map: Add accessors for pgoff and reloc
>  9 files changed, 33 insertions(+), 23 deletions(-)
> 368fe015adb2 perf map: Add accessors for prot, priv and flags
>  6 files changed, 28 insertions(+), 12 deletions(-)
> 2c6a8169826a perf map: Add helper for map_ip and unmap_ip
>  23 files changed, 80 insertions(+), 65 deletions(-)
> 929e59d49f4b perf map: Rename map_ip and unmap_ip
>  6 files changed, 13 insertions(+), 13 deletions(-)
> 4a38194aaaf5 perf map: Add accessor for start and end
>  24 files changed, 114 insertions(+), 103 deletions(-)
> 02b63e5c415e perf map: Add accessor for dso
>  48 files changed, 404 insertions(+), 293 deletions(-)
> 9324af6ccf42 perf maps: Add functions to access maps
>  20 files changed, 175 insertions(+), 111 deletions(-)
> 5c590d36a308 perf maps: Remove rb_node from struct map
>  16 files changed, 291 insertions(+), 184 deletions(-)
> af1d142eb777 perf map: Move map list node into symbol
>  2 files changed, 63 insertions(+), 35 deletions(-)
>
> v4. rebases on to acme's perf-tools-next, fixes more issues with
>     map/maps and breaks apart the accessor functions to reduce
>     individual patch sizes. The accessor functions are mechanical
>     changes where the single biggest one is refactoring use of
>     map->dso to be map__dso(map).
>
> The v3 change is available here:
> https://lore.kernel.org/lkml/20220211103415.2737789-1-irogers@google.com/
>
> Ian Rogers (17):
>   perf map: Move map list node into symbol
>   perf maps: Remove rb_node from struct map
>   perf maps: Add functions to access maps
>   perf map: Add accessor for dso
>   perf map: Add accessor for start and end
>   perf map: Rename map_ip and unmap_ip
>   perf map: Add helper for map_ip and unmap_ip
>   perf map: Add accessors for prot, priv and flags
>   perf map: Add accessors for pgoff and reloc
>   perf test: Add extra diagnostics to maps test
>   perf maps: Modify maps_by_name to hold a reference to a map
>   perf map: Changes to reference counting
>   libperf: Add reference count checking macros.
>   perf cpumap: Add reference count checking
>   perf namespaces: Add reference count checking
>   perf maps: Add reference count checking.
>   perf map: Add reference count checking
>
>  tools/lib/perf/Makefile                       |   2 +-
>  tools/lib/perf/cpumap.c                       |  94 ++---
>  tools/lib/perf/include/internal/cpumap.h      |   4 +-
>  tools/lib/perf/include/internal/rc_check.h    |  94 +++++
>  tools/perf/arch/s390/annotate/instructions.c  |   4 +-
>  tools/perf/arch/x86/tests/dwarf-unwind.c      |   2 +-
>  tools/perf/arch/x86/util/event.c              |  13 +-
>  tools/perf/builtin-annotate.c                 |  11 +-
>  tools/perf/builtin-buildid-list.c             |   4 +-
>  tools/perf/builtin-inject.c                   |  12 +-
>  tools/perf/builtin-kallsyms.c                 |   6 +-
>  tools/perf/builtin-kmem.c                     |   4 +-
>  tools/perf/builtin-lock.c                     |   4 +-
>  tools/perf/builtin-mem.c                      |  10 +-
>  tools/perf/builtin-report.c                   |  26 +-
>  tools/perf/builtin-script.c                   |  27 +-
>  tools/perf/builtin-top.c                      |  17 +-
>  tools/perf/builtin-trace.c                    |   2 +-
>  .../scripts/python/Perf-Trace-Util/Context.c  |  13 +-
>  tools/perf/tests/code-reading.c               |  37 +-
>  tools/perf/tests/cpumap.c                     |   4 +-
>  tools/perf/tests/hists_common.c               |   8 +-
>  tools/perf/tests/hists_cumulate.c             |  14 +-
>  tools/perf/tests/hists_filter.c               |  14 +-
>  tools/perf/tests/hists_link.c                 |  18 +-
>  tools/perf/tests/hists_output.c               |  12 +-
>  tools/perf/tests/maps.c                       |  69 ++--
>  tools/perf/tests/mmap-thread-lookup.c         |   3 +-
>  tools/perf/tests/symbols.c                    |   6 +-
>  tools/perf/tests/thread-maps-share.c          |  29 +-
>  tools/perf/tests/vmlinux-kallsyms.c           |  54 +--
>  tools/perf/ui/browsers/annotate.c             |   9 +-
>  tools/perf/ui/browsers/hists.c                |  19 +-
>  tools/perf/ui/browsers/map.c                  |   4 +-
>  tools/perf/util/annotate.c                    |  40 ++-
>  tools/perf/util/auxtrace.c                    |   2 +-
>  tools/perf/util/block-info.c                  |   4 +-
>  tools/perf/util/bpf-event.c                   |  10 +-
>  tools/perf/util/bpf_lock_contention.c         |   6 +-
>  tools/perf/util/build-id.c                    |   2 +-
>  tools/perf/util/callchain.c                   |  24 +-
>  tools/perf/util/cpumap.c                      |  40 ++-
>  tools/perf/util/data-convert-json.c           |  10 +-
>  tools/perf/util/db-export.c                   |  16 +-
>  tools/perf/util/dlfilter.c                    |  28 +-
>  tools/perf/util/dso.c                         |   8 +-
>  tools/perf/util/dsos.c                        |   2 +-
>  tools/perf/util/event.c                       |  27 +-
>  tools/perf/util/evsel_fprintf.c               |   4 +-
>  tools/perf/util/hist.c                        |  22 +-
>  tools/perf/util/intel-pt.c                    |  63 ++--
>  tools/perf/util/machine.c                     | 252 ++++++++------
>  tools/perf/util/map.c                         | 217 ++++++------
>  tools/perf/util/map.h                         |  74 +++-
>  tools/perf/util/maps.c                        | 318 ++++++++++-------
>  tools/perf/util/maps.h                        |  67 +++-
>  tools/perf/util/namespaces.c                  | 132 +++++---
>  tools/perf/util/namespaces.h                  |   3 +-
>  tools/perf/util/pmu.c                         |   8 +-
>  tools/perf/util/probe-event.c                 |  62 ++--
>  .../util/scripting-engines/trace-event-perl.c |  10 +-
>  .../scripting-engines/trace-event-python.c    |  26 +-
>  tools/perf/util/sort.c                        |  67 ++--
>  tools/perf/util/symbol-elf.c                  |  41 ++-
>  tools/perf/util/symbol.c                      | 320 +++++++++++-------
>  tools/perf/util/symbol_fprintf.c              |   2 +-
>  tools/perf/util/synthetic-events.c            |  34 +-
>  tools/perf/util/thread-stack.c                |   4 +-
>  tools/perf/util/thread.c                      |  39 +--
>  tools/perf/util/unwind-libdw.c                |  20 +-
>  tools/perf/util/unwind-libunwind-local.c      |  16 +-
>  tools/perf/util/unwind-libunwind.c            |  33 +-
>  tools/perf/util/vdso.c                        |   7 +-
>  73 files changed, 1665 insertions(+), 1044 deletions(-)
>  create mode 100644 tools/lib/perf/include/internal/rc_check.h
>
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>

On 4/04/23 18:58, Ian Rogers wrote:
> Ping. It would be nice to have this landed or at least the first 10
> patches that refactor the map API and are the bulk of the
> lines-of-code changed. Having those landed would make it easier to
> rebase in the future, but I also think the whole series is ready to
> go.

I was wondering if the handling of dynamic data like struct map makes
any sense at present.  Perhaps someone can reassure me.

A struct map can be updated when an MMAP event is processed.  So it
seems like anything racing with event processing is already broken, and
reference counting / locking cannot help - unless there is also
copy-on-write (which there isn't at present)?

For struct maps, referencing it while simultaneously processing
events seems to make even less sense?

Em Tue, Apr 04, 2023 at 08:25:41PM +0300, Adrian Hunter escreveu:
> On 4/04/23 18:58, Ian Rogers wrote:
> > Ping. It would be nice to have this landed or at least the first 10
> > patches that refactor the map API and are the bulk of the
> > lines-of-code changed. Having those landed would make it easier to
> > rebase in the future, but I also think the whole series is ready to
> > go.
> 
> I was wondering if the handling of dynamic data like struct map makes
> any sense at present.  Perhaps someone can reassure me.
> 
> A struct map can be updated when an MMAP event is processed.  So it

Yes, it can, and the update is made via a new PERF_RECORD_MMAP, right?

So:

	perf_event__process_mmap()
	  machine__process_mmap2_event()
	    map__new() + thread__insert_map(thread, map)
	    	maps__fixup_overlappings()
			maps__insert(thread->maps, map);

Ok, from this point on new samples on ] map->start .. map->end ] will
grab a refcount to this new map in its hist_entry, right?

When we want to sort by dso we will look at hist_entry->map->dso, etc.

> seems like anything racing with event processing is already broken, and
> reference counting / locking cannot help - unless there is also
> copy-on-write (which there isn't at present)?

> For struct maps, referencing it while simultaneously processing
> events seems to make even less sense?

Can you elaborate some more?

- Arnaldo

Em Tue, Apr 04, 2023 at 03:41:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Apr 04, 2023 at 08:25:41PM +0300, Adrian Hunter escreveu:
> > On 4/04/23 18:58, Ian Rogers wrote:
> > > Ping. It would be nice to have this landed or at least the first 10
> > > patches that refactor the map API and are the bulk of the
> > > lines-of-code changed. Having those landed would make it easier to
> > > rebase in the future, but I also think the whole series is ready to
> > > go.
> > 
> > I was wondering if the handling of dynamic data like struct map makes
> > any sense at present.  Perhaps someone can reassure me.
> > 
> > A struct map can be updated when an MMAP event is processed.  So it
> 
> Yes, it can, and the update is made via a new PERF_RECORD_MMAP, right?
> 
> So:
> 
> 	perf_event__process_mmap()
> 	  machine__process_mmap2_event()
> 	    map__new() + thread__insert_map(thread, map)
> 	    	maps__fixup_overlappings()
> 			maps__insert(thread->maps, map);
> 
> Ok, from this point on new samples on ] map->start .. map->end ] will
> grab a refcount to this new map in its hist_entry, right?
> 
> When we want to sort by dso we will look at hist_entry->map->dso, etc.

And in 'perf top' we go decaying hist entries, when we delete the
hist_entry, drop the reference count to things it holds, that will then
be finally deleted when no more hist_entries point to it.

> > seems like anything racing with event processing is already broken, and
> > reference counting / locking cannot help - unless there is also
> > copy-on-write (which there isn't at present)?
> 
> > For struct maps, referencing it while simultaneously processing
> > events seems to make even less sense?
> 
> Can you elaborate some more?

- Arnaldo

On 4/04/23 21:54, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 04, 2023 at 03:41:38PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Tue, Apr 04, 2023 at 08:25:41PM +0300, Adrian Hunter escreveu:
>>> On 4/04/23 18:58, Ian Rogers wrote:
>>>> Ping. It would be nice to have this landed or at least the first 10
>>>> patches that refactor the map API and are the bulk of the
>>>> lines-of-code changed. Having those landed would make it easier to
>>>> rebase in the future, but I also think the whole series is ready to
>>>> go.
>>>
>>> I was wondering if the handling of dynamic data like struct map makes
>>> any sense at present.  Perhaps someone can reassure me.
>>>
>>> A struct map can be updated when an MMAP event is processed.  So it
>>
>> Yes, it can, and the update is made via a new PERF_RECORD_MMAP, right?
>>
>> So:
>>
>> 	perf_event__process_mmap()
>> 	  machine__process_mmap2_event()
>> 	    map__new() + thread__insert_map(thread, map)
>> 	    	maps__fixup_overlappings()
>> 			maps__insert(thread->maps, map);
>>
>> Ok, from this point on new samples on ] map->start .. map->end ] will
>> grab a refcount to this new map in its hist_entry, right?
>>
>> When we want to sort by dso we will look at hist_entry->map->dso, etc.
> 
> And in 'perf top' we go decaying hist entries, when we delete the
> hist_entry, drop the reference count to things it holds, that will then
> be finally deleted when no more hist_entries point to it.
> 
>>> seems like anything racing with event processing is already broken, and
>>> reference counting / locking cannot help - unless there is also
>>> copy-on-write (which there isn't at present)?

So I checked, and struct map *is* copy-on-write in
maps__fixup_overlappings(), so that should not be a problem.

>>
>>> For struct maps, referencing it while simultaneously processing
>>> events seems to make even less sense?
>>
>> Can you elaborate some more?

Only that the maps are not necessarily stable e.g. the map that you
need has been replaced in the meantime.

But upon investigation, the only user at the moment is
maps__find_ams().  If we kept the removed maps (we used to),
it might be possible to make maps__find_ams() work correctly
in any case.

Em Wed, Apr 05, 2023 at 11:47:26AM +0300, Adrian Hunter escreveu:
> On 4/04/23 21:54, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Apr 04, 2023 at 03:41:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> >> Em Tue, Apr 04, 2023 at 08:25:41PM +0300, Adrian Hunter escreveu:
> >>> On 4/04/23 18:58, Ian Rogers wrote:
> >>>> Ping. It would be nice to have this landed or at least the first 10
> >>>> patches that refactor the map API and are the bulk of the
> >>>> lines-of-code changed. Having those landed would make it easier to
> >>>> rebase in the future, but I also think the whole series is ready to
> >>>> go.
> >>>
> >>> I was wondering if the handling of dynamic data like struct map makes
> >>> any sense at present.  Perhaps someone can reassure me.
> >>>
> >>> A struct map can be updated when an MMAP event is processed.  So it
> >>
> >> Yes, it can, and the update is made via a new PERF_RECORD_MMAP, right?
> >>
> >> So:
> >>
> >> 	perf_event__process_mmap()
> >> 	  machine__process_mmap2_event()
> >> 	    map__new() + thread__insert_map(thread, map)
> >> 	    	maps__fixup_overlappings()
> >> 			maps__insert(thread->maps, map);
> >>
> >> Ok, from this point on new samples on ] map->start .. map->end ] will
> >> grab a refcount to this new map in its hist_entry, right?
> >>
> >> When we want to sort by dso we will look at hist_entry->map->dso, etc.
> > 
> > And in 'perf top' we go decaying hist entries, when we delete the
> > hist_entry, drop the reference count to things it holds, that will then
> > be finally deleted when no more hist_entries point to it.
> > 
> >>> seems like anything racing with event processing is already broken, and
> >>> reference counting / locking cannot help - unless there is also
> >>> copy-on-write (which there isn't at present)?
 
> So I checked, and struct map *is* copy-on-write in
> maps__fixup_overlappings(), so that should not be a problem.
 
> >>> For struct maps, referencing it while simultaneously processing
> >>> events seems to make even less sense?

> >> Can you elaborate some more?
 
> Only that the maps are not necessarily stable e.g. the map that you
> need has been replaced in the meantime.

Well, it may be sliced in several or shrunk by new ones overlapping it,
but it if completely disappears, say a new map starts before the one
disappearing and ends after it, then it remains with reference counts if
there are hist_entries (or other data structure) pointing to them,
right?
 
> But upon investigation, the only user at the moment is
> maps__find_ams().  If we kept the removed maps (we used to),
> it might be possible to make maps__find_ams() work correctly
> in any case.

Humm, I think I see what you mean, maps__find_ams() is called when we
are annotating a symbol, not when we're processing a sample, so it may
be the case that at the time of annotation the executable that is being
found (its parsing the target IP of a 'call' assembly instruction) was
replaced, is that the case?

- Arnaldo

On 5/04/23 16:20, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 05, 2023 at 11:47:26AM +0300, Adrian Hunter escreveu:
>> On 4/04/23 21:54, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Apr 04, 2023 at 03:41:38PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>> Em Tue, Apr 04, 2023 at 08:25:41PM +0300, Adrian Hunter escreveu:
>>>>> On 4/04/23 18:58, Ian Rogers wrote:
>>>>>> Ping. It would be nice to have this landed or at least the first 10
>>>>>> patches that refactor the map API and are the bulk of the
>>>>>> lines-of-code changed. Having those landed would make it easier to
>>>>>> rebase in the future, but I also think the whole series is ready to
>>>>>> go.
>>>>>
>>>>> I was wondering if the handling of dynamic data like struct map makes
>>>>> any sense at present.  Perhaps someone can reassure me.
>>>>>
>>>>> A struct map can be updated when an MMAP event is processed.  So it
>>>>
>>>> Yes, it can, and the update is made via a new PERF_RECORD_MMAP, right?
>>>>
>>>> So:
>>>>
>>>> 	perf_event__process_mmap()
>>>> 	  machine__process_mmap2_event()
>>>> 	    map__new() + thread__insert_map(thread, map)
>>>> 	    	maps__fixup_overlappings()
>>>> 			maps__insert(thread->maps, map);
>>>>
>>>> Ok, from this point on new samples on ] map->start .. map->end ] will
>>>> grab a refcount to this new map in its hist_entry, right?
>>>>
>>>> When we want to sort by dso we will look at hist_entry->map->dso, etc.
>>>
>>> And in 'perf top' we go decaying hist entries, when we delete the
>>> hist_entry, drop the reference count to things it holds, that will then
>>> be finally deleted when no more hist_entries point to it.
>>>
>>>>> seems like anything racing with event processing is already broken, and
>>>>> reference counting / locking cannot help - unless there is also
>>>>> copy-on-write (which there isn't at present)?
>  
>> So I checked, and struct map *is* copy-on-write in
>> maps__fixup_overlappings(), so that should not be a problem.
>  
>>>>> For struct maps, referencing it while simultaneously processing
>>>>> events seems to make even less sense?
> 
>>>> Can you elaborate some more?
>  
>> Only that the maps are not necessarily stable e.g. the map that you
>> need has been replaced in the meantime.
> 
> Well, it may be sliced in several or shrunk by new ones overlapping it,
> but it if completely disappears, say a new map starts before the one
> disappearing and ends after it, then it remains with reference counts if
> there are hist_entries (or other data structure) pointing to them,
> right?
>  
>> But upon investigation, the only user at the moment is
>> maps__find_ams().  If we kept the removed maps (we used to),
>> it might be possible to make maps__find_ams() work correctly
>> in any case.
> 
> Humm, I think I see what you mean, maps__find_ams() is called when we
> are annotating a symbol, not when we're processing a sample, so it may
> be the case that at the time of annotation the executable that is being
> found (its parsing the target IP of a 'call' assembly instruction) was
> replaced, is that the case?

Yes, that is the possibility

Em Wed, Apr 05, 2023 at 07:25:27PM +0300, Adrian Hunter escreveu:
> On 5/04/23 16:20, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Apr 05, 2023 at 11:47:26AM +0300, Adrian Hunter escreveu:
> >> On 4/04/23 21:54, Arnaldo Carvalho de Melo wrote:
> >>> Em Tue, Apr 04, 2023 at 03:41:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> >>>> Em Tue, Apr 04, 2023 at 08:25:41PM +0300, Adrian Hunter escreveu:
> >>>>> I was wondering if the handling of dynamic data like struct map makes
> >>>>> any sense at present.  Perhaps someone can reassure me.

> >>>>> A struct map can be updated when an MMAP event is processed.  So it

> >>>> Yes, it can, and the update is made via a new PERF_RECORD_MMAP, right?

> >>>> So:

> >>>> 	perf_event__process_mmap()
> >>>> 	  machine__process_mmap2_event()
> >>>> 	    map__new() + thread__insert_map(thread, map)
> >>>> 	    	maps__fixup_overlappings()
> >>>> 			maps__insert(thread->maps, map);

> >>>> Ok, from this point on new samples on ] map->start .. map->end ] will
> >>>> grab a refcount to this new map in its hist_entry, right?

> >>>> When we want to sort by dso we will look at hist_entry->map->dso, etc.

> >>> And in 'perf top' we go decaying hist entries, when we delete the
> >>> hist_entry, drop the reference count to things it holds, that will then
> >>> be finally deleted when no more hist_entries point to it.

> >>>>> seems like anything racing with event processing is already broken, and
> >>>>> reference counting / locking cannot help - unless there is also
> >>>>> copy-on-write (which there isn't at present)?
> >  
> >> So I checked, and struct map *is* copy-on-write in
> >> maps__fixup_overlappings(), so that should not be a problem.
> >  
> >>>>> For struct maps, referencing it while simultaneously processing
> >>>>> events seems to make even less sense?
> > 
> >>>> Can you elaborate some more?
> >  
> >> Only that the maps are not necessarily stable e.g. the map that you
> >> need has been replaced in the meantime.
> > 
> > Well, it may be sliced in several or shrunk by new ones overlapping it,
> > but it if completely disappears, say a new map starts before the one
> > disappearing and ends after it, then it remains with reference counts if
> > there are hist_entries (or other data structure) pointing to them,
> > right?

> >> But upon investigation, the only user at the moment is
> >> maps__find_ams().  If we kept the removed maps (we used to),
> >> it might be possible to make maps__find_ams() work correctly
> >> in any case.

> > Humm, I think I see what you mean, maps__find_ams() is called when we
> > are annotating a symbol, not when we're processing a sample, so it may
> > be the case that at the time of annotation the executable that is being
> > found (its parsing the target IP of a 'call' assembly instruction) was
> > replaced, is that the case?
 
> Yes, that is the possibility

Yeah, this one gets a bit more difficult to support, we would have to
keep a sub-bucket for each annotation instruction with the ordered by
timestamp list of maps that were on that location (but then just to
places that had samples, not to all) and then when add some visual cue
to that annotation line to mean that it was patched and show the
original, then the (possibly) various patches and say that samples up to
N units of time were for some original DSO, then to another (overlapping
executable map), then to some patching (that we would catch with
PERF_RECORD_TEXT_POKE for the kernel, right?), etc.

Seems doable, and for most cases would be similar to what we have right
now, as self-modifying code its not so pervasive (famous last words
;-)).

- Arnaldo

On Tue, Apr 4, 2023 at 10:26 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 4/04/23 18:58, Ian Rogers wrote:
> > Ping. It would be nice to have this landed or at least the first 10
> > patches that refactor the map API and are the bulk of the
> > lines-of-code changed. Having those landed would make it easier to
> > rebase in the future, but I also think the whole series is ready to
> > go.
>
> I was wondering if the handling of dynamic data like struct map makes
> any sense at present.  Perhaps someone can reassure me.
>
> A struct map can be updated when an MMAP event is processed.  So it
> seems like anything racing with event processing is already broken, and
> reference counting / locking cannot help - unless there is also
> copy-on-write (which there isn't at present)?
>
> For struct maps, referencing it while simultaneously processing
> events seems to make even less sense?

Agreed. The point of this work isn't to reimplement the maps/map APIs
but to add a layer of reference count checking. A refactor to change
the implementation without reference counts can delete the reference
count checking and I think that is great! I'm trying to get the code
base, in its current shape, to be more correct guided by sanitizers.
Unfortunately the sanitizers come from a C++ RAII world where
maintaining reference counts is somewhat trivial, we have to work
harder as done here.

A similar thing to refactoring maps is changing symbol. The rb_node
there accounts for 3*8 bytes of pointer, but is just to sort the
symbol by address. A sorted array would suffice as well complexity
wise, freeing 16-bytes per symbol, and is already done for symbols
sorted by name.

Thanks,
Ian

Applied to:

perf map: Add accessor for dso


diff --git a/tools/perf/arch/powerpc/util/skip-callchain-idx.c b/tools/perf/arch/powerpc/util/skip-callchain-idx.c
index 20cd6244863b1a09..fe0e4530673c6661 100644
--- a/tools/perf/arch/powerpc/util/skip-callchain-idx.c
+++ b/tools/perf/arch/powerpc/util/skip-callchain-idx.c
@@ -255,7 +255,7 @@ int arch_skip_callchain_idx(struct thread *thread, struct ip_callchain *chain)
 	thread__find_symbol(thread, PERF_RECORD_MISC_USER, ip, &al);
 
 	if (al.map)
-		dso = al.map->dso;
+		dso = map__dso(al.map);
 
 	if (!dso) {
 		pr_debug("%" PRIx64 " dso is NULL\n", ip);
diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
index 0856b32f9e08a1f5..9f99fc88dbff9056 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -104,7 +104,7 @@ void arch__fix_tev_from_maps(struct perf_probe_event *pev,
 
 	lep_offset = PPC64_LOCAL_ENTRY_OFFSET(sym->arch_sym);
 
-	if (map->dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS)
+	if (map__dso(map)->symtab_type == DSO_BINARY_TYPE__KALLSYMS)
 		tev->point.offset += PPC64LE_LEP_OFFSET;
 	else if (lep_offset) {
 		if (pev->uprobes)
diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c
index a1c021a6d3c1f0f1..2effac77ca8c6742 100644
--- a/tools/perf/ui/gtk/annotate.c
+++ b/tools/perf/ui/gtk/annotate.c
@@ -165,6 +165,7 @@ static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
 				struct annotation_options *options,
 				struct hist_browser_timer *hbt)
 {
+	struct dso *dso = map__dso(ms->map);
 	struct symbol *sym = ms->sym;
 	GtkWidget *window;
 	GtkWidget *notebook;
@@ -172,13 +173,13 @@ static int symbol__gtk_annotate(struct map_symbol *ms, struct evsel *evsel,
 	GtkWidget *tab_label;
 	int err;
 
-	if (ms->map->dso->annotate_warned)
+	if (dso->annotate_warned)
 		return -1;
 
 	err = symbol__annotate(ms, evsel, options, NULL);
 	if (err) {
 		char msg[BUFSIZ];
-		ms->map->dso->annotate_warned = true;
+		dso->annotate_warned = true;
 		symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
 		ui__error("Couldn't annotate %s: %s\n", sym->name, msg);
 		return -1;
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 94e2d02009eb9f72..528a7fb066cfc9ec 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -865,6 +865,7 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
 	struct thread *thread;
 	struct machine *machine;
 	struct addr_location al;
+	struct dso *dso;
 	struct cs_etm_traceid_queue *tidq;
 
 	if (!etmq)
@@ -883,27 +884,29 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
 		thread = etmq->etm->unknown_thread;
 	}
 
-	if (!thread__find_map(thread, cpumode, address, &al) || !al.map->dso)
+	dso = map__dso(al.map);
+
+	if (!thread__find_map(thread, cpumode, address, &al) || !dso)
 		return 0;
 
-	if (al.map->dso->data.status == DSO_DATA_STATUS_ERROR &&
-	    dso__data_status_seen(al.map->dso, DSO_DATA_STATUS_SEEN_ITRACE))
+	if (dso->data.status == DSO_DATA_STATUS_ERROR &&
+	    dso__data_status_seen(dso, DSO_DATA_STATUS_SEEN_ITRACE))
 		return 0;
 
 	offset = al.map->map_ip(al.map, address);
 
 	map__load(al.map);
 
-	len = dso__data_read_offset(al.map->dso, machine, offset, buffer, size);
+	len = dso__data_read_offset(dso, machine, offset, buffer, size);
 
 	if (len <= 0) {
 		ui__warning_once("CS ETM Trace: Missing DSO. Use 'perf archive' or debuginfod to export data from the traced system.\n"
 				 "              Enable CONFIG_PROC_KCORE or use option '-k /path/to/vmlinux' for kernel symbols.\n");
-		if (!al.map->dso->auxtrace_warned) {
+		if (!dso->auxtrace_warned) {
 			pr_err("CS ETM Trace: Debug data not found for address %#"PRIx64" in %s\n",
 				    address,
-				    al.map->dso->long_name ? al.map->dso->long_name : "Unknown");
-			al.map->dso->auxtrace_warned = true;
+				    dso->long_name ? dso->long_name : "Unknown");
+			dso->auxtrace_warned = true;
 		}
 		return 0;
 	}
diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
index c487a249b33c62d4..108f7b1697a73465 100644
--- a/tools/perf/util/unwind-libunwind-local.c
+++ b/tools/perf/util/unwind-libunwind-local.c
@@ -328,7 +328,7 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct unwind_info *ui,
 	maps__for_each_entry(ui->thread->maps, map_node) {
 		struct map *map = map_node->map;
 
-		if (map->dso == dso && map->start < base_addr)
+		if (map__dso(map) == dso && map->start < base_addr)
 			base_addr = map->start;
 	}
 	base_addr -= dso->data.elf_base_addr;
@@ -424,19 +424,23 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
 {
 	struct unwind_info *ui = arg;
 	struct map *map;
+	struct dso *dso;
 	unw_dyn_info_t di;
 	u64 table_data, segbase, fde_count;
 	int ret = -EINVAL;
 
 	map = find_map(ip, ui);
-	if (!map || !map->dso)
+	if (!map)
 		return -EINVAL;
 
-	pr_debug("unwind: find_proc_info dso %s\n", map->dso->name);
+	dso = map__dso(map);
+	if (!dso)
+		return -EINVAL;
+
+	pr_debug("unwind: find_proc_info dso %s\n", dso->name);
 
 	/* Check the .eh_frame section for unwinding info */
-	if (!read_unwind_spec_eh_frame(map->dso, ui,
-				       &table_data, &segbase, &fde_count)) {
+	if (!read_unwind_spec_eh_frame(dso, ui, &table_data, &segbase, &fde_count)) {
 		memset(&di, 0, sizeof(di));
 		di.format   = UNW_INFO_FORMAT_REMOTE_TABLE;
 		di.start_ip = map->start;
@@ -452,16 +456,16 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
 #ifndef NO_LIBUNWIND_DEBUG_FRAME
 	/* Check the .debug_frame section for unwinding info */
 	if (ret < 0 &&
-	    !read_unwind_spec_debug_frame(map->dso, ui->machine, &segbase)) {
-		int fd = dso__data_get_fd(map->dso, ui->machine);
-		int is_exec = elf_is_exec(fd, map->dso->name);
+	    !read_unwind_spec_debug_frame(dso, ui->machine, &segbase)) {
+		int fd = dso__data_get_fd(dso, ui->machine);
+		int is_exec = elf_is_exec(fd, dso->name);
 		unw_word_t base = is_exec ? 0 : map->start;
 		const char *symfile;
 
 		if (fd >= 0)
-			dso__data_put_fd(map->dso);
+			dso__data_put_fd(dso);
 
-		symfile = map->dso->symsrc_filename ?: map->dso->name;
+		symfile = dso->symsrc_filename ?: dso->name;
 
 		memset(&di, 0, sizeof(di));
 		if (dwarf_find_debug_frame(0, &di, ip, base, symfile,
@@ -513,6 +517,7 @@ static int access_dso_mem(struct unwind_info *ui, unw_word_t addr,
 			  unw_word_t *data)
 {
 	struct map *map;
+	struct dso *dso;
 	ssize_t size;
 
 	map = find_map(addr, ui);
@@ -521,10 +526,12 @@ static int access_dso_mem(struct unwind_info *ui, unw_word_t addr,
 		return -1;
 	}
 
-	if (!map->dso)
+	dso = map__dso(map);
+
+	if (!dso)
 		return -1;
 
-	size = dso__data_read_addr(map->dso, map, ui->machine,
+	size = dso__data_read_addr(dso, map, ui->machine,
 				   addr, (u8 *) data, sizeof(*data));
 
 	return !(size == sizeof(*data));
diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
index 42528ade513e4975..4378daaafcd3b875 100644
--- a/tools/perf/util/unwind-libunwind.c
+++ b/tools/perf/util/unwind-libunwind.c
@@ -22,6 +22,7 @@ int unwind__prepare_access(struct maps *maps, struct map *map, bool *initialized
 	const char *arch;
 	enum dso_type dso_type;
 	struct unwind_libunwind_ops *ops = local_unwind_libunwind_ops;
+	struct dso *dso = map__dso(map);
 	struct machine *machine;
 	int err;
 
@@ -29,8 +30,7 @@ int unwind__prepare_access(struct maps *maps, struct map *map, bool *initialized
 		return 0;
 
 	if (maps__addr_space(maps)) {
-		pr_debug("unwind: thread map already set, dso=%s\n",
-			 map->dso->name);
+		pr_debug("unwind: thread map already set, dso=%s\n", dso->name);
 		if (initialized)
 			*initialized = true;
 		return 0;
@@ -41,7 +41,7 @@ int unwind__prepare_access(struct maps *maps, struct map *map, bool *initialized
 	if (!machine->env || !machine->env->arch)
 		goto out_register;
 
-	dso_type = dso__type(map->dso, machine);
+	dso_type = dso__type(dso, machine);
 	if (dso_type == DSO__TYPE_UNKNOWN)
 		return 0;

Applied to:

perf map: Add accessor for start and end

diff --git a/tools/perf/arch/arm/tests/dwarf-unwind.c b/tools/perf/arch/arm/tests/dwarf-unwind.c
index ccfa87055c4a3b9d..566fb6c0eae737c6 100644
--- a/tools/perf/arch/arm/tests/dwarf-unwind.c
+++ b/tools/perf/arch/arm/tests/dwarf-unwind.c
@@ -33,7 +33,7 @@ static int sample_ustack(struct perf_sample *sample,
 		return -1;
 	}
 
-	stack_size = map->end - sp;
+	stack_size = map__end(map) - sp;
 	stack_size = stack_size > STACK_SIZE ? STACK_SIZE : stack_size;
 
 	memcpy(buf, (void *) sp, stack_size);
diff --git a/tools/perf/arch/arm64/tests/dwarf-unwind.c b/tools/perf/arch/arm64/tests/dwarf-unwind.c
index 46147a483049615d..90a7ef293ce76879 100644
--- a/tools/perf/arch/arm64/tests/dwarf-unwind.c
+++ b/tools/perf/arch/arm64/tests/dwarf-unwind.c
@@ -33,7 +33,7 @@ static int sample_ustack(struct perf_sample *sample,
 		return -1;
 	}
 
-	stack_size = map->end - sp;
+	stack_size = map__end(map) - sp;
 	stack_size = stack_size > STACK_SIZE ? STACK_SIZE : stack_size;
 
 	memcpy(buf, (void *) sp, stack_size);
diff --git a/tools/perf/arch/powerpc/tests/dwarf-unwind.c b/tools/perf/arch/powerpc/tests/dwarf-unwind.c
index c9cb4b059392f6cf..32fffb593fbf0236 100644
--- a/tools/perf/arch/powerpc/tests/dwarf-unwind.c
+++ b/tools/perf/arch/powerpc/tests/dwarf-unwind.c
@@ -33,7 +33,7 @@ static int sample_ustack(struct perf_sample *sample,
 		return -1;
 	}
 
-	stack_size = map->end - sp;
+	stack_size = map__end(map) - sp;
 	stack_size = stack_size > STACK_SIZE ? STACK_SIZE : stack_size;
 
 	memcpy(buf, (void *) sp, stack_size);
diff --git a/tools/perf/arch/powerpc/util/skip-callchain-idx.c b/tools/perf/arch/powerpc/util/skip-callchain-idx.c
index fe0e4530673c6661..b7223feec770dc33 100644
--- a/tools/perf/arch/powerpc/util/skip-callchain-idx.c
+++ b/tools/perf/arch/powerpc/util/skip-callchain-idx.c
@@ -262,7 +262,7 @@ int arch_skip_callchain_idx(struct thread *thread, struct ip_callchain *chain)
 		return skip_slot;
 	}
 
-	rc = check_return_addr(dso, al.map->start, ip);
+	rc = check_return_addr(dso, map__start(al.map), ip);
 
 	pr_debug("[DSO %s, sym %s, ip 0x%" PRIx64 "] rc %d\n",
 				dso->long_name, al.sym->name, ip, rc);
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 7852b97da10aa336..1cc6f338728f5499 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -903,7 +903,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
 		}
 
 		map->start = event->ksymbol.addr;
-		map->end = map__start(map) + event->ksymbol.len;
+		map__end(map) = map__start(map) + event->ksymbol.len;
 		err = maps__insert(machine__kernel_maps(machine), map);
 		map__put(map);
 		if (err)
diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index 1fd57db7222678ad..21010a2b8e16cc2e 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -339,7 +339,7 @@ int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp)
 			map__put(before);
 		}
 
-		if (map->end < map__end(pos->map)) {
+		if (map__end(map) < map__end(pos->map)) {
 			struct map *after = map__clone(pos->map);
 
 			if (after == NULL) {
diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
index 108f7b1697a73465..1c13f43e7d22c84c 100644
--- a/tools/perf/util/unwind-libunwind-local.c
+++ b/tools/perf/util/unwind-libunwind-local.c
@@ -327,9 +327,10 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct unwind_info *ui,
 
 	maps__for_each_entry(ui->thread->maps, map_node) {
 		struct map *map = map_node->map;
+		u64 start = map__start(map);
 
-		if (map__dso(map) == dso && map->start < base_addr)
-			base_addr = map->start;
+		if (map__dso(map) == dso && start < base_addr)
+			base_addr = start;
 	}
 	base_addr -= dso->data.elf_base_addr;
 	/* Address of .eh_frame_hdr */
@@ -443,8 +444,8 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
 	if (!read_unwind_spec_eh_frame(dso, ui, &table_data, &segbase, &fde_count)) {
 		memset(&di, 0, sizeof(di));
 		di.format   = UNW_INFO_FORMAT_REMOTE_TABLE;
-		di.start_ip = map->start;
-		di.end_ip   = map->end;
+		di.start_ip = map__start(map);
+		di.end_ip   = map__end(map);
 		di.u.rti.segbase    = segbase;
 		di.u.rti.table_data = table_data;
 		di.u.rti.table_len  = fde_count * sizeof(struct table_entry)
@@ -459,7 +460,8 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
 	    !read_unwind_spec_debug_frame(dso, ui->machine, &segbase)) {
 		int fd = dso__data_get_fd(dso, ui->machine);
 		int is_exec = elf_is_exec(fd, dso->name);
-		unw_word_t base = is_exec ? 0 : map->start;
+		u64 start = map__start(map);
+		unw_word_t base = is_exec ? 0 : start;
 		const char *symfile;
 
 		if (fd >= 0)
@@ -468,8 +470,7 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
 		symfile = dso->symsrc_filename ?: dso->name;
 
 		memset(&di, 0, sizeof(di));
-		if (dwarf_find_debug_frame(0, &di, ip, base, symfile,
-					   map->start, map->end))
+		if (dwarf_find_debug_frame(0, &di, ip, base, symfile, start, map__end(map)))
 			return dwarf_search_unwind_table(as, ip, &di, pi,
 							 need_unwind_info, arg);
 	}

Em Tue, Apr 04, 2023 at 04:53:10PM -0300, Arnaldo Carvalho de Melo escreveu:
> Applied to:
> 
> perf map: Add accessor for start and end

> +++ b/tools/perf/util/machine.c
> @@ -903,7 +903,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
>  		}
>  
>  		map->start = event->ksymbol.addr;
> -		map->end = map__start(map) + event->ksymbol.len;
> +		map__end(map) = map__start(map) + event->ksymbol.len;

Ditch this one, duh.

>  		err = maps__insert(machine__kernel_maps(machine), map);
>  		map__put(map);
>  		if (err)

On 4/04/23 20:35, Ian Rogers wrote:
> On Tue, Apr 4, 2023 at 10:26 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 4/04/23 18:58, Ian Rogers wrote:
>>> Ping. It would be nice to have this landed or at least the first 10
>>> patches that refactor the map API and are the bulk of the
>>> lines-of-code changed. Having those landed would make it easier to
>>> rebase in the future, but I also think the whole series is ready to
>>> go.
>>
>> I was wondering if the handling of dynamic data like struct map makes
>> any sense at present.  Perhaps someone can reassure me.
>>
>> A struct map can be updated when an MMAP event is processed.  So it
>> seems like anything racing with event processing is already broken, and
>> reference counting / locking cannot help - unless there is also
>> copy-on-write (which there isn't at present)?
>>
>> For struct maps, referencing it while simultaneously processing
>> events seems to make even less sense?
> 
> Agreed. The point of this work isn't to reimplement the maps/map APIs
> but to add a layer of reference count checking. A refactor to change
> the implementation without reference counts can delete the reference
> count checking and I think that is great! I'm trying to get the code
> base, in its current shape, to be more correct guided by sanitizers.
> Unfortunately the sanitizers come from a C++ RAII world where
> maintaining reference counts is somewhat trivial, we have to work
> harder as done here.
> 
> A similar thing to refactoring maps is changing symbol. The rb_node
> there accounts for 3*8 bytes of pointer, but is just to sort the
> symbol by address. A sorted array would suffice as well complexity
> wise, freeing 16-bytes per symbol, and is already done for symbols
> sorted by name.

Ok, just stuff to keep in mind.

Em Tue, Apr 04, 2023 at 08:58:55AM -0700, Ian Rogers escreveu:
> Ping. It would be nice to have this landed or at least the first 10
> patches that refactor the map API and are the bulk of the
> lines-of-code changed. Having those landed would make it easier to
> rebase in the future, but I also think the whole series is ready to
> go.

I'm trying to get it to compile:

  CC      /tmp/build/perf-tools-next/util/bpf-event.o
In file included from util/machine.h:7,
                 from util/session.h:8,
                 from util/unwind-libunwind-local.c:35:
util/unwind-libunwind-local.c: In function ‘read_unwind_spec_eh_frame’:
util/maps.h:29:18: error: assignment to ‘struct map *’ from incompatible pointer type ‘struct map_rb_node *’ [-Werror=incompatible-pointer-types]
   29 |         for (map = maps__first(maps); map; map = map_rb_node__next(map))
      |                  ^
util/unwind-libunwind-local.c:328:9: note: in expansion of macro ‘maps__for_each_entry’
  328 |         maps__for_each_entry(ui->thread->maps, map) {
      |         ^~~~~~~~~~~~~~~~~~~~
util/unwind-libunwind-local.c:328:48: error: passing argument 1 of ‘map_rb_node__next’ from incompatible pointer type [-Werror=incompatible-pointer-types]
  328 |         maps__for_each_entry(ui->thread->maps, map) {
      |                                                ^~~
      |                                                |
      |                                                struct map *
util/maps.h:29:68: note: in definition of macro ‘maps__for_each_entry’
   29 |         for (map = maps__first(maps); map; map = map_rb_node__next(map))
      |                                                                    ^~~
util/maps.h:24:59: note: expected ‘struct map_rb_node *’ but argument is of type ‘struct map *’
   24 | struct map_rb_node *map_rb_node__next(struct map_rb_node *node);
      |                                       ~~~~~~~~~~~~~~~~~~~~^~~~
util/maps.h:29:48: error: assignment to ‘struct map *’ from incompatible pointer type ‘struct map_rb_node *’ [-Werror=incompatible-pointer-types]
   29 |         for (map = maps__first(maps); map; map = map_rb_node__next(map))
      |                                                ^
util/unwind-libunwind-local.c:328:9: note: in expansion of macro ‘maps__for_each_entry’
  328 |         maps__for_each_entry(ui->thread->maps, map) {
      |         ^~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[4]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:97: /tmp/build/perf-tools-next/util/unwind-libunwind-local.o] Error 1
make[4]: *** Waiting for unfinished jobs....
  LD      /tmp/build/perf-tools-next/util/scripting-engines/perf-in.o
make[3]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:140: util] Error 2
make[2]: *** [Makefile.perf:676: /tmp/build/perf-tools-next/perf-in.o] Error 2
make[1]: *** [Makefile.perf:236: sub-make] Error 2
make: *** [Makefile:113: install-bin] Error 2
make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'

 Performance counter stats for 'make -k BUILD_BPF_SKEL=1 CORESIGHT=1 PYTHON=python3 O=/tmp/build/perf-tools-next -C tools/perf install-bin':

      260622279301      cycles:u
      285362743453      instructions:u                   #    1.09  insn per cycle

       6.001315366 seconds time elapsed

      62.979105000 seconds user
      13.088797000 seconds sys


⬢[acme@toolbox perf-tools-next]$ git log --oneline -1
51a0f26e88c893ac (HEAD) perf maps: Remove rb_node from struct map
⬢[acme@toolbox perf-tools-next]$

I'm also making some changes to reduce the number of patch lines and
conserve the project 'git blame' usefulness, not changing the logic in
your patches.

- Arnaldo
 
> Thanks,
> Ian
> 
> On Mon, Mar 20, 2023 at 2:23 PM Ian Rogers <irogers@google.com> wrote:
> >
> > The perf tool has a class of memory problems where reference counts
> > are used incorrectly. Memory/address sanitizers and valgrind don't
> > provide useful ways to debug these problems, you see a memory leak
> > where the only pertinent information is the original allocation
> > site. What would be more useful is knowing where a get fails to have a
> > corresponding put, where there are double puts, etc.
> >
> > This work was motivated by the roll-back of:
> > https://lore.kernel.org/linux-perf-users/20211118193714.2293728-1-irogers@google.com/
> > where fixing a missed put resulted in a use-after-free in a different
> > context. There was a sense in fixing the issue that a game of
> > wac-a-mole had been embarked upon in adding missed gets and puts.
> >
> > The basic approach of the change is to add a level of indirection at
> > the get and put calls. Get allocates a level of indirection that, if
> > no corresponding put is called, becomes a memory leak (and associated
> > stack trace) that leak sanitizer can report. Similarly if two puts are
> > called for the same get, then a double free can be detected by address
> > sanitizer. This can also detect the use after put, which should also
> > yield a segv without a sanitizer.
> >
> > Adding reference count checking to cpu map was done as a proof of
> > concept, it yielded little other than a location where the use of get
> > could be cleaner by using its result. Reference count checking on
> > nsinfo identified a double free of the indirection layer and the
> > related threads, thereby identifying a data race as discussed here:
> >  https://lore.kernel.org/linux-perf-users/CAP-5=fWZH20L4kv-BwVtGLwR=Em3AOOT+Q4QGivvQuYn5AsPRg@mail.gmail.com/
> > Accordingly the dso->lock was extended and use to cover the race.
> >
> > The v3 version addresses problems in v2, in particular using macros to
> > avoid #ifdefs. The v3 version applies the reference count checking
> > approach to two more data structures, maps and map. While maps was
> > straightforward, struct map showed a problem where reference counted
> > thing can be on lists and rb-trees that are oblivious to the
> > reference count. To sanitize this, struct map is changed so that it is
> > referenced by either a list or rb-tree node and not part of it. This
> > simplifies the reference count and the patches have caught and fixed a
> > number of missed or mismatched reference counts relating to struct
> > map.
> >
> > The patches are arranged so that API refactors and bug fixes appear
> > first, then the reference count checker itself appears. This allows
> > for the refactor and fixes to be applied upstream first, as has
> > already happened with cpumap.
> >
> > A wider discussion of the approach is on the mailing list:
> >  https://lore.kernel.org/linux-perf-users/YffqnynWcc5oFkI5@kernel.org/T/#mf25ccd7a2e03de92cec29d36e2999a8ab5ec7f88
> > Comparing it to a past approach:
> >  https://lore.kernel.org/all/20151209021047.10245.8918.stgit@localhost.localdomain/
> > and to ref_tracker:
> >  https://lwn.net/Articles/877603/
> >
> > v5. rebase removing 5 merged changes. Add map_list_node__new to the
> >     1st patch (perf map: Move map list node into symbol) as suggested
> >     by Arnaldo. Remove unnecessary map__puts from patch 12 (perf map:
> >     Changes to reference counting) as suggested by Adrian. A summary
> >     of the sizes of the remaining patches is:
> > 74fd7ffafdd0 perf map: Add reference count checking
> >  12 files changed, 136 insertions(+), 114 deletions(-)
> > 4719196db8d3 perf maps: Add reference count checking.
> >  8 files changed, 64 insertions(+), 56 deletions(-)
> > 03943e7594cf perf namespaces: Add reference count checking
> >  7 files changed, 83 insertions(+), 62 deletions(-)
> > 0bb382cc52d7 perf cpumap: Add reference count checking
> >  6 files changed, 81 insertions(+), 71 deletions(-)
> > ef39f550c40d libperf: Add reference count checking macros.
> >  1 file changed, 94 insertions(+)
> > d9ac37c750e0 perf map: Changes to reference counting
> >  11 files changed, 112 insertions(+), 44 deletions(-)
> > 476014bc9b55 perf maps: Modify maps_by_name to hold a reference to a map
> >  2 files changed, 33 insertions(+), 18 deletions(-)
> > 91384676fddd perf test: Add extra diagnostics to maps test
> >  1 file changed, 36 insertions(+), 15 deletions(-)
> > fdc30434f826 perf map: Add accessors for pgoff and reloc
> >  9 files changed, 33 insertions(+), 23 deletions(-)
> > 368fe015adb2 perf map: Add accessors for prot, priv and flags
> >  6 files changed, 28 insertions(+), 12 deletions(-)
> > 2c6a8169826a perf map: Add helper for map_ip and unmap_ip
> >  23 files changed, 80 insertions(+), 65 deletions(-)
> > 929e59d49f4b perf map: Rename map_ip and unmap_ip
> >  6 files changed, 13 insertions(+), 13 deletions(-)
> > 4a38194aaaf5 perf map: Add accessor for start and end
> >  24 files changed, 114 insertions(+), 103 deletions(-)
> > 02b63e5c415e perf map: Add accessor for dso
> >  48 files changed, 404 insertions(+), 293 deletions(-)
> > 9324af6ccf42 perf maps: Add functions to access maps
> >  20 files changed, 175 insertions(+), 111 deletions(-)
> > 5c590d36a308 perf maps: Remove rb_node from struct map
> >  16 files changed, 291 insertions(+), 184 deletions(-)
> > af1d142eb777 perf map: Move map list node into symbol
> >  2 files changed, 63 insertions(+), 35 deletions(-)
> >
> > v4. rebases on to acme's perf-tools-next, fixes more issues with
> >     map/maps and breaks apart the accessor functions to reduce
> >     individual patch sizes. The accessor functions are mechanical
> >     changes where the single biggest one is refactoring use of
> >     map->dso to be map__dso(map).
> >
> > The v3 change is available here:
> > https://lore.kernel.org/lkml/20220211103415.2737789-1-irogers@google.com/
> >
> > Ian Rogers (17):
> >   perf map: Move map list node into symbol
> >   perf maps: Remove rb_node from struct map
> >   perf maps: Add functions to access maps
> >   perf map: Add accessor for dso
> >   perf map: Add accessor for start and end
> >   perf map: Rename map_ip and unmap_ip
> >   perf map: Add helper for map_ip and unmap_ip
> >   perf map: Add accessors for prot, priv and flags
> >   perf map: Add accessors for pgoff and reloc
> >   perf test: Add extra diagnostics to maps test
> >   perf maps: Modify maps_by_name to hold a reference to a map
> >   perf map: Changes to reference counting
> >   libperf: Add reference count checking macros.
> >   perf cpumap: Add reference count checking
> >   perf namespaces: Add reference count checking
> >   perf maps: Add reference count checking.
> >   perf map: Add reference count checking
> >
> >  tools/lib/perf/Makefile                       |   2 +-
> >  tools/lib/perf/cpumap.c                       |  94 ++---
> >  tools/lib/perf/include/internal/cpumap.h      |   4 +-
> >  tools/lib/perf/include/internal/rc_check.h    |  94 +++++
> >  tools/perf/arch/s390/annotate/instructions.c  |   4 +-
> >  tools/perf/arch/x86/tests/dwarf-unwind.c      |   2 +-
> >  tools/perf/arch/x86/util/event.c              |  13 +-
> >  tools/perf/builtin-annotate.c                 |  11 +-
> >  tools/perf/builtin-buildid-list.c             |   4 +-
> >  tools/perf/builtin-inject.c                   |  12 +-
> >  tools/perf/builtin-kallsyms.c                 |   6 +-
> >  tools/perf/builtin-kmem.c                     |   4 +-
> >  tools/perf/builtin-lock.c                     |   4 +-
> >  tools/perf/builtin-mem.c                      |  10 +-
> >  tools/perf/builtin-report.c                   |  26 +-
> >  tools/perf/builtin-script.c                   |  27 +-
> >  tools/perf/builtin-top.c                      |  17 +-
> >  tools/perf/builtin-trace.c                    |   2 +-
> >  .../scripts/python/Perf-Trace-Util/Context.c  |  13 +-
> >  tools/perf/tests/code-reading.c               |  37 +-
> >  tools/perf/tests/cpumap.c                     |   4 +-
> >  tools/perf/tests/hists_common.c               |   8 +-
> >  tools/perf/tests/hists_cumulate.c             |  14 +-
> >  tools/perf/tests/hists_filter.c               |  14 +-
> >  tools/perf/tests/hists_link.c                 |  18 +-
> >  tools/perf/tests/hists_output.c               |  12 +-
> >  tools/perf/tests/maps.c                       |  69 ++--
> >  tools/perf/tests/mmap-thread-lookup.c         |   3 +-
> >  tools/perf/tests/symbols.c                    |   6 +-
> >  tools/perf/tests/thread-maps-share.c          |  29 +-
> >  tools/perf/tests/vmlinux-kallsyms.c           |  54 +--
> >  tools/perf/ui/browsers/annotate.c             |   9 +-
> >  tools/perf/ui/browsers/hists.c                |  19 +-
> >  tools/perf/ui/browsers/map.c                  |   4 +-
> >  tools/perf/util/annotate.c                    |  40 ++-
> >  tools/perf/util/auxtrace.c                    |   2 +-
> >  tools/perf/util/block-info.c                  |   4 +-
> >  tools/perf/util/bpf-event.c                   |  10 +-
> >  tools/perf/util/bpf_lock_contention.c         |   6 +-
> >  tools/perf/util/build-id.c                    |   2 +-
> >  tools/perf/util/callchain.c                   |  24 +-
> >  tools/perf/util/cpumap.c                      |  40 ++-
> >  tools/perf/util/data-convert-json.c           |  10 +-
> >  tools/perf/util/db-export.c                   |  16 +-
> >  tools/perf/util/dlfilter.c                    |  28 +-
> >  tools/perf/util/dso.c                         |   8 +-
> >  tools/perf/util/dsos.c                        |   2 +-
> >  tools/perf/util/event.c                       |  27 +-
> >  tools/perf/util/evsel_fprintf.c               |   4 +-
> >  tools/perf/util/hist.c                        |  22 +-
> >  tools/perf/util/intel-pt.c                    |  63 ++--
> >  tools/perf/util/machine.c                     | 252 ++++++++------
> >  tools/perf/util/map.c                         | 217 ++++++------
> >  tools/perf/util/map.h                         |  74 +++-
> >  tools/perf/util/maps.c                        | 318 ++++++++++-------
> >  tools/perf/util/maps.h                        |  67 +++-
> >  tools/perf/util/namespaces.c                  | 132 +++++---
> >  tools/perf/util/namespaces.h                  |   3 +-
> >  tools/perf/util/pmu.c                         |   8 +-
> >  tools/perf/util/probe-event.c                 |  62 ++--
> >  .../util/scripting-engines/trace-event-perl.c |  10 +-
> >  .../scripting-engines/trace-event-python.c    |  26 +-
> >  tools/perf/util/sort.c                        |  67 ++--
> >  tools/perf/util/symbol-elf.c                  |  41 ++-
> >  tools/perf/util/symbol.c                      | 320 +++++++++++-------
> >  tools/perf/util/symbol_fprintf.c              |   2 +-
> >  tools/perf/util/synthetic-events.c            |  34 +-
> >  tools/perf/util/thread-stack.c                |   4 +-
> >  tools/perf/util/thread.c                      |  39 +--
> >  tools/perf/util/unwind-libdw.c                |  20 +-
> >  tools/perf/util/unwind-libunwind-local.c      |  16 +-
> >  tools/perf/util/unwind-libunwind.c            |  33 +-
> >  tools/perf/util/vdso.c                        |   7 +-
> >  73 files changed, 1665 insertions(+), 1044 deletions(-)
> >  create mode 100644 tools/lib/perf/include/internal/rc_check.h
> >
> > --
> > 2.40.0.rc1.284.g88254d51c5-goog
> >

-- 

- Arnaldo

Em Tue, Apr 04, 2023 at 02:02:36PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Apr 04, 2023 at 08:58:55AM -0700, Ian Rogers escreveu:
> > Ping. It would be nice to have this landed or at least the first 10
> > patches that refactor the map API and are the bulk of the
> > lines-of-code changed. Having those landed would make it easier to
> > rebase in the future, but I also think the whole series is ready to
> > go.
> 
> I'm trying to get it to compile:
> 
>   CC      /tmp/build/perf-tools-next/util/bpf-event.o
> In file included from util/machine.h:7,
>                  from util/session.h:8,
>                  from util/unwind-libunwind-local.c:35:
> util/unwind-libunwind-local.c: In function ‘read_unwind_spec_eh_frame’:
> util/maps.h:29:18: error: assignment to ‘struct map *’ from incompatible pointer type ‘struct map_rb_node *’ [-Werror=incompatible-pointer-types]
>    29 |         for (map = maps__first(maps); map; map = map_rb_node__next(map))
>       |                  ^
> util/unwind-libunwind-local.c:328:9: note: in expansion of macro ‘maps__for_each_entry’
> 
> ⬢[acme@toolbox perf-tools-next]$ git log --oneline -1
> 51a0f26e88c893ac (HEAD) perf maps: Remove rb_node from struct map
> ⬢[acme@toolbox perf-tools-next]$
> 
> I'm also making some changes to reduce the number of patch lines and
> conserve the project 'git blame' usefulness, not changing the logic in
> your patches.

The fix for the above problem demonstrate the changes I made to this
patch, see the

  struct map *map = map_node->map;

Line, to avoid touching the logic right after it.

Now I'm working on this other error:

  CC      /tmp/build/perf-tools-next/util/jitdump.o
  CC      /tmp/build/perf-tools-next/util/bpf-event.o
util/unwind-libunwind.c: In function ‘unwind__get_entries’:
util/unwind-libunwind.c:95:24: error: too few arguments to function ‘ops->get_entries’
   95 |                 return ops->get_entries(cb, arg, thread, data, max_stack);
      |                        ^~~
util/unwind-libunwind.c:90:31: error: unused parameter ‘best_effort’ [-Werror=unused-parameter]
   90 |                          bool best_effort)
      |                               ^
cc1: all warnings being treated as errors
make[4]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:97: /tmp/build/perf-tools-next/util/unwind-libunwind.o] Error 1
make[4]: *** Waiting for unfinished jobs....
  LD      /tmp/build/perf-tools-next/ui/browsers/perf-in.o
  LD      /tmp/build/perf-tools-next/ui/perf-in.o
  LD      /tmp/build/perf-tools-next/util/scripting-engines/perf-in.o
make[3]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:140: util] Error 2
make[2]: *** [Makefile.perf:676: /tmp/build/perf-tools-next/perf-in.o] Error 2
make[1]: *** [Makefile.perf:236: sub-make] Error 2
make: *** [Makefile:113: install-bin] Error 2
make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'

 Performance counter stats for 'make -k BUILD_BPF_SKEL=1 CORESIGHT=1 PYTHON=python3 O=/tmp/build/perf-tools-next -C tools/perf install-bin':

      162599516548      cycles:u
      194726899066      instructions:u                   #    1.20  insn per cycle

       4.991056085 seconds time elapsed

      39.350659000 seconds user
       8.413527000 seconds sys


⬢[acme@toolbox perf-tools-next]$ git log --oneline -1
a95f2d0f62bfd750 (HEAD) perf maps: Add functions to access maps
⬢[acme@toolbox perf-tools-next]$

- Arnaldo