llvm-project

llvm-project copied to clipboard

The following piece of code causes a lifetime warning about returning a dangling pointer:

class PrivateKey {
   // A random class that models a private key.
};

PrivateKey* getPrivateKey() {
    return std::unique_ptr<PrivateKey>(new PrivateKey()).release();
}

See in Godbolt: https://godbolt.org/z/fJyWl4

Is this correct? If we call release on a unique_ptr, I'd assume that the unique pointer no longer retains ownership and a call to the destructor no longer destructs the pointed-to PrivateKey object. I'd assume this is safe (not ideal because the caller would need to delete PrivateKey manually, though, but the pointer should not dangle).

Hi, thanks for taking the time to report your findings. I appreciate it! To your question: The lifetime checker builds on top of the C++ Core guidelines, which forbid to user raw pointer to "own" an object. So yes, the diagnostic you see isn't correct in the sense that the returned pointer doesn't dangle. But in the way I read it, the C++ Core Guidelines effectively don't allow to use std::unique_ptr::release.

My proposal to go forward would be to improve our diagnostics, and directly say warning: std::unique_ptr::release is not allowed. What do you think? For your code, you currently can only refactor it to get rid of needing unique_ptr::release. In the future, we will also provide a suppression syntax.

I'm not sure the C++ Core Guidelines explicitly ban std::unique_ptr::release. We can rephrase the diagnostic to comply with "F.26: Use a unique_ptr<T> to transfer ownership where a pointer is needed", and indicate that returning a locally allocated pointer is bad and returning a std::unique_ptr is preferred. That way we could cover, in the future, similarly problematic styles that do not involve using std::unique_ptr::release.

Irrespective of the wording, I agree with having a specific diagnostic in this case. People may be confused between incorrect code (returning a pointer to a temporary) and prone to error code (like returning a pointer without explicit ownership).

@mgehre would it be okay to extend the current LifetimeReporter for that kind of warning or should it better be handled by a new kind of Reporter ?

We can use the LifetimeReporter and add a generic "this function disables lifetime analysis" warning in the existing "lifetime-disabled" category. We should probably have an function attribute to say "this function disables lifetime analysis", and then "hard code" that attribute for "unique_ptr::release". We do something similar to mark known functions as [[lifetime_const]], see https://github.com/mgehre/llvm-project/blob/lifetime/clang/lib/Analysis/LifetimeTypeCategory.cpp#L437.