[clang] 2ddd57a - [clang][dataflow] Model the behavior of optional and std swap

[Stanislav Gatev via cfe-commits]

Author: Stanislav Gatev
Date: 2022-03-22T08:35:34Z
New Revision: 2ddd57ae1ec42c4aad8e70645cff82c877a94e3f

URL: https://github.com/llvm/llvm-project/commit/2ddd57ae1ec42c4aad8e70645cff82c877a94e3f
DIFF: https://github.com/llvm/llvm-project/commit/2ddd57ae1ec42c4aad8e70645cff82c877a94e3f.diff

LOG: [clang][dataflow] Model the behavior of optional and std swap

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

Reviewed-by: ymandel, xazax.hun

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 4d0b10435a094..bf325b04967c2 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -107,6 +107,12 @@ auto isOptionalNulloptAssignment() {
                              hasArgument(1, hasNulloptType()));
 }
 
+auto isStdSwapCall() {
+  return callExpr(callee(functionDecl(hasName("std::swap"))),
+                  argumentCountIs(2), hasArgument(0, hasOptionalType()),
+                  hasArgument(1, hasOptionalType()));
+}
+
 /// Creates a symbolic value for an `optional` value using `HasValueVal` as the
 /// symbolic value of its "has_value" property.
 StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) {
@@ -283,6 +289,50 @@ void transferNulloptAssignment(const CXXOperatorCallExpr *E,
   transferAssignment(E, State.Env.getBoolLiteralValue(false), State);
 }
 
+void transferSwap(const StorageLocation &OptionalLoc1,
+                  const StorageLocation &OptionalLoc2,
+                  LatticeTransferState &State) {
+  auto *OptionalVal1 = State.Env.getValue(OptionalLoc1);
+  assert(OptionalVal1 != nullptr);
+
+  auto *OptionalVal2 = State.Env.getValue(OptionalLoc2);
+  assert(OptionalVal2 != nullptr);
+
+  State.Env.setValue(OptionalLoc1, *OptionalVal2);
+  State.Env.setValue(OptionalLoc2, *OptionalVal1);
+}
+
+void transferSwapCall(const CXXMemberCallExpr *E,
+                      const MatchFinder::MatchResult &,
+                      LatticeTransferState &State) {
+  assert(E->getNumArgs() == 1);
+
+  auto *OptionalLoc1 = State.Env.getStorageLocation(
+      *E->getImplicitObjectArgument(), SkipPast::ReferenceThenPointer);
+  assert(OptionalLoc1 != nullptr);
+
+  auto *OptionalLoc2 =
+      State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference);
+  assert(OptionalLoc2 != nullptr);
+
+  transferSwap(*OptionalLoc1, *OptionalLoc2, State);
+}
+
+void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &,
+                         LatticeTransferState &State) {
+  assert(E->getNumArgs() == 2);
+
+  auto *OptionalLoc1 =
+      State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference);
+  assert(OptionalLoc1 != nullptr);
+
+  auto *OptionalLoc2 =
+      State.Env.getStorageLocation(*E->getArg(1), SkipPast::Reference);
+  assert(OptionalLoc2 != nullptr);
+
+  transferSwap(*OptionalLoc1, *OptionalLoc2, State);
+}
+
 auto buildTransferMatchSwitch() {
   // FIXME: Evaluate the efficiency of matchers. If using matchers results in a
   // lot of duplicated work (e.g. string comparisons), consider providing APIs
@@ -361,6 +411,13 @@ auto buildTransferMatchSwitch() {
                                 State.Env.getBoolLiteralValue(false));
           })
 
+      // optional::swap
+      .CaseOf<CXXMemberCallExpr>(isOptionalMemberCallWithName("swap"),
+                                 transferSwapCall)
+
+      // std::swap
+      .CaseOf<CallExpr>(isStdSwapCall(), transferStdSwapCall)
+
       .Build();
 }
 

diff  --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index fbc17668447ff..76340aad5fd4d 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -507,6 +507,9 @@ namespace std {
 template <typename T>
 constexpr remove_reference_t<T>&& move(T&& x);
 
+template <typename T>
+void swap(T& a, T& b) noexcept;
+
 } // namespace std
 
 #endif // UTILITY_H
@@ -718,6 +721,8 @@ class optional : private __optional_storage_base<_Tp> {
 
   constexpr explicit operator bool() const noexcept;
   using __base::has_value;
+
+  constexpr void swap(optional& __opt) noexcept;
 };
 
 template <typename T>
@@ -938,6 +943,8 @@ class optional {
 
   constexpr explicit operator bool() const noexcept;
   constexpr bool has_value() const noexcept;
+
+  void swap(optional& rhs) noexcept;
 };
 
 template <typename T>
@@ -1129,6 +1136,8 @@ class Optional {
 
   constexpr explicit operator bool() const noexcept;
   constexpr bool has_value() const noexcept;
+
+  void swap(Optional& other);
 };
 
 template <typename T>
@@ -1911,10 +1920,93 @@ TEST_P(UncheckedOptionalAccessTest, NulloptAssignment) {
       UnorderedElementsAre(Pair("check", "unsafe: input.cc:6:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, OptionalSwap) {
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    void target() {
+      $ns::$optional<int> opt1 = $ns::nullopt;
+      $ns::$optional<int> opt2 = 3;
+
+      opt1.swap(opt2);
+
+      opt1.value();
+      /*[[check-1]]*/
+
+      opt2.value();
+      /*[[check-2]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check-1", "safe"),
+                           Pair("check-2", "unsafe: input.cc:13:7")));
+
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    void target() {
+      $ns::$optional<int> opt1 = $ns::nullopt;
+      $ns::$optional<int> opt2 = 3;
+
+      opt2.swap(opt1);
+
+      opt1.value();
+      /*[[check-3]]*/
+
+      opt2.value();
+      /*[[check-4]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check-3", "safe"),
+                           Pair("check-4", "unsafe: input.cc:13:7")));
+}
+
+TEST_P(UncheckedOptionalAccessTest, StdSwap) {
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    void target() {
+      $ns::$optional<int> opt1 = $ns::nullopt;
+      $ns::$optional<int> opt2 = 3;
+
+      std::swap(opt1, opt2);
+
+      opt1.value();
+      /*[[check-1]]*/
+
+      opt2.value();
+      /*[[check-2]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check-1", "safe"),
+                           Pair("check-2", "unsafe: input.cc:13:7")));
+
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    void target() {
+      $ns::$optional<int> opt1 = $ns::nullopt;
+      $ns::$optional<int> opt2 = 3;
+
+      std::swap(opt2, opt1);
+
+      opt1.value();
+      /*[[check-3]]*/
+
+      opt2.value();
+      /*[[check-4]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check-3", "safe"),
+                           Pair("check-4", "unsafe: input.cc:13:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
-// - swap
 // - invalidation (passing optional by non-const reference/pointer)
 // - `value_or(nullptr) != nullptr`, `value_or(0) != 0`, `value_or("").empty()`
 // - nested `optional` values