feat: add support for analysis mode #15
Conversation
not-matthias
force-pushed
the
cod-1710-instrument-hooks-support-new-analysis-mode
branch
4 times, most recently
from
5737ac4 to
c4f95c0
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for a new "analysis" integration mode, allowing the instrumentation hooks to operate in three distinct modes: Valgrind, Perf, and Analysis. The implementation refactors existing code to use a common helper for FIFO-based instruments.
- Introduces the
IntegrationModeenum and related protocol commands for mode detection - Creates a generic
FifoInstrumenthelper that validates the integration mode during initialization - Refactors
PerfInstrumentto use the new helper and moves related code torunner_fifo.zig
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared.zig | Adds IntegrationMode enum and new Command variants for integration mode queries |
| src/runner_fifo.zig | New file containing refactored FIFO communication logic from perf.zig with mode detection |
| src/instruments/helper.zig | New generic FifoInstrument function that creates mode-validated instrument implementations |
| src/instruments/perf.zig | Refactored to use FifoInstrument helper, significantly reduced code |
| src/instruments/analysis.zig | New analysis instrument using the FifoInstrument helper |
| src/instruments/valgrind.zig | Updated init to return error when not instrumented for consistency |
| src/instruments/root.zig | Updated to support analysis mode with proper initialization order and delegation |
| src/c.zig | Removed protocol version validation logic (moved to runner_fifo.zig) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
not-matthias
force-pushed
the
cod-1710-instrument-hooks-support-new-analysis-mode
branch
from
4ecb2f7 to
d673228
Compare
not-matthias
force-pushed
the
cod-1710-instrument-hooks-support-new-analysis-mode
branch
from
d673228 to
a19e8c3
Compare
src/instruments/analysis.zig
Outdated
|
|
||
| const Self = @This(); | ||
|
|
||
| pub fn init(allocator: std.mem.Allocator) !Self { |
There was a problem hiding this comment.
Why don't we share most of this file's logic with the perf instrument ? Since the only non common part of this is the init where we ask the mode, it should be incorporated in the shared struct. We are basically definition something like a FifoInstrument that is defined by its interactions with the runner, in opposition to valgrind that interacts with the tool directly for legacy reasons.
Each instrument is then just a small wrapper around the init call with the proper argument.
Keep in mind I'm not familiar with zig tho
| } | ||
|
|
||
| pub fn get_integration_mode(self: *Self) !shared.IntegrationMode { | ||
| // NOTE: We have to send to the writer directly, and not wait for an ack. |
There was a problem hiding this comment.
explain why
My understanding is that this is just how we've defined the protocol
If that's so, is there a way we could encode in the type system that one command expects another as a response ? Whether it's an Ack, or something else ?
It would both make it clearer in the code and in the protocol message definition.
| const PerfInstrument = perf.PerfInstrument; | ||
| const AnalysisInstrument = analysis.AnalysisInstrument; | ||
|
|
||
| // TODO: Should we merge perf and analysis? -> Could just be a FifoInstrument |
There was a problem hiding this comment.
I think it's worth it to keep the separation, and have the FifoInstrument just be some sort of meta instrument
| const shared = @import("../shared.zig"); | ||
|
|
||
| /// Creates a complete FIFO-based instrument struct that validates a specific integration mode | ||
| pub fn FifoInstrument(comptime mode: shared.IntegrationMode, comptime error_type: anytype) type { |
There was a problem hiding this comment.
I see the future I guess, one could call me Nostradamus.
This kind of stuff is nice to incorporate directly in your commit when it's not too much works, it prevents useless comments
| @@ -0,0 +1,58 @@ | |||
| const std = @import("std"); | |||
There was a problem hiding this comment.
this file should not be helper.zig, but fifo_instrument.zig IMO.
If you wish, you could make a separation for abstract instruments vs concrete ones, but if we can we should avoid lazy names like utils or helpers when we can
No description provided.