[PATCH] D98726: [analyzer] Enabling MallocChecker to take up after SmartPtrModelling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D98726#2630947 <https://reviews.llvm.org/D98726#2630947>, @RedDocMD wrote:

> @NoQ, regarding https://godbolt.org/z/1EczEW, I don't think there should be a warning since the original pointer may be needed in the caller.

You're absolutely right! I missed this point!

I still think it's worth a few experiments to see if the very presence of `unique_ptr` in the code is a good-enough indication that the callee expects //unique// ownership. But no, that's definitely not the point of that TODO.

Here's a related example where the caller technically can still rely on keeping the pointer but that situation would be even more absurd: https://godbolt.org/z/Mnos89ehK - basically the only way for the caller to access the raw pointer value before it gets destroyed is from within a temporary destructor that precedes the destructor of the `unique_ptr` argument. Which is valid but i'm leaning heavily towards still emitting a warning.

Another example: https://godbolt.org/z/on13Kv1q4 - this is definitely questionable but probably still deserves experimentation.

Ok so long story short, the only information that we gain about the raw pointer on `.release()` is that (assuming the default deleter is used) it's //some// memory allocated by `new`. We already knew (no pun intended) that about the raw pointer but `.release()` sounds like a good place to re-attach it. We do not really gain knowledge that we have to release the value but we can try to see what happens if we pretend we do. I guess we could rephrase the TODO to reflect that - "experiment with detecting leaks of the raw pointer after .release()" or something like that. We also definitely don't gain the knowledge that the value is already released (it definitely isn't, despite the method's name).

> But thanks for pointing out the possible leaks due to release! :)
> In the line that you have pointed out, I have thought about the following cases which should have a warning (but currently don't):
>
> - https://godbolt.org/z/dzszWd (leak from explicit new)
> - https://godbolt.org/z/rdbKn3 (leak from `unique_ptr` created from `make_unique`)
> - https://godbolt.org/z/Y6d5qE (use after delete of passed pointer)

In these cases MallocChecker should already be tracking the pointer even before it was put into `unique_ptr`. So i guess we should investigate where/why this information gets lost. But we shouldn't be re-attaching it; it should be already there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98726/new/

https://reviews.llvm.org/D98726