[PATCH] D147698: [clang][dataflow] Add support for new and delete expressions.

[Dmitri Gribenko via Phabricator via cfe-commits]

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