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

[Duncan P. N. Exon Smith]

> 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.

I think using shallow MemoryBuffer is a reasonable approach.

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...)