Messages in this thread | | | From | Ian Rogers <> | Date | Tue, 27 Jun 2023 10:19:18 -0700 | Subject | Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found |
| |
On Tue, Jun 27, 2023 at 9:58 AM Namhyung Kim <namhyung@kernel.org> wrote: > > On Tue, Jun 27, 2023 at 9:43 AM Ian Rogers <irogers@google.com> wrote: > > > > On Mon, Jun 26, 2023 at 5:02 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > > > On Mon, Jun 26, 2023 at 05:10:58PM +0100, James Clark wrote: > > > > thread__find_map() chooses to exit without assigning a thread to the > > > > addr_location in some scenarios, for example when there are samples from > > > > a guest and perf_guest == false. This results in a segfault when adding > > > > to the histogram because it uses unguarded accesses to the thread member > > > > of the addr_location. > > > > > > Looking at the commit 0dd5041c9a0ea ("perf addr_location: Add > > > init/exit/copy functions") that introduced the change, I'm not sure if > > > it's the intend behavior. > > > > > > It might change maps and map, but not thread. Then I think no reason > > > to not set the al->thread at the beginning. > > > > > > How about this? Ian? > > > (I guess we can get rid of the duplicate 'al->map = NULL' part) > > > > It seemed strange that we were failing to find a map (the function's > > purpose) but then populating the address_location. The change below > > brings back that somewhat odd behavior. I'm okay with reverting to the > > old behavior, clearly there were users relying on it. We should > > probably also copy maps and not just thread, as that was the previous > > behavior. > > Probably. But it used to support samples without maps and I think > that's why it ignores the return value of thread__find_map(). So > we can expect al.map is NULL and maybe fine to leave it for now. > > As machine__resolve() returns -1 if it gets no thread, we should set > al.thread when it returns 0. > > Can I get your Acked-by?
Yep: Acked-by: Ian Rogers <irogers@google.com>
Thanks, Ian
> Thanks, > Namhyung
| |