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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Excellent! This utility is the first step on a lot of paths such as:

- Asserting that all expressions' values are of the right type. I expect this to uncover a lot of ridiculous mutually cancelling bugs.
- Modeling extents of RegionStore bindings - they're simply widths of their respective types.

In D104550#2827456 <https://reviews.llvm.org/D104550#2827456>, @vsavchenko wrote:

> - See if I missed any "typed" values from the hierarchy

All values in the hierarchy are typed. Well, ideally.

See inlined comment for `ConcreteInt`s.

One particularly important one is `nonloc::LocAsInteger` (And, well, why doesn't it still have signedness? - I had a patch for it like 6 years ago. Why am I so bad at committing patches?).

I think you can also easily support pointers-to-members and hopefully goto labels.

> - See if `getType` for supported types does make sense
>
> In particular, it would be great to hear your thoughts on `SymolicRegion` because it's not that obvious that we should return that type or not.  On one hand, this is the best we've got, on the other hand, it can be misleading.

The type of the object it points to is indeed technically unknown. The type of the //pointer// represented by a `loc::MemRegionVal(SymbolicRegion)` is unambiguous and coincides with the type of the symbol.

In particular, if the symbol is of type `void *`, then it represents a void pointer value, which doesn't mean that its pointee is a `void`.



================
Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:151
+  Optional<QualType> VisitNonLocLazyCompoundVal(nonloc::LazyCompoundVal LCV) {
+    return Visit(LCV.getRegion());
+  }
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Core/SVals.cpp:162
+  }
+  Optional<QualType> VisitSymExpr(const SymExpr *SE) { return SE->getType(); }
+};
----------------
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.


================
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());
----------------
`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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104550