steakhal added a comment. In D97183#2640559 <https://reviews.llvm.org/D97183#2640559>, @RedDocMD wrote:
> @NoQ, why does the following trigger a null-dereference warning? > (https://godbolt.org/z/Kxox8qd16) > > void g(std::unique_ptr<A> a) { > A *aptr = a.get(); > if (!aptr) {} > a->foo(); > } > > When `a->foo()` is called, the constraint `!aptr` is no longer valid and so > `InnerPointerVal` corresponding to `a` is no longer constrained to be null. > Am I missing something? When the if's condition is evaluated, it probably triggered a state split. On one path the `aptr` (aka. the inner pointer) will be constrained to `null`. The only way to be sure is by checking the exploded graph and see where it goes. --- In D97183#2643671 <https://reviews.llvm.org/D97183#2643671>, @RedDocMD wrote: > @NoQ, I have taken a different approach this time. I have used a visitor and > am storing some more data in the GDM. Together, they distinguish between the > following three cases: > > 1. If the raw pointer obtained from get() is constrained to null in a path > which leads to a Node (and thus State) where a smart-pointer-null-deref bug > occurs. > 2. If the raw pointer was null to begin with (because the smart-pointer was > null) > 3. If the raw pointer was not null to begin with but the smart-ptr became > null after that. > > Only in the first case should the note be emitted. I have added some more > tests to that effect. All in all, I see where it's going. I don't know, it might be the right thing to do. I haven't spent much time on this topic though. See my inline comments. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:87 + const ProgramStateRef BugState; + llvm::SmallPtrSet<const Expr *, 16> DoneExprs; + ---------------- I'm not sure if we should expect 16 unique places where `uptr::get()` called on a path. I would guess 4 or 2 is more than enough. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:106-109 + if (const auto *DS = llvm::dyn_cast<DeclStmt>(S)) { + for (const Decl *D : DS->getDeclGroup()) { + if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(D)) { + const Expr *Init = VD->getInit(); ---------------- So you are trying to find the assignment, where the inner pointer is assigned to a variable. This visitor logic seems to be somewhat convoluted. What you want to achieve is slightly similar to `FindLastStoreBRVisitor`. You should have a look at that. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:460 SVal InnerPointerVal; + const auto *CallExpr = Call.getOriginExpr(); if (const auto *InnerValPtr = State->get<TrackedRegionMap>(ThisRegion)) { ---------------- Nit: Declare the variable as close to the usage as you can. In the narrowest scope as well. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:475-476 + if (const auto *StmtSet = State->get<ExprsFromGet>(ThisRegion)) { + auto NewStmtSet = StmtSetFactory.add(*StmtSet, CallExpr); + State = State->set<ExprsFromGet>(ThisRegion, NewStmtSet); + } else { ---------------- Why don't you 'save' the MemRegion only if the inner pointer is not proven to be null. This would relieve you from checking it later. Nit: I don't like such if branches. The last statement is identical, which is a code smell. It's better to think of this as a function taking stuff and producing a State. An immediately called lambda expression would enclose any local variables and it would suggest that the algorithm it implements is self-contained. I know that I'm the only immediately called lambda expression fan though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97183/new/ https://reviews.llvm.org/D97183 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
