Re: [PATCH v2 06/30] perf env: Add host_env() singleton

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf header: write_cmdline() conditional host_env() fallback
  corrupts perf inject HEADER_CMDLINE
- [Low] perf trace: direct cmd_trace() call in main() skips host_env()
  setup/exit
--

commit 514f7c0886cdcd7ffafc72b5f2dce5946ffe0612
Author: Ian Rogers <irogers@google.com>

perf env: Add host_env() singleton

This commit introduces a statically initialized host_env() singleton to
represent the live host environment and drops the struct perf_env *
parameter from machine__new_host(), machine__new_kallsyms(),
machine__new_live(), and __perf_session__new().

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 4094141461f91..fb02fa6dd4377 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
[ ... ]
> @@ -2121,20 +2120,14 @@ static char *trace__machine__resolve_kernel_addr(void *vmachine, unsigned long l
>  	return machine__resolve_kernel_addr(vmachine, addrp, modp);
>  }
>  
> -static int trace__symbols_init(struct trace *trace, int argc, const char **argv,
> -			       struct evlist *evlist)
> +static int trace__symbols_init(struct trace *trace, struct evlist *evlist)
>  {
>  	int err = symbol__init(NULL);
>  
>  	if (err)
>  		return err;
>  
> -	perf_env__init(&trace->host_env);
> -	err = perf_env__set_cmdline(&trace->host_env, argc, argv);
> -	if (err)
> -		goto out;
> -
> -	trace->host = machine__new_host(&trace->host_env);
> +	trace->host = machine__new_host();
>  	if (trace->host == NULL) {
>  		err = -ENOMEM;
>  		goto out;
> @@ -2155,10 +2148,8 @@ static int trace__symbols_init(struct trace *trace, int argc, const char **argv,
>  					    /*mmap_data=*/false,
>  					    /*nr_threads_synthesize=*/1);
>  out:
> -	if (err) {
> -		perf_env__exit(&trace->host_env);
> +	if (err)
>  		symbol__exit();
> -	}
>  	return err;
>  }
>  
> @@ -2167,7 +2158,6 @@ static void trace__symbols__exit(struct trace *trace)
>  	machine__delete(trace->host);
>  	trace->host = NULL;
>  
> -	perf_env__exit(&trace->host_env);
>  	symbol__exit();
>  }
[ ... ]
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index d60bc929c2572..bcbc12295a387 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -380,12 +381,14 @@ static int run_builtin(const struct cmd_struct *p, int argc, const char **argv)
>  		use_pager = 1;
>  	commit_pager_choice();
>  
> +	perf_env__set_cmdline(host_env(), argc, argv);
>  	status = p->fn(argc, argv);
>  	perf_config__exit();
>  #ifdef HAVE_LIBTRACEEVENT
>  	trace_event__exit();
>  #endif
>  	exit_browser(status);
> +	perf_env__exit(host_env());
>  
>  	if (status)
>  		return status & 0xff;

[Severity: Low]
Does moving perf_env__set_cmdline() and perf_env__exit() from
trace__symbols_init() and trace__symbols__exit() into run_builtin() miss
the direct cmd_trace() invocation in main()?

tools/perf/perf.c:main() {
	...
	if (strstarts(cmd, "trace")) {
		setup_path();
		argv[0] = "trace";
		return cmd_trace(argc, argv);
	}
	...
}

When invoked via the trace alias, main() calls cmd_trace() directly
without going through run_builtin(), leaving host_env()->cmdline_argv
unpopulated for trace record and skipping perf_env__exit(host_env()) on
exit.

[ ... ]

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 9eec6deee190f..b6f86f557cafe 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -670,6 +670,9 @@ static int write_cmdline(struct feat_fd *ff,
>  	char pbuf[MAXPATHLEN], *buf;
>  	int i, ret, n;
>  
> +	if (!env->cmdline_argv)
> +		env = host_env();
> +
>  	/* actual path to perf binary */
>  	buf = perf_exe(pbuf, MAXPATHLEN);
>  

[Severity: Medium]
Can checking !env->cmdline_argv here corrupt HEADER_CMDLINE when perf
inject regenerates the output header?

When perf inject reads the input perf.data header, process_cmdline()
populates ff->ph->env.cmdline_argv from the input file, with the perf
binary path already stored at cmdline_argv[0].  Because keep_feat()
returns false for HEADER_CMDLINE, perf_session__inject_header() calls
write_cmdline() to update the feature section.  With env->cmdline_argv
already non-NULL, write_cmdline() will not switch to host_env(), so it
reuses the input file's command line instead of recording the perf inject
invocation and prepends perf_exe() a second time.  As noted above,
invoking trace directly via main() -> cmd_trace() also bypasses
perf_env__set_cmdline(host_env(), ...), leaving host_env()->cmdline_argv
NULL when write_cmdline() falls back to host_env().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113195335.2865725-1-irogers@google.com?part=6