[Patch] Change linkInModule to take a std::unique_ptr

[Teresa Johnson via llvm-commits]

Thanks! A few comments/suggestions below.
Teresa

> +* The C API function LLVMLinkModules is deprecated. It will be removed in the
> +  3.9 release. Please migrate to LLVMLinkModules2.

Do we need a similar migration strategy for
linker_ocaml.c:llvm_link_modules? I.e. create a new llvm_link_modules2
and indicate the former is deprecated? Otherwise, users may be
blissfully unaware that the implementation of that interface will
change.

> diff --git a/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp
> index 6325a72..e0f56e8 100644
> --- a/lib/Transforms/IPO/FunctionImport.cpp
> +++ b/lib/Transforms/IPO/FunctionImport.cpp
> @@ -71,6 +71,12 @@ public:
>
>    /// Retrieve a Module from the cache or lazily load it on demand.
>    Module &operator()(StringRef FileName);
> +
> +  std::unique_ptr<Module> takeModule(StringRef FileName) {
> +    auto I = ModuleMap.find(FileName);
> +    assert(I != ModuleMap.end());
> +    return std::move(I->second);

Should also remove it from ModuleMap at this point, since ownership
has transferred.

> +  }
>  };
>
>  // Get a Module for \p FileName from the cache, or load it lazily.
> @@ -288,13 +294,14 @@ bool FunctionImporter::importFunctions(Module &DestModule) {
>    for (auto &FunctionsToImportPerModule : ModuleToFunctionsToImportMap) {
>      // Get the module for the import
>      auto &FunctionsToImport = FunctionsToImportPerModule.second.second;
> -    auto *SrcModule = FunctionsToImportPerModule.second.first;

The ModuleToFunctionsToImportMap should change to simply:

std::map<StringRef, DenseSet<const GlobalValue *>> ModuleToFunctionsToImportMap;

i.e. remove the Module pointer, just map directly from the Module
identifier to the function set. The Module pointer isn't used any
longer and becomes corrupted/invalid after linkInModule anyway.

> +    std::unique_ptr<Module> SrcModule =
> +        ModuleLoaderCache.takeModule(FunctionsToImportPerModule.first);
>      assert(&DestModule.getContext() == &SrcModule->getContext() &&
>             "Context mismatch");
>
>      // Link in the specified functions.
> -    if (TheLinker.linkInModule(*SrcModule, Linker::Flags::None, &Index,
> -                               &FunctionsToImport))
> +    if (TheLinker.linkInModule(std::move(SrcModule), Linker::Flags::None,
> +                               &Index, &FunctionsToImport))
>        report_fatal_error("Function Import: link error");
>
>      ImportedCount += FunctionsToImport.size();

On Thu, Dec 10, 2015 at 6:50 AM, Rafael Espíndola
<llvm-commits at lists.llvm.org> wrote:
> Rebased again.
>
>
> On 9 December 2015 at 17:59, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>> rebased patch attached.
>>
>> On 9 December 2015 at 10:55, Rafael Espíndola
>> <rafael.espindola at gmail.com> wrote:
>>> Looking a bit more at the C API I realized it is probably used as
>>> implemented, not as documented. I.E.: callers do free the second
>>> argument.
>>>
>>> Given that, it seems we will have to take the long road: Add a new API
>>> and deprecate the old one. That is what the attached patch does.
>>>
>>> Eric, could you please check that the patch does the deprecation dance
>>> correctly? Thanks.
>>>
>>> Cheers,
>>> Rafael
>>>
>>>
>>> On 8 December 2015 at 21:50, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>>> LGTM.
>>>>
>>>> —
>>>> Mehdi
>>>>
>>>>> On Dec 8, 2015, at 5:07 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>>>>>
>>>>> Passing in a std::unique_ptr should help find errors when the module
>>>>> is used after being linked into another module.
>>>>>
>>>>> This changes a C api, but changes it to what it is documented to do:
>>>>>
>>>>> * Links the source module into the destination module, taking ownership
>>>>> * of the source module away from the caller.
>>>>>
>>>>> Cheers,
>>>>> Rafael
>>>>> <t.diff>
>>>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>



-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413