Re: [PATCH v5 08/23] perf record: Make --buildid-mmap the default

Re: [PATCH v5 08/23] perf record: Make --buildid-mmap the default

[Namhyung Kim]

On Fri, Jun 27, 2025 at 09:50:02PM -0700, Ian Rogers wrote:
> Support for build IDs in mmap2 perf events has been present since
> Linux v5.12:
> https://lore.kernel.org/lkml/20210219194619.1780437-1-acme@kernel.org/
> Build ID mmap events don't avoid the need to inject build IDs for DSO
> touched by samples as the build ID cache is populated by perf
> record. They can avoid some cases of symbol mis-resolution caused by
> the file system changing from when a sample occurred and when the DSO
> is sought. To disable build ID scanning
> 
> Unlike the --buildid-mmap option, this doesn't disable the build ID
> cache but it does disable the processing of samples looking for DSOs
> to inject build IDs for. To disable the build ID cache the -B
> (--no-buildid) option should be used.

I think we need to add a config option to control this behavior later.
Let me think about this more.

> 
> Making this option the default was raised on the list in:
> https://lore.kernel.org/linux-perf-users/CAP-5=fXP7jN_QrGUcd55_QH5J-Y-FCaJ6=NaHVtyx0oyNh8_-Q@mail.gmail.com/
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-record.c        | 33 +++++++++++++++++++-----------
>  tools/perf/util/symbol_conf.h      |  2 +-
>  tools/perf/util/synthetic-events.c | 16 +++++++--------
>  3 files changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 53971b9de3ba..29428fb8ac67 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -171,6 +171,7 @@ struct record {
>  	bool			no_buildid_cache_set;
>  	bool			buildid_all;
>  	bool			buildid_mmap;
> +	bool			buildid_mmap_set;
>  	bool			timestamp_filename;
>  	bool			timestamp_boundary;
>  	bool			off_cpu;
> @@ -1811,6 +1812,7 @@ record__finish_output(struct record *rec)
>  			data->dir.files[i].size = lseek(data->dir.files[i].fd, 0, SEEK_CUR);
>  	}
>  
> +	/* Buildid scanning disabled or build ID in kernel and synthesized map events. */
>  	if (!rec->no_buildid) {
>  		process_buildids(rec);
>  
> @@ -3005,6 +3007,8 @@ static int perf_record_config(const char *var, const char *value, void *cb)
>  			rec->no_buildid = true;
>  		else if (!strcmp(value, "mmap"))
>  			rec->buildid_mmap = true;
> +		else if (!strcmp(value, "no-mmap"))
> +			rec->buildid_mmap = false;
>  		else
>  			return -1;
>  		return 0;
> @@ -3411,6 +3415,7 @@ static struct record record = {
>  		.synth               = PERF_SYNTH_ALL,
>  		.off_cpu_thresh_ns   = OFFCPU_THRESH,
>  	},
> +	.buildid_mmap = true,
>  };
>  
>  const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
> @@ -3577,8 +3582,8 @@ static struct option __record_options[] = {
>  		   "file", "vmlinux pathname"),
>  	OPT_BOOLEAN(0, "buildid-all", &record.buildid_all,
>  		    "Record build-id of all DSOs regardless of hits"),
> -	OPT_BOOLEAN(0, "buildid-mmap", &record.buildid_mmap,
> -		    "Record build-id in map events"),
> +	OPT_BOOLEAN_SET(0, "buildid-mmap", &record.buildid_mmap, &record.buildid_mmap_set,
> +			"Legacy record build-id in map events option which is now the default. Behaves indentically to --no-buildid. Disable with --no-buildid-mmap"),

Looks too long.  Maybe just like below:
  "Record build-id in mmap events and skip build-id processing."

The detailed explanation can go to the documentation.


