[LLVMdev] [RFC] LTO: deallocating llvm::Module inside lto_codegen_add_module

[Duncan P. N. Exon Smith]

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?