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?
Thanks, Ian
Thanks, Namhyung
-
James Clark