llvm dev - Nov 2016 - Using Linker with a cloned module

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.

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