[PATCH] unique_ptrify llvm::getLazyBitcodeModule's MemoryBuffer parameter

[David Blaikie]

On Tue, Aug 12, 2014 at 3:41 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2014-Aug-12, at 14:51, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> Hi lhames, rafael,
>>
>> This turned out to be a bit tricky as there are some callers doing
>> strange things with ownership of the this MemoryBuffer.
>>
>> A couple of callers (llvm::getBitcodeTargetTriple,
>> llvm::parseBitcodeFile) don't take ownership of the MemoryBuffer, but
>> still want to construct a Module (but they either ensure it's fully
>> materialized, or otherwise destroy it before the function is over, being
>> sure to take the MemoryBuffer back from the Module so it is not
>> destroyed). Some of these callers were adjusted to use ScopeGuards to
>> ensure the release of a fake local unique_ptr, others were able to be
>> adjusted to pass ownership transiently, then take it back again later.
>>
>> Other ideas are welcome - one possible option would be to create new
>> shallow MemoryBuffers in these cases. Another would be to make these
>> data structures consistently non-owning, though that sounds difficult
>> for other users. I'd like to throw a few ideas around here because I
>> think this pattern shows up a few times in LLVM - where an API has
>> multiple users, some of whom want to give ownership, some of whom don't.
>> I'd like to figure out a good/consistent/simple answer for when we come
>> across this again.
>>
>> http://reviews.llvm.org/D4876
>>
>> Files:
>>  include/llvm/ADT/ScopeGuard.h
>>  include/llvm/Bitcode/ReaderWriter.h
>>  include/llvm/IR/GVMaterializer.h
>>  include/llvm/IR/Module.h
>>  lib/Bitcode/Reader/BitReader.cpp
>>  lib/Bitcode/Reader/BitcodeReader.cpp
>>  lib/Bitcode/Reader/BitcodeReader.h
>>  lib/IR/Module.cpp
>>  lib/IRReader/IRReader.cpp
>>  lib/LTO/LTOModule.cpp
>>  lib/Object/IRObjectFile.cpp
>>  unittests/Bitcode/BitReaderTest.cpp
>>  unittests/ExecutionEngine/JIT/JITTest.cpp
>> <D4876.12420.patch>_______________________________________________
>
> I'm not sure this is the right direction -- in particular, I think using
> `std::unique_ptr<>` in BitcodeReader is really confusing when it doesn't
> actually own the buffer, and *that* problem should be solved first.

Yeah, you might be right - all I'm doing here is making that "lie" a
bit more obvious (well, pushing it up to the point where the lying
starts). If there's a nice way not to lie, I'm all for that.

> I think using shallow MemoryBuffer is a reasonable approach.

Yep - this leans on MemoryBuffer's "not always owning" mechanics which
is something Lang & I have talked about killing off at some point (I
think trending in favor of "not owning" but I forget - or at least
trying to make as many uses as possible explicitly non-owning,
perhaps) so that might not be a viable long term solution. But maybe
it is - I'm not sure how hazy the future of MemoryBuffer is. In any
case, multiple layers of optional ownership don't exactly make me
happy.

Even one layer of optional ownership makes me a bit sad, though maybe
it's the right tool.

> Another way to make `BitcodeReader::Buffer` easier to reason about is to
> use a type something like this (name is terrible):
>
>     enum class sometimes_unique_policy { Unowned, Owned };
>     template <class T> class sometimes_unique_ptr {
>       PointerIntPair<T *, 1> Ptr;
>     public:
>       sometimes_unique_ptr(T *Ptr, sometimes_unique_policy Policy)
>           : Ptr(Ptr, Policy == sometimes_unique_policy::Owned) {}
>       sometimes_unique_ptr(std::unique_ptr<T> Unique)
>           : Ptr(Unique.release(), true) {}
>       explicit sometimes_unique_ptr(T &Ref) : Ptr(&Ref, false) {}
>       ~sometimes_unique_ptr() {
>         if (isOwned())
>           delete get();
>       }
>       bool isOwned() const { return Ptr.getInt(); }
>       T *get() const { return Ptr.getPointer(); }
>       // etc.
>     };
>
> `getLazyBitcodeModule()` can look like this:
>
>     ErrorOr<Module *>
>     getLazyBitcodeModule(sometimes_unique_ptr<MemoryBuffer> Buffer,
>                          LLVMContext &Context);
>
> If you pass it a reference, it releases it; if you pass it a unique_ptr<>,
> it deletes it.
>
> (I'm not familiar enough with ADT to know if we have a type like this
> already...)

Maybe that's what it'll come to - I really dislike optional ownership,
but if used judiciously it might be a tool we need in our toolkit.
This certainly isn't the only place where this API conundrum has come
up.

- David