[clang][dataflow] Add support for new expressions.

[gribozavr2]

clang/include/clang/Analysis/FlowSensitive/Value.h

315 ↗

(On Diff #511366)

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.

clang/lib/Analysis/FlowSensitive/Transfer.cpp

485

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++.

490

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).

[mboehme]

@gribozavr2 Just to make sure I understand you correctly here (before I make any changes to the code): IIUC you recommend simply doing nothing on a delete expression? (Your arguments for this make sense to me, just want to double-check I understood correctly.)

[gribozavr2]

In D147698#4257698, @mboehme wrote:

@gribozavr2 Just to make sure I understand you correctly here (before I make any changes to the code): IIUC you recommend simply doing nothing on a delete expression? (Your arguments for this make sense to me, just want to double-check I understood correctly.)

Yes, that's my suggestion.

[mboehme]

Eliminate specific handling of delete expressions.

[mboehme]

clang/include/clang/Analysis/FlowSensitive/Value.h

315 ↗

(On Diff #511366)

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 StructValues 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.

clang/lib/Analysis/FlowSensitive/Transfer.cpp

485

Good point. This makes everything much simpler. Done.

490

(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.

clang/lib/Analysis/FlowSensitive/Transfer.cpp

if (ThisPointeeLoc == nullptr)
       // `this` expression's pointee.
       return;
 
     auto &Loc = Env.createStorageLocation(*S);
     Env.setStorageLocation(*S, Loc);
     Env.setValue(Loc, Env.create<PointerValue>(*ThisPointeeLoc));
   }
 
+  void VisitCXXNewExpr(const CXXNewExpr *S) {
+    auto &Loc = Env.createStorageLocation(*S);
+    Env.setStorageLocation(*S, Loc);
+    if (Value *Val = Env.createValue(S->getType()))
+      Env.setValue(Loc, *Val);
+  }
+
+  void VisitCXXDeleteExpr(const CXXDeleteExpr *S) {
+    // Empty method.
+    // We consciously don't do anything on deletes.  Diagnosing double deletes
+    // (for example) should be done by a specific analysis, not by the
+    // framework.
+  }
+
   void VisitReturnStmt(const ReturnStmt *S) {
     if (!Env.getAnalysisOptions().ContextSensitiveOpts)
       return;
 
     auto *Ret = S->getRetValue();
     if (Ret == nullptr)
       return;
 
     auto *Val = Env.getValue(*Ret, SkipPast::None);
     if (Val == nullptr)
       return;
 
     // FIXME: Support reference-type returns.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

runDataflow(
         EXPECT_TRUE(Env.flowConditionImplies(NoreturnOnLhsMakesAndUnreachable));
 
         auto &NoreturnOnLhsMakesOrUnreachable = getValueForDecl<BoolValue>(
             ASTCtx, Env, "NoreturnOnLhsMakesOrUnreachable");
         EXPECT_TRUE(Env.flowConditionImplies(NoreturnOnLhsMakesOrUnreachable));
       });
 }
 
+TEST(TransferTest, NewExpressions) {
+  std::string Code = R"(
+    void target() {
+      int *p = new int(42);
+      // [[after_new]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env =
+            getEnvironmentAtAnnotation(Results, "after_new");
+
+        auto &P = getValueForDecl<PointerValue>(ASTCtx, Env, "p");
+
+        EXPECT_THAT(Env.getValue(P.getPointeeLoc()), NotNull());
+      });
+}
+
+TEST(TransferTest, NewExpressions_Structs) {
+  std::string Code = R"(
+    struct Inner {
+      int InnerField;
+    };
+
+    struct Outer {
+      Inner OuterField;
+    };
+
+    void target() {
+      Outer *p = new Outer;
+      // Access the fields to make sure the analysis actually generates children
+      // for them in the `AggregateStorageLoc` and `StructValue`.
+      p->OuterField.InnerField;
+      // [[after_new]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env =
+            getEnvironmentAtAnnotation(Results, "after_new");
+
+        const ValueDecl *OuterField = findValueDecl(ASTCtx, "OuterField");
+        const ValueDecl *InnerField = findValueDecl(ASTCtx, "InnerField");
+
+        auto &P = getValueForDecl<PointerValue>(ASTCtx, Env, "p");
+
+        auto &OuterLoc = cast<AggregateStorageLocation>(P.getPointeeLoc());
+        auto &OuterFieldLoc =
+            cast<AggregateStorageLocation>(OuterLoc.getChild(*OuterField));
+        auto &InnerFieldLoc = OuterFieldLoc.getChild(*InnerField);
+
+        // Values for the struct and all fields exist after the new.
+        EXPECT_THAT(Env.getValue(OuterLoc), NotNull());
+        EXPECT_THAT(Env.getValue(OuterFieldLoc), NotNull());
+        EXPECT_THAT(Env.getValue(InnerFieldLoc), NotNull());
+      });
+}
+
 } // namespace