llvm dev - Jan 2014 - [LLVMdev] [RFC] LTO: deallocating llvm::Module inside lto_codegen_add_module

LTOCodeGenerator::addModule (which is wrapped by lto_codegen_add_module) takes
an LTOModule as an argument and links the latter's llvm::Module into its
own.
It does not change the latter's llvm::Module.

The lto_module_dispose API call destroys an LTOModule.  This frees the
LTOModule's llvm::Module, but also invalidates strings returned by, e.g.,
lto_module_get_symbol_name.

I propose that LTOCodeGenerator::addModule be changed to also deallocate the
(consumed and redundant) llvm::Module and llvm::TargetMachine objects left
behind in LTOModule.  I.e., something akin to:

    diff --git a/include/llvm/LTO/LTOModule.h b/include/llvm/LTO/LTOModule.h
    index c70afa4..1180e58 100644
    --- a/include/llvm/LTO/LTOModule.h
    +++ b/include/llvm/LTO/LTOModule.h
    @@ -164,6 +164,11 @@ public:
         return _asm_undefines;
       }
     
    +  void destroyLLVMModule() {
    +    _module.reset();
    +    _target.reset();
    +  }
    +
     private:
       /// parseMetadata - Parse metadata from the module
       // FIXME: it only parses "Linker Options" metadata at the
moment
    diff --git a/lib/LTO/LTOCodeGenerator.cpp b/lib/LTO/LTOCodeGenerator.cpp
    index cae0ea2..a947fa8 100644
    --- a/lib/LTO/LTOCodeGenerator.cpp
    +++ b/lib/LTO/LTOCodeGenerator.cpp
    @@ -121,6 +121,8 @@ bool LTOCodeGenerator::addModule(LTOModule* mod,
std::string& errMsg) {
       for (int i = 0, e = undefs.size(); i != e; ++i)
         AsmUndefinedRefs[undefs[i]] = 1;
     
    +  mod->destroyLLVMModule();
    +
       return !ret;
     }

With this change, the ownership of the llvm::Module and llvm::TargetMachine is
being transferred from the LTOModule to the LTOCodeGenerator.

Practically, this would decouple the deallocation of the main contents of
LTOModule from the deallocation of its strings that have been passed to the
linker (e.g., ld64).  It would also give a memory win for consumers that do not
immediately call lto_module_dispose, without requiring a change to their code.

However, any consumers of the LTO interface that rely on the llvm::Module
and/or llvm::TargetMachine being accessible *after* LTOCodeGenerator::addModule
would be in trouble.

A few questions for comment:

1. Is this considered a change to the C API (and, thus, banned)?
2. Are there any consumers that rely on llvm::Module being accessible after
   LTOCodeGenerator::addModule?  What about llvm::TargetMachine?
3. Would the proposed behaviour be especially surprising?

My 2 cents...

On Tue, Jan 21, 2014 at 5:39 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
> 1. Is this considered a change to the C API (and, thus, banned)?
>
I think this would indeed be a significant semantic change to the C API.

> 2. Are there any consumers that rely on llvm::Module being accessible after
>    LTOCodeGenerator::addModule?  What about llvm::TargetMachine?
>
I think we have to assume so. That's the down side to saying it is a stable
C API.

> 3. Would the proposed behaviour be especially surprising?
>
No, the behavior you describe seems sensible. I would just expect it to
need to go under a separate/new API to preserve backwards compatibility.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140121/9df8b202/attachment.html>

On Jan 21, 2014, at 6:38 PM, Chandler Carruth <chandlerc at google.com>
wrote:
> My 2 cents...
> 
> On Tue, Jan 21, 2014 at 5:39 PM, Duncan P. N. Exon Smith <dexonsmith at
apple.com> wrote:
> 1. Is this considered a change to the C API (and, thus, banned)?
> 
> I think this would indeed be a significant semantic change to the C API.
>  
> 2. Are there any consumers that rely on llvm::Module being accessible after
>    LTOCodeGenerator::addModule?  What about llvm::TargetMachine?
> 
> I think we have to assume so. That's the down side to saying it is a
stable C API.
>  
> 3. Would the proposed behaviour be especially surprising?
> 
> No, the behavior you describe seems sensible. I would just expect it to
need to go under a separate/new API to preserve backwards compatibility.
Thanks Chandler.  I’ll go ahead with new API.