>  	OPT_BOOLEAN(0, "timestamp-filename", &record.timestamp_filename,
>  		    "append timestamp to output filename"),
>  	OPT_BOOLEAN(0, "timestamp-boundary", &record.timestamp_boundary,
> @@ -4108,19 +4113,23 @@ int cmd_record(int argc, const char **argv)
>  		record.opts.record_switch_events = true;
>  	}
>  
> +	if (!rec->buildid_mmap) {
> +		pr_debug("Disabling build id in synthesized mmap2 events.\n");
> +		symbol_conf.no_buildid_mmap2 = true;
> +	} else if (rec->buildid_mmap_set) {
> +		/*
> +		 * Explicitly passing --buildid-mmap disables buildid processing
> +		 * and cache generation.
> +		 */
> +		rec->no_buildid = true;
> +	}
> +	if (rec->buildid_mmap && !perf_can_record_build_id()) {
> +		pr_warning("Missing support for build id in kernel mmap events. Disable this warning with --no-buildid-mmap\n");

This can be in two lines.  I can make the changes if you're ok.

Thanks,
Namhyung


> +		rec->buildid_mmap = false;
> +	}
>  	if (rec->buildid_mmap) {
> -		if (!perf_can_record_build_id()) {
> -			pr_err("Failed: no support to record build id in mmap events, update your kernel.\n");
> -			err = -EINVAL;
> -			goto out_opts;
> -		}
> -		pr_debug("Enabling build id in mmap2 events.\n");
> -		/* Enable mmap build id synthesizing. */
> -		symbol_conf.buildid_mmap2 = true;
>  		/* Enable perf_event_attr::build_id bit. */
>  		rec->opts.build_id = true;
> -		/* Disable build id cache. */
> -		rec->no_buildid = true;
>  	}
>  
>  	if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
> diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
> index cd9aa82c7d5a..7a80d2c14d9b 100644
> --- a/tools/perf/util/symbol_conf.h
> +++ b/tools/perf/util/symbol_conf.h
> @@ -43,7 +43,7 @@ struct symbol_conf {
>  			report_individual_block,
>  			inline_name,
>  			disable_add2line_warn,
> -			buildid_mmap2,
> +			no_buildid_mmap2,
>  			guest_code,
>  			lazy_load_kernel_maps,
>  			keep_exited_threads,
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 69b98023ce74..638d7dd7fa4b 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -532,7 +532,7 @@ int perf_event__synthesize_mmap_events(const struct perf_tool *tool,
>  		event->mmap2.pid = tgid;
>  		event->mmap2.tid = pid;
>  
> -		if (symbol_conf.buildid_mmap2)
> +		if (!symbol_conf.no_buildid_mmap2)
>  			perf_record_mmap2__read_build_id(&event->mmap2, machine, false);
>  
>  		if (perf_tool__process_synth_event(tool, event, machine, process) != 0) {
> @@ -690,7 +690,7 @@ static int perf_event__synthesize_modules_maps_cb(struct map *map, void *data)
>  		return 0;
>  
>  	dso = map__dso(map);
> -	if (symbol_conf.buildid_mmap2) {
> +	if (!symbol_conf.no_buildid_mmap2) {
>  		size = PERF_ALIGN(dso__long_name_len(dso) + 1, sizeof(u64));
>  		event->mmap2.header.type = PERF_RECORD_MMAP2;
>  		event->mmap2.header.size = (sizeof(event->mmap2) -
> @@ -734,9 +734,9 @@ int perf_event__synthesize_modules(const struct perf_tool *tool, perf_event__han
>  		.process = process,
>  		.machine = machine,
>  	};
> -	size_t size = symbol_conf.buildid_mmap2
> -		? sizeof(args.event->mmap2)
> -		: sizeof(args.event->mmap);
> +	size_t size = symbol_conf.no_buildid_mmap2
> +		? sizeof(args.event->mmap)
> +		: sizeof(args.event->mmap2);
>  
>  	args.event = zalloc(size + machine->id_hdr_size);
>  	if (args.event == NULL) {
> @@ -1124,8 +1124,8 @@ static int __perf_event__synthesize_kernel_mmap(const struct perf_tool *tool,
>  						struct machine *machine)
>  {
>  	union perf_event *event;
> -	size_t size = symbol_conf.buildid_mmap2 ?
> -			sizeof(event->mmap2) : sizeof(event->mmap);
> +	size_t size = symbol_conf.no_buildid_mmap2 ?
> +			sizeof(event->mmap) : sizeof(event->mmap2);
>  	struct map *map = machine__kernel_map(machine);
>  	struct kmap *kmap;
>  	int err;
> @@ -1159,7 +1159,7 @@ static int __perf_event__synthesize_kernel_mmap(const struct perf_tool *tool,
>  		event->header.misc = PERF_RECORD_MISC_GUEST_KERNEL;
>  	}
>  
> -	if (symbol_conf.buildid_mmap2) {
> +	if (!symbol_conf.no_buildid_mmap2) {
>  		size = snprintf(event->mmap2.filename, sizeof(event->mmap2.filename),
>  				"%s%s", machine->mmap_name, kmap->ref_reloc_sym->name) + 1;
>  		size = PERF_ALIGN(size, sizeof(u64));
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
> 

Re: [PATCH v5 16/23] perf bench synthesize: Avoid use of global perf_env

[Namhyung Kim]

On Fri, Jun 27, 2025 at 09:50:10PM -0700, Ian Rogers wrote:
> The benchmark doesn't use a data file and so the header perf_env isn't
> used. Stack allocate a host perf_env for use to avoid the use of the
> global perf_env.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/bench/synthesize.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/bench/synthesize.c b/tools/perf/bench/synthesize.c
> index 9b333276cbdb..79d99ba50284 100644
> --- a/tools/perf/bench/synthesize.c
> +++ b/tools/perf/bench/synthesize.c
> @@ -114,10 +114,13 @@ static int run_single_threaded(void)
>  		.pid = "self",
>  	};
>  	struct perf_thread_map *threads;
> +	struct perf_env host_env;
>  	int err;
>  
>  	perf_set_singlethreaded();
> -	session = perf_session__new(NULL, NULL);
> +	perf_env__init(&host_env);
> +	session = __perf_session__new(/*data=*/NULL, /*tool=*/NULL,
> +				      /*trace_event_repipe=*/false, &host_env);
>  	if (IS_ERR(session)) {
>  		pr_err("Session creation failed.\n");
>  		return PTR_ERR(session);

Missing perf_env__exit() ?


> @@ -144,6 +147,7 @@ static int run_single_threaded(void)
>  		perf_thread_map__put(threads);
>  
>  	perf_session__delete(session);
> +	perf_env__exit(&host_env);
>  	return err;
>  }
>  
> @@ -154,17 +158,21 @@ static int do_run_multi_threaded(struct target *target,
>  	u64 runtime_us;
>  	unsigned int i;
>  	double time_average, time_stddev, event_average, event_stddev;
> -	int err;
> +	int err = 0;
>  	struct stats time_stats, event_stats;
>  	struct perf_session *session;
> +	struct perf_env host_env;
>  
> +	perf_env__init(&host_env);
>  	init_stats(&time_stats);
>  	init_stats(&event_stats);
>  	for (i = 0; i < multi_iterations; i++) {
> -		session = perf_session__new(NULL, NULL);
> -		if (IS_ERR(session))
> -			return PTR_ERR(session);
> -
> +		session = __perf_session__new(/*data=*/NULL, /*tool=*/NULL,
> +					      /*trace_event_repipe=*/false, &host_env);
> +		if (IS_ERR(session)) {
> +			err = PTR_ERR(session);
> +			goto err_out;
> +		}
>  		atomic_set(&event_count, 0);
>  		gettimeofday(&start, NULL);
>  		err = __machine__synthesize_threads(&session->machines.host,
> @@ -173,10 +181,8 @@ static int do_run_multi_threaded(struct target *target,
>  						process_synthesized_event,
>  						true, false,
>  						nr_threads_synthesize);
> -		if (err) {
> -			perf_session__delete(session);
> -			return err;
> -		}
> +		if (err)
> +			goto err_out;

Missing perf_session__delete() ?


>  
>  		gettimeofday(&end, NULL);
>  		timersub(&end, &start, &diff);
> @@ -198,7 +204,9 @@ static int do_run_multi_threaded(struct target *target,
>  
>  	printf("    Average time per event %.3f usec\n",
>  		time_average / event_average);
> -	return 0;
> +err_out:
> +	perf_env__exit(&host_env);
> +	return err;
>  }
>  
>  static int run_multi_threaded(void)
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
> 

Re: [PATCH v5 23/23] perf sort: Use perf_env to set arch sort keys and header

[Namhyung Kim]

On Fri, Jun 27, 2025 at 09:50:17PM -0700, Ian Rogers wrote:
> Previously arch_support_sort_key and arch_perf_header_entry used a
> weak symbol to compile as appropriate for x86 and powerpc. A
> limitation to this is that the handling of a data file could vary in
> cross-platform development. Change to using the perf_env of the
> current session to determine the architecture kind and set the sort
> key and header entries as appropriate.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
[SNIP]
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index ada8e0166c78..6f24540bdee9 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1790,7 +1790,7 @@ int cmd_report(int argc, const char **argv)
>  	}
>  
>  	if ((last_key != K_SWITCH_INPUT_DATA && last_key != K_RELOAD) &&
> -	    (setup_sorting(session->evlist) < 0)) {
> +	    (setup_sorting(session->evlist, &session->header.env) < 0)) {

					    perf_session__env(session) ?

Thanks,
Namhyung


>  		if (sort_order)
>  			parse_options_usage(report_usage, options, "s", 1);
>  		if (field_order)

Re: [PATCH v5 10/23] perf session: Add an env pointer for the current perf_env

[Namhyung Kim]

On Fri, Jun 27, 2025 at 09:50:04PM -0700, Ian Rogers wrote:
> Initialize to `&header.env`. This will later allow the env value to be
> changed.

I'm curious when it is changed.

Thanks,
Namhyung

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/session.c | 3 ++-
>  tools/perf/util/session.h | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index b09d157f7d04..e39a1df7c044 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -156,6 +156,7 @@ struct perf_session *__perf_session__new(struct perf_data *data,
>  			     ordered_events__deliver_event, NULL);
>  
>  	perf_env__init(&session->header.env);
> +	session->env = &session->header.env;
>  	if (data) {
>  		ret = perf_data__open(data);
>  		if (ret < 0)
> @@ -2750,5 +2751,5 @@ int perf_session__dsos_hit_all(struct perf_session *session)
>  
>  struct perf_env *perf_session__env(struct perf_session *session)
>  {
> -	return &session->header.env;
> +	return session->env;
>  }
> diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
> index e7f7464b838f..088868f1004a 100644
> --- a/tools/perf/util/session.h
> +++ b/tools/perf/util/session.h
> @@ -45,6 +45,8 @@ struct perf_session {
>  	struct perf_header	header;
>  	/** @machines: Machines within the session a host and 0 or more guests. */
>  	struct machines		machines;
> +	/** @env: The perf_env being worked with, either from a data file or the host's. */
> +	struct perf_env		*env;
>  	/** @evlist: List of evsels/events of the session. */
>  	struct evlist	*evlist;
>  	/** @auxtrace: callbacks to allow AUX area data decoding. */
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
> 

Re: [PATCH v5 11/23] perf evlist: Change env variable to session

[Namhyung Kim]

On Fri, Jun 27, 2025 at 09:50:05PM -0700, Ian Rogers wrote:
> The session holds a perf_env pointer env. In UI code container_of is
> used to turn the env to a session, but this assumes the session
> header's env is in use. Rather than a dubious container_of, hold the
> session in the evlist and derive the env from the session with
> evsel__env, perf_session__env, etc.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-report.c      |  6 +++++-
>  tools/perf/builtin-script.c      |  2 +-
>  tools/perf/builtin-top.c         |  2 +-
>  tools/perf/tests/topology.c      |  1 +
>  tools/perf/ui/browser.h          |  4 ++--
>  tools/perf/ui/browsers/header.c  |  4 +---
>  tools/perf/ui/browsers/hists.c   |  2 +-
>  tools/perf/util/amd-sample-raw.c |  2 +-
>  tools/perf/util/arm-spe.c        |  2 +-
>  tools/perf/util/evlist.h         |  2 +-
>  tools/perf/util/evsel.c          | 12 +++++++++---
>  tools/perf/util/evsel.h          |  1 +
>  tools/perf/util/header.c         |  2 +-
>  tools/perf/util/s390-cpumsf.c    |  2 +-
>  tools/perf/util/sample-raw.c     |  7 ++++---
>  tools/perf/util/sample-raw.h     |  2 +-
>  tools/perf/util/session.c        |  4 +++-
>  17 files changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 704576e46e4b..ada8e0166c78 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1274,6 +1274,8 @@ static int process_attr(const struct perf_tool *tool __maybe_unused,
>  			union perf_event *event,
>  			struct evlist **pevlist)
>  {
> +	struct perf_session *session;
> +	struct perf_env *env;
>  	u64 sample_type;
>  	int err;
>  
> @@ -1286,7 +1288,9 @@ static int process_attr(const struct perf_tool *tool __maybe_unused,
>  	 * on events sample_type.
>  	 */
>  	sample_type = evlist__combined_sample_type(*pevlist);
> -	callchain_param_setup(sample_type, perf_env__arch((*pevlist)->env));
> +	session = (*pevlist)->session;
> +	env = perf_session__env(session);
> +	callchain_param_setup(sample_type, perf_env__arch(env));
>  	return 0;
>  }
>  
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 8a452353c867..2c25eda4be26 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -2535,7 +2535,7 @@ static int process_attr(const struct perf_tool *tool, union perf_event *event,
>  	 * on events sample_type.
>  	 */
>  	sample_type = evlist__combined_sample_type(evlist);
> -	callchain_param_setup(sample_type, perf_env__arch((*pevlist)->env));
> +	callchain_param_setup(sample_type, perf_env__arch(perf_session__env(scr->session)));
>  
>  	/* Enable fields for callchain entries */
>  	if (symbol_conf.use_callchain &&
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 84b223a94dcf..72f9be5a3b30 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1654,7 +1654,6 @@ int cmd_top(int argc, const char **argv)
>  			"Couldn't read the cpuid for this machine: %s\n",
>  			str_error_r(errno, errbuf, sizeof(errbuf)));
>  	}
> -	top.evlist->env = &perf_env;
>  
>  	argc = parse_options(argc, argv, options, top_usage, 0);
>  	if (argc)
> @@ -1822,6 +1821,7 @@ int cmd_top(int argc, const char **argv)
>  		perf_top__update_print_entries(&top);
>  		signal(SIGWINCH, winch_sig);
>  	}
> +	top.session->env = &perf_env;

Looks like it's accessing top.session before it's created?

Thanks,
Namhyung

>  
>  	top.session = perf_session__new(NULL, NULL);
>  	if (IS_ERR(top.session)) {

Re: [PATCH v5 03/23] perf build-id: Change sprintf functions to snprintf

[Namhyung Kim]

On Fri, Jun 27, 2025 at 09:49:57PM -0700, Ian Rogers wrote:
> Pass in a size argument rather than implying all build id strings must
> be SBUILD_ID_SIZE.

I got these errors:

  CC      util/parse-event.o                                                   
util/probe-event.c: In function '__show_line_range':                            
util/probe-event.c:1093:17: error: implicit declaration of function 'build_id__sprintf'; did you mean 'build_id__snprintf'? [-Wimplicit-function-declaration]
 1093 |                 build_id__sprintf(&bid, sbuild_id);                     
      |                 ^~~~~~~~~~~~~~~~~                                       
      |                 build_id__snprintf    
  CC      util/parse-finder.o                                                   
util/probe-finder.c: In function 'find_probe_point_lazy':                       
util/probe-finder.c:863:25: error: implicit declaration of function 'build_id__sprintf'; did you mean 'build_id__snprintf'? [-Wimplicit-function-declaration]
  863 |                         build_id__sprintf(&bid, sbuild_id);             
      |                         ^~~~~~~~~~~~~~~~~                               
      |                         build_id__snprintf                              
make[4]: *** [/usr/local/google/home/namhyung/project/linux/tools/build/Makefile.build:86: util/probe-finder.o] Error 1
make[4]: *** Waiting for unfinished jobs....                                    
make[4]: *** [/usr/local/google/home/namhyung/project/linux/tools/build/Makefile.build:85: util/probe-event.o] Error 1
make[3]: *** [/usr/local/google/home/namhyung/project/linux/tools/build/Makefile.build:142: util] Error 2
make[2]: *** [Makefile.perf:797: perf-util-in.o] Error 2                        
make[2]: *** Waiting for unfinished jobs....                                    
make[1]: *** [Makefile.perf:289: sub-make] Error 2                              
make: *** [Makefile:76: all] Error 2

Thanks,
Namhyung

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-buildid-cache.c            | 12 +++----
>  tools/perf/builtin-buildid-list.c             |  6 ++--
>  tools/perf/util/build-id.c                    | 33 ++++++++-----------
>  tools/perf/util/build-id.h                    |  6 ++--
>  tools/perf/util/disasm.c                      |  2 +-
>  tools/perf/util/dso.c                         |  4 +--
>  tools/perf/util/dsos.c                        |  2 +-
>  tools/perf/util/event.c                       |  2 +-
>  tools/perf/util/header.c                      |  2 +-
>  tools/perf/util/map.c                         |  2 +-
>  tools/perf/util/probe-file.c                  |  4 +--
>  .../scripting-engines/trace-event-python.c    |  7 ++--
>  tools/perf/util/symbol.c                      |  2 +-
>  13 files changed, 38 insertions(+), 46 deletions(-)
> 
> diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
> index b0511d16aeb6..3f7739b21148 100644
> --- a/tools/perf/builtin-buildid-cache.c
> +++ b/tools/perf/builtin-buildid-cache.c
> @@ -31,7 +31,7 @@
>  #include <linux/string.h>
>  #include <linux/err.h>
>  
> -static int build_id_cache__kcore_buildid(const char *proc_dir, char *sbuildid)
> +static int build_id_cache__kcore_buildid(const char *proc_dir, char *sbuildid, size_t sbuildid_size)
>  {
>  	char root_dir[PATH_MAX];
>  	char *p;
> @@ -42,7 +42,7 @@ static int build_id_cache__kcore_buildid(const char *proc_dir, char *sbuildid)
>  	if (!p)
>  		return -1;
>  	*p = '\0';
> -	return sysfs__sprintf_build_id(root_dir, sbuildid);
> +	return sysfs__snprintf_build_id(root_dir, sbuildid, sbuildid_size);
>  }
>  
>  static int build_id_cache__kcore_dir(char *dir, size_t sz)
> @@ -128,7 +128,7 @@ static int build_id_cache__add_kcore(const char *filename, bool force)
>  		return -1;
>  	*p = '\0';
>  
> -	if (build_id_cache__kcore_buildid(from_dir, sbuildid) < 0)
> +	if (build_id_cache__kcore_buildid(from_dir, sbuildid, sizeof(sbuildid)) < 0)
>  		return -1;
>  
>  	scnprintf(to_dir, sizeof(to_dir), "%s/%s/%s",
> @@ -187,7 +187,7 @@ static int build_id_cache__add_file(const char *filename, struct nsinfo *nsi)
>  		return -1;
>  	}
>  
> -	build_id__sprintf(&bid, sbuild_id);
> +	build_id__snprintf(&bid, sbuild_id, sizeof(sbuild_id));
>  	err = build_id_cache__add_s(sbuild_id, filename, nsi,
>  				    false, false);
>  	pr_debug("Adding %s %s: %s\n", sbuild_id, filename,
> @@ -211,7 +211,7 @@ static int build_id_cache__remove_file(const char *filename, struct nsinfo *nsi)
>  		return -1;
>  	}
>  
> -	build_id__sprintf(&bid, sbuild_id);
> +	build_id__snprintf(&bid, sbuild_id, sizeof(sbuild_id));
>  	err = build_id_cache__remove_s(sbuild_id);
>  	pr_debug("Removing %s %s: %s\n", sbuild_id, filename,
>  		 err ? "FAIL" : "Ok");
> @@ -317,7 +317,7 @@ static int build_id_cache__update_file(const char *filename, struct nsinfo *nsi)
>  	}
>  	err = 0;
>  
> -	build_id__sprintf(&bid, sbuild_id);
> +	build_id__snprintf(&bid, sbuild_id, sizeof(sbuild_id));
>  	if (build_id_cache__cached(sbuild_id))
>  		err = build_id_cache__remove_s(sbuild_id);
>  
> diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
> index 52dfacaff8e3..ba8ba0303920 100644
> --- a/tools/perf/builtin-buildid-list.c
> +++ b/tools/perf/builtin-buildid-list.c
> @@ -31,7 +31,7 @@ static int buildid__map_cb(struct map *map, void *arg __maybe_unused)
>  
>  	memset(bid_buf, 0, sizeof(bid_buf));
>  	if (dso__has_build_id(dso))
> -		build_id__sprintf(dso__bid_const(dso), bid_buf);
> +		build_id__snprintf(dso__bid_const(dso), bid_buf, sizeof(bid_buf));
>  	printf("%s %16" PRIx64 " %16" PRIx64, bid_buf, map__start(map), map__end(map));
>  	if (dso_long_name != NULL)
>  		printf(" %s", dso_long_name);
> @@ -57,7 +57,7 @@ static int sysfs__fprintf_build_id(FILE *fp)
>  	char sbuild_id[SBUILD_ID_SIZE];
>  	int ret;
>  
> -	ret = sysfs__sprintf_build_id("/", sbuild_id);
> +	ret = sysfs__snprintf_build_id("/", sbuild_id, sizeof(sbuild_id));
>  	if (ret != sizeof(sbuild_id))
>  		return ret < 0 ? ret : -EINVAL;
>  
> @@ -69,7 +69,7 @@ static int filename__fprintf_build_id(const char *name, FILE *fp)
>  	char sbuild_id[SBUILD_ID_SIZE];
>  	int ret;
>  
> -	ret = filename__sprintf_build_id(name, sbuild_id);
> +	ret = filename__snprintf_build_id(name, sbuild_id, sizeof(sbuild_id));
>  	if (ret != sizeof(sbuild_id))
>  		return ret < 0 ? ret : -EINVAL;
>  
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index 5bc2040bdd0d..aa35dceace90 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -67,24 +67,17 @@ int build_id__mark_dso_hit(const struct perf_tool *tool __maybe_unused,
>  	return 0;
>  }
>  
> -int build_id__sprintf(const struct build_id *build_id, char *bf)
> +int build_id__snprintf(const struct build_id *build_id, char *bf, size_t bf_size)
>  {
> -	char *bid = bf;
> -	const u8 *raw = build_id->data;
> -	size_t i;
> -
> -	bf[0] = 0x0;
> +	size_t offs = 0;
>  
> -	for (i = 0; i < build_id->size; ++i) {
> -		sprintf(bid, "%02x", *raw);
> -		++raw;
> -		bid += 2;
> -	}
> +	for (size_t i = 0; i < build_id->size && offs < bf_size; ++i)
> +		offs += snprintf(bf + offs, bf_size - offs, "%02x", build_id->data[i]);
>  
> -	return (bid - bf) + 1;
> +	return offs;
>  }
>  
> -int sysfs__sprintf_build_id(const char *root_dir, char *sbuild_id)
> +int sysfs__snprintf_build_id(const char *root_dir, char *sbuild_id, size_t sbuild_id_size)
>  {
>  	char notes[PATH_MAX];
>  	struct build_id bid;
> @@ -99,10 +92,10 @@ int sysfs__sprintf_build_id(const char *root_dir, char *sbuild_id)
>  	if (ret < 0)
>  		return ret;
>  
> -	return build_id__sprintf(&bid, sbuild_id);
> +	return build_id__snprintf(&bid, sbuild_id, sbuild_id_size);
>  }
>  
> -int filename__sprintf_build_id(const char *pathname, char *sbuild_id)
> +int filename__snprintf_build_id(const char *pathname, char *sbuild_id, size_t sbuild_id_size)
>  {
>  	struct build_id bid;
>  	int ret;
> @@ -111,7 +104,7 @@ int filename__sprintf_build_id(const char *pathname, char *sbuild_id)
>  	if (ret < 0)
>  		return ret;
>  
> -	return build_id__sprintf(&bid, sbuild_id);
> +	return build_id__snprintf(&bid, sbuild_id, sbuild_id_size);
>  }
>  
>  /* asnprintf consolidates asprintf and snprintf */
> @@ -212,9 +205,9 @@ static bool build_id_cache__valid_id(char *sbuild_id)
>  		return false;
>  
>  	if (!strcmp(pathname, DSO__NAME_KALLSYMS))
> -		ret = sysfs__sprintf_build_id("/", real_sbuild_id);
> +		ret = sysfs__snprintf_build_id("/", real_sbuild_id, sizeof(real_sbuild_id));
>  	else if (pathname[0] == '/')
> -		ret = filename__sprintf_build_id(pathname, real_sbuild_id);
> +		ret = filename__snprintf_build_id(pathname, real_sbuild_id, sizeof(real_sbuild_id));
>  	else
>  		ret = -EINVAL;	/* Should we support other special DSO cache? */
>  	if (ret >= 0)
> @@ -243,7 +236,7 @@ char *__dso__build_id_filename(const struct dso *dso, char *bf, size_t size,
>  	if (!dso__has_build_id(dso))
>  		return NULL;
>  
> -	build_id__sprintf(dso__bid_const(dso), sbuild_id);
> +	build_id__snprintf(dso__bid_const(dso), sbuild_id, sizeof(sbuild_id));
>  	linkname = build_id_cache__linkname(sbuild_id, NULL, 0);
>  	if (!linkname)
>  		return NULL;
> @@ -769,7 +762,7 @@ static int build_id_cache__add_b(const struct build_id *bid,
>  {
>  	char sbuild_id[SBUILD_ID_SIZE];
>  
> -	build_id__sprintf(bid, sbuild_id);
> +	build_id__snprintf(bid, sbuild_id, sizeof(sbuild_id));
>  
>  	return __build_id_cache__add_s(sbuild_id, name, nsi, is_kallsyms,
>  				       is_vdso, proper_name, root_dir);
> diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
> index e3e0a446ff0c..47e621cebe1b 100644
> --- a/tools/perf/util/build-id.h
> +++ b/tools/perf/util/build-id.h
> @@ -21,10 +21,10 @@ struct feat_fd;
>  struct nsinfo;
>  
>  void build_id__init(struct build_id *bid, const u8 *data, size_t size);
> -int build_id__sprintf(const struct build_id *build_id, char *bf);
> +int build_id__snprintf(const struct build_id *build_id, char *bf, size_t bf_size);
>  bool build_id__is_defined(const struct build_id *bid);
> -int sysfs__sprintf_build_id(const char *root_dir, char *sbuild_id);
> -int filename__sprintf_build_id(const char *pathname, char *sbuild_id);
> +int sysfs__snprintf_build_id(const char *root_dir, char *sbuild_id, size_t sbuild_id_size);
> +int filename__snprintf_build_id(const char *pathname, char *sbuild_id, size_t sbuild_id_size);
>  char *build_id_cache__kallsyms_path(const char *sbuild_id, char *bf,
>  				    size_t size);
>  
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 8f0eb56c6fc6..96e6a5d6eacc 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1218,7 +1218,7 @@ int symbol__strerror_disassemble(struct map_symbol *ms, int errnum, char *buf, s
>  		char *build_id_msg = NULL;
>  
>  		if (dso__has_build_id(dso)) {
> -			build_id__sprintf(dso__bid(dso), bf + 15);
> +			build_id__snprintf(dso__bid(dso), bf + 15, sizeof(bf) - 15);
>  			build_id_msg = bf;
>  		}
>  		scnprintf(buf, buflen,
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 057fcf4225ac..97664610c37e 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -217,7 +217,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
>  			break;
>  		}
>  
> -		build_id__sprintf(dso__bid_const(dso), build_id_hex);
> +		build_id__snprintf(dso__bid_const(dso), build_id_hex, sizeof(build_id_hex));
>  		len = __symbol__join_symfs(filename, size, "/usr/lib/debug/.build-id/");
>  		snprintf(filename + len, size - len, "%.2s/%s.debug",
>  			 build_id_hex, build_id_hex + 2);
> @@ -1704,7 +1704,7 @@ static size_t dso__fprintf_buildid(struct dso *dso, FILE *fp)
>  {
>  	char sbuild_id[SBUILD_ID_SIZE];
>  
> -	build_id__sprintf(dso__bid(dso), sbuild_id);
> +	build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
>  	return fprintf(fp, "%s", sbuild_id);
>  }
>  
> diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
> index 4d213017d202..47538273915d 100644
> --- a/tools/perf/util/dsos.c
> +++ b/tools/perf/util/dsos.c
> @@ -373,7 +373,7 @@ static int dsos__fprintf_buildid_cb(struct dso *dso, void *data)
>  
>  	if (args->skip && args->skip(dso, args->parm))
>  		return 0;
> -	build_id__sprintf(dso__bid(dso), sbuild_id);
> +	build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
>  	args->ret += fprintf(args->fp, "%-40s %s\n", sbuild_id, dso__long_name(dso));
>  	return 0;
>  }
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 14b0d3689137..fcf44149feb2 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -334,7 +334,7 @@ size_t perf_event__fprintf_mmap2(union perf_event *event, FILE *fp)
>  
>  		build_id__init(&bid, event->mmap2.build_id,
>  			       event->mmap2.build_id_size);
> -		build_id__sprintf(&bid, sbuild_id);
> +		build_id__snprintf(&bid, sbuild_id, sizeof(sbuild_id));
>  
>  		return fprintf(fp, " %d/%d: [%#" PRI_lx64 "(%#" PRI_lx64 ") @ %#" PRI_lx64
>  				   " <%s>]: %c%c%c%c %s\n",
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 2dea35237e81..44941b3adddd 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2291,7 +2291,7 @@ static int __event_process_build_id(struct perf_record_header_build_id *bev,
>  			free(m.name);
>  		}
>  
> -		build_id__sprintf(dso__bid(dso), sbuild_id);
> +		build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
>  		pr_debug("build id event received for %s: %s [%zu]\n",
>  			 dso__long_name(dso), sbuild_id, size);
>  		dso__put(dso);
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index d729438b7d65..0f6b185f9589 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -354,7 +354,7 @@ int map__load(struct map *map)
>  		if (dso__has_build_id(dso)) {
>  			char sbuild_id[SBUILD_ID_SIZE];
>  
> -			build_id__sprintf(dso__bid(dso), sbuild_id);
> +			build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
>  			pr_debug("%s with build id %s not found", name, sbuild_id);
>  		} else
>  			pr_debug("Failed to open %s", name);
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index ec8ac242fedb..5069fb61f48c 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -448,10 +448,10 @@ static int probe_cache__open(struct probe_cache *pcache, const char *target,
>  	if (!target || !strcmp(target, DSO__NAME_KALLSYMS)) {
>  		target = DSO__NAME_KALLSYMS;
>  		is_kallsyms = true;
> -		ret = sysfs__sprintf_build_id("/", sbuildid);
> +		ret = sysfs__snprintf_build_id("/", sbuildid, sizeof(sbuildid));
>  	} else {
>  		nsinfo__mountns_enter(nsi, &nsc);
> -		ret = filename__sprintf_build_id(target, sbuildid);
> +		ret = filename__snprintf_build_id(target, sbuildid, sizeof(sbuildid));
>  		nsinfo__mountns_exit(&nsc);
>  	}
>  
> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> index 00f2c6c5114d..6655c0bbe0d8 100644
> --- a/tools/perf/util/scripting-engines/trace-event-python.c
> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> @@ -780,14 +780,13 @@ static void set_sym_in_dict(PyObject *dict, struct addr_location *al,
>  			    const char *sym_field, const char *symoff_field,
>  			    const char *map_pgoff)
>  {
> -	char sbuild_id[SBUILD_ID_SIZE];
> -
>  	if (al->map) {
> +		char sbuild_id[SBUILD_ID_SIZE];
>  		struct dso *dso = map__dso(al->map);
>  
>  		pydict_set_item_string_decref(dict, dso_field,
>  					      _PyUnicode_FromString(dso__name(dso)));
> -		build_id__sprintf(dso__bid(dso), sbuild_id);
> +		build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
>  		pydict_set_item_string_decref(dict, dso_bid_field,
>  			_PyUnicode_FromString(sbuild_id));
>  		pydict_set_item_string_decref(dict, dso_map_start,
> @@ -1238,7 +1237,7 @@ static int python_export_dso(struct db_export *dbe, struct dso *dso,
>  	char sbuild_id[SBUILD_ID_SIZE];
>  	PyObject *t;
>  
> -	build_id__sprintf(dso__bid(dso), sbuild_id);
> +	build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
>  
>  	t = tuple_new(5);
>  
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 8b30c6f16a9e..ea86a6253f04 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -2151,7 +2151,7 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map)
>  			goto proc_kallsyms;
>  	}
>  
> -	build_id__sprintf(dso__bid(dso), sbuild_id);
> +	build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
>  
>  	/* Find kallsyms in build-id cache with kcore */
>  	scnprintf(path, sizeof(path), "%s/%s/%s",
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
> 

Re: [PATCH v5 10/23] perf session: Add an env pointer for the current perf_env

[Ian Rogers]

On Mon, Jun 30, 2025 at 11:27 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, Jun 27, 2025 at 09:50:04PM -0700, Ian Rogers wrote:
> > Initialize to `&header.env`. This will later allow the env value to be
> > changed.
>
> I'm curious when it is changed.

Thanks for the reviews! I'll dig into them for v6. Looking at this one
I don't see a current use of the changed perf_session__env so I
suspect we can drop the change. I need to think about the lifetime of
header.env, use cases where have >1 env like perf inject (in vs out),
perf diff and what's going on with TUI in patch:
https://lore.kernel.org/lkml/20250628045017.1361563-12-irogers@google.com/

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/session.c | 3 ++-
> >  tools/perf/util/session.h | 2 ++
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index b09d157f7d04..e39a1df7c044 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -156,6 +156,7 @@ struct perf_session *__perf_session__new(struct perf_data *data,
> >                            ordered_events__deliver_event, NULL);
> >
> >       perf_env__init(&session->header.env);
> > +     session->env = &session->header.env;
> >       if (data) {
> >               ret = perf_data__open(data);
> >               if (ret < 0)
> > @@ -2750,5 +2751,5 @@ int perf_session__dsos_hit_all(struct perf_session *session)
> >
> >  struct perf_env *perf_session__env(struct perf_session *session)
> >  {
> > -     return &session->header.env;
> > +     return session->env;
> >  }
> > diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
> > index e7f7464b838f..088868f1004a 100644
> > --- a/tools/perf/util/session.h
> > +++ b/tools/perf/util/session.h
> > @@ -45,6 +45,8 @@ struct perf_session {
> >       struct perf_header      header;
> >       /** @machines: Machines within the session a host and 0 or more guests. */
> >       struct machines         machines;
> > +     /** @env: The perf_env being worked with, either from a data file or the host's. */
> > +     struct perf_env         *env;
> >       /** @evlist: List of evsels/events of the session. */
> >       struct evlist   *evlist;
> >       /** @auxtrace: callbacks to allow AUX area data decoding. */
> > --
> > 2.50.0.727.gbf7dc18ff4-goog
> >