gribozavr2 added inline comments.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:315
+ /// `D` must currently be associated with a value.
+ void unsetChild(const ValueDecl &D) {
+ auto It = Children.find(&D);
----------------
Modifying already-created values is wrong since values are supposed to be
immutable (I know we already do it in setChild above, but let's not add more?)
A value can be stored in multiple locations, and if one of them is deleted, the
other users of the same value shouldn't observe the change.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:485
+
+ Env.unsetValue(PointerVal->getPointeeLoc());
+
----------------
I'm not convinced that we need to break the loc/value association for deleted
heap allocations. After all, we don't do that for stack allocations that go out
of scope. So why bother for the heap? Especially since "delete" is very rare in
modern idiomatic C++.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:490
+ // For example, an analysis that diagnoses double deletes may want to
attach
+ // properties to the `PointerValue` and access those after the first
delete.
+ }
----------------
It is correct that we shouldn't deallocate PointerValue, but the reasons for
that are different:
(1) The code being analyzed might have multiple copies of the pointer, and it
might have a double-free or a use-after-free. The analyzer shouldn't execute UB
when the program being analyzed has UB.
(2) The PointerValue is needed to analyze other basic blocks.
(3) The downstream code that is going to inspect the environments at various
program points will need the PointerValues (like the test in this patch).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147698/new/
https://reviews.llvm.org/D147698
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
