[clang] b88023c - [analyzer][NFC] Use std::optional instead of custom "empty" state
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Thu May 4 03:58:31 PDT 2023
Author: Donát Nagy
Date: 2023-05-04T12:56:15+02:00
New Revision: b88023c25729f4dd4548a25e5e12d6624e2adbaf
URL: https://github.com/llvm/llvm-project/commit/b88023c25729f4dd4548a25e5e12d6624e2adbaf
DIFF: https://github.com/llvm/llvm-project/commit/b88023c25729f4dd4548a25e5e12d6624e2adbaf.diff
LOG: [analyzer][NFC] Use std::optional instead of custom "empty" state
This commit eliminates the uninitialized error state from the class
RegionRawOffsetV2 (which is locally used by the Clang Static Analyzer
checker alpha.security.ArrayBoundV2) and replaces its use with
std::optional.
Motivated by https://reviews.llvm.org/D148355#inline-1437928
Moreover, the code of RegionRawOffsetV2::computeOffset() is rearranged
to clarify its behavior. The helper function getValue() was eliminated
by picking a better initial value for the variable Offset; two other
helper functions were replaced by the lambda function Calc() because
this way it doesn't need to take the "context" objects as parameters.
This reorganization revealed some surprising (but not outright buggy)
behavior that's marked by a FIXME and will be revisited in a separate
commit.
Differential Revision: https://reviews.llvm.org/D149259
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
index 6d0cc505756b8..269277aaf357f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -53,21 +53,17 @@ class ArrayBoundCheckerV2 :
class RegionRawOffsetV2 {
private:
const SubRegion *baseRegion;
- SVal byteOffset;
-
- RegionRawOffsetV2()
- : baseRegion(nullptr), byteOffset(UnknownVal()) {}
+ NonLoc byteOffset;
public:
RegionRawOffsetV2(const SubRegion *base, NonLoc offset)
: baseRegion(base), byteOffset(offset) { assert(base); }
- NonLoc getByteOffset() const { return byteOffset.castAs<NonLoc>(); }
+ NonLoc getByteOffset() const { return byteOffset; }
const SubRegion *getRegion() const { return baseRegion; }
- static RegionRawOffsetV2 computeOffset(ProgramStateRef state,
- SValBuilder &svalBuilder,
- SVal location);
+ static std::optional<RegionRawOffsetV2>
+ computeOffset(ProgramStateRef State, SValBuilder &SVB, SVal Location);
void dump() const;
void dumpToStream(raw_ostream &os) const;
@@ -169,16 +165,16 @@ void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad,
ProgramStateRef state = checkerContext.getState();
SValBuilder &svalBuilder = checkerContext.getSValBuilder();
- const RegionRawOffsetV2 &rawOffset =
- RegionRawOffsetV2::computeOffset(state, svalBuilder, location);
+ const std::optional<RegionRawOffsetV2> &RawOffset =
+ RegionRawOffsetV2::computeOffset(state, svalBuilder, location);
- if (!rawOffset.getRegion())
+ if (!RawOffset)
return;
- NonLoc ByteOffset = rawOffset.getByteOffset();
+ NonLoc ByteOffset = RawOffset->getByteOffset();
// CHECK LOWER BOUND
- const MemSpaceRegion *SR = rawOffset.getRegion()->getMemorySpace();
+ const MemSpaceRegion *SR = RawOffset->getRegion()->getMemorySpace();
if (!llvm::isa<UnknownSpaceRegion>(SR)) {
// A pointer to UnknownSpaceRegion may point to the middle of
// an allocated region.
@@ -199,7 +195,7 @@ void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad,
// CHECK UPPER BOUND
DefinedOrUnknownSVal Size =
- getDynamicExtent(state, rawOffset.getRegion(), svalBuilder);
+ getDynamicExtent(state, RawOffset->getRegion(), svalBuilder);
if (auto KnownSize = Size.getAs<NonLoc>()) {
auto [state_withinUpperBound, state_exceedsUpperBound] =
compareValueToThreshold(state, ByteOffset, *KnownSize, svalBuilder);
@@ -307,84 +303,55 @@ void RegionRawOffsetV2::dumpToStream(raw_ostream &os) const {
}
#endif
-// Lazily computes a value to be used by 'computeOffset'. If 'val'
-// is unknown or undefined, we lazily substitute '0'. Otherwise,
-// return 'val'.
-static inline SVal getValue(SVal val, SValBuilder &svalBuilder) {
- return val.isUndef() ? svalBuilder.makeZeroArrayIndex() : val;
-}
-
-// Scale a base value by a scaling factor, and return the scaled
-// value as an SVal. Used by 'computeOffset'.
-static inline SVal scaleValue(ProgramStateRef state,
- NonLoc baseVal, CharUnits scaling,
- SValBuilder &sb) {
- return sb.evalBinOpNN(state, BO_Mul, baseVal,
- sb.makeArrayIndex(scaling.getQuantity()),
- sb.getArrayIndexType());
-}
-
-// Add an SVal to another, treating unknown and undefined values as
-// summing to UnknownVal. Used by 'computeOffset'.
-static SVal addValue(ProgramStateRef state, SVal x, SVal y,
- SValBuilder &svalBuilder) {
- // We treat UnknownVals and UndefinedVals the same here because we
- // only care about computing offsets.
- if (x.isUnknownOrUndef() || y.isUnknownOrUndef())
- return UnknownVal();
-
- return svalBuilder.evalBinOpNN(state, BO_Add, x.castAs<NonLoc>(),
- y.castAs<NonLoc>(),
- svalBuilder.getArrayIndexType());
-}
-
-/// Compute a raw byte offset from a base region. Used for array bounds
-/// checking.
-RegionRawOffsetV2 RegionRawOffsetV2::computeOffset(ProgramStateRef state,
- SValBuilder &svalBuilder,
- SVal location)
-{
- const MemRegion *region = location.getAsRegion();
- SVal offset = UndefinedVal();
-
- while (region) {
- switch (region->getKind()) {
- default: {
- if (const SubRegion *subReg = dyn_cast<SubRegion>(region)) {
- if (auto Offset = getValue(offset, svalBuilder).getAs<NonLoc>())
- return RegionRawOffsetV2(subReg, *Offset);
- }
- return RegionRawOffsetV2();
- }
- case MemRegion::ElementRegionKind: {
- const ElementRegion *elemReg = cast<ElementRegion>(region);
- SVal index = elemReg->getIndex();
- if (!isa<NonLoc>(index))
- return RegionRawOffsetV2();
- QualType elemType = elemReg->getElementType();
+/// For a given Location that can be represented as a symbolic expression
+/// Arr[Idx] (or perhaps Arr[Idx1][Idx2] etc.), return the parent memory block
+/// Arr and the distance of Location from the beginning of Arr (expressed in a
+/// NonLoc that specifies the number of CharUnits). Returns nullopt when these
+/// cannot be determined.
+std::optional<RegionRawOffsetV2>
+RegionRawOffsetV2::computeOffset(ProgramStateRef State, SValBuilder &SVB,
+ SVal Location) {
+ QualType T = SVB.getArrayIndexType();
+ auto Calc = [&SVB, State, T](BinaryOperatorKind Op, NonLoc LHS, NonLoc RHS) {
+ // We will use this utility to add and multiply values.
+ return SVB.evalBinOpNN(State, Op, LHS, RHS, T).getAs<NonLoc>();
+ };
+
+ const MemRegion *Region = Location.getAsRegion();
+ NonLoc Offset = SVB.makeZeroArrayIndex();
+
+ while (Region) {
+ if (const auto *ERegion = dyn_cast<ElementRegion>(Region)) {
+ if (const auto Index = ERegion->getIndex().getAs<NonLoc>()) {
+ QualType ElemType = ERegion->getElementType();
// If the element is an incomplete type, go no further.
- ASTContext &astContext = svalBuilder.getContext();
- if (elemType->isIncompleteType())
- return RegionRawOffsetV2();
-
- // Update the offset.
- offset = addValue(state,
- getValue(offset, svalBuilder),
- scaleValue(state,
- index.castAs<NonLoc>(),
- astContext.getTypeSizeInChars(elemType),
- svalBuilder),
- svalBuilder);
-
- if (offset.isUnknownOrUndef())
- return RegionRawOffsetV2();
-
- region = elemReg->getSuperRegion();
- continue;
+ if (ElemType->isIncompleteType())
+ return std::nullopt;
+
+ // Perform Offset += Index * sizeof(ElemType); then continue the offset
+ // calculations with SuperRegion:
+ NonLoc Size = SVB.makeArrayIndex(
+ SVB.getContext().getTypeSizeInChars(ElemType).getQuantity());
+ if (auto Delta = Calc(BO_Mul, *Index, Size)) {
+ if (auto NewOffset = Calc(BO_Add, Offset, *Delta)) {
+ Offset = *NewOffset;
+ Region = ERegion->getSuperRegion();
+ continue;
+ }
+ }
}
+ } else if (const auto *SRegion = dyn_cast<SubRegion>(Region)) {
+ // NOTE: The dyn_cast<>() is expected to succeed, it'd be very surprising
+ // to see a MemSpaceRegion at this point.
+ // FIXME: We may return with {<Region>, 0} even if we didn't handle any
+ // ElementRegion layers. I think that this behavior was introduced
+ // accidentally by 8a4c760c204546aba566e302f299f7ed2e00e287 in 2011, so
+ // it may be useful to review it in the future.
+ return RegionRawOffsetV2(SRegion, Offset);
}
+ return std::nullopt;
}
- return RegionRawOffsetV2();
+ return std::nullopt;
}
void ento::registerArrayBoundCheckerV2(CheckerManager &mgr) {
More information about the cfe-commits
mailing list