[clang] [analyzer] Enforce not making overly complicated symbols (PR #144327)

[Balázs Benics via cfe-commits]

================
@@ -317,21 +317,21 @@ class SValBuilder {
     return nonloc::LocAsInteger(BasicVals.getPersistentSValWithData(loc, bits));
   }
 
-  nonloc::SymbolVal makeNonLoc(const SymExpr *lhs, BinaryOperator::Opcode op,
-                               APSIntPtr rhs, QualType type);
+  DefinedOrUnknownSVal makeNonLoc(const SymExpr *lhs, BinaryOperator::Opcode op,
+                                  APSIntPtr rhs, QualType type);
 
-  nonloc::SymbolVal makeNonLoc(APSIntPtr rhs, BinaryOperator::Opcode op,
-                               const SymExpr *lhs, QualType type);
+  DefinedOrUnknownSVal makeNonLoc(APSIntPtr rhs, BinaryOperator::Opcode op,
+                                  const SymExpr *lhs, QualType type);
 
-  nonloc::SymbolVal makeNonLoc(const SymExpr *lhs, BinaryOperator::Opcode op,
-                               const SymExpr *rhs, QualType type);
+  DefinedOrUnknownSVal makeNonLoc(const SymExpr *lhs, BinaryOperator::Opcode op,
+                                  const SymExpr *rhs, QualType type);
 
-  NonLoc makeNonLoc(const SymExpr *operand, UnaryOperator::Opcode op,
-                    QualType type);
+  DefinedOrUnknownSVal makeNonLoc(const SymExpr *operand,
+                                  UnaryOperator::Opcode op, QualType type);
 
   /// Create a NonLoc value for cast.
-  nonloc::SymbolVal makeNonLoc(const SymExpr *operand, QualType fromTy,
-                               QualType toTy);
+  DefinedOrUnknownSVal makeNonLoc(const SymExpr *operand, QualType fromTy,
+                                  QualType toTy);
----------------
balazs-benics-sonarsource wrote:

> So with that in mind, what do you folks think about returning a fresh atomic symbol instead of the `Unknown`?

I thought about this, and problem is that conjuring a fresh symbol needs quite a bit of context: CFGElement, LocationContex, VisitCount that are not present. Passing these would be really annoying and intrusive for every use of the SValBuilder. I don't think this is the right way.

> Eg., make a new symbol class `SymbolTooComplex` (name TBD) whose only field is the pointer to the symbolic expression that _would have been_ constructed if we didn't reach the complexity limit. But you're not supposed to access that pointer while folding arithmetic, it's only there for deduplication purposes. In other words, every time we evaluate the same operation and hit the same complexity limit, we'd get the same symbol.
> 
> This would probably avoid a lot of clumsiness in these high-level APIs (they will continue to make a `NonLoc` as promised), as well as improve deduplication of symbols instead of worsening it.

I didn't think about adding a new Symbol kind. I'll give it a try.

> (The caller would still need to avoid hard-casting the value back to `SymIntExpr` or something like that. But why would anybody want to do that when they already have the raw parts?)

I don't think anybody does that. They are definitely not supposed to unconditionally cast the results in moooost cases.

https://github.com/llvm/llvm-project/pull/144327