run-clang-tidy-cached: surface fatal clang-tidy errors instead of caching empty output#10185
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a mechanism to detect systemic clang-tidy failures by checking for non-zero exit codes without findings, preventing the caching of empty results. The review feedback points out that the regex for identifying check findings needs to include digits to correctly match many standard clang-tidy checks. Additionally, the fatal_lock mutex is identified as redundant because the atomic compare_exchange_strong operation on fatal_stop already ensures thread-safe access to the error information.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
openroad-ci
force-pushed
the
catch-tidy-error
branch
from
9391dfb to
3d53da1
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
openroad-ci
force-pushed
the
catch-tidy-error
branch
from
3d53da1 to
cbbf29b
Compare
|
@codex review |
|
clang-tidy review says "All clean, LGTM! 👍" |
![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: cbbf29b3ca
ℹ️ 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".
…hing empty output Previously, when clang-tidy exited non-zero without producing any check findings (e.g. bad config, missing compile db, unknown check), stderr was discarded via `2>/dev/null` and an empty output file was cached as if the file had no findings. Subsequent runs would hit the cache and never notice. Redirect stderr into the output, detect non-zero exits with no findings as fatal, stop other worker threads via an atomic flag, and print the captured output before aborting the run. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
openroad-ci
force-pushed
the
catch-tidy-error
branch
from
cbbf29b to
8a2d808
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
maliberty
merged commit 4de4328
into
The-OpenROAD-Project:master
2 participants
Summary
Previously
run-clang-tidy-cached.ccredirected clang-tidy's stderr to/dev/nulland inferred findings solely from the captured stdout. When clang-tidy exited non-zero without emitting any check findings (bad config, missing compile DB, unknown check name, crash in a plugin), the runner cached an empty output file as if that translation unit was clean. Subsequent runs hit the cache and continued to show zero findings — effectively masking a broken configuration.This change:
2>&1) so error text is preserved.[check-name]-shaped finding, treat it as a fatal systemic failure.std::atomic<bool> fatal_stopto stop dequeuing new work.Type of Change
Impact
Verification
./etc/Build.sh).Related Issues
None.