[PATCH] D116368: [clang][dataflow] Add transfer function for VarDecl statements

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Thanks! I have some questions inline.



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:61
+  ///  `D` must not be assigned a storage location.
+  void setStorageLocation(const VarDecl &D, StorageLocation &Loc) {
+    assert(VarDeclToLoc.find(&D) == VarDeclToLoc.end());
----------------
Are other language constructs coming in a follow-up PR? (E.g.: FieldDecl, locations on the heap). I'd like to some TODO comments if that is the case. Or if they are not supported by design, I'd love to some some comments about that.

Environment is referencing `ValueDecl` as opposed to `VarDecl`. What is the reason for this asymmetry?
I think I overall find it confusing that some of the DeclToLoc mapping is here some is in the Environment.
Does this has something to do with the fact that certain mappings should be the same regardless of which basic blocks we are processing (i.e., invariant globally so we do not need to duplicate it)? If that is the case please make it clear in the comments. 


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:76
+  std::vector<std::unique_ptr<StorageLocation>> Locs;
+  std::vector<std::unique_ptr<Value>> Vals;
+  llvm::DenseMap<const VarDecl *, StorageLocation *> VarDeclToLoc;
----------------
Just curious: would it make sense to deduplicate these values?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:33
+
+  virtual ~Value() {}
+
----------------
` = default;`


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:78
+
+  StorageLocation *getLocation() const { return Loc; }
+
----------------
I wonder if getLocation is ambiguous. (The pointer's own location vs the pointee location.) How about something like `getPointeeLoc`?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:32
+template <typename K, typename V>
+bool denseMapsAreEqual(const llvm::DenseMap<K, V> &Map1,
+                       const llvm::DenseMap<K, V> &Map2) {
----------------
Shouldn't we add `operator==` instead?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:46
+template <typename K, typename V>
+llvm::DenseMap<K, V> intersectDenseMaps(const llvm::DenseMap<K, V> &Map1,
+                                        const llvm::DenseMap<K, V> &Map2) {
----------------
I wonder if these algorithms should rather be somewhere in the support library.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:88
+    for (const FieldDecl *Field : Type->getAsRecordDecl()->fields()) {
+      FieldLocs.insert({Field, &createStorageLocation(Field->getType())});
+    }
----------------
Could this end up creating an overly large state? There might be objects with quite a lot fields but each function would only access a small subset of those. Alternatively, we could attempt to create the representations for fields on demand (this is the approach what the clang static analyzer is using). 


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:117
+
+Value *Environment::initValueInStorageLocation(const StorageLocation &Loc,
+                                               QualType Type) {
----------------
I think this function implementation is better to be moved closer to the called `initValueInStorageLocationUnlessSelfReferential` for readability.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:144
+  if (Type->isRealFloatingType()) {
+    auto &Value = DACtx->takeOwnership(std::make_unique<FloatValue>());
+    setValue(Loc, Value);
----------------
Are we planning to track float values in the near future? If not, would it make sense to defer creating these objects until then?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:154
+    if (!Visited.contains(PointeeType)) {
+      Visited.insert(PointeeType);
+      initValueInStorageLocationUnlessSelfReferential(PointeeLoc, PointeeType,
----------------
Do we want to canonicalize types before inserting into this set?


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:33
+static void transferDeclStmt(const DeclStmt &S, Environment &Env) {
+  if (const auto *D = dyn_cast<VarDecl>(S.getSingleDecl())) {
+    transferVarDecl(*D, Env);
----------------
Maybe add a TODO for `int a, b;` and similar codes?


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:41
+
+  if (const auto *DS = dyn_cast<DeclStmt>(&S)) {
+    transferDeclStmt(*DS, Env);
----------------
Instead of using a long `if else` and dynamic casts, don't we want to use an `StmtVisitor` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116368