Fix unnecessary irept copies found by check-irep-moves#8978
Open
tautschnig wants to merge 8 commits into
Open
Conversation
tautschnig
requested review from
a team,
kroening,
martin-cs,
peterschrammel and
remi-delmas-3000
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8978 +/- ##
===========================================
- Coverage 80.59% 80.59% -0.01%
===========================================
Files 1713 1713
Lines 189403 189403
Branches 73 73
===========================================
- Hits 152647 152644 -3
- Misses 36756 36759 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kroening
reviewed
May 26, 2026
| std::size_t index_width = | ||
| std::max(address_bits(src_bv_width), index_bv_width); | ||
| unsignedbv_typet index_type(index_width); | ||
| unsignedbv_typet index_type(std::move(index_width)); |
Collaborator
There was a problem hiding this comment.
There is no need to std::move an integer.
kroening
reviewed
May 26, 2026
| ns); | ||
|
|
||
| byte_extract_exprt be = byte_extract_expr; | ||
| byte_extract_exprt be = std::move(byte_extract_expr); |
Collaborator
There was a problem hiding this comment.
byte_extract_expr is const
kroening
reviewed
May 26, 2026
| if(!new_comp.empty()) | ||
| { | ||
| struct_union_typet abstracted_type = struct_union_type; | ||
| struct_union_typet abstracted_type = std::move(struct_union_type); |
Collaborator
There was a problem hiding this comment.
struct_union_type is const
kroening
reviewed
May 26, 2026
| if(result_is_used) | ||
| { | ||
| exprt tmp = op; | ||
| exprt tmp = std::move(op); |
Address 8 findings from the clang-based irept copy checker: - 1 const reference (shadow_memory_util.cpp: unmodified copy) - 7 std::move (string_abstraction, goto_convert_side_effect, string_instrumentation, value_set, boolbv_extractbit, boolbv_index: source not used after copy) Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
Heuristic grep-based checker that flags local variables initialized by copying an irept-derived type, where the variable is subsequently modified and the source appears unused after the copy. These are candidates for std::move to avoid unnecessary copy-on-write detach. Found 22 candidates in goto-symex, 50 across the hot paths. Each needs manual review — some are false positives (const ref sources, loop-reused sources). Usage: python3 scripts/check_irep_copies.py src/goto-symex/*.cpp Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
The check-irep-moves clang-tooling driver intentionally lives outside the CBMC module hierarchy and uses upstream clang/llvm conventions (using-directives, llvm/clang headers, classes that don't end in 't'). Mirror the existing miniz exclusion to keep the file out of the cpplint scope. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
The check-irep-copies job links the tool against -lclang-cpp, which on Ubuntu 24.04 is provided by libclang-cpp18-dev (not by libclang-18-dev, which only ships the C library libclang.so). Without this the link step fails with /usr/bin/ld: cannot find -lclang-cpp: No such file or directory Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
Running the checker on develop's hot-paths produced seven findings:
the four reviewer-flagged \`std::move\`-of-const-source false
positives, plus three more (one converting constructor, one MemberExpr-
sourced unmodified copy, one loop-reused source) that are similar in
character. All seven are spurious -- they would either silently
copy-construct anyway, or break correctness if a developer trusted
the suggestion.
Tighten the heuristic so the checker stops emitting them:
* In the unnecessary-copy path:
- require the chosen constructor to be a copy constructor
(rejects converting one-arg ctors like
\`unsignedbv_typet(std::size_t)\`);
- look through references when checking \`isConstQualified\` on
the source's type, so \`const auto &x = ...\` is recognised
as a const source;
- skip when the copy is inside a loop whose enclosing scope
does not contain the source's declaration -- the source
will be copied (and would be moved) once per iteration.
* In the unmodified-copy path:
- skip MemberExpr sources, because the existing
MutableUseChecker only tracks mutations of the destination
and so cannot detect a \`this->member = ...\` between the
copy and a later use of the destination;
- skip reference-typed sources for the same reason: a
\`T &alias\` is often introduced precisely to be reseated/
mutated, and \`const T &\` aliases would observe the
post-mutation value rather than the snapshot the caller
wants.
After these changes the checker reports zero findings on the hot
paths covered by run_irep_copy_check.sh, including the four sites
the previous commit had to revert from \`std::move\` back to plain
copies.
Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
The CI job was named check-irep-copies, but the tool it built was check-irep-moves (from scripts/check_irep_moves.cpp) and an orphaned check_irep_copies.py prototype was left alongside it. Standardise on the "irep-copies" term: rename the source to check_irep_copies.cpp, the binary to check-irep-copies and the runner to run_irep_copies_check.sh, update the workflow and the cpplint exclusion in run_diff.sh to match, and drop the superseded Python prototype. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
Replace the hand-picked hot-path list in run_irep_copies_check.sh with a scan of every translation unit in compile_commands.json, excluding only the unit/ and regression/ test trees (which deliberately copy ireps to exercise sharing semantics). This catches unnecessary copies anywhere in the code base, including jbmc. Findings that are genuine false positives of the "copy-initialized but never modified" heuristic -- where the copy is required for correctness and a const reference would change behaviour -- are grandfathered in a baseline file so the check fails only on new findings. Pin the on-demand build of the checker to clang/LLVM 18 to match the version installed by the workflow. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
Add regression/check-irep-copies/ with a self-contained fixture exercising both detectors: two positive cases (an unmodified copy that should be a const reference, and a last-use copy that should be a std::move) and negative cases for const, loop-reused, member-expression, converting-constructor and non-irept sources, plus a copy that is modified and whose source is reused. run_self_test.sh builds the checker on demand and diffs its findings against expected.txt, and the CI job runs it before scanning the tree so heuristic regressions are caught. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Address 8 findings from the clang-based irept copy checker: