[PATCH] D104550: [analyzer] Implement getType for SVal

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko marked an inline comment as done.
vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:151
+  Optional<QualType> VisitNonLocLazyCompoundVal(nonloc::LazyCompoundVal LCV) {
+    return Visit(LCV.getRegion());
+  }
----------------
NoQ wrote:
> This is correct except you need to get the value type, not the pointer type.
> 
> `LazyCompoundVal` is a `prvalue` and its parent region is the location in which this `prvalue` resided some time in the past. So the parent region is of the right type and it's always typed but you need the pointee type.
OK then, can you maybe hint how can I write a test where this is going to be a pointer type (or maybe then `getType` for regions works incorrectly).


================
Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:162
+  }
+  Optional<QualType> VisitSymExpr(const SymExpr *SE) { return SE->getType(); }
+};
----------------
NoQ wrote:
> The biggest disappointment here is that this case is technically incorrect for the very basic case of integral types.
> 
> Because we drop casts, a symbol of an integral type may technically represent a value of a completely different integral type, the same symbol may represent multiple different values of different integral types, and so on.
> 
> This is what ruins the dream of modeling Region Store binding extents as value widths: for the single most important case we'd be getting incorrect answers.
I hope that the introduction of symbolic casts is around the corner.


================
Comment at: clang/unittests/StaticAnalyzer/SValTest.cpp:144
+  SVal X = getByName("x");
+  // TODO: Find a way how to get type of nonloc::ConcreteInt
+  EXPECT_FALSE(X.getType().hasValue());
----------------
NoQ wrote:
> `nonloc::ConcreteInt` is basically an `APSInt`. You can extract bit width and signedness and feed it to `ASTContext::getIntTypeForBitwidth()`.
> 
> `loc::ConcreteInt` is always of the pointer type. Unless, of course, you have a target architecture with pointers of different widths, then whelp we aren't quite there yet. But when we get there, I guess it's still `APSInt` under the hood so a similar trick could be used.
OK, I guess having `ASTContext` as a parameter is the best choice we can have here and not such a high price to pay.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104550/new/

https://reviews.llvm.org/D104550