[ADT] Potential SmallVectorImpl memory leak

[andykaylor]

I got a report from a static analysis tool telling me that my downstream project had a potential memory leak caused by SmallVectorImpl not having a virtual destructor. I think it was caused by a bit of code that was doing something like this:

std::unique_ptr<SmallVectorImpl> Result = std::make_unique<SmallVector<SomeClass, 16>>;

When the SmallVectorImpl object here gets destructed, it doesn’t call the SmallVector<> destructor, so things leak. We changed this code so that it’s no longer using unique_ptr with SmallVectorImpl, but it seems like something should be done to make this sort of problem impossible.

Like I said, the static analysis tool wanted me to make the SmallVectorImpl destructor virtual, but I don’t think we want to do that. I would like to make it protected instead.

The only place I could find in the code base that would be affected by this is the MLIR SerializeToHsacoPass class, which I know nothing about. However, there could obviously be other downstream clients that also got hit by it.

Any opinions on what the right thing to do here is?

[kuhar]

@andykaylor

std::unique_ptr Result = std::make_unique<SmallVector<SomeClass, 16>>;

What led to code like this? By dynamically allocating a small vector you lose the benefit of memory locality. Why not change it to SmallVector<SomeClass, 0> Result;?

Putting any upstream fixes aside, I think you can make this work if you specify a custom deleter.

[andykaylor]

@kuhar

What led to code like this? By dynamically allocating a small vector you lose the benefit of memory locality. Why not change it to SmallVector<SomeClass, 0> Result;?

I’m not sure what led to it. Someone else wrote the code. I’m not trying to make it work. The problem has already been fixed in my project. The point is, I’m trying to prevent other people from accidentally introducing resource leaks by doing things like this.

[kuhar]

Ah, OK. I’m not sure if it’s generally desirable to support polymorphic destruction and introduce a vtable.

@andykaylor

Like I said, the static analysis tool wanted me to make the SmallVectorImpl destructor virtual, but I don’t think we want to do that. I would like to make it protected instead.

Do we have any protected destructors in LLVM already or would this be setting a precedent?

[andykaylor]

@kuhar

Do we have any protected destructors in LLVM already or would this be setting a precedent?

That’s a good question, and I’m not sure how to search for such a precedent. I can say that there is a C++ core guideline rule about this: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c35-a-base-class-destructor-should-be-either-public-and-virtual-or-protected-and-non-virtual and a clang-tidy check for it.

[mehdi_amini]

@andykaylor

The only place I could find in the code base that would be affected by this is the MLIR SerializeToHsacoPass class, which I know nothing about.

If it isn’t trivially fixable, can you send a log of the failure when you make it protected? Maybe you have already a branch you can share?
We’ll happily look into fixing this right now!

[andykaylor]

@mehdi_amini

If it isn’t trivially fixable, can you send a log of the failure when you make it protected? Maybe you have already a branch you can share?

I’ve put up a PR (Make SmallVectorImpl destructor protected by andykaylor · Pull Request #71439 · llvm/llvm-project · GitHub) that both changes SerializeToHsaco and makes the SmallVectorImpl destructor protected. Everything builds with this change, but I don’t think the SerializeToHsaco change are likely to be what you want. Without that change, making the SmallVectorImpl destructor causes a build failure in that code, as expected. I think this confirms that the code has a resource leak issue.

[andykaylor]

I stumbled through the process of updating my branch and something got mixed up, so there’s a new PR here: Make SmallVectorImpl destructor protected by andykaylor · Pull Request #71746 · llvm/llvm-project (github.com)