[clang] [analyzer] Don't invalidate pointee of pointer-to-const on callback arguments (PR #225489)

[Donát Nagy via cfe-commits]

NagyDonat wrote:

@benedekaibas

Let's first enumerate the situations that may cause `argumentsMayEscape()` to return true:
- (1) `CallEvent::argumentsMayEscape()` returns true for calls that take a non-null callback argument (introduced by [this commit](https://github.com/llvm/llvm-project/commit/228f9c7b68fd2bf56b32623251b54f3d3c9ee3b7))
  - when this returns true, the overriding methods also return true
- (2) `AnyFunctionCall::argumentsMayEscape()` returns true if `hasVoidPointerToNonConstArg()` is true (added in [this old commit](https://github.com/llvm/llvm-project/commit/fe1eca516988c2e79378e833b1cbde1a2907d042))
- (3) `AnyFunctionCall::argumentsMayEscape()` also returns true if the called function is among a set of roughly ten explicitly listed functions (logic moved to `CallEvent.cpp` from MallocChecker in  [this old commit](https://github.com/llvm/llvm-project/commit/27ab0182e33eee81dfc884e9cf95863bcaf30bab0)
- (4) `BlockCall::argumentsMayEscape` always returns true (IIUC introduced in [the commit that introduced `BlockCall`](https://github.com/llvm/llvm-project/commit/2a833ca575946d96987df063470fe9f3b3a865d3))
- (5) `ObjCMethodCall::argumentsMayEscape` has a heuristic for `valueWithPointer:` calls coming from system header (this may be justified?)

The result of `argumentsMayEscape()` is used in only a few situations:
- (A) When it is true, `CallEvent::invalidateRegions` disables the logic that preserves the pointees of the pointer-to-const parameters (and handles a pointer-to-const parameter the same way as a pointer-to-non-const).
- (B) `MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly` practically always returns true if  `argumentsMayEscape()` returns true for the current call.
  - The logic of this method is convoluted, it contains two calls to `argumentsMayEscape()` (one in the case handling `ObjCMethodCall`s, the other in the case handling plain function calls) and there are a few explicitly handled functions for which it can return false even if `argumentsMayEscape()` would return true.
- (C) `SimpleStreamChecker::guaranteedNotToCloseFile` returns false (= may close the file) if `argumentsMayEscape()` returns true.
  - Note that `SimpleStreamChecker.cpp` is just a demo/tutorial checker which is basically a simplified variant of `StreamChecker.cpp`. Despite this, the `checkPointerEscape` callback of `SimpleStreamChecker` invokes `argumentsMayEscape()` (and has always used it since 2012), while the `checkPointerEscape` callback of the production-quality `StreamChecker` was only introduced in 2020 and it does not invoke (and IIUC has never invoked)   `argumentsMayEscape()`.

Among these three usecases:
- (A) is mostly unjustified, but until the Store limitations described in https://github.com/llvm/llvm-project/pull/225489#discussion_r4094142023 are resolved/dodged somehow, we need to keep invoking heuristic (2) in situation (A)
  - (A) also influences whether the invalidation triggers the `checkPointerEscape` callbacks or the `checkConstPointerEscape` callbacks. Note that many checkers have `checkPointerEscape` callbacks but only `MallocChecker` has `checkConstPointerEscape` callback, so modifying (A) and removing some heuristics from it could influence many checkers.
- (B) is probably the right place for these heuristics – it lets us suppress the MallocChecker false positives (that were the original motivation for many heuristics) without influencing other logic.
- (C) is completely superfluous, because it is [at least in the current status quo] redundant with the `checkPointerEscape` vs `checkConstPointerEscape` distinction. We should create a separate commit to replace the `checkPointerEscape()` callback of `SimpleStreamChecker.cpp` with the (simpler!) `checkPointerEscape()` callback of `StreamChecker.cpp`.

In conclusion, I suggest the following roadmap:
- Create a separate PR that removes usecase (C) by replacing the `checkPointerEscape()` callback of `SimpleStreamChecker.cpp` with the (simpler!) `checkPointerEscape()` callback of `StreamChecker.cpp` and get that PR merged before this PR.
- In this PR replace usecase (A) [i.e. the check `if (!argumentsMayEscape())` in  `CallEvent::invalidateRegions`] with a check that just calls heuristic (2) [i.e. `if (!hasVoidPointerToNonConstArg())`.
- Also in this PR `argumentsMayEscape()` from the codebase and inline its logic into `MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly` (the only remaining function that calls it).

I'm not 100% sure that this change won't break any tests, but it is probable and worth a try.

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