[clang][dataflow] Analyze method bodies

[samestep]

Summary

This patch adds the ability to context-sensitively analyze method bodies, by moving ThisPointeeLoc from DataflowAnalysisContext to Environment, and adding code in pushCall to set it.

[sgatev]

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

383–385

Let's add a brief comment explaining what this is and when it is expected to be non-null.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

217

What if Arg is null?

[samestep]

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

383–385

Will do, thanks.

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

217

Good point, thanks; under what circumstances can that happen? In any case, I'm adding a guard for this.

[samestep]

Address Stanislav's comments

[sgatev]

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

217

It can be null if the argument isn't modeled and there's no value assigned to its storage location.

[samestep]

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

217

Hmm I don't quite follow; Arg just comes from calling getImplicitObjectArgument on the CXXMemberCallExpr, right? So shouldn't it only depend on the AST, and be completely independent of what we do or don't model?

[sgatev]

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

217

Ah, right. Well, I'm not familiar with the details, but apparently it returns null in some cases.

[ymandel]

Nice!!

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h

public:
 
   /// Returns the storage location assigned to `E` or null if `E` has no
   /// assigned storage location.
   StorageLocation *getStorageLocation(const Expr &E) const {
     auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E));
     return It == ExprToLoc.end() ? nullptr : It->second;
   }
 
-  /// Assigns `Loc` as the storage location of the `this` pointee.
-  ///
-  /// Requirements:
-  ///
-  ///  The `this` pointee must not be assigned a storage location.
-  void setThisPointeeStorageLocation(StorageLocation &Loc) {
-    assert(ThisPointeeLoc == nullptr);
-    ThisPointeeLoc = &Loc;
-  }
-
-  /// Returns the storage location assigned to the `this` pointee or null if the
-  /// `this` pointee has no assigned storage location.
-  StorageLocation *getThisPointeeStorageLocation() const {
-    return ThisPointeeLoc;
-  }
-
   /// Returns a pointer value that represents a null pointer. Calls with
   /// `PointeeType` that are canonically equivalent will return the same result.
   /// A null `PointeeType` can be used for the pointee of `std::nullptr_t`.
   PointerValue &getOrCreateNullPointerValue(QualType PointeeType);
 
   /// Returns a symbolic boolean value that models a boolean literal equal to
   /// `Value`.
   AtomicBoolValue &getBoolLiteralValue(bool Value) const {
private:
   // Maps from program declarations and statements to storage locations that are
   // assigned to them. These assignments are global (aggregated across all basic
   // blocks) and are used to produce stable storage locations when the same
   // basic blocks are evaluated multiple times. The storage locations that are
   // in scope for a particular basic block are stored in `Environment`.
   llvm::DenseMap<const ValueDecl *, StorageLocation *> DeclToLoc;
   llvm::DenseMap<const Expr *, StorageLocation *> ExprToLoc;
 
-  // FIXME: Move this into `Environment`.
-  StorageLocation *ThisPointeeLoc = nullptr;
-
   // Null pointer values, keyed by the canonical pointee type.
   //
   // FIXME: The pointer values are indexed by the pointee types which are
   // required to initialize the `PointeeLoc` field in `PointerValue`. Consider
   // creating a type-independent `NullPointerValue` without a `PointeeLoc`
   // field.
   llvm::DenseMap<QualType, PointerValue *, NullableQualTypeDenseMapInfo>
       NullPointerVals;

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

private:
   const StorageLocation &skip(const StorageLocation &Loc, SkipPast SP) const;
 
   // `DACtx` is not null and not owned by this object.
   DataflowAnalysisContext *DACtx;
 
   // In a properly initialized `Environment`, `ReturnLoc` should only be null if
   // its `DeclContext` could not be cast to a `FunctionDecl`.
   StorageLocation *ReturnLoc = nullptr;
-  // FIXME: Move `ThisPointeeLoc` here from `DataflowAnalysisContext`.
+  // The storage location of the `this` pointee. Should only be null if the
+  // function being analyzed is only a function and not a method.
+  StorageLocation *ThisPointeeLoc = nullptr;
 
   // Maps from program declarations and statements to storage locations that are
   // assigned to them. Unlike the maps in `DataflowAnalysisContext`, these
   // include only storage locations that are in scope for a particular basic
   // block.
   llvm::DenseMap<const ValueDecl *, StorageLocation *> DeclToLoc;
   llvm::DenseMap<const Expr *, StorageLocation *> ExprToLoc;
 

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

static void initGlobalVars(const Stmt &S, Environment &Env) {
   }
 }
 
 Environment::Environment(DataflowAnalysisContext &DACtx)
     : DACtx(&DACtx), FlowConditionToken(&DACtx.makeFlowConditionToken()) {}
 
 Environment::Environment(const Environment &Other)
     : DACtx(Other.DACtx), ReturnLoc(Other.ReturnLoc),
