[llvm-dev] Using Linker with a cloned module

[Mehdi Amini via llvm-dev]

Hi,

My current take on this issue is that named types should not be entirely stored in the context.

We should only store there the “anonymous” type, and have the mapping of name -> type in the modules themselves.

There might be some good reasons why this wouldn’t work, but I haven’t looked deeply enough to be sure.

— 
Mehdi

> On Nov 9, 2016, at 12:45 AM, Savonichev, Andrew via llvm-dev <llvm-dev at lists.llvm.org> wrote:
> 
> Hi,
> 
> I was trying to use the Linker with a module cloned from another module
> via CloneModule(), and it seems the Linker does not expect this use case.
> 
> The issue is in the addTypeMapping() function, which removes a name of
> a StructType from the Src module if an isomorphic StructType was found
> in the Dst module.
> 
> This behavior was introduced in r222986:
> http://llvm.org/viewvc/llvm-project?view=revision&revision=222986
> "Change how we keep track of which types are in the dest module.
> Instead of keeping an explicit set, just drop the names of types we choose
> to map to some other type."
> 
> This approach does not work if we pass a module clone created by
> CloneModule() as the Src, because 'prototype' and 'clone' modules share
> the same Type objects, therefore a type name change in one module would
> affect the other module as well.
> My expectation is that the 'prototype' should not be changed after we
> link a 'clone'.
> 
> Is there any expected limitations on using cloned modules?
> 
> I've attached a unittest to cover this issue.
> If fails with the current trunk:
> Failure:
>  Value of: STyNamesAfter.size()
>    Actual: 0
>  Expected: STyNamesBefore.size()
>  Which is: 1
> Failure:
>  Value of: NameAfterLink
>    Actual: ""
>  Expected: NameBeforeLink
>  Which is: "struct.foo.0"
> 
> 
>    - Andrew
> 
> ---
> unittests/Linker/LinkModulesTest.cpp | 58 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
> 
> diff --git a/unittests/Linker/LinkModulesTest.cpp b/unittests/Linker/LinkModulesTest.cpp
> index 92c4832..0a20759 100644
> --- a/unittests/Linker/LinkModulesTest.cpp
> +++ b/unittests/Linker/LinkModulesTest.cpp
> @@ -7,6 +7,7 @@
> //
> //===----------------------------------------------------------------------===//
> 
> +#include "llvm/ADT/DenseMap.h"
> #include "llvm/ADT/STLExtras.h"
> #include "llvm/AsmParser/Parser.h"
> #include "llvm/IR/BasicBlock.h"
> @@ -16,6 +17,7 @@
> #include "llvm/IR/Module.h"
> #include "llvm/Linker/Linker.h"
> #include "llvm/Support/SourceMgr.h"
> +#include <llvm/Transforms/Utils/Cloning.h>
> #include "llvm-c/Core.h"
> #include "llvm-c/Linker.h"
> #include "gtest/gtest.h"
> @@ -360,4 +362,60 @@ TEST_F(LinkModuleTest, RemangleIntrinsics) {
>   ASSERT_EQ(F->getNumUses(), (unsigned)2);
> }
> 
> +TEST_F(LinkModuleTest, ModuleClone) {
> +  LLVMContext C;
> +  SMDiagnostic Err;
> +
> +  Ctx.setDiagnosticHandler(expectNoDiags);
> +
> +  const char *M1Str =
> +    "%struct.foo = type { i32 }"
> +    "declare %struct.foo* @f(...)"
> +    "define void @baz() {"
> +    "entry:"
> +    "  %0 = alloca %struct.foo*, align 8"
> +    "  %call = call %struct.foo* (...) @f()"
> +    "  store %struct.foo* %call, %struct.foo** %0, align 8"
> +    "  ret void"
> +    "}";
> +
> +  const char *M2Str =
> +    "%struct.foo = type { i32 }"
> +    "define %struct.foo* @f() {"
> +    "entry:"
> +    "  ret %struct.foo* null"
> +    "}";
> +
> +  std::unique_ptr<Module> M1 = parseAssemblyString(M1Str, Err, C);
> +  std::unique_ptr<Module> M2 = parseAssemblyString(M2Str, Err, C);
> +
> +  ValueToValueMapTy VMap;
> +  std::unique_ptr<Module> M2Clone = CloneModule(M2.get(), VMap);
> +
> +  DenseMap<StructType *, std::string> STyNamesBefore;
> +  for (StructType *Ty : M2->getIdentifiedStructTypes()) {
> +    STyNamesBefore[Ty] = (Ty->hasName()) ? Ty->getName()
> +                                         : "";
> +  }
> +
> +  // use the clone instead of M2
> +  Linker::linkModules(*M1, std::move(M2Clone));
> +
> +  DenseMap<StructType *, std::string> STyNamesAfter;
> +  for (StructType *Ty : M2->getIdentifiedStructTypes()) {
> +    STyNamesAfter[Ty] = (Ty->hasName()) ? Ty->getName()
> +                                        : "";
> +  }
> +
> +  // check that the types are not changed in the prototype module
> +  // after link
> +  EXPECT_EQ(STyNamesBefore.size(), STyNamesAfter.size());
> +  for (auto TyNamePair : STyNamesBefore) {
> +    std::string NameBeforeLink = TyNamePair.second;
> +    std::string NameAfterLink = STyNamesAfter[TyNamePair.first];
> +    EXPECT_EQ(NameBeforeLink, NameAfterLink);
> +  }
> +}
> +
> +
> } // end anonymous namespace
> --
> 2.8.3
> 
> --------------------------------------------------------------------
> Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park,
> 17 Krylatskaya Str., Bldg 4, Moscow 121614,
> Russian Federation
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev