[analyzer] Fix crash on spaceship operator (PR47511)

[vsavchenko]

Summary

rdar://68954187

[steakhal]

I'm always in favor of bugfixes. However, I have some concerns about this one.

clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp

169–171

What a madness, we deal with every binary operator kind here O.O

Why do we get Undefined here? If we don't model something - theoretically - we should get Unknown.
I'm confused.

clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp

331

So, there are some corner cases already.
I can't see any way of implementing this reasonably here, as you pointed out the return value of the function is not a good fit.

Shouldn't put your mock 'implementation' to a different place?
I'm just nitpicking though.

[vsavchenko]

clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp

331

evalAPSInt is called in a lot of different contexts, I don't know how else to ensure that it won't crash on BO_Cmp in all of those contexts.

[steakhal]

clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp

331

I don't know. It doesn't feel like a strong argument. I still don't feel that this is the right direction.
The code already looks convoluted and unnecessarily complex, your change wouldn't worsen that too much.
Considering that it actually fixes something, eh, so be it.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp

169–171

Yes we should get Unknown. We should only return Undefined when there's undefined behavior in the program. Sounds like returning nullptr from BasicValueFactory causes SValBuilder to produce Undefined instead. I believe this needs to be fixed in SValBuilder instead.

[loic-joly-sonarsource]

clang/test/Analysis/PR47511.cpp

5

For such a simple test, there is no need to use the full-fledged header, the following would suffice:

namespace std { 
struct strong_ordering {
  int n;
  constexpr operator int() const { return n; }
  static const strong_ordering equal, greater, less;
};
constexpr strong_ordering strong_ordering::equal = { 0 };
constexpr strong_ordering strong_ordering::greater = { 1 };
constexpr strong_ordering strong_ordering::less = { -1 };
} // namespace std

[vsavchenko]

Change produced SVal for the spaceship operator from Undefined to Unknown

[NoQ]

Looks great now!

clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp

if (Ex) {
         const llvm::APSInt *RHS =
             SB.getKnownValue(State, C.getSVal(B->getRHS()));
         OS << "The result of the left shift is undefined due to shifting \'"
            << LHS->getSExtValue() << "\' by \'" << RHS->getZExtValue()
            << "\', which is unrepresentable in the unsigned version of "
            << "the return type \'" << B->getLHS()->getType().getAsString()
            << "\'";
         Ex = B->getLHS();
+
+      } else if (B->getOpcode() == BinaryOperatorKind::BO_Cmp) {
+        // We can't reason about C++20 spaceship operator yet.
+        return;
       } else {
         OS << "The result of the '"
            << BinaryOperator::getOpcodeStr(B->getOpcode())
            << "' expression is undefined";
       }
     }
     auto report = std::make_unique<PathSensitiveBugReport>(*BT, OS.str(), N);
     if (Ex) {

clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp

case BO_GE:
       return &getTruthValue( V1 >= V2 );
 
     case BO_EQ:
       return &getTruthValue( V1 == V2 );
 
     case BO_NE:
       return &getTruthValue( V1 != V2 );
 
       // Note: LAnd, LOr, Comma are handled specially by higher-level logic.
 
     case BO_And:
       return &getValue( V1 & V2 );
 
     case BO_Or:
       return &getValue( V1 | V2 );
 
     case BO_Xor:
       return &getValue( V1 ^ V2 );
+
+    case BO_Cmp:
+      // FIXME: Support C++20 spaceship operator.
+      //        The main problem here is that the result is not integer.
+      return nullptr;
   }
 }
 
 const std::pair<SVal, uintptr_t>&
 BasicValueFactory::getPersistentSValWithData(const SVal& V, uintptr_t Data) {
   // Lazily create the folding set.
   if (!PersistentSVals) PersistentSVals = new PersistentSValsTy();
 

clang/test/Analysis/Inputs/std-compare.h

+#ifndef STD_COMPARE_H
+#define STD_COMPARE_H
+
+namespace std {
+inline namespace __1 {
+
+// exposition only
+enum class _EqResult : unsigned char {
+  __equal = 0,
+  __equiv = __equal,
+};
+
+enum class _OrdResult : signed char {
+  __less = -1,
+  __greater = 1
+};
+
+enum class _NCmpResult : signed char {
+  __unordered = -127
+};
+
+struct _CmpUnspecifiedType;
+using _CmpUnspecifiedParam = void (_CmpUnspecifiedType::*)();
+
+class partial_ordering {
+  using _ValueT = signed char;
+  explicit constexpr partial_ordering(_EqResult __v) noexcept
+      : __value_(_ValueT(__v)) {}
+  explicit constexpr partial_ordering(_OrdResult __v) noexcept
+      : __value_(_ValueT(__v)) {}
+  explicit constexpr partial_ordering(_NCmpResult __v) noexcept
+      : __value_(_ValueT(__v)) {}
+
+  constexpr bool __is_ordered() const noexcept {
+    return __value_ != _ValueT(_NCmpResult::__unordered);
+  }
+
+public:
+  // valid values
+  static const partial_ordering less;
+  static const partial_ordering equivalent;
+  static const partial_ordering greater;
+  static const partial_ordering unordered;
+
+  // comparisons
+  friend constexpr bool operator==(partial_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator!=(partial_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator<(partial_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator<=(partial_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator>(partial_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator>=(partial_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator==(_CmpUnspecifiedParam, partial_ordering __v) noexcept;
+  friend constexpr bool operator!=(_CmpUnspecifiedParam, partial_ordering __v) noexcept;
+  friend constexpr bool operator<(_CmpUnspecifiedParam, partial_ordering __v) noexcept;
+  friend constexpr bool operator<=(_CmpUnspecifiedParam, partial_ordering __v) noexcept;
+  friend constexpr bool operator>(_CmpUnspecifiedParam, partial_ordering __v) noexcept;
+  friend constexpr bool operator>=(_CmpUnspecifiedParam, partial_ordering __v) noexcept;
+
+  friend constexpr partial_ordering operator<=>(partial_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr partial_ordering operator<=>(_CmpUnspecifiedParam, partial_ordering __v) noexcept;
+
+  // test helper
+  constexpr bool test_eq(partial_ordering const &other) const noexcept {
+    return __value_ == other.__value_;
+  }
+
+private:
+  _ValueT __value_;
+};
+
+inline constexpr partial_ordering partial_ordering::less(_OrdResult::__less);
+inline constexpr partial_ordering partial_ordering::equivalent(_EqResult::__equiv);
+inline constexpr partial_ordering partial_ordering::greater(_OrdResult::__greater);
+inline constexpr partial_ordering partial_ordering::unordered(_NCmpResult ::__unordered);
+constexpr bool operator==(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__is_ordered() && __v.__value_ == 0;
+}
+constexpr bool operator<(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__is_ordered() && __v.__value_ < 0;
+}
+constexpr bool operator<=(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__is_ordered() && __v.__value_ <= 0;
+}
+constexpr bool operator>(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__is_ordered() && __v.__value_ > 0;
+}
+constexpr bool operator>=(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__is_ordered() && __v.__value_ >= 0;
+}
+constexpr bool operator==(_CmpUnspecifiedParam, partial_ordering __v) noexcept {
+  return __v.__is_ordered() && 0 == __v.__value_;
+}
+constexpr bool operator<(_CmpUnspecifiedParam, partial_ordering __v) noexcept {
+  return __v.__is_ordered() && 0 < __v.__value_;
+}
+constexpr bool operator<=(_CmpUnspecifiedParam, partial_ordering __v) noexcept {
+  return __v.__is_ordered() && 0 <= __v.__value_;
+}
+constexpr bool operator>(_CmpUnspecifiedParam, partial_ordering __v) noexcept {
+  return __v.__is_ordered() && 0 > __v.__value_;
+}
+constexpr bool operator>=(_CmpUnspecifiedParam, partial_ordering __v) noexcept {
+  return __v.__is_ordered() && 0 >= __v.__value_;
+}
+constexpr bool operator!=(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return !__v.__is_ordered() || __v.__value_ != 0;
+}
+constexpr bool operator!=(_CmpUnspecifiedParam, partial_ordering __v) noexcept {
+  return !__v.__is_ordered() || __v.__value_ != 0;
+}
+
+constexpr partial_ordering operator<=>(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v;
+}
+constexpr partial_ordering operator<=>(_CmpUnspecifiedParam, partial_ordering __v) noexcept {
+  return __v < 0 ? partial_ordering::greater : (__v > 0 ? partial_ordering::less : __v);
+}
+
+class weak_ordering {
+  using _ValueT = signed char;
+  explicit constexpr weak_ordering(_EqResult __v) noexcept : __value_(_ValueT(__v)) {}
+  explicit constexpr weak_ordering(_OrdResult __v) noexcept : __value_(_ValueT(__v)) {}
+
+public:
+  static const weak_ordering less;
+  static const weak_ordering equivalent;
+  static const weak_ordering greater;
+
+  // conversions
+  constexpr operator partial_ordering() const noexcept {
+    return __value_ == 0 ? partial_ordering::equivalent
+                         : (__value_ < 0 ? partial_ordering::less : partial_ordering::greater);
+  }
+
+  // comparisons
+  friend constexpr bool operator==(weak_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator!=(weak_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator<(weak_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator<=(weak_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator>(weak_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator>=(weak_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator==(_CmpUnspecifiedParam, weak_ordering __v) noexcept;
+  friend constexpr bool operator!=(_CmpUnspecifiedParam, weak_ordering __v) noexcept;
+  friend constexpr bool operator<(_CmpUnspecifiedParam, weak_ordering __v) noexcept;
+  friend constexpr bool operator<=(_CmpUnspecifiedParam, weak_ordering __v) noexcept;
+  friend constexpr bool operator>(_CmpUnspecifiedParam, weak_ordering __v) noexcept;
+  friend constexpr bool operator>=(_CmpUnspecifiedParam, weak_ordering __v) noexcept;
+
+  friend constexpr weak_ordering operator<=>(weak_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr weak_ordering operator<=>(_CmpUnspecifiedParam, weak_ordering __v) noexcept;
+
+  // test helper
+  constexpr bool test_eq(weak_ordering const &other) const noexcept {
+    return __value_ == other.__value_;
+  }
+
+private:
+  _ValueT __value_;
+};
+
+inline constexpr weak_ordering weak_ordering::less(_OrdResult::__less);
+inline constexpr weak_ordering weak_ordering::equivalent(_EqResult::__equiv);
+inline constexpr weak_ordering weak_ordering::greater(_OrdResult::__greater);
+constexpr bool operator==(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ == 0;
+}
+constexpr bool operator!=(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ != 0;
+}
+constexpr bool operator<(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ < 0;
+}
+constexpr bool operator<=(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ <= 0;
+}
+constexpr bool operator>(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ > 0;
+}
+constexpr bool operator>=(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ >= 0;
+}
+constexpr bool operator==(_CmpUnspecifiedParam, weak_ordering __v) noexcept {
+  return 0 == __v.__value_;
+}
+constexpr bool operator!=(_CmpUnspecifiedParam, weak_ordering __v) noexcept {
+  return 0 != __v.__value_;
+}
+constexpr bool operator<(_CmpUnspecifiedParam, weak_ordering __v) noexcept {
+  return 0 < __v.__value_;
+}
+constexpr bool operator<=(_CmpUnspecifiedParam, weak_ordering __v) noexcept {
+  return 0 <= __v.__value_;
+}
+constexpr bool operator>(_CmpUnspecifiedParam, weak_ordering __v) noexcept {
+  return 0 > __v.__value_;
+}
+constexpr bool operator>=(_CmpUnspecifiedParam, weak_ordering __v) noexcept {
+  return 0 >= __v.__value_;
+}
+
+constexpr weak_ordering operator<=>(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v;
+}
+constexpr weak_ordering operator<=>(_CmpUnspecifiedParam, weak_ordering __v) noexcept {
+  return __v < 0 ? weak_ordering::greater : (__v > 0 ? weak_ordering::less : __v);
+}
+
+class strong_ordering {
+  using _ValueT = signed char;
+  explicit constexpr strong_ordering(_EqResult __v) noexcept : __value_(static_cast<signed char>(__v)) {}
+  explicit constexpr strong_ordering(_OrdResult __v) noexcept : __value_(static_cast<signed char>(__v)) {}
+
+public:
+  static const strong_ordering less;
+  static const strong_ordering equal;
+  static const strong_ordering equivalent;
+  static const strong_ordering greater;
+
+  // conversions
+  constexpr operator partial_ordering() const noexcept {
+    return __value_ == 0 ? partial_ordering::equivalent
+                         : (__value_ < 0 ? partial_ordering::less : partial_ordering::greater);
+  }
+  constexpr operator weak_ordering() const noexcept {
+    return __value_ == 0 ? weak_ordering::equivalent
+                         : (__value_ < 0 ? weak_ordering::less : weak_ordering::greater);
+  }
+
+  // comparisons
+  friend constexpr bool operator==(strong_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator!=(strong_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator<(strong_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator<=(strong_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator>(strong_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator>=(strong_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr bool operator==(_CmpUnspecifiedParam, strong_ordering __v) noexcept;
+  friend constexpr bool operator!=(_CmpUnspecifiedParam, strong_ordering __v) noexcept;
+  friend constexpr bool operator<(_CmpUnspecifiedParam, strong_ordering __v) noexcept;
+  friend constexpr bool operator<=(_CmpUnspecifiedParam, strong_ordering __v) noexcept;
+  friend constexpr bool operator>(_CmpUnspecifiedParam, strong_ordering __v) noexcept;
+  friend constexpr bool operator>=(_CmpUnspecifiedParam, strong_ordering __v) noexcept;
+
+  friend constexpr strong_ordering operator<=>(strong_ordering __v, _CmpUnspecifiedParam) noexcept;
+  friend constexpr strong_ordering operator<=>(_CmpUnspecifiedParam, strong_ordering __v) noexcept;
+
+  // test helper
+  constexpr bool test_eq(strong_ordering const &other) const noexcept {
+    return __value_ == other.__value_;
+  }
+
+private:
+  _ValueT __value_;
+};
+
+inline constexpr strong_ordering strong_ordering::less(_OrdResult::__less);
+inline constexpr strong_ordering strong_ordering::equal(_EqResult::__equal);
+inline constexpr strong_ordering strong_ordering::equivalent(_EqResult::__equiv);
+inline constexpr strong_ordering strong_ordering::greater(_OrdResult::__greater);
+
+constexpr bool operator==(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ == 0;
+}
+constexpr bool operator!=(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ != 0;
+}
+constexpr bool operator<(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ < 0;
+}
+constexpr bool operator<=(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ <= 0;
+}
+constexpr bool operator>(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ > 0;
+}
+constexpr bool operator>=(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v.__value_ >= 0;
+}
+constexpr bool operator==(_CmpUnspecifiedParam, strong_ordering __v) noexcept {
+  return 0 == __v.__value_;
+}
+constexpr bool operator!=(_CmpUnspecifiedParam, strong_ordering __v) noexcept {
+  return 0 != __v.__value_;
+}
+constexpr bool operator<(_CmpUnspecifiedParam, strong_ordering __v) noexcept {
+  return 0 < __v.__value_;
+}
+constexpr bool operator<=(_CmpUnspecifiedParam, strong_ordering __v) noexcept {
+  return 0 <= __v.__value_;
+}
+constexpr bool operator>(_CmpUnspecifiedParam, strong_ordering __v) noexcept {
+  return 0 > __v.__value_;
+}
+constexpr bool operator>=(_CmpUnspecifiedParam, strong_ordering __v) noexcept {
+  return 0 >= __v.__value_;
+}
+
+constexpr strong_ordering operator<=>(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
+  return __v;
+}
+constexpr strong_ordering operator<=>(_CmpUnspecifiedParam, strong_ordering __v) noexcept {
+  return __v < 0 ? strong_ordering::greater : (__v > 0 ? strong_ordering::less : __v);
+}
+
+} // namespace __1
+} // end namespace std
+
+#endif // STD_COMPARE_H

clang/test/Analysis/PR47511.cpp

+// RUN: %clang_analyze_cc1 -std=c++20 -w -analyzer-checker=core -verify %s
+
+// expected-no-diagnostics
+
+#include "Inputs/std-compare.h"
+
+void test() {
+  // no crash
+  (void)(0 <=> 0);
+}