-      DeclToLoc(Other.DeclToLoc), ExprToLoc(Other.ExprToLoc),
-      LocToVal(Other.LocToVal), MemberLocToStruct(Other.MemberLocToStruct),
+      ThisPointeeLoc(Other.ThisPointeeLoc), DeclToLoc(Other.DeclToLoc),
+      ExprToLoc(Other.ExprToLoc), LocToVal(Other.LocToVal),
+      MemberLocToStruct(Other.MemberLocToStruct),
       FlowConditionToken(&DACtx->forkFlowCondition(*Other.FlowConditionToken)) {
 }
 
 Environment &Environment::operator=(const Environment &Other) {
   Environment Copy(Other);
   *this = std::move(Copy);
   return *this;
 }
if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(&DeclCtx)) {
     assert(Parent != nullptr);
     if (Parent->isLambda())
       MethodDecl = dyn_cast<CXXMethodDecl>(Parent->getDeclContext());
 
     if (MethodDecl && !MethodDecl->isStatic()) {
       QualType ThisPointeeType = MethodDecl->getThisObjectType();
       // FIXME: Add support for union types.
       if (!ThisPointeeType->isUnionType()) {
-        auto &ThisPointeeLoc = createStorageLocation(ThisPointeeType);
-        DACtx.setThisPointeeStorageLocation(ThisPointeeLoc);
+        ThisPointeeLoc = &createStorageLocation(ThisPointeeType);
         if (Value *ThisPointeeVal = createValue(ThisPointeeType))
-          setValue(ThisPointeeLoc, *ThisPointeeVal);
+          setValue(*ThisPointeeLoc, *ThisPointeeVal);
       }
     }
   }
 }
 
 Environment Environment::pushCall(const CallExpr *Call) const {
   Environment Env(*this);
   Env.ReturnLoc = Env.getStorageLocation(*Call, SkipPast::Reference);
 
   const auto *FuncDecl = Call->getDirectCallee();
   assert(FuncDecl != nullptr);
   // FIXME: In order to allow the callee to reference globals, we probably need
   // to call `initGlobalVars` here in some way.
 
+  if (const auto *MethodCall = dyn_cast<CXXMemberCallExpr>(Call)) {
+    if (const Expr *Arg = MethodCall->getImplicitObjectArgument()) {
+      Env.ThisPointeeLoc = Env.getStorageLocation(*Arg, SkipPast::Reference);
+    }
+  }
+
   auto ParamIt = FuncDecl->param_begin();
   auto ArgIt = Call->arg_begin();
   auto ArgEnd = Call->arg_end();
 
   // FIXME: Parameters don't always map to arguments 1:1; examples include
   // overloaded operators implemented as member functions, and parameter packs.
   for (; ArgIt != ArgEnd; ++ParamIt, ++ArgIt) {
     assert(ParamIt != FuncDecl->param_end());
for (; ArgIt != ArgEnd; ++ParamIt, ++ArgIt) {
     }
   }
 
   return Env;
 }
 
 void Environment::popCall(const Environment &CalleeEnv) {
   // We ignore `DACtx` because it's already the same in both. We don't want the
-  // callee's `ReturnLoc`. We don't bring back `DeclToLoc` and `ExprToLoc`
-  // because we want to be able to later analyze the same callee in a different
-  // context, and `setStorageLocation` requires there to not already be a
-  // storage location assigned. Conceptually, these maps capture information
-  // from the local scope, so when popping that scope, we do not propagate the
-  // maps.
+  // callee's `ReturnLoc` or `ThisPointeeLoc`. We don't bring back `DeclToLoc`
+  // and `ExprToLoc` because we want to be able to later analyze the same callee
+  // in a different context, and `setStorageLocation` requires there to not
+  // already be a storage location assigned. Conceptually, these maps capture
+  // information from the local scope, so when popping that scope, we do not
+  // propagate the maps.
   this->LocToVal = std::move(CalleeEnv.LocToVal);
   this->MemberLocToStruct = std::move(CalleeEnv.MemberLocToStruct);
   this->FlowConditionToken = std::move(CalleeEnv.FlowConditionToken);
 }
 
 bool Environment::equivalentTo(const Environment &Other,
                                Environment::ValueModel &Model) const {
   assert(DACtx == Other.DACtx);
 
   if (ReturnLoc != Other.ReturnLoc)
     return false;
 
+  if (ThisPointeeLoc != Other.ThisPointeeLoc)
+    return false;
+
   if (DeclToLoc != Other.DeclToLoc)
     return false;
 
   if (ExprToLoc != Other.ExprToLoc)
     return false;
 
   // Compare the contents for the intersection of their domains.
   for (auto &Entry : LocToVal) {
bool Environment::equivalentTo(const Environment &Other,
 
   return true;
 }
 
 LatticeJoinEffect Environment::join(const Environment &Other,
                                     Environment::ValueModel &Model) {
   assert(DACtx == Other.DACtx);
   assert(ReturnLoc == Other.ReturnLoc);
+  assert(ThisPointeeLoc == Other.ThisPointeeLoc);
 
   auto Effect = LatticeJoinEffect::Unchanged;
 
   Environment JoinedEnv(*DACtx);
 
   JoinedEnv.ReturnLoc = ReturnLoc;
+  JoinedEnv.ThisPointeeLoc = ThisPointeeLoc;
 
   JoinedEnv.DeclToLoc = intersectDenseMaps(DeclToLoc, Other.DeclToLoc);
   if (DeclToLoc.size() != JoinedEnv.DeclToLoc.size())
     Effect = LatticeJoinEffect::Changed;
 
   JoinedEnv.ExprToLoc = intersectDenseMaps(ExprToLoc, Other.ExprToLoc);
   if (ExprToLoc.size() != JoinedEnv.ExprToLoc.size())
     Effect = LatticeJoinEffect::Changed;

 StorageLocation *Environment::getStorageLocation(const Expr &E,
                                                  SkipPast SP) const {
   // FIXME: Add a test with parens.
   auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E));
   return It == ExprToLoc.end() ? nullptr : &skip(*It->second, SP);
 }
 
 StorageLocation *Environment::getThisPointeeStorageLocation() const {
-  return DACtx->getThisPointeeStorageLocation();
+  return ThisPointeeLoc;
 }
 
 StorageLocation *Environment::getReturnStorageLocation() const {
   return ReturnLoc;
 }
 
 PointerValue &Environment::getOrCreateNullPointerValue(QualType PointeeType) {
   return DACtx->getOrCreateNullPointerValue(PointeeType);

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

runDataflow(Code,
                 auto &BazVal =
                     *cast<BoolValue>(Env.getValue(*BazDecl, SkipPast::None));
                 EXPECT_TRUE(Env.flowConditionImplies(BazVal));
               },
               {/*.ApplyBuiltinTransfer=*/true,
                /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}});
 }
 
