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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
