NagyDonat wrote: As I re-read the PR description, I feel that I would like to propose a rewrite. I see and value that you summarized the changes that you performed, but I feel that this PR is trickier than usual and some of the tricks are not uncovered in the commit message.
My suggestion: > Currently `CallEvent::invalidateRegions()` ensures that if the type of a > function parameter is pointer-to-const then the pointee of the corresponding > argument is not invalidated. However, this logic was put behind a `if > (!argumentsMayEscape())` check which disabled it for certain classes of > function calls (e.g. when one argument is a non-null callback, and also for > many explicitly named functions) to suppress false positive leak reports by > MallocChecker. > > All but one of these heuristic exceptions were only relevant for > `MallocChecker`, so this commit moves them there, where they are naturally > placed next to other analogous heuristics (that were already present in > `MallocChecker`). This way these heuristics still suppress false positive > leak reports, but they don't influence the invalidation process (where > `Store` contents are replaced by fresh conjured/derived symbols). > > The only heuristic that still needs to be called from > `CallEvent::invalidateRegions` is `hasVoidPointerToNonConstArg` which is a > workaround to a bug in the invalidation logic. Currently, when a pointer is > received through a pointer-to-const parameter, the _region targeted by that > pointer_ is marked as "don't invalidate contents", even when the call also > receives another pointer to that region through a pointer-to-non-const > parameter. Checking `hasVoidPointerToNonConstArg` mitigates the consequences > of this by not marking regions behind pointer-to-const parameters as "don't > invalidate contents" when those regions _might_ be also accessible through a > non-const `void *` parameter. > > I intend to fix this bug; after that this workaround can and should be > removed. > > The vague and confusing virtual method `CallEvent::argumentsMayEscape` was > only called from two places, `CallEvent::invalidateRegions()` and > `MallocChecker`, so this commit deletes it and moves its logic into > `MallocChecker` (the only place where it is useful). The handling of > escape-related heuristics in `MallocChecker` is refactored to clarify their > role and accommodate that `CallEvent::invalidateRegions` no longer invokes > such heuristics. > > This PR fixes #222438 My thought process: - Someone reading your description would've assumed that `if (!argumentsMayEscape())` _only_ disabled the pointer-to-const logic in the case when in case when one of the arguments is a non-null callback. - It is important to mention that these heuristics _were introduced to suppress false positive leak reports_ and _they will have a natural place in MallocChecker_, which justifies moving them to MallocChecker. (As opposed to e.g. completely deleting them, or keeping them in `argumentsMayEscape` but only calling that method from MallocChecker.) - It is also important to justify why you preserve `hasVoidPointerToNonConstArg()` in `CallEvent::invalidateRegions`, how is it different from the other heuristics. Here I fully explain the bug in the invalidation logic, which is a subjective choice – perhaps others might prefer shortening this paragraph. - You wrote a long paragraph about the roles of the individual helper functions – for me this was hard to follow without seeing the code. In general, the roles of functions should be clear from the code (i.e. from their name + from source code comments if necessary), so you shouldn't write about them in the PR description / commit message. I think your description focuses too much on the problem described in PR #222438, which was the initial motivation for creating this PR, but does not explain and justify the full extent of the changes. By the way, composing a description for this PR is significantly more difficult than the average; I thought a lot about writing the one that I suggest (and gained more understanding of the PR this way). https://github.com/llvm/llvm-project/pull/225489 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
