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

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 3 inline comments as done.
mboehme 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);
----------------
gribozavr2 wrote:
> 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.
> 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?)

Good point -- removed.

> 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.

No longer relevant, but just to complete the discussion: I don't believe this is true for `StructValue`s because of the way that prvalues of class type get materialized into result objects. I believe this causes every `StructValue` to be materialized into exactly one `AggregateStorageLocation`.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:485
+
+    Env.unsetValue(PointerVal->getPointeeLoc());
+
----------------
gribozavr2 wrote:
> 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++.
> 
> 
Good point. This makes everything much simpler. Done.


================
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.
+  }
----------------
gribozavr2 wrote:
> 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).
> 
> 
> (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.

That's what I was trying to say with my comment here -- but this is now moot anyway, as we no longer do anything for deletes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147698/new/

https://reviews.llvm.org/D147698