[PATCH] D115149: [analyzer][solver] Fix assertion on (NonLoc, Op, Loc) expressions

Mon Dec 13 02:59:29 PST 2021

steakhal marked an inline comment as done.
steakhal added a comment.

In D115149#3182196 <https://reviews.llvm.org/D115149#3182196>, @ASDenysPetrov wrote:

> @steakhal
> Please provide a case which asserts before your patch.

I don't get this one. I've provided a bunch of tests, even annotated with `no-crash` comments where we crashed prior to this change.

In D115149#3181552 <https://reviews.llvm.org/D115149#3181552>, @NoQ wrote:

>> There is the reinterpret-cast operation which is capable of crossing these two domains, producing an expression that can participate in arithmetic operations, but on the abstract domain side, we still stick to Locs
>
> Such cast should turn the `loc::ConcreteInt` into a `nonloc::ConcreteInt` with the same integral value.
>
> The distinction between Loc and NonLoc is very important. It's at the core of our type correctness. We should fight tooth and nail to preserve it because assertions about these things (such as the one removed here) help us discover a lot of bugs in other places (such as genuinely misplacing a value).

Yes, it makes sense to look for bugs in the cast handling. Ideally, we should not transform `loc::ConcreteInt` to `nonloc::ConcreteInt` by hand. The reinterpret-cast should have done this transformation for us.
If that is fixed, I'm fine reverting this one. Sorry for rushing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115149