[analyzer][NFC] Introduce APSIntPtr, a safe wrapper of APSInt (1/4) (#120435)

steakhal · web-flow · commit b41240be6b9e · 2024-12-19T12:04:04.000+01:00
One could create dangling APSInt references in various ways in the past, that were sometimes assumed to be persisted in the BasicValueFactor.

One should always use BasicValueFactory to create persistent APSInts, that could be used by ConcreteInts or SymIntExprs and similar long-living objects.
If one used a temporary or local variables for this, these would dangle.
To enforce the contract of the analyzer BasicValueFactory and the uses of APSInts, let's have a dedicated strong-type for this.

The idea is that APSIntPtr is always owned by the BasicValueFactory, and that is the only component that can construct it.

These PRs are all NFC - besides fixing dangling APSInt references.
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntPtr.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntPtr.h
@@ -0,0 +1,64 @@
+//== APSIntPtr.h - Wrapper for APSInt objects owned separately -*- C++ -*--==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_APSIntPtr_H
+#define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_APSIntPtr_H
+
+#include "llvm/ADT/APSInt.h"
+#include "llvm/Support/Compiler.h"
+
+namespace clang::ento {
+
+/// A safe wrapper around APSInt objects allocated and owned by
+/// \c BasicValueFactory. This just wraps a common llvm::APSInt.
+class APSIntPtr {
+  using APSInt = llvm::APSInt;
+
+public:
+  APSIntPtr() = delete;
+  APSIntPtr(const APSIntPtr &) = default;
+  APSIntPtr &operator=(const APSIntPtr &) & = default;
+  ~APSIntPtr() = default;
+
+  /// You should not use this API.
+  /// If do, ensure that the \p Ptr not going to dangle.
+  /// Prefer using \c BasicValueFactory::getValue() to get an APSIntPtr object.
+  static APSIntPtr unsafeConstructor(const APSInt *Ptr) {
+    return APSIntPtr(Ptr);
+  }
+
+  LLVM_ATTRIBUTE_RETURNS_NONNULL
+  const APSInt *get() const { return Ptr; }
+  /*implicit*/ operator const APSInt &() const { return *get(); }
+
+  APSInt operator-() const { return -*Ptr; }
+  APSInt operator~() const { return ~*Ptr; }
+
+#define DEFINE_OPERATOR(OP)                                                    \
+  bool operator OP(APSIntPtr Other) const { return (*Ptr)OP(*Other.Ptr); }
+  DEFINE_OPERATOR(>)
+  DEFINE_OPERATOR(>=)
+  DEFINE_OPERATOR(<)
+  DEFINE_OPERATOR(<=)
+  DEFINE_OPERATOR(==)
+  DEFINE_OPERATOR(!=)
+#undef DEFINE_OPERATOR
+
+  const APSInt &operator*() const { return *Ptr; }
+  const APSInt *operator->() const { return Ptr; }
+
+private:
+  explicit APSIntPtr(const APSInt *Ptr) : Ptr(Ptr) {}
+
+  /// Owned by \c BasicValueFactory.
+  const APSInt *Ptr;
+};
+
+} // namespace clang::ento
+
+#endif // LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_APSIntPtr_H
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -18,10 +18,11 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/Type.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/APSIntPtr.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/StoreRef.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/ImmutableList.h"
@@ -129,7 +130,7 @@ class BasicValueFactory {
 
   // This is private because external clients should use the factory
   // method that takes a QualType.
-  const llvm::APSInt& getValue(uint64_t X, unsigned BitWidth, bool isUnsigned);
+  APSIntPtr getValue(uint64_t X, unsigned BitWidth, bool isUnsigned);
 
 public:
   BasicValueFactory(ASTContext &ctx, llvm::BumpPtrAllocator &Alloc)
@@ -140,9 +141,9 @@ class BasicValueFactory {
 
   ASTContext &getContext() const { return Ctx; }
 
-  const llvm::APSInt& getValue(const llvm::APSInt& X);
-  const llvm::APSInt& getValue(const llvm::APInt& X, bool isUnsigned);
-  const llvm::APSInt& getValue(uint64_t X, QualType T);
+  APSIntPtr getValue(const llvm::APSInt &X);
+  APSIntPtr getValue(const llvm::APInt &X, bool isUnsigned);
+  APSIntPtr getValue(uint64_t X, QualType T);
 
   /// Returns the type of the APSInt used to store values of the given QualType.
   APSIntType getAPSIntType(QualType T) const {
@@ -165,79 +166,70 @@ class BasicValueFactory {
 
   /// Convert - Create a new persistent APSInt with the same value as 'From'
   ///  but with the bitwidth and signedness of 'To'.
-  const llvm::APSInt &Convert(const llvm::APSInt& To,
-                              const llvm::APSInt& From) {
+  APSIntPtr Convert(const llvm::APSInt &To, const llvm::APSInt &From) {
     APSIntType TargetType(To);
     if (TargetType == APSIntType(From))
-      return From;
+      return getValue(From);
 
     return getValue(TargetType.convert(From));
   }
 
-  const llvm::APSInt &Convert(QualType T, const llvm::APSInt &From) {
+  APSIntPtr Convert(QualType T, const llvm::APSInt &From) {
     APSIntType TargetType = getAPSIntType(T);
     return Convert(TargetType, From);
   }
 
-  const llvm::APSInt &Convert(APSIntType TargetType, const llvm::APSInt &From) {
+  APSIntPtr Convert(APSIntType TargetType, const llvm::APSInt &From) {
     if (TargetType == APSIntType(From))
-      return From;
+      return getValue(From);
 
     return getValue(TargetType.convert(From));
   }
 
-  const llvm::APSInt &getIntValue(uint64_t X, bool isUnsigned) {
+  APSIntPtr getIntValue(uint64_t X, bool isUnsigned) {
     QualType T = isUnsigned ? Ctx.UnsignedIntTy : Ctx.IntTy;
     return getValue(X, T);
   }
 
-  const llvm::APSInt &getMaxValue(const llvm::APSInt &v) {
+  APSIntPtr getMaxValue(const llvm::APSInt &v) {
     return getValue(APSIntType(v).getMaxValue());
   }
 
-  const llvm::APSInt &getMinValue(const llvm::APSInt &v) {
+  APSIntPtr getMinValue(const llvm::APSInt &v) {
     return getValue(APSIntType(v).getMinValue());
   }
 
-  const llvm::APSInt &getMaxValue(QualType T) {
-    return getMaxValue(getAPSIntType(T));
-  }
+  APSIntPtr getMaxValue(QualType T) { return getMaxValue(getAPSIntType(T)); }
 
-  const llvm::APSInt &getMinValue(QualType T) {
-    return getMinValue(getAPSIntType(T));
-  }
+  APSIntPtr getMinValue(QualType T) { return getMinValue(getAPSIntType(T)); }
 
-  const llvm::APSInt &getMaxValue(APSIntType T) {
-    return getValue(T.getMaxValue());
-  }
+  APSIntPtr getMaxValue(APSIntType T) { return getValue(T.getMaxValue()); }
 
-  const llvm::APSInt &getMinValue(APSIntType T) {
-    return getValue(T.getMinValue());
-  }
+  APSIntPtr getMinValue(APSIntType T) { return getValue(T.getMinValue()); }
 
-  const llvm::APSInt &Add1(const llvm::APSInt &V) {
+  APSIntPtr Add1(const llvm::APSInt &V) {
     llvm::APSInt X = V;
     ++X;
     return getValue(X);
   }
 
-  const llvm::APSInt &Sub1(const llvm::APSInt &V) {
+  APSIntPtr Sub1(const llvm::APSInt &V) {
     llvm::APSInt X = V;
     --X;
     return getValue(X);
   }
 
-  const llvm::APSInt &getZeroWithTypeSize(QualType T) {
+  APSIntPtr getZeroWithTypeSize(QualType T) {
     assert(T->isScalarType());
     return getValue(0, Ctx.getTypeSize(T), true);
   }
 
-  const llvm::APSInt &getTruthValue(bool b, QualType T) {
+  APSIntPtr getTruthValue(bool b, QualType T) {
     return getValue(b ? 1 : 0, Ctx.getIntWidth(T),
                     T->isUnsignedIntegerOrEnumerationType());
   }
 
-  const llvm::APSInt &getTruthValue(bool b) {
+  APSIntPtr getTruthValue(bool b) {
     return getTruthValue(b, Ctx.getLogicalOperationType());
   }
 
@@ -273,9 +265,9 @@ class BasicValueFactory {
   accumCXXBase(llvm::iterator_range<CastExpr::path_const_iterator> PathRange,
                const nonloc::PointerToMember &PTM, const clang::CastKind &kind);
 
-  const llvm::APSInt* evalAPSInt(BinaryOperator::Opcode Op,
-                                     const llvm::APSInt& V1,
-                                     const llvm::APSInt& V2);
+  std::optional<APSIntPtr> evalAPSInt(BinaryOperator::Opcode Op,
+                                      const llvm::APSInt &V1,
+                                      const llvm::APSInt &V2);
 
   const std::pair<SVal, uintptr_t>&
   getPersistentSValWithData(const SVal& V, uintptr_t Data);
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -70,7 +70,6 @@ template <typename T> struct ProgramStateTrait {
 ///  values will never change.
 class ProgramState : public llvm::FoldingSetNode {
 public:
-  typedef llvm::ImmutableSet<llvm::APSInt*>                IntSetTy;
   typedef llvm::ImmutableMap<void*, void*>                 GenericDataMap;
 
 private:
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
@@ -16,6 +16,7 @@
 
 #include "clang/Basic/JsonSupport.h"
 #include "clang/Basic/TargetInfo.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h"
 #include <optional>
@@ -154,7 +155,7 @@ class SMTConstraintManager : public clang::ento::SimpleConstraintManager {
         return nullptr;
 
       // This is the only solution, store it
-      return &BVF.getValue(Value);
+      return BVF.getValue(Value).get();
     }
 
     if (const SymbolCast *SC = dyn_cast<SymbolCast>(Sym)) {
@@ -167,7 +168,7 @@ class SMTConstraintManager : public clang::ento::SimpleConstraintManager {
       const llvm::APSInt *Value;
       if (!(Value = getSymVal(State, CastSym)))
         return nullptr;
-      return &BVF.Convert(SC->getType(), *Value);
+      return BVF.Convert(SC->getType(), *Value).get();
     }
 
     if (const BinarySymExpr *BSE = dyn_cast<BinarySymExpr>(Sym)) {
@@ -195,7 +196,9 @@ class SMTConstraintManager : public clang::ento::SimpleConstraintManager {
       std::tie(ConvertedRHS, RTy) = SMTConv::fixAPSInt(Ctx, *RHS);
       SMTConv::doIntTypeConversion<llvm::APSInt, &SMTConv::castAPSInt>(
           Solver, Ctx, ConvertedLHS, LTy, ConvertedRHS, RTy);
-      return BVF.evalAPSInt(BSE->getOpcode(), ConvertedLHS, ConvertedRHS);
+      std::optional<APSIntPtr> Res =
+          BVF.evalAPSInt(BSE->getOpcode(), ConvertedLHS, ConvertedRHS);
+      return Res ? Res.value().get() : nullptr;
     }
 
     llvm_unreachable("Unsupported expression to get symbol value!");
diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1025,8 +1025,8 @@ SVal CStringChecker::getCStringLengthForRegion(CheckerContext &C,
       BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
       const llvm::APSInt &maxValInt = BVF.getMaxValue(sizeTy);
       llvm::APSInt fourInt = APSIntType(maxValInt).getValue(4);
-      const llvm::APSInt *maxLengthInt = BVF.evalAPSInt(BO_Div, maxValInt,
-                                                        fourInt);
+      std::optional<APSIntPtr> maxLengthInt =
+          BVF.evalAPSInt(BO_Div, maxValInt, fourInt);
       NonLoc maxLength = svalBuilder.makeIntVal(*maxLengthInt);
       SVal evalLength = svalBuilder.evalBinOpNN(state, BO_LE, *strLn, maxLength,
                                                 svalBuilder.getConditionType());
diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1643,7 +1643,7 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   public:
     GetMaxValue(BasicValueFactory &BVF) : BVF(BVF) {}
     std::optional<RangeInt> operator()(QualType Ty) {
-      return BVF.getMaxValue(Ty).getLimitedValue();
+      return BVF.getMaxValue(Ty)->getLimitedValue();
     }
     std::optional<RangeInt> operator()(std::optional<QualType> Ty) {
       if (Ty) {
@@ -1687,11 +1687,11 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   const QualType SizePtrTy = getPointerTy(SizeTy);
   const QualType SizePtrRestrictTy = getRestrictTy(SizePtrTy);
 
-  const RangeInt IntMax = BVF.getMaxValue(IntTy).getLimitedValue();
+  const RangeInt IntMax = BVF.getMaxValue(IntTy)->getLimitedValue();
   const RangeInt UnsignedIntMax =
-      BVF.getMaxValue(UnsignedIntTy).getLimitedValue();
-  const RangeInt LongMax = BVF.getMaxValue(LongTy).getLimitedValue();
-  const RangeInt SizeMax = BVF.getMaxValue(SizeTy).getLimitedValue();
+      BVF.getMaxValue(UnsignedIntTy)->getLimitedValue();
+  const RangeInt LongMax = BVF.getMaxValue(LongTy)->getLimitedValue();
+  const RangeInt SizeMax = BVF.getMaxValue(SizeTy)->getLimitedValue();
 
   // Set UCharRangeMax to min of int or uchar maximum value.
   // The C standard states that the arguments of functions like isalpha must
@@ -1700,7 +1700,7 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   // to be true for commonly used and well tested instruction set
   // architectures, but not for others.
   const RangeInt UCharRangeMax =
-      std::min(BVF.getMaxValue(ACtx.UnsignedCharTy).getLimitedValue(), IntMax);
+      std::min(BVF.getMaxValue(ACtx.UnsignedCharTy)->getLimitedValue(), IntMax);
 
   // Get platform dependent values of some macros.
   // Try our best to parse this from the Preprocessor, otherwise fallback to a
@@ -3704,7 +3704,7 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
 
   // Functions for testing.
   if (AddTestFunctions) {
-    const RangeInt IntMin = BVF.getMinValue(IntTy).getLimitedValue();
+    const RangeInt IntMin = BVF.getMinValue(IntTy)->getLimitedValue();
 
     addToFunctionSummaryMap(
         "__not_null", Signature(ArgTypes{IntPtrTy}, RetType{IntTy}),
diff --git a/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -96,7 +96,7 @@ ProgramStateRef VLASizeChecker::checkVLA(CheckerContext &C,
   SValBuilder &SVB = C.getSValBuilder();
   CanQualType SizeTy = Ctx.getSizeType();
   uint64_t SizeMax =
-      SVB.getBasicValueFactory().getMaxValue(SizeTy).getZExtValue();
+      SVB.getBasicValueFactory().getMaxValue(SizeTy)->getZExtValue();
 
   // Get the element size.
   CharUnits EleSize = Ctx.getTypeSizeInChars(VLALast->getElementType());
diff --git a/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp b/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
diff --git a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
