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

[Donát Nagy via cfe-commits]

================
@@ -731,33 +731,36 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
     ExplodedNodeSet checkDst;
     NodeBuilder B(Pred, checkDst, Eng.getBuilderContext());
 
+    ProgramStateRef State = Pred->getState();
+    CallEventRef<> UpdatedCall = Call.cloneWithState(State);
+
     // Check if any of the EvalCall callbacks can evaluate the call.
     for (const auto &EvalCallChecker : EvalCallCheckers) {
       // TODO: Support the situation when the call doesn't correspond
       // to any Expr.
       ProgramPoint L = ProgramPoint::getProgramPoint(
-          Call.getOriginExpr(), ProgramPoint::PostStmtKind,
+          UpdatedCall->getOriginExpr(), ProgramPoint::PostStmtKind,
           Pred->getLocationContext(), EvalCallChecker.Checker);
       bool evaluated = false;
       { // CheckerContext generates transitions(populates checkDest) on
         // destruction, so introduce the scope to make sure it gets properly
         // populated.
         CheckerContext C(B, Eng, Pred, L);
-        evaluated = EvalCallChecker(Call, C);
+        evaluated = EvalCallChecker(*UpdatedCall, C);
       }
 #ifndef NDEBUG
       if (evaluated && evaluatorChecker) {
-        const auto toString = [](const CallEvent &Call) -> std::string {
+        const auto toString = [](CallEventRef<> Call) -> std::string {
           std::string Buf;
           llvm::raw_string_ostream OS(Buf);
-          Call.dump(OS);
+          Call->dump(OS);
----------------
NagyDonat wrote:

> Why did you change this?

It is true that this dump (which IIUC just prints the function name) doesn't rely on the `State` within the `CallEvent` object, so it would work with the non-updated instance -- but as we needed to create the `UpdatedCall` (because we need to pass that to the checkers), I thought that it would be better to consistently use it.

I don't exactly recall why I changed the type of this lambda, I think the compiler complained about my first attempt. I can try to avoid these changes (`CallEventRef<>` is a somewhat esoteric smart pointer type, but it _should_ be possible to get a `const CallEvent &` from it).

`<offtopic>`
By the way I feel an urge to get rid of this lambda, because it is much more verbose than e.g.
```c++
std::string FunctionName;
Call.dump(llvm:raw_string_ostream(FunctionName));
```
(I don't see a reason why we would need to declare a variable for the stream instead of just using a temporary object -- but I'd test this before applying it.) However, I acknowledge that this is subjective bikeshedding, and I won't remove the lambda if you prefer to keep it.
`</offtopic>`

> But generally, in the checker loop, the updated call event indicates that the call was evaluated by multiple checkers. Which should be impossible in practice but it looks like we've decided to act gracefully when assertions are turned off.

What do you mean by this? My understanding of this situation is that:
- When assertions are turned off (`#ifdef NDEBUG` block a few lines below the displayed area) we `break` and leave the loop eagerly when one checker evaluated the call.
- When assertions are turned on, we always iterate over all the checker callback, and do this formatted error printout if a second checker callback evaluated the call.

> That said, we aren't doing a great job at that, given that we're using `llvm_unreachable()` that translates to pure undefined behavior (aka `__builtin_unreachable()`) when assertions are turned off.

Good to know in general, but this particular `llvm_unreachable()` call is within an `#ifndef NDEBUG` block so I would guess that it is completely skipped when assertions are turned off. (Is this correct? What is the relationship between assertions and `NDEBUG`?)

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