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