[analyzer] Simplify functions SVal::getAsSymbolicExpression and similar ones

[ASDenysPetrov]

Summary

Simplify functions SVal::getAsSymbolicExpression, SVal::getAsSymExpr and SVal::getAsSymbol. After revision I concluded that getAsSymbolicExpression and getAsSymExpr repeat functionality of getAsSymbol, thus they can be removed.

Fix: Remove functions SVal::getAsSymbolicExpression and SVal::getAsSymExpr.

[vsavchenko]

Thanks for working on improving the quality of the codebase!
I again have to nitpick about the commit message, can you please change it to "Simplify ..."?

clang/lib/StaticAnalyzer/Core/SVals.cpp

135–149

Oof, it's a mess!

[steakhal]

It would be great to simplify these. I have also wondered once why those different functions exist.
Does anyone know what was the original intention of these functions?
Since I'm not confident in this area, I resign.

[NoQ]

*standing ovation*

This was long overdue, thank you!

[ASDenysPetrov]

@vsavchenko

I again have to nitpick about the commit message, can you please change it to "Simplify ..."?

Ready! Sorry, I remember, you've already told me.

Thanks all for review!

[vsavchenko]

Ready! Sorry, I remember, you've already told me.

Nothing to be sorry about! It's really not that big of a deal. Thanks again!

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h

public:
   /// If this SVal wraps a symbol return that SymbolRef.
   /// Otherwise, return 0.
   ///
   /// Casts are ignored during lookup.
   /// \param IncludeBaseRegions The boolean that controls whether the search
   /// should continue to the base regions if the region is not symbolic.
   SymbolRef getAsSymbol(bool IncludeBaseRegions = false) const;
 
-  /// getAsSymbolicExpression - If this Sval wraps a symbolic expression then
-  ///  return that expression.  Otherwise return NULL.
-  const SymExpr *getAsSymbolicExpression() const;
-
-  const SymExpr *getAsSymExpr() const;
-
   const MemRegion *getAsRegion() const;
 
   /// printJson - Pretty-prints in JSON format.
   void printJson(raw_ostream &Out, bool AddQuotes) const;
 
   void dumpToStream(raw_ostream &OS) const;
   void dump() const;
 

clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp

 
 /// When a symbol is assumed to be nil, remove it from the set of symbols
 /// require to be nil.
 ProgramStateRef ObjCDeallocChecker::evalAssume(ProgramStateRef State, SVal Cond,
                                                bool Assumption) const {
   if (State->get<UnreleasedIvarMap>().isEmpty())
     return State;
 
-  auto *CondBSE = dyn_cast_or_null<BinarySymExpr>(Cond.getAsSymExpr());
+  auto *CondBSE = dyn_cast_or_null<BinarySymExpr>(Cond.getAsSymbol());
   if (!CondBSE)
     return State;
 
   BinaryOperator::Opcode OpCode = CondBSE->getOpcode();
   if (Assumption) {
     if (OpCode != BO_EQ)
       return State;
   } else {

clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp

 /// from consideration.
 ProgramStateRef MacOSKeychainAPIChecker::evalAssume(ProgramStateRef State,
                                                     SVal Cond,
                                                     bool Assumption) const {
   AllocatedDataTy AMap = State->get<AllocatedData>();
   if (AMap.isEmpty())
     return State;
 
-  auto *CondBSE = dyn_cast_or_null<BinarySymExpr>(Cond.getAsSymExpr());
+  auto *CondBSE = dyn_cast_or_null<BinarySymExpr>(Cond.getAsSymbol());
   if (!CondBSE)
     return State;
   BinaryOperator::Opcode OpCode = CondBSE->getOpcode();
   if (OpCode != BO_EQ && OpCode != BO_NE)
     return State;
 
   // Match for a restricted set of patterns for cmparison of error codes.
   // Note, the comparisons of type '0 == st' are transformed into SymIntExpr.

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

static Optional<unsigned> findArgIdxOfSymbol(ProgramStateRef CurrSt,
                                              SymbolRef &Sym,
                                              Optional<CallEventRef<>> CE) {
   if (!CE)
     return None;
 
   for (unsigned Idx = 0; Idx < (*CE)->getNumArgs(); Idx++)
     if (const MemRegion *MR = (*CE)->getArgSVal(Idx).getAsRegion())
       if (const auto *TR = dyn_cast<TypedValueRegion>(MR))
-        if (CurrSt->getSVal(MR, TR->getValueType()).getAsSymExpr() == Sym)
+        if (CurrSt->getSVal(MR, TR->getValueType()).getAsSymbol() == Sym)
           return Idx;
 
   return None;
 }
 
 static Optional<std::string> findMetaClassAlloc(const Expr *Callee) {
   if (const auto *ME = dyn_cast<MemberExpr>(Callee)) {
     if (ME->getMemberDecl()->getNameAsString() != "alloc")

clang/lib/StaticAnalyzer/Checkers/Taint.cpp

 
 bool taint::isTainted(ProgramStateRef State, const Stmt *S,
                       const LocationContext *LCtx, TaintTagType Kind) {
   SVal val = State->getSVal(S, LCtx);
   return isTainted(State, val, Kind);
 }
 
 bool taint::isTainted(ProgramStateRef State, SVal V, TaintTagType Kind) {
-  if (const SymExpr *Sym = V.getAsSymExpr())
+  if (SymbolRef Sym = V.getAsSymbol())
     return isTainted(State, Sym, Kind);
   if (const MemRegion *Reg = V.getAsRegion())
     return isTainted(State, Reg, Kind);
   return false;
 }
 
 bool taint::isTainted(ProgramStateRef State, const MemRegion *Reg,
                       TaintTagType K) {

clang/lib/StaticAnalyzer/Core/ProgramState.cpp

if (Optional<nonloc::LazyCompoundVal> X =
     return scan(*X);
 
   if (Optional<nonloc::LocAsInteger> X = val.getAs<nonloc::LocAsInteger>())
     return scan(X->getLoc());
 
   if (SymbolRef Sym = val.getAsSymbol())
     return scan(Sym);
 
-  if (const SymExpr *Sym = val.getAsSymbolicExpression())
-    return scan(Sym);
-
   if (Optional<nonloc::CompoundVal> X = val.getAs<nonloc::CompoundVal>())
     return scan(*X);
 
   return true;
 }
 
 bool ScanReachableSymbols::scan(const MemRegion *R) {
   if (isa<MemSpaceRegion>(R))

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

default: {
     return None;
   }
   }
 }
 
 SVal SValBuilder::makeSymExprValNN(BinaryOperator::Opcode Op,
                                    NonLoc LHS, NonLoc RHS,
                                    QualType ResultTy) {
-  const SymExpr *symLHS = LHS.getAsSymExpr();
-  const SymExpr *symRHS = RHS.getAsSymExpr();
+  SymbolRef symLHS = LHS.getAsSymbol();
+  SymbolRef symRHS = RHS.getAsSymbol();
 
   // TODO: When the Max Complexity is reached, we should conjure a symbol
   // instead of generating an Unknown value and propagate the taint info to it.
   const unsigned MaxComp = StateMgr.getOwningEngine()
                                .getAnalysisManager()
                                .options.MaxSymbolComplexity;
 
   if (symLHS && symRHS &&

 // At the moment, this function will redirect to evalCast, except when the range
 // of the original value is known to be greater than the max of the target type.
 SVal SValBuilder::evalIntegralCast(ProgramStateRef state, SVal val,
                                    QualType castTy, QualType originalTy) {
   // No truncations if target type is big enough.
   if (getContext().getTypeSize(castTy) >= getContext().getTypeSize(originalTy))
     return evalCast(val, castTy, originalTy);
 
-  const SymExpr *se = val.getAsSymbolicExpression();
+  SymbolRef se = val.getAsSymbol();
   if (!se) // Let evalCast handle non symbolic expressions.
     return evalCast(val, castTy, originalTy);
 
   // Find the maximum value of the target type.
   APSIntType ToType(getContext().getTypeSize(castTy),
                     castTy->isUnsignedIntegerType());
   llvm::APSInt ToTypeMax = ToType.getMaxValue();
   NonLoc ToTypeMaxVal =

clang/lib/StaticAnalyzer/Core/SVals.cpp

if (const auto *SymR = dyn_cast<SymbolicRegion>(SR))
       return SymR->getSymbol();
     else
       R = SR->getSuperRegion();
   }
 
   return nullptr;
 }
 
-// TODO: The next 3 functions have to be simplified.
-
 /// If this SVal wraps a symbol return that SymbolRef.
 /// Otherwise, return 0.
 ///
 /// Casts are ignored during lookup.
 /// \param IncludeBaseRegions The boolean that controls whether the search
 /// should continue to the base regions if the region is not symbolic.
 SymbolRef SVal::getAsSymbol(bool IncludeBaseRegions) const {
   // FIXME: should we consider SymbolRef wrapped in CodeTextRegion?
   if (Optional<nonloc::SymbolVal> X = getAs<nonloc::SymbolVal>())
     return X->getSymbol();
 
   return getAsLocSymbol(IncludeBaseRegions);
 }
 
-/// getAsSymbolicExpression - If this Sval wraps a symbolic expression then
-///  return that expression.  Otherwise return NULL.
-const SymExpr *SVal::getAsSymbolicExpression() const {
-  if (Optional<nonloc::SymbolVal> X = getAs<nonloc::SymbolVal>())
-    return X->getSymbol();
-
-  return getAsSymbol();
-}
-
-const SymExpr* SVal::getAsSymExpr() const {
-  const SymExpr* Sym = getAsSymbol();
-  if (!Sym)
-    Sym = getAsSymbolicExpression();
-  return Sym;
-}
-
 const MemRegion *SVal::getAsRegion() const {
   if (Optional<loc::MemRegionVal> X = getAs<loc::MemRegionVal>())
     return X->getRegion();
 
   if (Optional<nonloc::LocAsInteger> X = getAs<nonloc::LocAsInteger>())
     return X->getLoc().getAsRegion();
 
   return nullptr;

clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp

 ProgramStateRef SimpleConstraintManager::assumeAux(ProgramStateRef State,
                                                    NonLoc Cond,
                                                    bool Assumption) {
 
   // We cannot reason about SymSymExprs, and can only reason about some
   // SymIntExprs.
   if (!canReasonAbout(Cond)) {
     // Just add the constraint to the expression without trying to simplify.
-    SymbolRef Sym = Cond.getAsSymExpr();
+    SymbolRef Sym = Cond.getAsSymbol();
     assert(Sym);
     return assumeSymUnsupported(State, Sym, Assumption);
   }
 
   switch (Cond.getSubKind()) {
   default:
     llvm_unreachable("'Assume' not implemented for this NonLoc");
 
ProgramStateRef SimpleConstraintManager::assumeInclusiveRange(
     const llvm::APSInt &To, bool InRange) {
 
   assert(From.isUnsigned() == To.isUnsigned() &&
          From.getBitWidth() == To.getBitWidth() &&
          "Values should have same types!");
 
   if (!canReasonAbout(Value)) {
     // Just add the constraint to the expression without trying to simplify.
-    SymbolRef Sym = Value.getAsSymExpr();
+    SymbolRef Sym = Value.getAsSymbol();
     assert(Sym);
     return assumeSymInclusiveRange(State, Sym, From, To, InRange);
   }
 
   switch (Value.getSubKind()) {
   default:
     llvm_unreachable("'assumeInclusiveRange' is not implemented"
                      "for this NonLoc");

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

if (isLocType)
       return LI->getLoc();
     // FIXME: Correctly support promotions/truncations.
     unsigned castSize = Context.getIntWidth(castTy);
     if (castSize == LI->getNumBits())
       return val;
     return makeLocAsInteger(LI->getLoc(), castSize);
   }
 
-  if (const SymExpr *se = val.getAsSymbolicExpression()) {
+  if (SymbolRef se = val.getAsSymbol()) {
     QualType T = Context.getCanonicalType(se->getType());
     // If types are the same or both are integers, ignore the cast.
     // FIXME: Remove this hack when we support symbolic truncation/extension.
     // HACK: If both castTy and T are integers, ignore the cast.  This is
     // not a permanent solution.  Eventually we want to precisely handle
     // extension/truncation of symbolic integers.  This prevents us from losing
     // precision when we assign 'x = y' and 'y' is symbolic and x and y are
     // different integer types.