Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c8b06a379
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ALL_ALGOS="2q 3LCache CAR GLCache RandomLRU arc arcv0 cacheus clock clock2qplus | ||
| clockpro fifo fifo-merge fifo-reinsertion fifomerge flashProb gdsf gl-cache | ||
| lecar lecarv0 lfu lfucpp lfuda lhd lirs lrb lru lru-k lru-prob nop | ||
| pluginCache qdlp random randomTwo s3-fifo s3-fifov0 s3fifo s3fifod s3fifov0 | ||
| sieve size slru slruv0 tinyLFU twoq wtinyLFU" |
There was a problem hiding this comment.
ALL_ALGOS omits hyperbolic, belady, and beladySize, although all three are registered in libCacheSim/cache/cacheAlgoRegistry.c and the selected oracle trace makes the Belady policies valid. Consequently, neither the -e print sweep nor the replay sweep exercises their cachesim initialization and teardown paths, so regressions in these built-in algorithms can pass this target despite the stated all-algorithm coverage; the later Belady MINISIM checks exercise a different executable path and do not close this gap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a new CLI-focused regression test to the test/ suite to exercise end-to-end argument parsing and “run to completion” behavior that isn’t covered by in-process C unit tests. This fits into the codebase’s CTest-driven testing by introducing a testCLI target that runs a shell script from the build directory.
Changes:
- Add
test/test_cli.sh, a bash-based regression suite that drivescachesim,traceAnalyzer, andmrcProfileracross many option/parameter combinations. - Wire the script into CTest as a new
testCLItest viatest/CMakeLists.txt.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/test_cli.sh | New bash regression suite that runs CLI binaries and asserts clean failures vs crashes across many parameter/option cases. |
| test/CMakeLists.txt | Registers the new testCLI CTest target to run the shell regression suite from the build directory. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ALL_ALGOS="2q 3LCache CAR GLCache RandomLRU arc arcv0 cacheus clock clock2qplus | ||
| clockpro fifo fifo-merge fifo-reinsertion fifomerge flashProb gdsf gl-cache | ||
| lecar lecarv0 lfu lfucpp lfuda lhd lirs lrb lru lru-k lru-prob nop | ||
| pluginCache qdlp random randomTwo s3-fifo s3-fifov0 s3fifo s3fifod s3fifov0 | ||
| sieve size slru slruv0 tinyLFU twoq wtinyLFU" |
| add_test(NAME testCLI | ||
| COMMAND ${CMAKE_COMMAND} -E env bash ${CMAKE_CURRENT_SOURCE_DIR}/test_cli.sh | ||
| WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) |
| TRACE_CSV="${DATA_DIR}/cloudPhysicsIO.csv" | ||
| TRACE_TXT="${DATA_DIR}/cloudPhysicsIO.txt" | ||
|
|
||
| WORK_DIR=$(mktemp -d) |
720f1d5 to
aa99429
Compare
3c8b06a to
803fa29
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 803fa298de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif ! grep -qi "error" <<<"${out}"; then | ||
| _report 1 "${desc} failed without an error message (exit ${rc})" | ||
| else | ||
| _report 0 "" |
There was a problem hiding this comment.
When this suite is run against an AddressSanitizer build, a segfault is typically converted into exit code 1 with output containing ERROR: AddressSanitizer; this branch therefore counts the crash as a clean rejection solely because the output contains “error.” That allows the invalid-input crash regressions this helper is intended to detect to pass under a common diagnostic configuration, so sanitizer error signatures should be checked before the generic message check.
Useful? React with 👍 / 👎.
| # The script prints SKIP: and exits 0 when the binaries or sample traces are | ||
| # absent. Without this, ctest reports that as a pass, so a run that tested | ||
| # nothing looks the same as a run that tested everything. | ||
| set_tests_properties(testCLI PROPERTIES SKIP_REGULAR_EXPRESSION "^SKIP:") |
There was a problem hiding this comment.
On CMake 3.12–3.15, which the root cmake_minimum_required(VERSION 3.12) explicitly supports, SKIP_REGULAR_EXPRESSION is unavailable because CMake introduced this test property in 3.16. Consequently, a missing-binary or missing-data run exits zero and is reported as passed rather than skipped on those supported versions; either raise the minimum to 3.16 or use a skip mechanism available in 3.12.
Useful? React with 👍 / 👎.
aa99429 to
cddf41e
Compare
803fa29 to
dbe53fc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbe53fcc0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| n_skipped=0 | ||
| for algo in ${ALL_ALGOS}; do | ||
| out=$("${BIN_DIR}/cachesim" "${TRACE_ORACLE}" oracleGeneral "${algo}" 1gb -e print 2>&1) |
There was a problem hiding this comment.
-e print
When built with ENABLE_LRB or ENABLE_3L_CACHE, this sweep includes the now-registered lrb and 3LCache entries, but their parsers reject bare print before reaching their print branches because value is NULL (LRB_Interface.cpp:333-338 and ThreeLCache_Interface.cpp:343-348). These configurations therefore record a failure here on every ctest run; either fix those parsers as part of this change or exclude/handle algorithms that cannot accept -e print.
Useful? React with 👍 / 👎.
| "${BIN_DIR}/traceAnalyzer" --num-req=10000 -o out-nreq "${TRACE}" vscsi | ||
| fi | ||
|
|
||
| if [[ -x "${BIN_DIR}/mrcProfiler" ]]; then |
There was a problem hiding this comment.
If mrcProfiler is absent or moved from bin/—for example after building only the cachesim target—this condition silently skips all SHARDS and MINISIM checks, yet the script can still exit successfully and CTest reports testCLI as passed. mrcProfiler and traceAnalyzer are unconditional project executables rather than optional features, so the preflight should require them (or skip/fail the whole test) instead of treating their missing coverage as success.
Useful? React with 👍 / 👎.
cddf41e to
74d0d5e
Compare
dbe53fc to
f3a18a6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3a18a6d68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| local out | ||
| out=$("$@" 2>&1) | ||
| local rc=$? | ||
| if [[ ${rc} -eq 0 ]]; then |
There was a problem hiding this comment.
When this target runs against a recovering sanitizer such as UBSan, a command can print runtime error: and still exit 0; this branch therefore records the invocation as passing. The same exit-code-only behavior appears in expect_output and the manual algorithm sweeps, so undefined behavior in most valid-input paths can evade the regression target even though expect_clean_error already recognizes these diagnostics. Check sanitizer signatures before accepting successful commands as well.
Useful? React with 👍 / 👎.
74d0d5e to
7393ce8
Compare
f3a18a6 to
70efb28
Compare
|
Consolidated response to the review comments on this PR. Sanitizer diagnostics counted as clean rejections — fixed, and it was the best catch here. The follow-up — that a successful command can also print
Missing algorithms — fixed.
Not addressed, both real: Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70efb28ccb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [[ ${rc} -ne 0 ]] && grep -qi "do not support algorithm" <<<"${out}"; then | ||
| n_skipped=$((n_skipped + 1)) | ||
| continue |
There was a problem hiding this comment.
If an always-built entry such as lru is accidentally removed from g_cache_algos, create_cache_by_name() returns NULL and cache_init.h emits this exact “do not support algorithm” message, so this branch skips the failure; the replay sweep repeats the same logic. Only 3LCache, GLCache/gl-cache, and lrb are feature-gated, so restrict skipping to those optional names or pass the enabled feature set into the test, otherwise the target can pass after a mandatory CLI algorithm disappears.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and fixed — this one undermined the sweep's whole purpose, so it was worth taking even though I had otherwise stopped adding to this PR.
The skip keyed off the do not support algorithm message alone, but cache_init.h emits that whenever a name is missing from the registry for any reason, not just an uncompiled feature. So a mandatory algorithm dropping out of g_cache_algos — precisely the regression the registry refactor could introduce — would have been skipped rather than reported.
Your list is exactly right: 3LCache, GLCache, gl-cache and lrb are the only feature-gated entries. The skip is restricted to those now.
Verified by deleting an entry and rebuilding, using gdsf rather than lru — lru is asserted directly elsewhere in the suite, so its removal would have been caught regardless, whereas gdsf is covered by the sweeps and nothing else, which is the case that was genuinely silent:
FAIL: gdsf -e print (exit 134) do not support algorithm gdsf
FAIL: gdsf replay (exit 134) do not support algorithm gdsf
(4 algorithms not compiled in, skipped)
The four genuinely optional ones still skip, and the suite is 180/180 with the registry restored.
Generated by Claude Code
70efb28 to
482d28d
Compare
7393ce8 to
7d8f16e
Compare
482d28d to
8c87ad4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c87ad4142
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --size=100MB,500MB,3 2>/dev/null | grep '^104857600B' | awk '{printf "%.4f", $2}') | ||
| _cachesim_exact=$("${BIN_DIR}/cachesim" "${TRACE_ORACLE}" oracleGeneral lru 100mb \ | ||
| 2>/dev/null | tail -1 | grep -oE 'miss ratio [0-9.]+' | head -1 | awk '{printf "%.4f", $3}') | ||
| if [[ "${_minisim_unsampled}" == "${_cachesim_exact}" ]]; then |
There was a problem hiding this comment.
If both extraction pipelines fail to find their expected rows—for example after both tools' output formats change—each command substitution produces an empty string, and this equality check records the regression assertion as passed because "" == ""; the pipeline statuses are discarded because the script does not use set -e. Require both extracted values to be nonempty before comparing them so this test cannot succeed without actually observing either miss ratio.
Useful? React with 👍 / 👎.
7d8f16e to
52d2e27
Compare
8c87ad4 to
73893f3
Compare
52d2e27 to
9e35ceb
Compare
73893f3 to
94bd4b3
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
9e35ceb to
6066e22
Compare
538e6e1 to
7bf76b9
Compare
14d01d7 to
c12617b
Compare
7bf76b9 to
83ff21e
Compare
c12617b to
ea0e6cc
Compare
83ff21e to
75bc50c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75bc50cd42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [[ ${rc} -eq 0 ]]; then | ||
| _report 0 "" |
There was a problem hiding this comment.
When an algorithm silently ignores cache_specific_params, this condition counts the invocation as passing after a normal trace replay rather than verifying the -e print path; FIFO_init is a current example because it never parses the supplied print value. Consequently, this newly added sweep is already green for algorithms that produce no parameter report, and regressions that turn printing into a no-op can escape the target. Require an expected reporting marker, or explicitly separate algorithms that do not support printable parameters.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and worse than the example suggests — fixed in a420ff36.
I measured every name in ALL_ALGOS rather than assuming. Eighteen of the fifty ignore -e print and replay the trace, so the sweep was passing for all of them without the print path ever running: arc arcv0 belady cacheus fifo gdsf lecarv0 lfu lfucpp lfuda lhd lirs lru nop random randomTwo sieve size. fifo was a good pick — it's one of them.
The sweep now requires a parameter report, with the non-reporting algorithms listed as the exception. I made the list the exception rather than the rule deliberately: a new algorithm added to ALL_ALGOS lands in the strict branch and fails loudly, instead of quietly joining the lenient bucket and recreating this hole.
One trap worth recording, because I fell into it first. My initial classification said all fifty print. The reason is that cachesim's INFO banner echoes back eviction-params: print, which matches any loose search for a parameter report — so the check passed for every run, including the ones that never print. Exactly the failure mode you're describing, reproduced in the fix for it. The assertion now strips the colour codes and the logger's lines before anchoring on the report itself; the tree spells it four ways (parameters:, current parameters:, <name> parameters:, default params:).
Verified load-bearing two ways, since an assertion of this shape is precisely the kind that passes for the wrong reason:
- moving
lruinto the strict branch →FAIL: lru -e print exited 0 without reporting parameters(184 passed, 1 failed) - deleting the
printffromS3FIFO_parse_paramsand rebuilding → fails for boths3fifoand itss3-fifoalias (183 passed, 2 failed)
185 checks pass with the tree as it stands, up from 184.
One thing I found while classifying and did not fix. arc and arcv0 don't belong in either category cleanly: ARC.c and ARCv0.c both define a *_parse_params with a working print branch that ARC_init and ARCv0_init never call, so the flag is dropped on the floor. Wiring it up is one line each — but their *_current_params helpers return an empty string, so arc -e print would report parameters: and nothing else. That reads as a feature left unfinished rather than one that broke, and finishing it is the maintainers' call, not mine. They're listed with that reasoning recorded next to them in the source, so moving them out is a one-line change whenever those inits start parsing their params.
Generated by Claude Code
ea0e6cc to
bcbe9e9
Compare
a420ff3 to
933550e
Compare
Before the fix the run aborted inside the thread pool without printing an error message, so expect_clean_error fails on it. 185 checks pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YUq1vM4g82TkaX2jvLmuQc
The sweep only asserted exit 0, so any algorithm that ignores cache_specific_params passed after an ordinary trace replay without the print path having run at all. Eighteen of the fifty names in ALL_ALGOS behave that way, so a regression that turned printing into a no-op would have gone straight through. Require the report, and list the algorithms that genuinely have nothing to report. The list is the exception rather than the rule so that an algorithm newly added to ALL_ALGOS lands in the strict branch and fails loudly instead of passing silently. Match carefully: cachesim's INFO banner echoes back "eviction-params: print", so a loose search for a parameter report matches every run, including the runs that never print. Strip the colour codes and the logger's lines first, then anchor on the report itself -- the four spellings in the tree are "parameters:", "current parameters:", "<name> parameters:" and "default params:". Verified load-bearing two ways, since an assertion of this shape is exactly the kind that passes for the wrong reason. Moving lru into the strict branch fails it (184/1); deleting the printf from S3FIFO_parse_params fails it for both s3fifo and its s3-fifo alias (183/2). 185 checks pass with the tree as it stands, up from 184. While classifying, ARC and ARCv0 turned out not to belong in either category cleanly: both define a *_parse_params with a working `print` branch that ARC_init and ARCv0_init never call, so the flag is dropped. Their *_current_params helpers return an empty string, so this reads as an unfinished feature rather than a broken one, and finishing it is a maintainer's call. They are listed with that reasoning recorded next to them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YUq1vM4g82TkaX2jvLmuQc
Both now parse their eviction parameters, so `-e print` reports and exits instead of replaying the trace. Move them out of the no-report list into the strict branch, where a regression in either parser is caught. 185 checks pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YUq1vM4g82TkaX2jvLmuQc
933550e to
7ae9b2a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ae9b2a048
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # report. The INFO banner echoes back "eviction-params: print", so a looser | ||
| # search matches every run -- including the ones that never print -- and the | ||
| # assertion passes without the print path having run at all. | ||
| _printed=$(sed 's/\x1b\[[0-9;]*m//g' <<<"${out}" | grep -v '^\[') |
There was a problem hiding this comment.
In the macos / clang job in .github/workflows/build.yml:13-28, this runs with BSD sed, which does not support GNU sed's \xHH regular-expression escape. Since the logger emits an ANSI reset without a following newline, the buffered parameters: output can retain that escape prefix; this substitution then fails to remove it (or reports a regex error), so the anchored check on the next line records every strict -e print case as failed. Use a literal escape supplied through Bash ANSI-C quoting, or another portable ANSI filter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The portability point is right and it's fixed in 34452199 — but the predicted failure isn't happening, and I want to be precise about which part I'm agreeing with.
Right: \xHH is a GNU extension. BSD sed reads \x as a plain x, matches nothing, and strips no colour, so on the macOS job the guard is absent — on exactly the platform where a failure would be hardest to read. Now a literal escape through Bash ANSI-C quoting, which both seds handle.
Not right: that this "records every strict -e print case as failed" today. Two checks say otherwise.
testCLI runs in the macOS job and passes, in 38 seconds, on the head that carries the GNU-only form:
Start 10: testCLI
9/10 Test #10: testCLI .......................... Passed 38.18 sec
100% tests passed out of 10
And the premise doesn't hold: the algorithms that honour -e print exit inside create_cache(), which runs before print_parsed_args() logs anything, so the logger never gets to emit. I checked all thirty-one strict-branch algorithms — every one produces zero bytes on stderr for -e print, and the whole combined output is the 86-byte plain parameters: line. There is no colour to strip on any platform, which is why BSD sed doing nothing costs nothing.
So this was dormant-and-wrong rather than broken, and it's worth having correct rather than removing: fed a synthetic line with a real escape prefix, the new expression strips it and the anchored check matches; left unstripped — what BSD sed produces — the same check does not match. It will do its job the day an algorithm warns while initialising.
185 checks pass, suite 10/10.
Generated by Claude Code
The sweep used sed 's/\x1b\[[0-9;]*m//g'. \xHH is a GNU extension: BSD sed, which is what the macos / clang job runs, reads \x as a plain x, matches nothing and strips no colour at all, so the guard is absent on exactly the platform where a failure would be hardest to read. Use a literal escape through Bash ANSI-C quoting, which both seds handle. This is not a live failure. Nothing writes to stderr on this path: the algorithms that honour `-e print` exit inside create_cache(), before print_parsed_args() logs anything, so there is no colour to strip on any platform. Checked all thirty-one strict-branch algorithms -- every one produces zero bytes on stderr -- and testCLI runs and passes in the macOS job today, in 38 seconds, on the head that carries the GNU-only form. The guard still earns its place, so it should be correct rather than dormant and wrong. Fed a synthetic line carrying a real escape prefix, the new expression strips it and the anchored check matches; left unstripped, which is what BSD sed produces, the same check does not match. It will do its job the day an algorithm warns while initialising. 185 checks pass, suite 10/10. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YUq1vM4g82TkaX2jvLmuQc
|
Closing as redundant: #324 was merged into GitHub didn't detect it because #324 was squash-merged, so this head is not an ancestor of Generated by Claude Code |
Part 5 of 5, split out of #324. Stacked on #329 — the diff shown here is against that branch; merge #328 and #329 first.
Two files:
test/test_cli.shand itstest/CMakeLists.txtwiring, as a newtestCLIctest target. 174 checks over ground nothing covered before.Why a shell target rather than more C unit tests
The existing C tests build caches in-process. Every bug fixed in #328 and #329 lives in the path between the command line and that point — argp callbacks, parameter parsing, the
-e printearly exit — or only becomes visible when a binary runs to completion. So this drives the actual binaries.Two design choices carried most of the value:
s3fifov0andflashProbcrashes were found; both were absent from every list I would have written by hand.-e printcallsexit(0)before teardown, and LeakSanitizer still counts that memory as reachable, so a parse-only test sees nothing.Invalid input is asserted to fail cleanly — non-zero with a message, but not SIGSEGV, SIGFPE or SIGBUS. The project's
ERROR()aborts, so a deliberate rejection has to stay distinguishable from a crash, and several of these bugs presented as exactly that difference.Coverage
-o,--verbose,-e printfor every algorithmSLRUn-segat 0/-1/4/16/20 andseg-sizeempty /0:0/ 24 entriesFIX_SIZE— rate 1 is the undefined-behavior case, whereUINT64_MAX * sample_raterounds to 2^64 and overflows the castcachesimon unsampled runs, not merely exiting 0, so the size-scaling bug cannot come back quietlybeladyandbeladySizeaccepted on oracle traces and rejected on others--hashpowerat 4, 5, 6 and 8 across the composite policieswtinyLFU --consider-obj-metadata=true, which used to segfaultobj_id = 0fails the test rather than returning a believable numberAre the tests load-bearing?
Green tests prove nothing on their own, so I checked the other direction: reverting the six originally-fixed sources to their previous state and rebuilding gives 16 failures, and 0 with them restored.
Testing
ctest --output-on-failure— 10/10, both plain Release and under-fsanitize=leakwith CI'sASAN_OPTIONS.Generated by Claude Code