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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
