[PATCH] D155075: [clang][dataflow] Add `DataflowEnvironment::createObject()`.

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

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6d768548ecc0: [clang][dataflow] Add 
`DataflowEnvironment::createObject()`. (authored by mboehme).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155075

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp

Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -208,62 +208,15 @@
 if (D.hasGlobalStorage())
   return;
 
-if (D.getType()->isReferenceType()) {
-  // If this is the holding variable for a `BindingDecl`, we may already
-  // have a storage location set up -- so check. (See also explanation below
-  // where we process the `BindingDecl`.)
-  if (Env.getStorageLocation(D) == nullptr) {
-const Expr *InitExpr = D.getInit();
-assert(InitExpr != nullptr);
-if (auto *InitExprLoc =
-Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
-  Env.setStorageLocation(D, *InitExprLoc);
-} else {
-  // Even though we have an initializer, we might not get an
-  // InitExprLoc, for example if the InitExpr is a CallExpr for which we
-  // don't have a function body. In this case, we just invent a storage
-  // location and value -- it's the best we can do.
-  StorageLocation  =
-  Env.createStorageLocation(D.getType().getNonReferenceType());
-  Env.setStorageLocation(D, Loc);
-  if (Value *Val = Env.createValue(D.getType().getNonReferenceType()))
-Env.setValue(Loc, *Val);
-}
-  }
-} else {
-  // Not a reference type.
+// If this is the holding variable for a `BindingDecl`, we may already
+// have a storage location set up -- so check. (See also explanation below
+// where we process the `BindingDecl`.)
+if (D.getType()->isReferenceType() && Env.getStorageLocation(D) != nullptr)
+  return;
 
-  assert(Env.getStorageLocation(D) == nullptr);
-  StorageLocation  = Env.createStorageLocation(D);
-  Env.setStorageLocation(D, Loc);
+assert(Env.getStorageLocation(D) == nullptr);
 
-  const Expr *InitExpr = D.getInit();
-  if (InitExpr == nullptr) {
-// No initializer expression - associate `Loc` with a new value.
-if (Value *Val = Env.createValue(D.getType()))
-  Env.setValue(Loc, *Val);
-return;
-  }
-
-  if (auto *InitExprVal = Env.getValueStrict(*InitExpr))
-Env.setValue(Loc, *InitExprVal);
-
-  if (Env.getValue(Loc) == nullptr) {
-// We arrive here in (the few) cases where an expression is
-// intentionally "uninterpreted". There are two ways to handle this
-// situation: propagate the status, so that uninterpreted initializers
-// result in uninterpreted variables, or provide a default value. We
-// choose the latter so that later refinements of the variable can be
-// used for reasoning about the surrounding code.
-//
-// FIXME. If and when we interpret all language cases, change this to
-// assert that `InitExpr` is interpreted, rather than supplying a
-// default value (assuming we don't update the environment API to return
-// references).
-if (Value *Val = Env.createValue(D.getType()))
-  Env.setValue(Loc, *Val);
-  }
-}
+Env.setStorageLocation(D, Env.createObject(D));
 
 // `DecompositionDecl` must be handled after we've interpreted the loc
 // itself, because the binding expression refers back to the
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -260,10 +260,8 @@
   for (const VarDecl *D : Vars) {
 if (getStorageLocation(*D) != nullptr)
   continue;
-auto  = createStorageLocation(D->getType().getNonReferenceType());
-setStorageLocation(*D, Loc);
-if (auto *Val = createValue(D->getType().getNonReferenceType()))
-  setValue(Loc, *Val);
+
+setStorageLocation(*D, createObject(*D));
   }
 
   for (const FunctionDecl *FD : Funcs) {
@@ -296,16 +294,7 @@
 
 for (const auto *ParamDecl : FuncDecl->parameters()) {
   assert(ParamDecl != nullptr);
-  // References aren't objects, so the reference itself doesn't have a
-  // storage location. Instead, the storage location for a reference 

[PATCH] D155075: [clang][dataflow] Add `DataflowEnvironment::createObject()`.

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

mboehme updated this revision to Diff 540871.
mboehme added a comment.

Add comment for `createObjectInternal()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155075

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp

Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -208,62 +208,15 @@
 if (D.hasGlobalStorage())
   return;
 
-if (D.getType()->isReferenceType()) {
-  // If this is the holding variable for a `BindingDecl`, we may already
-  // have a storage location set up -- so check. (See also explanation below
-  // where we process the `BindingDecl`.)
-  if (Env.getStorageLocation(D) == nullptr) {
-const Expr *InitExpr = D.getInit();
-assert(InitExpr != nullptr);
-if (auto *InitExprLoc =
-Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
-  Env.setStorageLocation(D, *InitExprLoc);
-} else {
-  // Even though we have an initializer, we might not get an
-  // InitExprLoc, for example if the InitExpr is a CallExpr for which we
-  // don't have a function body. In this case, we just invent a storage
-  // location and value -- it's the best we can do.
-  StorageLocation  =
-  Env.createStorageLocation(D.getType().getNonReferenceType());
-  Env.setStorageLocation(D, Loc);
-  if (Value *Val = Env.createValue(D.getType().getNonReferenceType()))
-Env.setValue(Loc, *Val);
-}
-  }
-} else {
-  // Not a reference type.
+// If this is the holding variable for a `BindingDecl`, we may already
+// have a storage location set up -- so check. (See also explanation below
+// where we process the `BindingDecl`.)
+if (D.getType()->isReferenceType() && Env.getStorageLocation(D) != nullptr)
+  return;
 
-  assert(Env.getStorageLocation(D) == nullptr);
-  StorageLocation  = Env.createStorageLocation(D);
-  Env.setStorageLocation(D, Loc);
+assert(Env.getStorageLocation(D) == nullptr);
 
-  const Expr *InitExpr = D.getInit();
-  if (InitExpr == nullptr) {
-// No initializer expression - associate `Loc` with a new value.
-if (Value *Val = Env.createValue(D.getType()))
-  Env.setValue(Loc, *Val);
-return;
-  }
-
-  if (auto *InitExprVal = Env.getValueStrict(*InitExpr))
-Env.setValue(Loc, *InitExprVal);
-
-  if (Env.getValue(Loc) == nullptr) {
-// We arrive here in (the few) cases where an expression is
-// intentionally "uninterpreted". There are two ways to handle this
-// situation: propagate the status, so that uninterpreted initializers
-// result in uninterpreted variables, or provide a default value. We
-// choose the latter so that later refinements of the variable can be
-// used for reasoning about the surrounding code.
-//
-// FIXME. If and when we interpret all language cases, change this to
-// assert that `InitExpr` is interpreted, rather than supplying a
-// default value (assuming we don't update the environment API to return
-// references).
-if (Value *Val = Env.createValue(D.getType()))
-  Env.setValue(Loc, *Val);
-  }
-}
+Env.setStorageLocation(D, Env.createObject(D));
 
 // `DecompositionDecl` must be handled after we've interpreted the loc
 // itself, because the binding expression refers back to the
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -260,10 +260,8 @@
   for (const VarDecl *D : Vars) {
 if (getStorageLocation(*D) != nullptr)
   continue;
-auto  = createStorageLocation(D->getType().getNonReferenceType());
-setStorageLocation(*D, Loc);
-if (auto *Val = createValue(D->getType().getNonReferenceType()))
-  setValue(Loc, *Val);
+
+setStorageLocation(*D, createObject(*D));
   }
 
   for (const FunctionDecl *FD : Funcs) {
@@ -296,16 +294,7 @@
 
 for (const auto *ParamDecl : FuncDecl->parameters()) {
   assert(ParamDecl != nullptr);
-  // References aren't objects, so the reference itself doesn't have a
-  // storage location. Instead, the storage location for a reference refers
-  // directly to an object of the referenced type -- so strip off any
-  // reference from the type.
-  auto  =
-  

[PATCH] D155075: [clang][dataflow] Add `DataflowEnvironment::createObject()`.

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

mboehme updated this revision to Diff 540022.
mboehme added a comment.

Rebased to head


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155075

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp

Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -208,62 +208,15 @@
 if (D.hasGlobalStorage())
   return;
 
-if (D.getType()->isReferenceType()) {
-  // If this is the holding variable for a `BindingDecl`, we may already
-  // have a storage location set up -- so check. (See also explanation below
-  // where we process the `BindingDecl`.)
-  if (Env.getStorageLocation(D) == nullptr) {
-const Expr *InitExpr = D.getInit();
-assert(InitExpr != nullptr);
-if (auto *InitExprLoc =
-Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
-  Env.setStorageLocation(D, *InitExprLoc);
-} else {
-  // Even though we have an initializer, we might not get an
-  // InitExprLoc, for example if the InitExpr is a CallExpr for which we
-  // don't have a function body. In this case, we just invent a storage
-  // location and value -- it's the best we can do.
-  StorageLocation  =
-  Env.createStorageLocation(D.getType().getNonReferenceType());
-  Env.setStorageLocation(D, Loc);
-  if (Value *Val = Env.createValue(D.getType().getNonReferenceType()))
-Env.setValue(Loc, *Val);
-}
-  }
-} else {
-  // Not a reference type.
+// If this is the holding variable for a `BindingDecl`, we may already
+// have a storage location set up -- so check. (See also explanation below
+// where we process the `BindingDecl`.)
+if (D.getType()->isReferenceType() && Env.getStorageLocation(D) != nullptr)
+  return;
 
-  assert(Env.getStorageLocation(D) == nullptr);
-  StorageLocation  = Env.createStorageLocation(D);
-  Env.setStorageLocation(D, Loc);
+assert(Env.getStorageLocation(D) == nullptr);
 
-  const Expr *InitExpr = D.getInit();
-  if (InitExpr == nullptr) {
-// No initializer expression - associate `Loc` with a new value.
-if (Value *Val = Env.createValue(D.getType()))
-  Env.setValue(Loc, *Val);
-return;
-  }
-
-  if (auto *InitExprVal = Env.getValueStrict(*InitExpr))
-Env.setValue(Loc, *InitExprVal);
-
-  if (Env.getValue(Loc) == nullptr) {
-// We arrive here in (the few) cases where an expression is
-// intentionally "uninterpreted". There are two ways to handle this
-// situation: propagate the status, so that uninterpreted initializers
-// result in uninterpreted variables, or provide a default value. We
-// choose the latter so that later refinements of the variable can be
-// used for reasoning about the surrounding code.
-//
-// FIXME. If and when we interpret all language cases, change this to
-// assert that `InitExpr` is interpreted, rather than supplying a
-// default value (assuming we don't update the environment API to return
-// references).
-if (Value *Val = Env.createValue(D.getType()))
-  Env.setValue(Loc, *Val);
-  }
-}
+Env.setStorageLocation(D, Env.createObject(D));
 
 // `DecompositionDecl` must be handled after we've interpreted the loc
 // itself, because the binding expression refers back to the
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -260,10 +260,8 @@
   for (const VarDecl *D : Vars) {
 if (getStorageLocation(*D) != nullptr)
   continue;
-auto  = createStorageLocation(D->getType().getNonReferenceType());
-setStorageLocation(*D, Loc);
-if (auto *Val = createValue(D->getType().getNonReferenceType()))
-  setValue(Loc, *Val);
+
+setStorageLocation(*D, createObject(*D));
   }
 
   for (const FunctionDecl *FD : Funcs) {
@@ -296,16 +294,7 @@
 
 for (const auto *ParamDecl : FuncDecl->parameters()) {
   assert(ParamDecl != nullptr);
-  // References aren't objects, so the reference itself doesn't have a
-  // storage location. Instead, the storage location for a reference refers
-  // directly to an object of the referenced type -- so strip off any
-  // reference from the type.
-  auto  =
-  

[PATCH] D155075: [clang][dataflow] Add `DataflowEnvironment::createObject()`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel accepted this revision.
ymandel added a comment.
This revision is now accepted and ready to land.

Looks great!




Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:609
 
+  StorageLocation (const VarDecl *D, QualType Ty,
+const Expr *InitExpr);

At least document the nullability of D and InitExpr?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155075

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155075: [clang][dataflow] Add `DataflowEnvironment::createObject()`.

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

mboehme created this revision.
Herald added subscribers: martong, xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
mboehme requested review of this revision.
Herald added subscribers: cfe-commits, wangpc.
Herald added a project: clang.

This consolidates the code used in various places to initialize objects (usually
for variables) into one central location.

It will also help reduce the number of changes needed when we make the upcoming
API changes to `AggregateStorageLocation` and `StructValue`.

Depends On D155074 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155075

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/Transfer.cpp

Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -208,62 +208,15 @@
 if (D.hasGlobalStorage())
   return;
 
-if (D.getType()->isReferenceType()) {
-  // If this is the holding variable for a `BindingDecl`, we may already
-  // have a storage location set up -- so check. (See also explanation below
-  // where we process the `BindingDecl`.)
-  if (Env.getStorageLocation(D) == nullptr) {
-const Expr *InitExpr = D.getInit();
-assert(InitExpr != nullptr);
-if (auto *InitExprLoc =
-Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
-  Env.setStorageLocation(D, *InitExprLoc);
-} else {
-  // Even though we have an initializer, we might not get an
-  // InitExprLoc, for example if the InitExpr is a CallExpr for which we
-  // don't have a function body. In this case, we just invent a storage
-  // location and value -- it's the best we can do.
-  StorageLocation  =
-  Env.createStorageLocation(D.getType().getNonReferenceType());
-  Env.setStorageLocation(D, Loc);
-  if (Value *Val = Env.createValue(D.getType().getNonReferenceType()))
-Env.setValue(Loc, *Val);
-}
-  }
-} else {
-  // Not a reference type.
+// If this is the holding variable for a `BindingDecl`, we may already
+// have a storage location set up -- so check. (See also explanation below
+// where we process the `BindingDecl`.)
+if (D.getType()->isReferenceType() && Env.getStorageLocation(D) != nullptr)
+  return;
 
-  assert(Env.getStorageLocation(D) == nullptr);
-  StorageLocation  = Env.createStorageLocation(D);
-  Env.setStorageLocation(D, Loc);
+assert(Env.getStorageLocation(D) == nullptr);
 
-  const Expr *InitExpr = D.getInit();
-  if (InitExpr == nullptr) {
-// No initializer expression - associate `Loc` with a new value.
-if (Value *Val = Env.createValue(D.getType()))
-  Env.setValue(Loc, *Val);
-return;
-  }
-
-  if (auto *InitExprVal = Env.getValueStrict(*InitExpr))
-Env.setValue(Loc, *InitExprVal);
-
-  if (Env.getValue(Loc) == nullptr) {
-// We arrive here in (the few) cases where an expression is
-// intentionally "uninterpreted". There are two ways to handle this
-// situation: propagate the status, so that uninterpreted initializers
-// result in uninterpreted variables, or provide a default value. We
-// choose the latter so that later refinements of the variable can be
-// used for reasoning about the surrounding code.
-//
-// FIXME. If and when we interpret all language cases, change this to
-// assert that `InitExpr` is interpreted, rather than supplying a
-// default value (assuming we don't update the environment API to return
-// references).
-if (Value *Val = Env.createValue(D.getType()))
-  Env.setValue(Loc, *Val);
-  }
-}
+Env.setStorageLocation(D, Env.createObject(D));
 
 // `DecompositionDecl` must be handled after we've interpreted the loc
 // itself, because the binding expression refers back to the
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -256,10 +256,8 @@
   for (const VarDecl *D : Vars) {
 if (getStorageLocation(*D) != nullptr)
   continue;
-auto  = createStorageLocation(D->getType().getNonReferenceType());
-setStorageLocation(*D, Loc);
-if (auto *Val = createValue(D->getType().getNonReferenceType()))
-  setValue(Loc, *Val);
+
+setStorageLocation(*D, createObject(*D));
   }
 
   for (const FunctionDecl *FD : Funcs) {
@@