Messages in this thread | | | Date | Mon, 3 Jul 2023 09:18:50 +0100 | Subject | Re: [PATCH 2/2] perf report: Don't add to histogram when there is no thread found | From | James Clark <> |
| |
On 30/06/2023 22:02, Namhyung Kim wrote: > On Wed, Jun 28, 2023 at 1:06 PM Namhyung Kim <namhyung@kernel.org> wrote: >> >> On Wed, Jun 28, 2023 at 3:34 AM James Clark <james.clark@arm.com> wrote: >>> >>> >>> >>> On 27/06/2023 18:19, Ian Rogers wrote: >>>> 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> >>> >>> Looks good to me too. Should I resend the set with this change instead >>> of my one? >> >> No, I can take care of that. I'll take this as your Acked-by. :) > > This part is applied to perf-tools-next, thanks!
Thanks Namhyung
| |