[PATCH] D93595: [analyzer] Fix extraction of punned and known scalar SVals

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1648
+    else
+      bitPos = ORegionRawOffs.getOffset().getQuantity();
+    return bitPos * Ctx.getCharWidth();
----------------
NoQ wrote:
> This assignment can overflow. Both because the raw offset can be very large and because it can be negative.
> 
> Say, the following code compiles just fine (and may even run) so we need to be able to analyze it but you dodge that by always checking that the base type is a scalar type (if you intend to keep it that way i'd rather assert that; the other reason to assert that would be because endianness logic is screwed without it):
> ```lang=c
> int main() {
>   int x[5000000000]; // Over 2³² elements.
>   x[0] = 1;
>   char *y = &x;
>   char c = y[19999999999];
> }
> ```
> 
> The following code is valid but you seem to be protected from it because you dodge `SymbolicRegion`s by checking that **R** is a `TypedValueRegion`:
> ```lang=c
> void foo(int *x) {
>   long *y = (long *)(x - 1);
>   y = 1;
>   char c = *(char *)y;
> }
> ```
> 
> The following code has an obvious UB but it still compiles so you have to make sure the analyzer behaves sanely:
> 
> ```lang=c
> void foo() {
>   int x;
>   long *y = (long *)(x - 1);
>   y = 1;
>   char c = *(char *)y;
> }
> ```
(I see that you brought this up and tested it but overflows are still scary. If you're consciously relying on an overflow you should probably at least comment that.)


================
Comment at: clang/test/Analysis/concrete-endian.cpp:56
+#endif
+  // Array out of bounds should yield UNKNOWN
+  clang_analyzer_eval(pps[4]);  // expected-warning{{UNKNOWN}}
----------------
Ideally no they shouldn't, they should yield `Undefined`. I'd rather mark it as `FIXME:` because ultimately we could have a warning here. It's ok if we miss these cases for now though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93595