[CIR][LifetimeCheck] Add use-after-move detection #2077
Conversation
…eters Extends the CIR LifetimeCheck pass to detect use-after-move for all types including primitives (int, double, etc.) when passed to functions via rvalue reference parameters. Example now detected: void consume(int&& a); int x = 10; consume(std::move(x)); int y = x; // Warning: use of moved-from value 'x' Key changes: - Add checkMoveViaCall() to detect rvalue reference parameters - Track moved-from state for Value types in existing pmap/pset - Handle reinitialization via assignment clearing moved-from state - Extend diagnostics to distinguish moved-from values from pointers The implementation leverages existing InvalidStyle::MovedFrom and pmap/pset infrastructure, requiring only ~120 new lines of code. Test coverage includes primitives, reinitialization, and negative cases (lvalue references, by-value passing). Note: Lambda capture support will be added in a follow-up commit.
Extends use-after-move detection to lambda init-captures initialized
with std::move. Since move captures and value captures generate
identical CIR (both are load+store), this implementation accesses the
Clang AST to determine if the capture initializer uses std::move.
Example now detected:
int x = 10;
auto lambda = [y = std::move(x)]() { };
int z = x; // Warning: use of moved-from value 'x'
Key changes:
- Extend checkLambdaCaptureStore() to access lambda AST
- Add checkLambdaInitCaptureMove() helper to detect std::move
- Detect init-captures via VarDecl::isInitCapture()
- Identify std::move in initializer via CallExpr analysis
- Mark source variable as moved-from when capture uses std::move
- Match captures by field name to avoid false positives
The implementation uses AST inspection because CIR lowering does not
preserve the distinction between move and copy captures.
Test coverage includes init-captures with/without move, reference
captures, value captures, and multiple capture scenarios.
Add 8 new test cases to provide parity with clang-tidy's bugprone-use-after-move checker tests: - Move-after-move detection - Function parameter moves - Loops with conditional moves - Lambda captures after move (explicit and implicit) - Switch statement fallthrough - Move in declaration - Multiple uses after move (only first warned) No implementation changes needed - existing code already handles these scenarios correctly.
Fix three issues discovered by expanded test coverage: 1. Only warn once per moved-from variable (not once per use location) 2. Detect move-after-move (using an already-moved value) 3. Detect moves in variable declarations: int b(std::move(a)) These fixes ensure use-after-move detection works correctly for all primitive types across different usage patterns.
…ection Smart pointers (std::unique_ptr, std::shared_ptr) have well-defined null state after move, unlike other owner types which become invalid. This patch: - Adds isSmartPointerType() to detect std::unique_ptr/std::shared_ptr - Marks smart pointers as null (not invalid) after move operations - Distinguishes safe operations (get, reset, bool) from unsafe (*, ->) - Adds checkMoveCtor() to handle move constructor semantics - Adds markOwnerAsMovedFrom() helper to unify move handling New test file with 13 tests covering move constructor, move assignment, safe/unsafe operations, and function parameter moves.
ivanmurashko
requested review from
andykaylor,
bcardosolopes,
lanza and
xlauko
as code owners
|
@ivanmurashko this is a great addition of functionality - I'll go for a deeper review soon. In the meantime, seems like a few tests are failing! |
Cool, I will look at the failed tests |
Temporaries (e.g., ref.tmp0) being moved into variables via move constructors or rvalue references don't need to be tracked as moved-from since they are about to be destroyed anyway. Tracking them leads to false positives where the moved-from state pollutes the target variable's pset. This fix adds an isTemporary() check that skips marking temporaries as moved-from in: - Move constructors (checkMoveCtor) - Rvalue reference parameters (checkMoveViaCall) Coroutine tasks are explicitly excluded from this optimization via isTaskType() since they need lifetime tracking even as temporaries. Additionally, fixes a critical issue where coroutine task tracking was skipped due to early return in checkStore(). Moved task checking before the Value type reinitialization check to ensure tasks are properly tracked. Fixes 2 test regressions: - lifetime-this.cpp (smart pointer false positive) - lifetime-check-coro-task.cpp (coroutine task tracking)
@bcardosolopes I fixed the failed tests at 9cc5da2 |
|
|
||
| // Track which values have already been warned about for moved-from state | ||
| // to avoid multiple warnings for the same variable. | ||
| llvm::DenseSet<mlir::Value> warnedMovedFromValues; |
There was a problem hiding this comment.
Can this be tracked by a location in emittedDiagnostics instead?
| if (!recordDecl) | ||
| return false; | ||
|
|
||
| std::string qualifiedName = recordDecl->getQualifiedNameAsString(); |
There was a problem hiding this comment.
This should use the AST interface too, astAttr.isInStdNamespace(..) (we have similar hasPromiseType or isLambda), and you can then just check for the unqualified name.
|
|
||
| std::string qualifiedName = recordDecl->getQualifiedNameAsString(); | ||
| return qualifiedName == "std::unique_ptr" || | ||
| qualifiedName == "std::shared_ptr"; |
There was a problem hiding this comment.
Is it worth adding a cache like done for lambda and others?
|
|
||
| auto name = allocaOp.getName(); | ||
| // Temporaries have names starting with "ref.tmp" | ||
| if (!name.starts_with("ref.tmp")) |
There was a problem hiding this comment.
I'm afraid "ref.tmp" is not a reliable name. Perhaps the alloca operation could have a is_temporary unit attr that is set by CIRGen instead? If you can fix that in a follow up PR that would be great. For now you should add a TODO comment here.
| qualifiedName == "std::shared_ptr"; | ||
| } | ||
|
|
||
| bool LifetimeCheckPass::isTemporary(mlir::Value v) { |
There was a problem hiding this comment.
Because it has more constraints than being just about temporaries, perhaps should be called something like movesFromTemporary?
| // Check each argument for rvalue reference parameters | ||
| unsigned numArgs = callOp.getNumArgOperands(); | ||
| for (unsigned i = 0; i < numArgs; ++i) { | ||
| if (i >= clangFuncDecl->getNumParams()) |
There was a problem hiding this comment.
Add another helper function that check each param, something like checkArgForRValueRef
| std::string methodName = m.getDeclName().getAsString(); | ||
|
|
||
| // Safe methods: get, release, reset, operator bool | ||
| if (methodName == "get" || methodName == "release" || |
There was a problem hiding this comment.
This check should be in its own helper function, isSmartPointerSafeMethod or something akin
| mlir::Location loc) { | ||
| // Smart pointers have well-defined null state after move, while other | ||
| // owners are marked invalid. | ||
| if (isSmartPointerType(addr.getType())) |
There was a problem hiding this comment.
Do we really need to conditionalize this much on "smart pointer" abstraction? Can't this be generalized based on being an OwnerType? This would probably integrate better with the rest of the pass.
| } | ||
|
|
||
| void LifetimeCheckPass::checkMoveCtor(CallOp callOp, cir::CXXCtorAttr ctor) { | ||
| if (ctor.getCtorKind() != cir::CtorKind::Move) |
There was a problem hiding this comment.
Please add a new entry to CIR_CXXSpecialMemberAttr and use the info from FuncOp directly
| trackCallToCoroutine(callOp); | ||
|
|
||
| // Track moves via rvalue reference parameters for Value types | ||
| checkMoveViaCall(callOp); |
There was a problem hiding this comment.
checkMoveViaCall -> checkMoveInCallArgs?
bcardosolopes
left a comment
bcardosolopes
left a comment
There was a problem hiding this comment.
Overall this is great, thank you!
Added comments, questions and some requests!
2 participants
Extends LifetimeCheck to detect use-after-move using CIR's pmap/pset infrastructure. Handles primitive types (int, float, etc.) moved via rvalue reference parameters and lambda captures, plus smart pointers (std::unique_ptr, std::shared_ptr) with their well-defined null state semantics.
Commits
1. Basic detection via rvalue reference parameters (1b6691172cdf)
T&¶metersconsume(std::move(x)); int y = x; // warns2. Lambda init-capture support (3769a2a37cfe)
[b = std::move(a)]by accessing Clang AST from CIR3. Test coverage (8 new tests) (94870c91a068)
4. Edge case fixes (b69b7e7f29f4)
int b(std::move(a))via rvalue initialization5. Smart pointer support (eca591755916)
std::unique_ptrandstd::shared_ptrby qualified nameget(),reset(),bool) from unsafe (*,->)checkMoveCtor()to handle move constructor semanticsmarkOwnerAsMovedFrom()helper to unify move handling logiclifetime-check-smart-pointer-after-move.cppwith 13 testsImplementation
Uses
InvalidStyle::MovedFromin pmap to track state. AST access via CIR's AstAttr for lambda analysis and smart pointer type detection.Smart pointers use
State::getNullPtr()instead ofState::getInvalid()to reflect their well-defined null state after move.Limitations
Smart pointer reinitialization via
reset(new T)is not yet tracked as a state change (future work).Testing
clang/test/CIR/Transforms/lifetime-check-use-after-move.cppclang/test/CIR/Transforms/lifetime-check-smart-pointer-after-move.cpp