Messages in this thread | | | From | Namhyung Kim <> | Date | Tue, 27 Jun 2023 09:57:48 -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: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?
Thanks, Namhyung
| |