Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 27 Feb 2022 23:50:38 +0100
From:   Jiri Olsa <olsajiri@gmail.com>
To:     James Clark <james.clark@arm.com>
Cc:     acme@kernel.org, linux-perf-users@vger.kernel.org,
        coresight@lists.linaro.org, Denis Nikitin <denik@chromium.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] perf: Set build-id using build-id header on new mmap
 records
Message-ID: <YhwAPrOP/ky4HLfC@krava>
References: <20220224171955.862983-1-james.clark@arm.com>
 <20220224171955.862983-2-james.clark@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220224171955.862983-2-james.clark@arm.com>
Precedence: bulk

On Thu, Feb 24, 2022 at 05:19:55PM +0000, James Clark wrote:
> MMAP records that occur after the build-id header is parsed do not have
> their build-id set even if the filename matches an entry from the
> header. Set the build-id on these dsos as long as the MMAP record
> doesn't have its own build-id set.
> 
> This fixes an issue with off target analysis where the local version of
> a dso is loaded rather than one from ~/.debug via a build-id.

nice catch :)

> 
> Reported-by: Denis Nikitin <denik@chromium.org>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/dso.h    |  1 +
>  tools/perf/util/header.c |  1 +
>  tools/perf/util/map.c    | 16 ++++++++++++++--
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index 011da3924fc1..3a9fd4d389b5 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -167,6 +167,7 @@ struct dso {
>  	enum dso_load_errno	load_errno;
>  	u8		 adjust_symbols:1;
>  	u8		 has_build_id:1;
> +	u8		 header_build_id:1;
>  	u8		 has_srcline:1;
>  	u8		 hit:1;
>  	u8		 annotate_warned:1;
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 6da12e522edc..571d73d4f976 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2200,6 +2200,7 @@ static int __event_process_build_id(struct perf_record_header_build_id *bev,
>  
>  		build_id__init(&bid, bev->data, size);
>  		dso__set_build_id(dso, &bid);
> +		dso->header_build_id = 1;
>  
>  		if (dso_space != DSO_SPACE__USER) {
>  			struct kmod_path m = { .name = NULL, };
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 1803d3887afe..4ae91e491e23 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -127,7 +127,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
>  
>  	if (map != NULL) {
>  		char newfilename[PATH_MAX];
> -		struct dso *dso;
> +		struct dso *dso, *header_bid_dso;
>  		int anon, no_dso, vdso, android;
>  
>  		android = is_android_lib(filename);
> @@ -185,7 +185,19 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
>  
>  		if (build_id__is_defined(bid))
>  			dso__set_build_id(dso, bid);
> -
> +		else {

nit please add { } to the if clause as well

> +			/*
> +			 * If the mmap event had no build ID, search for an existing dso from the
> +			 * build ID header by name. Otherwise only the dso loaded at the time of
> +			 * reading the header will have the build ID set and all future mmaps will
> +			 * have it missing.
> +			 */
> +			header_bid_dso = __dsos__find(&machine->dsos, filename, false);

is this 'perf top' safe? I think dso should be added in the
same thread, but please check and add comment why we don't
need locking in here

thanks,
jirka

> +			if (header_bid_dso && header_bid_dso->header_build_id) {
> +				dso__set_build_id(dso, &header_bid_dso->bid);
> +				dso->header_build_id = 1;
> +			}
> +		}
>  		dso__put(dso);
>  	}
>  	return map;
> -- 
> 2.28.0
>