Skip to content

Commit 88fd6f2

Browse files
committed
fix: use exact executable matching for perf unwinding mode detection
Replace substring `contains()` checks with token-based exact matching to prevent false positives (e.g. "javascript" matching "java"). Also adds gradlew, mvnw, python3 to the recognized executables.
1 parent fec652a commit 88fd6f2

File tree

3 files changed

+62
-9
lines changed

3 files changed

+62
-9
lines changed
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
use std::path::Path;
2+
3+
/// Characters that act as command separators in shell commands.
4+
const SHELL_SEPARATORS: &[char] = &['|', ';', '&', '(', ')'];
5+
6+
/// Split a command string into tokens on whitespace and shell operators,
7+
/// extracting the file name from each token (stripping directory paths).
8+
fn tokenize(command: &str) -> impl Iterator<Item = &str> + '_ {
9+
command
10+
.split(|c: char| c.is_whitespace() || SHELL_SEPARATORS.contains(&c))
11+
.filter(|t| !t.is_empty())
12+
.filter_map(|token| Path::new(token).file_name()?.to_str())
13+
}
14+
15+
/// Check if a command string contains any of the given executable names.
16+
///
17+
/// Splits the command into tokens on whitespace and shell operators, then checks
18+
/// for exact matches on the file name component. This is strictly better than
19+
/// `command.contains("java")` which would false-positive on "javascript".
20+
pub fn command_has_executable(command: &str, names: &[&str]) -> bool {
21+
tokenize(command).any(|token| names.contains(&token))
22+
}
23+
24+
#[cfg(test)]
25+
mod tests {
26+
use super::*;
27+
use rstest::rstest;
28+
29+
#[rstest]
30+
#[case("java -jar bench.jar", &["java"])]
31+
#[case("/usr/bin/java -jar bench.jar", &["java"])]
32+
#[case("FOO=bar java -jar bench.jar", &["java"])]
33+
#[case("cd /app && gradle bench", &["gradle"])]
34+
#[case("cat file | python script.py", &["python"])]
35+
#[case("sudo java -jar bench.jar", &["java"])]
36+
#[case("(cd /app && java -jar bench.jar)", &["java"])]
37+
#[case("setup.sh; java -jar bench.jar", &["java"])]
38+
#[case("try_first || java -jar bench.jar", &["java"])]
39+
#[case("cargo codspeed bench\npytest tests/ --codspeed", &["cargo"])]
40+
#[case("mvn test", &["gradle", "java", "maven", "mvn"])]
41+
#[case("./java -jar bench.jar", &["java"])]
42+
fn matches(#[case] command: &str, #[case] names: &[&str]) {
43+
assert!(command_has_executable(command, names));
44+
}
45+
46+
#[rstest]
47+
#[case("javascript-runtime run", &["java"])]
48+
#[case("/home/user/javascript/run.sh", &["java"])]
49+
#[case("scargoship build", &["cargo"])]
50+
#[case("node index.js", &["gradle", "java", "maven", "mvn"])]
51+
fn does_not_match(#[case] command: &str, #[case] names: &[&str]) {
52+
assert!(!command_has_executable(command, names));
53+
}
54+
}

src/executor/helpers/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
pub mod apt;
22
pub mod command;
3+
pub mod detect_executable;
34
pub mod env;
45
pub mod get_bench_command;
56
pub mod harvest_perf_maps_for_pids;

src/executor/wall_time/perf/mod.rs

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
use crate::cli::UnwindingMode;
44
use crate::executor::ExecutorConfig;
55
use crate::executor::helpers::command::CommandBuilder;
6+
use crate::executor::helpers::detect_executable::command_has_executable;
67
use crate::executor::helpers::env::is_codspeed_debug_enabled;
78
use crate::executor::helpers::harvest_perf_maps_for_pids::harvest_perf_maps_for_pids;
89
use crate::executor::helpers::run_command_with_log_pipe::run_command_with_log_pipe_and_callback;
@@ -96,18 +97,15 @@ impl PerfRunner {
9697
// Infer the unwinding mode from the benchmark cmd
9798
let (cg_mode, stack_size) = if let Some(mode) = config.perf_unwinding_mode {
9899
(mode, None)
99-
} else if config.command.contains("gradle")
100-
|| config.command.contains("java")
101-
|| config.command.contains("maven")
102-
{
100+
} else if command_has_executable(
101+
&config.command,
102+
&["gradle", "gradlew", "java", "maven", "mvn", "mvnw"],
103+
) {
103104
// In Java, we must use FP unwinding otherwise we'll have broken call stacks.
104105
(UnwindingMode::FramePointer, None)
105-
} else if config.command.contains("cargo") {
106+
} else if command_has_executable(&config.command, &["cargo"]) {
106107
(UnwindingMode::Dwarf, None)
107-
} else if config.command.contains("pytest")
108-
|| config.command.contains("uv")
109-
|| config.command.contains("python")
110-
{
108+
} else if command_has_executable(&config.command, &["pytest", "uv", "python", "python3"]) {
111109
// Note that the higher the stack size, the larger the file, although it is mitigated
112110
// by zstd compression
113111
(UnwindingMode::Dwarf, Some(16 * 1024))

0 commit comments

Comments
 (0)