Skip to content

Commit

Permalink
[analyzer][NFC] Introduce APSIntPtr, a safe wrapper of APSInt (1/4) (l…
Browse files Browse the repository at this point in the history 
…lvm#120435)

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.
  • Loading branch information
@steakhal
steakhal authored Dec 19, 2024
1 parent 056e5ec commit b41240b
Show file tree
Hide file tree
Showing 9 changed files with 148 additions and 90 deletions.
64 changes: 64 additions & 0 deletions clang/include/clang/StaticAnalyzer/Core/PathSensitive/APSIntPtr.h
View file
Original file line number Diff line number Diff line change
@@ -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
60 changes: 26 additions & 34 deletions clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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());
}

Expand Down Expand Up @@ -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);
Expand Down
1 change: 0 additions & 1 deletion clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
9 changes: 6 additions & 3 deletions clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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)) {
Expand All @@ -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)) {
Expand Down Expand Up @@ -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!");
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
14 changes: 7 additions & 7 deletions clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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}),
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Loading

0 comments on commit b41240b

Please sign in to comment.