[clang][dataflow] Add test for crash repro and clean up const accessor handling

[jvoung]

Add test for #125589

The crash is actually incidentally fixed by #128437 since it added a branch for the reference case and would no longer fall through when the return type is a reference to a pointer.

Clean up a bit as well:

• make the fallback for early returns more consistent (check if
  returning optional and call transfer function for that case)
• check RecordLoc == nullptr in one place
• clean up extra spaces in test
• clean up parameterization in test of std:: vs $ns::$

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Jan Voung (jvoung)

Changes

Add test for #125589

The crash is actually incidentally fixed by #128437 since it added a branch for the reference case and would no longer fall through when the return type is a reference to a pointer.

Clean up a bit as well:

• make the fallback for early returns more consistent (check if
  returning optional and call transfer function for that case)
• check RecordLoc == nullptr in one place
• clean up extra spaces in test
• clean up parameterization in test of std:: vs $ns::$

---

Full diff: https://github.com/llvm/llvm-project/pull/129930.diff

2 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+39-31)
• (modified) clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp (+46-18)

diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 9381c5c42e566..9c873d2e7e1f4 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -551,15 +551,18 @@ void transferCallReturningOptional(const CallExpr *E,
   setHasValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
 }
 
-void handleConstMemberCall(const CallExpr *CE,
+bool handleConstMemberCall(const CallExpr *CE,
                            dataflow::RecordStorageLocation *RecordLoc,
                            const MatchFinder::MatchResult &Result,
                            LatticeTransferState &State) {
+  if (RecordLoc == nullptr)
+    return false;
+
   // If the const method returns an optional or reference to an optional.
-  if (RecordLoc != nullptr && isSupportedOptionalType(CE->getType())) {
+  if (isSupportedOptionalType(CE->getType())) {
     const FunctionDecl *DirectCallee = CE->getDirectCallee();
     if (DirectCallee == nullptr)
-      return;
+      return false;
     StorageLocation &Loc =
         State.Lattice.getOrCreateConstMethodReturnStorageLocation(
             *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
@@ -577,57 +580,62 @@ void handleConstMemberCall(const CallExpr *CE,
       auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
       copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
     }
-    return;
+    return true;
   }
 
   // Cache if the const method returns a reference
-  if (RecordLoc != nullptr && CE->isGLValue()) {
+  if (CE->isGLValue()) {
     const FunctionDecl *DirectCallee = CE->getDirectCallee();
     if (DirectCallee == nullptr)
-      return;
+      return false;
 
     StorageLocation &Loc =
         State.Lattice.getOrCreateConstMethodReturnStorageLocation(
             *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
               // no-op
+              // NOTE: if we want to support const ref to pointers or bools
+              // we should initialize their values here.
             });
 
     State.Env.setStorageLocation(*CE, Loc);
-    return;
-  }
-
-  // Cache if the const method returns a boolean or pointer type.
-  // We may decide to cache other return types in the future.
-  if (RecordLoc != nullptr &&
-      (CE->getType()->isBooleanType() || CE->getType()->isPointerType())) {
+    return true;
+  } else if (CE->getType()->isBooleanType() || CE->getType()->isPointerType()) {
+    // Cache if the const method returns a boolean or pointer type.
     Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, CE,
                                                                  State.Env);
     if (Val == nullptr)
-      return;
+      return false;
     State.Env.setValue(*CE, *Val);
-    return;
+    return true;
   }
 
-  // Perform default handling if the call returns an optional
-  // but wasn't handled above (if RecordLoc is nullptr).
-  if (isSupportedOptionalType(CE->getType())) {
-    transferCallReturningOptional(CE, Result, State);
-  }
+  return false;
 }
 
-void transferValue_ConstMemberCall(const CXXMemberCallExpr *MCE,
-                                   const MatchFinder::MatchResult &Result,
-                                   LatticeTransferState &State) {
-  handleConstMemberCall(
-      MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result, State);
+void transferConstMemberCall(const CXXMemberCallExpr *MCE,
+                             const MatchFinder::MatchResult &Result,
+                             LatticeTransferState &State) {
+  if (!handleConstMemberCall(
+          MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result,
+          State)) {
+    // Perform default handling if the call returns an optional,
+    // but wasn't handled.
+    if (isSupportedOptionalType(MCE->getType()))
+      transferCallReturningOptional(MCE, Result, State);
+  }
 }
 
-void transferValue_ConstMemberOperatorCall(
-    const CXXOperatorCallExpr *OCE, const MatchFinder::MatchResult &Result,
-    LatticeTransferState &State) {
+void transferConstMemberOperatorCall(const CXXOperatorCallExpr *OCE,
+                                     const MatchFinder::MatchResult &Result,
+                                     LatticeTransferState &State) {
   auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
       State.Env.getStorageLocation(*OCE->getArg(0)));
-  handleConstMemberCall(OCE, RecordLoc, Result, State);
+  if (!handleConstMemberCall(OCE, RecordLoc, Result, State)) {
+    // Perform default handling if the call returns an optional,
+    // but wasn't handled.
+    if (isSupportedOptionalType(OCE->getType()))
+      transferCallReturningOptional(OCE, Result, State);
+  }
 }
 
 void handleNonConstMemberCall(const CallExpr *CE,
@@ -1094,9 +1102,9 @@ auto buildTransferMatchSwitch() {
 
       // const accessor calls
       .CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
-                                        transferValue_ConstMemberCall)
+                                        transferConstMemberCall)
       .CaseOfCFGStmt<CXXOperatorCallExpr>(isZeroParamConstMemberOperatorCall(),
-                                          transferValue_ConstMemberOperatorCall)
+                                          transferConstMemberOperatorCall)
       // non-const member calls that may modify the state of an object.
       .CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
                                         transferValue_NonConstMemberCall)
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 5031e17188e17..46fad6b655c4d 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3829,7 +3829,7 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessor) {
     };
 
     void target(A& a) {
-      std::optional<int> opt;
+      $ns::$optional<int> opt;
       if (a.isFoo()) {
         opt = 1;
       }
@@ -3851,7 +3851,7 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) {
     };
 
     void target(A& a) {
-      std::optional<int> opt;
+      $ns::$optional<int> opt;
       if (a.isFoo()) {
         opt = 1;
       }
@@ -3870,13 +3870,13 @@ TEST_P(UncheckedOptionalAccessTest,
 
     struct A {
       const $ns::$optional<int>& get() const { return x; }
-      
+
       $ns::$optional<int> x;
     };
 
     struct B {
       const A& getA() const { return a; }
-    
+
       A a;
     };
 
@@ -3896,13 +3896,13 @@ TEST_P(
 
     struct A {
       const $ns::$optional<int>& get() const { return x; }
-      
+
       $ns::$optional<int> x;
     };
 
     struct B {
       const A& getA() const { return a; }
-    
+
       A a;
     };
 
@@ -3919,13 +3919,13 @@ TEST_P(UncheckedOptionalAccessTest,
 
     struct A {
       const $ns::$optional<int>& get() const { return x; }
-      
+
       $ns::$optional<int> x;
     };
 
     struct B {
       const A& getA() const { return a; }
-    
+
       A a;
     };
 
@@ -3945,13 +3945,13 @@ TEST_P(UncheckedOptionalAccessTest,
 
     struct A {
       const $ns::$optional<int>& get() const { return x; }
-      
+
       $ns::$optional<int> x;
     };
 
     struct B {
       const A copyA() const { return a; }
-    
+
       A a;
     };
 
@@ -3970,13 +3970,13 @@ TEST_P(UncheckedOptionalAccessTest,
 
     struct A {
       const $ns::$optional<int>& get() const { return x; }
-      
+
       $ns::$optional<int> x;
     };
 
     struct B {
       A& getA() { return a; }
-    
+
       A a;
     };
 
@@ -4031,23 +4031,23 @@ TEST_P(
     ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithAnotherConstCallAfterCheck) {
   ExpectDiagnosticsFor(R"cc(
       #include "unchecked_optional_access_test.h"
-  
+
       struct A {
         const $ns::$optional<int>& get() const { return x; }
-      
+
         $ns::$optional<int> x;
       };
-  
+
       struct B {
         const A& getA() const { return a; }
-  
+
         void callWithoutChanges() const { 
           // no-op 
         }
-  
+
         A a;
       };
-  
+
       void target(B& b) {  
         if (b.getA().get().has_value()) {
           b.callWithoutChanges(); // calling const method which cannot change A
@@ -4057,6 +4057,34 @@ TEST_P(
     )cc");
 }
 
+TEST_P(UncheckedOptionalAccessTest, ConstPointerRefAccessor) {
+  // A crash reproducer for https://github.com/llvm/llvm-project/issues/125589
+  // NOTE: we currently cache const ref accessors's locations.
+  // If we want to support const ref to pointers or bools, we should initialize
+  // their values.
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      $ns::$optional<int> x;
+    };
+
+    struct PtrWrapper {
+      A*& getPtrRef() const;
+      void reset(A*);
+    };
+
+    void target(PtrWrapper p) {
+      if (p.getPtrRef()->x) {
+        *p.getPtrRef()->x;  // [[unsafe]]
+        p.reset(nullptr);
+        *p.getPtrRef()->x;  // [[unsafe]]
+      }
+    }
+  )cc",
+                       /*IgnoreSmartPointerDereference=*/false);
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)

[jvoung]

Thanks!