Skip to content

Commit 1ca36aa

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 29db959 commit 1ca36aa

File tree

3 files changed

+63
-10
lines changed

3 files changed

+63
-10
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: 8 additions & 10 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::env::suppress_go_perf_unwinding_warning;
89
use crate::executor::helpers::harvest_perf_maps_for_pids::harvest_perf_maps_for_pids;
@@ -97,20 +98,17 @@ impl PerfRunner {
9798
// Infer the unwinding mode from the benchmark cmd
9899
let (cg_mode, stack_size) = if let Some(mode) = config.perf_unwinding_mode {
99100
(mode, None)
100-
} else if config.command.contains("gradle")
101-
|| config.command.contains("java")
102-
|| config.command.contains("maven")
103-
{
101+
} else if command_has_executable(
102+
&config.command,
103+
&["gradle", "gradlew", "java", "maven", "mvn", "mvnw"],
104+
) {
104105
// In Java, we must use FP unwinding otherwise we'll have broken call stacks.
105106
(UnwindingMode::FramePointer, None)
106-
} else if config.command.contains("cargo") {
107+
} else if command_has_executable(&config.command, &["cargo"]) {
107108
(UnwindingMode::Dwarf, None)
108-
} else if config.command.contains("go test") {
109+
} else if command_has_executable(&config.command, &["go"]) {
109110
(UnwindingMode::FramePointer, None)
110-
} else if config.command.contains("pytest")
111-
|| config.command.contains("uv")
112-
|| config.command.contains("python")
113-
{
111+
} else if command_has_executable(&config.command, &["pytest", "uv", "python", "python3"]) {
114112
// Note that the higher the stack size, the larger the file, although it is mitigated
115113
// by zstd compression
116114
(UnwindingMode::Dwarf, Some(16 * 1024))

0 commit comments

Comments
 (0)