+TEST(TransferTest, ContextSensitiveMethodLiteral) {
+  std::string Code = R"(
+    class MyClass {
+    public:
+      bool giveBool() { return true; }
+    };
+
+    void target() {
+      MyClass MyObj;
+      bool Foo = MyObj.giveBool();
+      // [[p]]
+    }
+  )";
+  runDataflow(Code,
+              [](llvm::ArrayRef<
+                     std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+                     Results,
+                 ASTContext &ASTCtx) {
+                ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+                const Environment &Env = Results[0].second.Env;
+
+                const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+                ASSERT_THAT(FooDecl, NotNull());
+
+                auto &FooVal =
+                    *cast<BoolValue>(Env.getValue(*FooDecl, SkipPast::None));
+                EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+              },
+              {/*.ApplyBuiltinTransfer=*/true,
+               /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}});
+}
+
+TEST(TransferTest, ContextSensitiveMethodGetter) {
+  std::string Code = R"(
+    class MyClass {
+    public:
+      bool getField() { return Field; }
+
+      bool Field;
+    };
+
+    void target() {
+      MyClass MyObj;
+      MyObj.Field = true;
+      bool Foo = MyObj.getField();
+      // [[p]]
+    }
+  )";
+  runDataflow(Code,
+              [](llvm::ArrayRef<
+                     std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+                     Results,
+                 ASTContext &ASTCtx) {
+                ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+                const Environment &Env = Results[0].second.Env;
+
+                const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+                ASSERT_THAT(FooDecl, NotNull());
+
+                auto &FooVal =
+                    *cast<BoolValue>(Env.getValue(*FooDecl, SkipPast::None));
+                EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+              },
+              {/*.ApplyBuiltinTransfer=*/true,
+               /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}});
+}
+
+TEST(TransferTest, ContextSensitiveMethodSetter) {
+  std::string Code = R"(
+    class MyClass {
+    public:
+      bool setField(bool Val) { Field = Val; }
+
+      bool Field;
+    };
+
+    void target() {
+      MyClass MyObj;
+      MyObj.setField(true);
+      bool Foo = MyObj.Field;
+      // [[p]]
+    }
+  )";
+  runDataflow(Code,
+              [](llvm::ArrayRef<
+                     std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+                     Results,
+                 ASTContext &ASTCtx) {
+                ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+                const Environment &Env = Results[0].second.Env;
+
+                const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+                ASSERT_THAT(FooDecl, NotNull());
+
+                auto &FooVal =
+                    *cast<BoolValue>(Env.getValue(*FooDecl, SkipPast::None));
+                EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+              },
+              {/*.ApplyBuiltinTransfer=*/true,
+               /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}});
+}
+
+TEST(TransferTest, ContextSensitiveMethodGetterAndSetter) {
+  std::string Code = R"(
+    class MyClass {
+    public:
+      bool getField() { return Field; }
+      bool setField(bool Val) { Field = Val; }
+
+    private:
+      bool Field;
+    };
+
+    void target() {
+      MyClass MyObj;
+      MyObj.setField(true);
+      bool Foo = MyObj.getField();
+      // [[p]]
+    }
+  )";
+  runDataflow(Code,
+              [](llvm::ArrayRef<
+                     std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+                     Results,
+                 ASTContext &ASTCtx) {
+                ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+                const Environment &Env = Results[0].second.Env;
+
+                const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+                ASSERT_THAT(FooDecl, NotNull());
+
+                auto &FooVal =
+                    *cast<BoolValue>(Env.getValue(*FooDecl, SkipPast::None));
+                EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+              },
+              {/*.ApplyBuiltinTransfer=*/true,
+               /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}});
+}
+
 } // namespace