[llvm] r211861 - ParseIR: don't take ownership of the MemoryBuffer

[David Blaikie]

On Thu, Jun 26, 2014 at 9:33 PM, Alp Toker <alp at nuanti.com> wrote:
> Author: alp
> Date: Thu Jun 26 23:33:58 2014
> New Revision: 211861
>
> URL: http://llvm.org/viewvc/llvm-project?rev=211861&view=rev
> Log:
> ParseIR: don't take ownership of the MemoryBuffer
>
> clang was needlessly duplicating whole memory buffer contents in an attempt to
> satisfy unclear ownership semantics. Let's just hide internal LLVM quirks and
> present a simple non-owning interface.
>
> The public C API preserves previous behaviour for stability.
>
> Modified:
>     llvm/trunk/include/llvm/IRReader/IRReader.h
>     llvm/trunk/lib/AsmParser/Parser.cpp
>     llvm/trunk/lib/IRReader/IRReader.cpp
>
> Modified: llvm/trunk/include/llvm/IRReader/IRReader.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IRReader/IRReader.h?rev=211861&r1=211860&r2=211861&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/IRReader/IRReader.h (original)
> +++ llvm/trunk/include/llvm/IRReader/IRReader.h Thu Jun 26 23:33:58 2014
> @@ -40,9 +40,9 @@ Module *getLazyIRFileModule(const std::s
>
>  /// If the given MemoryBuffer holds a bitcode image, return a Module
>  /// for it.  Otherwise, attempt to parse it as LLVM Assembly and return
> -/// a Module for it. This function *always* takes ownership of the given
> -/// MemoryBuffer.
> -Module *ParseIR(MemoryBuffer *Buffer, SMDiagnostic &Err, LLVMContext &Context);
> +/// a Module for it. This function *never* takes ownership of Buffer.
> +Module *ParseIR(const MemoryBuffer *Buffer, SMDiagnostic &Err,
> +                LLVMContext &Context);
>
>  /// If the given file holds a bitcode image, return a Module for it.
>  /// Otherwise, attempt to parse it as LLVM Assembly and return a Module
>
> Modified: llvm/trunk/lib/AsmParser/Parser.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/AsmParser/Parser.cpp?rev=211861&r1=211860&r2=211861&view=diff
> ==============================================================================
> --- llvm/trunk/lib/AsmParser/Parser.cpp (original)
> +++ llvm/trunk/lib/AsmParser/Parser.cpp Thu Jun 26 23:33:58 2014
> @@ -54,8 +54,7 @@ Module *llvm::ParseAssemblyFile(const st
>  Module *llvm::ParseAssemblyString(const char *AsmString, Module *M,
>                                    SMDiagnostic &Err, LLVMContext &Context) {
>    MemoryBuffer *F =
> -    MemoryBuffer::getMemBuffer(StringRef(AsmString, strlen(AsmString)),
> -                               "<string>");
> +      MemoryBuffer::getMemBuffer(StringRef(AsmString), "<string>");
>
>    return ParseAssembly(F, M, Err, Context);
>  }
>
> Modified: llvm/trunk/lib/IRReader/IRReader.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IRReader/IRReader.cpp?rev=211861&r1=211860&r2=211861&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IRReader/IRReader.cpp (original)
> +++ llvm/trunk/lib/IRReader/IRReader.cpp Thu Jun 26 23:33:58 2014
> @@ -39,7 +39,7 @@ Module *llvm::getLazyIRModule(MemoryBuff
>      if (std::error_code EC = ModuleOrErr.getError()) {
>        Err = SMDiagnostic(Buffer->getBufferIdentifier(), SourceMgr::DK_Error,
>                           EC.message());
> -      // ParseBitcodeFile does not take ownership of the Buffer in the
> +      // getLazyBitcodeModule does not take ownership of the Buffer in the
>        // case of an error.
>        delete Buffer;
>        return nullptr;
> @@ -62,13 +62,14 @@ Module *llvm::getLazyIRFileModule(const
>    return getLazyIRModule(File.release(), Err, Context);
>  }
>
> -Module *llvm::ParseIR(MemoryBuffer *Buffer, SMDiagnostic &Err,
> +Module *llvm::ParseIR(const MemoryBuffer *Buffer, SMDiagnostic &Err,
>                        LLVMContext &Context) {
>    NamedRegionTimer T(TimeIRParsingName, TimeIRParsingGroupName,
>                       TimePassesIsEnabled);
>    if (isBitcode((const unsigned char *)Buffer->getBufferStart(),
>                  (const unsigned char *)Buffer->getBufferEnd())) {
> -    ErrorOr<Module *> ModuleOrErr = parseBitcodeFile(Buffer, Context);
> +    ErrorOr<Module *> ModuleOrErr =
> +        parseBitcodeFile(const_cast<MemoryBuffer *>(Buffer), Context);

What's the reason for this const_cast? Does parseBitcodeFile actually
modify the MemoryBuffer? If so, I'd rather keep the ParseIR contract
truthful and keep the non-const pointer parameter. If not, a comment
to explain that parseBitcodeFile is actually non-mutating (either
statically (in which case a FIXME suggesting it should be changed to
be const correct), or dynamically for this particular caller)?

>      Module *M = nullptr;
>      if (std::error_code EC = ModuleOrErr.getError())
>        Err = SMDiagnostic(Buffer->getBufferIdentifier(), SourceMgr::DK_Error,
> @@ -76,11 +77,12 @@ Module *llvm::ParseIR(MemoryBuffer *Buff
>      else
>        M = ModuleOrErr.get();
>      // parseBitcodeFile does not take ownership of the Buffer.
> -    delete Buffer;
>      return M;
>    }
>
> -  return ParseAssembly(Buffer, nullptr, Err, Context);
> +  return ParseAssembly(MemoryBuffer::getMemBuffer(
> +                           Buffer->getBuffer(), Buffer->getBufferIdentifier()),
> +                       nullptr, Err, Context);
>  }
>
>  Module *llvm::ParseIRFile(const std::string &Filename, SMDiagnostic &Err,
> @@ -92,7 +94,7 @@ Module *llvm::ParseIRFile(const std::str
>      return nullptr;
>    }
>
> -  return ParseIR(File.release(), Err, Context);
> +  return ParseIR(File.get(), Err, Context);
>  }
>
>  //===----------------------------------------------------------------------===//
> @@ -104,7 +106,8 @@ LLVMBool LLVMParseIRInContext(LLVMContex
>                                char **OutMessage) {
>    SMDiagnostic Diag;
>
> -  *OutM = wrap(ParseIR(unwrap(MemBuf), Diag, *unwrap(ContextRef)));
> +  std::unique_ptr<MemoryBuffer> MB(unwrap(MemBuf));
> +  *OutM = wrap(ParseIR(MB.get(), Diag, *unwrap(ContextRef)));
>
>    if(!*OutM) {
>      if (OutMessage) {
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits