[clang] [analyzer] Avoid use of `CallEvent`s with obsolete state (PR #160707)

[Donát Nagy via cfe-commits]

NagyDonat wrote:

> I agree that CallEvents have this trap. I've seen it. I don't think it's too widespread. I think we usually catch the use of stale states of CallEvents during review.

Noted. It is true that good enough code review can prevent any bugs, but I still think that we should prefer solutions that automatically prevent the errors.

>  It also helps that the State is part of our vocabulary and its passed liberally as a parameter, which sets a good example.

If the `State` is passed around liberally (to ensure that we always inject the most recent one), then why do we have a second `State` within the `CallEvent` as well? I would prefer an approach where `CallEvent` doesn't own a state (and perhaps there is a `CallEventWithState` type if there are use cases where that is needed).

> This is a critical piece of code. Many of the changed lines look aesthetic, aka. refactor.

I had to change lots of lines because I needed to switch between use of `const CallEvent &` (a reference type, the members can be accessed by dot) and `CallEventRef<>` (a pointer type, members can be accessed by arrow).

> You mention some bug if I recall my sloppy reading, but I could not spot behavioral changes or at least it didn't stand out to me.

There are three bugs that are fixed by this commit:

(1) Tthe `eval::Call` checkers were called with a `CallEvent` object whose associated state was obsolete: it definitely didn't include the state changes from the PreCall checkers and (according to the comment at the beginning of `ExprEngine::EvalCall` might have been even more obsolete). 

(2) The use of `Call.getArgSVal(Arg)` within the "run PointerEscape for the newly conjured symbols" part ([at the end of the `ExprEngine::evalCall`](https://github.com/llvm/llvm-project/blob/9552e899e494e619093e8685173a4af0ba73e049/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp#L725)) relies on the state within the `CallEvent` (which may have been obsolete even at the beginning of this method call) instead of the variable `State` (which  is up-to date and corresponds to one of the potentially multiple nodes resulting from the pre/eval/post steps).

(3) A bit later (still in the "run PointerEscape for the newly conjured symbols" step) the `CallEvent` with the ancient state is also passed to `processPointerEscapedOnBind` which directly forwards it to checkers (without updating the state).

> If this is NFC, let's adjust the PR title. If has behavioral change, let's have a test exposing that and split off the refactor parts.

I will create a test with a new debug checker which modifies the state in its `PreCall` callback and validates that the modification is visible through the `CallEvent` objects received by the `EvalCall` and `PointerEscape` callbacks. Is this sufficient, or would you prefer a different kind of test?

I don't think that it is possible to meaningfully simplify this commit by splitting off "refactor parts". There are only a few non-essential changes (comment changes, renamed variables) and without them the "core" change would be more difficult to understand.

https://github.com/llvm/llvm-project/pull/160707