[PATCH] D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D88477#2303376 <https://reviews.llvm.org/D88477#2303376>, @steakhal wrote:

> In the Summary's example, at location `#2` the value of `**p` is `&Element{SymRegion{reg_$0<int * a>},0 S64b,unsigned char}` - which is exactly what we stored at line `#1`.
>
>   void test(int *a, char ***b, float *c) {
>     *(unsigned char **)b = (unsigned char *)a; // #1
>     if (**b == 0) // no-crash, #2
>       ;
>     // ...
>   }

And I believe that this part is already incorrect. Like, regardless of how we do the dereference (the implicit lvalue-to-rvalue cast), or *whether* we do it at all (nobody guarantees we'll ever do that!), the part of the static analyzer that computes the lvalue `**b` has to work correctly. As of now it computes an lvalue of incorrect type (currently it's `unsigned char` but it has to be `char *`).

In D88477#2303376 <https://reviews.llvm.org/D88477#2303376>, @steakhal wrote:

> IMO, we should relax this invariant in the following way:
> If the region is typed and the type also explicitly specified, we will perform a cast to the explicitly stated type.
> In other words, overwrite the LoadTy only if that was not given.

This would teach the modeling of the load step for the pointee how to undo the damage done by the previous load step for the pointer. In the general case the load step for the pointee does not necessarily exist, therefore we cannot rely on it to undo the damage. This is why it's extremely important for every modeling step to be correct. You can't miss a detail on one step and account for it in another step because there's almost never a guarantee that the other step will actually have a chance to kick in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88477