[PATCH] D127746: [clang][dataflow] Convert `PointeeLoc` of `PointerValue` from reference to pointer.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.
Herald added a subscriber: martong.

Overall, this seems to be an important API change, and I'm having trouble seeing the big picture here.  I think it would be great if you could expand (in the CL description) on the motivation behind the change and the thinking behind the particular design choices you've made here.  For example, you mentioned that this change supports explicit modeling of `nullptr`, but what is gained by this explicit modeling? Also, regarding the design choice itself: is the intent that native `nullptr` now represents an interpreted `nullptr`? If so, have you considered instead introducing an explicit `NullptrLocation` or the like and if so, what were the tradeoffs in this decision?



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:332
 
-  StorageLocation &skip(StorageLocation &Loc, SkipPast SP) const;
-  const StorageLocation &skip(const StorageLocation &Loc, SkipPast SP) const;
+  StorageLocation *skip(StorageLocation &Loc, SkipPast SP) const;
+  const StorageLocation *skip(const StorageLocation &Loc, SkipPast SP) const;
----------------
Please document the conditions under which these functions return nullptr.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:348
 
 StorageLocation *Environment::getStorageLocation(const ValueDecl &D,
                                                  SkipPast SP) const {
----------------
If I understand correctly, the `nullptr` result from this function is now ambiguous: either there's no entry for that storagelocation, or there is and it is null. But, perhaps I'm misunderstanding -- is this not a concern?


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:266-267
 
-      auto &Loc = Env.createStorageLocation(*S);
-      Env.setStorageLocation(*S, Loc);
-      Env.setValue(Loc, Env.takeOwnership(std::make_unique<ReferenceValue>(
-                            SubExprVal->getPointeeLoc())));
+      // If PointeeLoc is null, then we are dereferencing a nullptr, skip
+      // creating a value for the dereference
+      if (auto *PointeeLoc = SubExprVal->getPointeeLoc()) {
----------------
sgatev wrote:
> Can you please add a test for this?
How does this work in practice? It seems to be an unsual case that we statically know a pointer is null. So, what is the use of this change and (more importantly) what is the impact on the framework, if any?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127746