Skip to content

feat(runner): distinguish instrumented command and tool error in codspeed #67

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

not-matthias
Copy link
Member

@not-matthias not-matthias commented Mar 6, 2025

Added a small wrapper script, which executes cargo codspeed run in a subprocess and then writes the status code to a file.

Logs when the benchmark failed (in this case with a panic):

[DEBUG::codspeed::run::runner::valgrind::measure] Valgrind exit code = Some(0), Program exit code = 1
Error: failed to execute the benchmark process

Logs when valgrind error is detected (only an example, this doesn't cause valgrind to crash)

[DEBUG::codspeed::run::runner::valgrind::measure] valgrind.log: ==486607== Using source line as position.
--486607-- warning: L3 cache found, using its data for the LL simulation.
--486607-- warning: specified LL cache: line_size 64  assoc 12  total_size 18,874,368
--486607-- warning: simulated LL cache: line_size 64  assoc 18  total_size 18,874,368
==486607== 
==486607== Process terminating with default action of signal 4 (SIGILL): dumping core
==486607==  Illegal opcode at address 0x1205CF
==486607==    at 0x1205CF: my_bench::fibonacci (in /app/rust-crate/my_bench)
==486607==    by 0x120E57: codspeed_divan_compat::compat_divan::compat::bench::args::bench (in /app/rust-crate/my_bench)
==486607==    by 0x12313B: codspeed_divan_compat::compat_divan::compat::main (in /app/rust-crate/my_bench)
==486607==    by 0x121362: std::sys::backtrace::__rust_begin_short_backtrace (in /app/rust-crate/my_bench)
==486607==    by 0x121358: std::rt::lang_start::{{closure}} (in /app/rust-crate/my_bench)
==486607==    by 0x140DDF: std::rt::lang_start_internal (in /app/rust-crate/my_bench)
==486607==    by 0x12067B: main (in /app/rust-crate/my_bench)

Error: failed to execute valgrind

@not-matthias not-matthias force-pushed the cod-615-distinguish-instrumented-command-and-tool-error-in-codspeed branch 2 times, most recently from 74043ce to f35cac9 Compare March 7, 2025 13:49
@not-matthias not-matthias requested review from art049 and GuillaumeLagrange and removed request for art049 March 7, 2025 13:51
@not-matthias not-matthias force-pushed the cod-615-distinguish-instrumented-command-and-tool-error-in-codspeed branch from f35cac9 to be2db7e Compare March 7, 2025 15:28
@not-matthias
Copy link
Member Author

Depends on CodSpeedHQ/codspeed-rust#86

The correct exit code is now displayed (illegal instruction = signal 4)

[DEBUG::codspeed::run::runner::valgrind::measure] Valgrind exit code = Some(0), Valgrind signal = None, Program exit code = 132
Error: failed to execute the benchmark process, exit code: 132

@art049
Copy link
Member

art049 commented Mar 10, 2025

Is there an easy way for us to write a test of this behavior? At least testing the script isolated from the rest

@not-matthias not-matthias force-pushed the cod-615-distinguish-instrumented-command-and-tool-error-in-codspeed branch 2 times, most recently from 479fc5c to 1d94040 Compare March 11, 2025 12:54
@art049 art049 self-requested a review March 12, 2025 09:15
Copy link
Member

@art049 art049 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super confident merging this right now, mostly because of the quoting and wrapping of parameters.

In

cmd.args(["sh", "-c", &run_cmd]);

I think we should replace sh -c with our wrapper script directly. I think it would avoid messing with the quotes and formatting command as strings.

@not-matthias not-matthias force-pushed the cod-615-distinguish-instrumented-command-and-tool-error-in-codspeed branch from 1d94040 to 5668cde Compare March 13, 2025 13:05
@not-matthias not-matthias requested a review from art049 March 13, 2025 13:06
Copy link
Member

@art049 art049 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one last thing to simplify the script and I think we're good to go

@not-matthias not-matthias force-pushed the cod-615-distinguish-instrumented-command-and-tool-error-in-codspeed branch from 5668cde to 1500c44 Compare March 13, 2025 16:05
@not-matthias not-matthias removed the request for review from GuillaumeLagrange March 14, 2025 08:11
@not-matthias not-matthias force-pushed the cod-615-distinguish-instrumented-command-and-tool-error-in-codspeed branch from 1500c44 to dda455a Compare March 14, 2025 09:11
@not-matthias not-matthias force-pushed the cod-615-distinguish-instrumented-command-and-tool-error-in-codspeed branch from dda455a to 998b2e3 Compare March 14, 2025 09:28
@not-matthias not-matthias merged commit 998b2e3 into main Mar 14, 2025
9 checks passed
@not-matthias not-matthias deleted the cod-615-distinguish-instrumented-command-and-tool-error-in-codspeed branch March 14, 2025 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants