[clang] 3bc1ea5 - [clang][dataflow] Fix a bug in handling of `operator->` for optional checker.

[Martin Braenne via cfe-commits]

Author: Martin Braenne
Date: 2023-05-22T06:51:15Z
New Revision: 3bc1ea5b0ac90e04e7b935a5d964613f8fbad4bf

URL: https://github.com/llvm/llvm-project/commit/3bc1ea5b0ac90e04e7b935a5d964613f8fbad4bf
DIFF: https://github.com/llvm/llvm-project/commit/3bc1ea5b0ac90e04e7b935a5d964613f8fbad4bf.diff

LOG: [clang][dataflow] Fix a bug in handling of `operator->` for optional checker.

Prior to this patch, `operator->` was being handled like `operator*`: It was
associating a `Value` of type `T` with the expression result (where `T` is the
template argument of the `optional<T>`). This is correct for `operator*`, which
returns a reference (of some flavor) to `T`, so that the result of the
`CXXOperatorCallExpr` is a glvalue of type `T`. However, `operator*` returns a
`T*`, so the result of the `CXXOperatorCallExpr` is a prvalue `T*`, which should
therefore be associated with `PointerValue` that in turn refers to a `T`.

I noticed this issue while working on the migration to strict handling of
value categories (see https://discourse.llvm.org/t/70086). The current behavior
also seems problematic more generally because it's plausible that the framework
may at some point introduce behavior that assumes an `Expr` of pointer type is
always associated with a `PointerValue`.

As it turns out, this patch fixes an existing FIXME in the test
`OptionalValueInitialization`.

Depends On D150657

Reviewed By: ymandel

Differential Revision: https://reviews.llvm.org/D150775

Added: 
    

Modified: 
    clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
    clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 24a3682f438cb..7d30473ea5226 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -399,6 +399,18 @@ void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
   }
 }
 
+void transferArrowOpCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
+                         LatticeTransferState &State) {
+  if (auto *OptionalVal =
+          getValueBehindPossiblePointer(*ObjectExpr, State.Env)) {
+    if (auto *Loc = maybeInitializeOptionalValueMember(
+            UnwrapExpr->getType()->getPointeeType(), *OptionalVal, State.Env)) {
+      State.Env.setValueStrict(*UnwrapExpr,
+                               State.Env.create<PointerValue>(*Loc));
+    }
+  }
+}
+
 void transferMakeOptionalCall(const CallExpr *E,
                               const MatchFinder::MatchResult &,
                               LatticeTransferState &State) {
@@ -774,25 +786,22 @@ auto buildTransferMatchSwitch() {
             transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
           })
 
-      // optional::operator*, optional::operator->
-      // FIXME: This does something slightly strange for `operator->`.
-      // `transferUnwrapCall()` may create a new value of type `T` for the
-      // `optional<T>`, and it associates that value with `E`. In the case of
-      // `operator->`, `E` is a pointer. As a result, we associate an
-      // expression of pointer type with a storage location of non-pointer type
-      // `T`. This can confound other code that expects expressions of
-      // pointer type to be associated with `PointerValue`s, such as the
-      // centrally provided accessors `getImplicitObjectLocation()` and
-      // `getBaseObjectLocation()`, and this is the reason we need to use our
-      // own 'maybeSkipPointer()` and `getValueBehindPossiblePointer()` instead
-      // of these accessors.
-      .CaseOfCFGStmt<CallExpr>(valueOperatorCall(std::nullopt),
+      // optional::operator*
+      .CaseOfCFGStmt<CallExpr>(isOptionalOperatorCallWithName("*"),
                                [](const CallExpr *E,
                                   const MatchFinder::MatchResult &,
                                   LatticeTransferState &State) {
                                  transferUnwrapCall(E, E->getArg(0), State);
                                })
 
+      // optional::operator->
+      .CaseOfCFGStmt<CallExpr>(isOptionalOperatorCallWithName("->"),
+                               [](const CallExpr *E,
+                                  const MatchFinder::MatchResult &,
+                                  LatticeTransferState &State) {
+                                 transferArrowOpCall(E, E->getArg(0), State);
+                               })
+
       // optional::has_value
       .CaseOfCFGStmt<CXXMemberCallExpr>(
           isOptionalMemberCallWithName("has_value"),

diff  --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index a9d2357a57fb2..4bac015c995f6 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2764,9 +2764,6 @@ TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct) {
 }
 
 TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
-  // FIXME: Fix when to initialize `value`. All unwrapping should be safe in
-  // this example, but `value` initialization is done multiple times during the
-  // fixpoint iterations and joining the environment won't correctly merge them.
   ExpectDiagnosticsFor(
       R"(
     #include "unchecked_optional_access_test.h"
@@ -2786,7 +2783,7 @@ TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
       }
       // Now we merge the two values. UncheckedOptionalAccessModel::merge() will
       // throw away the "value" property.
-      foo->value(); // [[unsafe]]
+      foo->value();
     }
   )");
 }