Change checked arithmetic functions API to return Optional

[george.karpenkov]

Summary

Returning optional is much safer.
The previous API had potential to cause use of undefined variables, if the value passed by pointer was accidentally read afterwards.

[dblaikie]

llvm/trunk/unittests/Support/CheckedArithmeticTest.cpp

41 ↗

(On Diff #151212)

You might be able to write this and other non-None values as the bare integer, rather than explicitly wrapping it in Optional? (there should be an implicit conversion, I'd think) Or does something about the gtest framework get in the way of that?

[george.karpenkov]

llvm/trunk/unittests/Support/CheckedArithmeticTest.cpp

41 ↗

(On Diff #151212)

@dblaikie Hm I'm not sure whether it would work, EXPECT_EQ is not explicitly marked to expect a specific optional as a second parameter.
I wanted to have an explicit version here, but if implicit works as well I would not mind the change.

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

       return SymR;
     SubR = dyn_cast<SubRegion>(SubR->getSuperRegion());
   }
   return nullptr;
 }
 
 /// Perform a given operation on two integers, return whether it overflows.
 /// Optionally write the resulting output into \p Res.
-static bool checkedOp(
+static llvm::Optional<int64_t> checkedOp(
     int64_t LHS,
     int64_t RHS,
-    std::function<llvm::APInt(llvm::APInt *, const llvm::APInt &, bool &)> Op,
-    int64_t *Res = nullptr) {
+    std::function<llvm::APInt(llvm::APInt *, const llvm::APInt &, bool &)> Op) {
   llvm::APInt ALHS(/*BitSize=*/64, LHS, /*Signed=*/true);
   llvm::APInt ARHS(/*BitSize=*/64, RHS, /*Signed=*/true);
   bool Overflow;
   llvm::APInt Out = Op(&ALHS, ARHS, Overflow);
-  if (!Overflow && Res)
-    *Res = Out.getSExtValue();
-  return Overflow;
+  if (Overflow)
+    return None;
+  return Out.getSExtValue();
 }
 
-static bool checkedAdd(
+static llvm::Optional<int64_t> checkedAdd(
     int64_t LHS,
-    int64_t RHS,
-    int64_t *Res=nullptr) {
-  return checkedOp(LHS, RHS, &llvm::APInt::sadd_ov, Res);
+    int64_t RHS) {
+  return checkedOp(LHS, RHS, &llvm::APInt::sadd_ov);
 }
 
-static bool checkedMul(
+static llvm::Optional<int64_t> checkedMul(
     int64_t LHS,
-    int64_t RHS,
-    int64_t *Res=nullptr) {
-  return checkedOp(LHS, RHS, &llvm::APInt::smul_ov, Res);
+    int64_t RHS) {
+  return checkedOp(LHS, RHS, &llvm::APInt::smul_ov);
 }
 
 RegionRawOffset ElementRegion::getAsArrayOffset() const {
   CharUnits offset = CharUnits::Zero();
   const ElementRegion *ER = this;
   const MemRegion *superR = nullptr;
   ASTContext &C = getContext();
 

 
         // If we are pointing to an incomplete type, go no further.
         if (elemType->isIncompleteType()) {
           superR = ER;
           break;
         }
 
         CharUnits size = C.getTypeSizeInChars(elemType);
+        bool OffsetUpdated = false;
+        if (auto OffsetDiff = checkedMul(i, size.getQuantity())) {
+          if (auto NewOffset = checkedAdd(*OffsetDiff, offset.getQuantity())) {
+            offset = CharUnits::fromQuantity(*NewOffset);
+            OffsetUpdated = true;
+          }
+        }
 
-        int64_t Mult;
-        bool Overflow = checkedAdd(i, size.getQuantity(), &Mult);
-        if (!Overflow)
-          Overflow = checkedMul(Mult, offset.getQuantity());
-        if (Overflow) {
+        if (OffsetUpdated) {
           LLVM_DEBUG(llvm::dbgs() << "MemRegion::getAsArrayOffset: "
                                   << "offset overflowing, returning unknown\n");
 
           return nullptr;
         }
-
-        offset += (i * size);
       }
 
       // Go to the next ElementRegion (if any).
       ER = dyn_cast<ElementRegion>(superR);
       continue;
     }
 
     return nullptr;

llvm/include/llvm/Support/CheckedArithmetic.h

 // give the indication on whether the operation has overflown.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_CHECKEDARITHMETIC_H
 #define LLVM_SUPPORT_CHECKEDARITHMETIC_H
 
 #include "llvm/ADT/APInt.h"
+#include "llvm/ADT/Optional.h"
 
 #include <type_traits>
 
 namespace {
 
 /// Utility function to apply a given method of \c APInt \p F to \p LHS and
-/// \p RHS, and write the output into \p Res.
-/// \return Whether the operation overflows.
+/// \p RHS.
+/// \return Empty optional if the operation overflows, or result otherwise.
 template <typename T, typename F>
 typename std::enable_if<std::is_integral<T>::value && sizeof(T) * 8 <= 64,
-                        bool>::type
-checkedOp(T LHS, T RHS, F Op, T *Res = nullptr, bool Signed = true) {
+                        llvm::Optional<T>>::type
+checkedOp(T LHS, T RHS, F Op, bool Signed = true) {
   llvm::APInt ALHS(/*BitSize=*/sizeof(T) * 8, LHS, Signed);
   llvm::APInt ARHS(/*BitSize=*/sizeof(T) * 8, RHS, Signed);
   bool Overflow;
   llvm::APInt Out = (ALHS.*Op)(ARHS, Overflow);
-  if (Res)
-    *Res = Signed ? Out.getSExtValue() : Out.getZExtValue();
-  return Overflow;
+  if (Overflow)
+    return llvm::None;
+  return Signed ? Out.getSExtValue() : Out.getZExtValue();
 }
 }
 
 namespace llvm {
 
-/// Add two signed integers \p LHS and \p RHS, write into \p Res if non-null.
-/// Does not guarantee saturating arithmetic.
-/// \return Whether the result overflows.
+/// Add two signed integers \p LHS and \p RHS, return wrapped result if
+/// available.
 template <typename T>
-typename std::enable_if<std::is_signed<T>::value, bool>::type
-checkedAdd(T LHS, T RHS, T *Res = nullptr) {
-  return checkedOp(LHS, RHS, &llvm::APInt::sadd_ov, Res);
+typename std::enable_if<std::is_signed<T>::value, llvm::Optional<T>>::type
+checkedAdd(T LHS, T RHS) {
+  return checkedOp(LHS, RHS, &llvm::APInt::sadd_ov);
 }
 
-/// Multiply two signed integers \p LHS and \p RHS, write into \p Res if
-/// non-null.
-/// Does not guarantee saturating arithmetic.
-/// \return Whether the result overflows.
+/// Multiply two signed integers \p LHS and \p RHS, return wrapped result
+/// if available.
 template <typename T>
-typename std::enable_if<std::is_signed<T>::value, bool>::type
-checkedMul(T LHS, T RHS, T *Res = nullptr) {
-  return checkedOp(LHS, RHS, &llvm::APInt::smul_ov, Res);
+typename std::enable_if<std::is_signed<T>::value, llvm::Optional<T>>::type
+checkedMul(T LHS, T RHS) {
+  return checkedOp(LHS, RHS, &llvm::APInt::smul_ov);
 }
 
-/// Add two unsigned integers \p LHS and \p RHS, write into \p Res if non-null.
-/// Does not guarantee saturating arithmetic.
-/// \return Whether the result overflows.
+/// Add two unsigned integers \p LHS and \p RHS, return wrapped result
+/// if available.
 template <typename T>
-typename std::enable_if<std::is_unsigned<T>::value, bool>::type
-checkedAddUnsigned(T LHS, T RHS, T *Res = nullptr) {
-  return checkedOp(LHS, RHS, &llvm::APInt::uadd_ov, Res, /*Signed=*/false);
+typename std::enable_if<std::is_unsigned<T>::value, llvm::Optional<T>>::type
+checkedAddUnsigned(T LHS, T RHS) {
+  return checkedOp(LHS, RHS, &llvm::APInt::uadd_ov, /*Signed=*/false);
 }
 
-/// Multiply two unsigned integers \p LHS and \p RHS, write into \p Res if
-/// non-null.
-/// Does not guarantee saturating arithmetic.
-/// \return Whether the result overflows.
+/// Multiply two unsigned integers \p LHS and \p RHS, return wrapped result
+/// if available.
 template <typename T>
-typename std::enable_if<std::is_unsigned<T>::value, bool>::type
-checkedMulUnsigned(T LHS, T RHS, T *Res = nullptr) {
-  return checkedOp(LHS, RHS, &llvm::APInt::umul_ov, Res, /*Signed=*/false);
+typename std::enable_if<std::is_unsigned<T>::value, llvm::Optional<T>>::type
+checkedMulUnsigned(T LHS, T RHS) {
+  return checkedOp(LHS, RHS, &llvm::APInt::umul_ov, /*Signed=*/false);
 }
 
 } // End llvm namespace
 
 #endif

llvm/unittests/Support/CheckedArithmeticTest.cpp

 #include "llvm/Support/CheckedArithmetic.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
 TEST(CheckedArithmetic, CheckedAdd) {
-  int64_t Out;
   const int64_t Max = std::numeric_limits<int64_t>::max();
   const int64_t Min = std::numeric_limits<int64_t>::min();
-  EXPECT_EQ(checkedAdd<int64_t>(Max, Max, &Out), true);
-  EXPECT_EQ(checkedAdd<int64_t>(Min, -1, &Out), true);
-  EXPECT_EQ(checkedAdd<int64_t>(Max, 1, &Out), true);
-  EXPECT_EQ(checkedAdd<int64_t>(10, 1, &Out), false);
-  EXPECT_EQ(Out, 11);
+  EXPECT_EQ(checkedAdd<int64_t>(Max, Max), None);
+  EXPECT_EQ(checkedAdd<int64_t>(Min, -1), None);
+  EXPECT_EQ(checkedAdd<int64_t>(Max, 1), None);
+  EXPECT_EQ(checkedAdd<int64_t>(10, 1), Optional<int64_t>(11));
 }
 
 TEST(CheckedArithmetic, CheckedAddSmall) {
-  int16_t Out;
   const int16_t Max = std::numeric_limits<int16_t>::max();
   const int16_t Min = std::numeric_limits<int16_t>::min();
-  EXPECT_EQ(checkedAdd<int16_t>(Max, Max, &Out), true);
-  EXPECT_EQ(checkedAdd<int16_t>(Min, -1, &Out), true);
-  EXPECT_EQ(checkedAdd<int16_t>(Max, 1, &Out), true);
-  EXPECT_EQ(checkedAdd<int16_t>(10, 1, &Out), false);
-  EXPECT_EQ(Out, 11);
+  EXPECT_EQ(checkedAdd<int16_t>(Max, Max), None);
+  EXPECT_EQ(checkedAdd<int16_t>(Min, -1), None);
+  EXPECT_EQ(checkedAdd<int16_t>(Max, 1), None);
+  EXPECT_EQ(checkedAdd<int16_t>(10, 1), Optional<int64_t>(11));
 }
 
 TEST(CheckedArithmetic, CheckedMul) {
-  int64_t Out;
   const int64_t Max = std::numeric_limits<int64_t>::max();
   const int64_t Min = std::numeric_limits<int64_t>::min();
-  EXPECT_EQ(checkedMul<int64_t>(Max, 2, &Out), true);
-  EXPECT_EQ(checkedMul<int64_t>(Max, Max, &Out), true);
-  EXPECT_EQ(checkedMul<int64_t>(Min, 2, &Out), true);
-  EXPECT_EQ(checkedMul<int64_t>(10, 2, &Out), false);
-  EXPECT_EQ(Out, 20);
+  EXPECT_EQ(checkedMul<int64_t>(Max, 2), None);
+  EXPECT_EQ(checkedMul<int64_t>(Max, Max), None);
+  EXPECT_EQ(checkedMul<int64_t>(Min, 2), None);
+  EXPECT_EQ(checkedMul<int64_t>(10, 2), Optional<int64_t>(20));
 }
 
 TEST(CheckedArithmetic, CheckedMulSmall) {
-  int16_t Out;
   const int16_t Max = std::numeric_limits<int16_t>::max();
   const int16_t Min = std::numeric_limits<int16_t>::min();
-  EXPECT_EQ(checkedMul<int16_t>(Max, 2, &Out), true);
-  EXPECT_EQ(checkedMul<int16_t>(Max, Max, &Out), true);
-  EXPECT_EQ(checkedMul<int16_t>(Min, 2, &Out), true);
-  EXPECT_EQ(checkedMul<int16_t>(10, 2, &Out), false);
-  EXPECT_EQ(Out, 20);
+  EXPECT_EQ(checkedMul<int16_t>(Max, 2), None);
+  EXPECT_EQ(checkedMul<int16_t>(Max, Max), None);
+  EXPECT_EQ(checkedMul<int16_t>(Min, 2), None);
+  EXPECT_EQ(checkedMul<int16_t>(10, 2), Optional<int16_t>(20));
 }
 
 TEST(CheckedArithmetic, CheckedAddUnsigned) {
-  uint64_t Out;
   const uint64_t Max = std::numeric_limits<uint64_t>::max();
-  EXPECT_EQ(checkedAddUnsigned<uint64_t>(Max, Max, &Out), true);
-  EXPECT_EQ(checkedAddUnsigned<uint64_t>(Max, 1, &Out), true);
-  EXPECT_EQ(checkedAddUnsigned<uint64_t>(10, 1, &Out), false);
-  EXPECT_EQ(Out, uint64_t(11));
+  EXPECT_EQ(checkedAddUnsigned<uint64_t>(Max, Max), None);
+  EXPECT_EQ(checkedAddUnsigned<uint64_t>(Max, 1), None);
+  EXPECT_EQ(checkedAddUnsigned<uint64_t>(10, 1), Optional<uint64_t>(11));
 }
 
 TEST(CheckedArithmetic, CheckedMulUnsigned) {
-  uint64_t Out;
   const uint64_t Max = std::numeric_limits<uint64_t>::max();
-  EXPECT_EQ(checkedMulUnsigned<uint64_t>(Max, 2, &Out), true);
-  EXPECT_EQ(checkedMulUnsigned<uint64_t>(Max, Max, &Out), true);
-  EXPECT_EQ(checkedMulUnsigned<uint64_t>(10, 2, &Out), false);
-  EXPECT_EQ(Out, uint64_t(20));
+  EXPECT_EQ(checkedMulUnsigned<uint64_t>(Max, 2), llvm::None);
+  EXPECT_EQ(checkedMulUnsigned<uint64_t>(Max, Max), llvm::None);
+  EXPECT_EQ(checkedMulUnsigned<uint64_t>(10, 2), Optional<uint64_t>(20));
 }
 
 
 } // namespace