[remangleIntrinsicFunction] Detect and resolve name clash

[jeroen.dobbelaere]

Summary

It is possible that the remangled name for an intrinsic already exists with a different (and wrong) prototype within the module.
As the bitcode reader keeps both versions of all remangled intrinsics around for a longer time, this can result in a
crash, as can be seen in https://bugs.llvm.org/show_bug.cgi?id=50923

This patch makes 'remangleIntrinsicFunction' aware of this situation. When it is detected, it moves the version with the wrong prototype to a different name. That version will be removed anyway once the module is completely loaded.

With thanks to @asbirlea for reporting this issue when trying out an lto build with the full restrict patches, and @efriedma for suggesting a sane resolution mechanism.

[efriedma]

llvm/lib/IR/Function.cpp

1670

Probably better not to assume the GlobalValue is a Function. Probably an invalid module if it isn't a function, but we might not check for that until later.

[jeroen.dobbelaere]

• handle the case where the clashing GV is not a function.

[apilipenko]

llvm/lib/IR/Function.cpp

1667

I think it should be remangleIntrinsicFunction caller responsibility to do this kind of adjustment. Some of the callers (like LLParser) will just assert that the resulting function matches the signature. Others, like bitcode reader, will need to handle this situation.

[jeroen.dobbelaere]

llvm/lib/IR/Function.cpp

1667

Even in the LLParser use case, I think we can end up in a similar situation: a first rewrite could end up in a name clash with a function that is only rewritten later.

[jeroen.dobbelaere]

Added testcase showing a similar problem in LLParser that is also fixed by this patch.

[apilipenko]

Looks good.

May I ask you to document the new side effect (possible renaming of another function in the Module) of remangleIntrinsicFunction in the corresponding header?

llvm/lib/IR/Function.cpp

1666–1681

Stylistic suggestions. This can be restructured to use a lambda and early exits. Something like this:

Function *NewDecl = [&] () {
  if (auto *ExistingGV = F->getParent()->getNamedValue(WantedName)) {
    if (auto *ExistingF = dyn_cast<Function>(ExistingGV))
      if (ExistingF->getFunctionType() == F->getFunctionType())
        return ExistingF;

    // The name already exists, but is not a function or has the wrong
    // prototype. Make place for the new one by renaming the old version.
    // Either this old version will be removed later on or the module is
    // invalid and we'll get an error.
    ExistingGV->setName(WantedName + ".renamed");
  }

  return Intrinsic::getDeclaration(F->getParent(), ID, ArgTys);
}();

[jeroen.dobbelaere]

Rebased; Added comment about the side effect. Stylistic change as proposed by Artur.

[jeroen.dobbelaere]

Thanks for the review and the suggestions.

I'll do the actual commit tomorrow.

[asbirlea]

Thank you for this fix!

llvm/lib/IR/Function.cpp

 
 Optional<Function *> Intrinsic::remangleIntrinsicFunction(Function *F) {
   SmallVector<Type *, 4> ArgTys;
   if (!getIntrinsicSignature(F, ArgTys))
     return None;
 
   Intrinsic::ID ID = F->getIntrinsicID();
   StringRef Name = F->getName();
-  if (Name ==
-      Intrinsic::getName(ID, ArgTys, F->getParent(), F->getFunctionType()))
+  std::string WantedName =
+      Intrinsic::getName(ID, ArgTys, F->getParent(), F->getFunctionType());
+  if (Name == WantedName)
     return None;
 
-  auto NewDecl = Intrinsic::getDeclaration(F->getParent(), ID, ArgTys);
+  GlobalValue *ExistingF = F->getParent()->getNamedValue(WantedName);
+
+  Function *NewDecl = nullptr;
+  if (ExistingF) {
+    if (cast<Function>(ExistingF)->getFunctionType() == F->getFunctionType()) {
+      NewDecl = cast<Function>(ExistingF);
+    } else {
+      // The name already exists, but has the wrong prototype. Make place for
+      // the new one by renaming the old version.
+      ExistingF->setName(Name + ".renamed");
+    }
+  }
+
+  if (!NewDecl)
+    NewDecl = Intrinsic::getDeclaration(F->getParent(), ID, ArgTys);
   NewDecl->setCallingConv(F->getCallingConv());
   assert(NewDecl->getFunctionType() == F->getFunctionType() &&
          "Shouldn't change the signature");
   return NewDecl;
 }
 
 /// hasAddressTaken - returns true if there are any uses of this function
 /// other than direct calls or invokes to it. Optionally ignores callback
 /// uses, assume like pointer annotation calls, and references in llvm.used

llvm/test/tools/llvm-link/Inputs/remangle1.ll

+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%aaa = type <{ %aab, i32, [4 x i8] }>
+%aab = type { i64 }
+%fum = type { %aac, i8, [7 x i8] }
+%aac = type { [8 x i8] }
+
+declare void @bar01(%aaa*)
+declare void @bar02(%fum*)

llvm/test/tools/llvm-link/Inputs/remangle2.ll

+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%fum = type { %aab, i8, [7 x i8] }
+%aab = type { %aba }
+%aba = type { [8 x i8] }
+%fum.1 = type { %abb, i8, [7 x i8] }
+%abb = type { %abc }
+%abc = type { [4 x i8] }
+
+declare void @foo(%fum*)
+
+declare %fum.1** @"llvm.ssa.copy.p0p0s_fum.1s"(%fum.1**)
+
+declare %fum** @"llvm.ssa.copy.p0p0s_fums"(%fum**)
+
+define void @foo1(%fum** %a, %fum.1 ** %b) {
+  %b.copy = call %fum.1** @"llvm.ssa.copy.p0p0s_fum.1s"(%fum.1** %b)
+  %a.copy = call %fum** @"llvm.ssa.copy.p0p0s_fums"(%fum** %a)
+  ret void
+}
+
+define void @foo2(%fum.1 ** %b, %fum** %a) {
+  %a.copy = call %fum** @"llvm.ssa.copy.p0p0s_fums"(%fum** %a)
+  %b.copy = call %fum.1** @"llvm.ssa.copy.p0p0s_fum.1s"(%fum.1** %b)
+  ret void
+}

llvm/test/tools/llvm-link/remangle.ll

+# RUN: llvm-as %S/Inputs/remangle1.ll -o %t.remangle1.bc
+# RUN: llvm-as %S/Inputs/remangle2.ll -o %t.remangle2.bc
+# RUN: llvm-link %t.remangle1.bc %t.remangle2.bc -o %t.remangle.linked.bc
+# RUN: llvm-dis %t.remangle.linked.bc -o - | FileCheck %s
+
+; CHECK-DAG: %fum.1 = type { %aab.0, i8, [7 x i8] }
+; CHECK-DAG: %aab.0 = type { %aba }
+; CHECK-DAG: %aba = type { [8 x i8] }
+; CHECK-DAG: %fum.1.2 = type { %abb, i8, [7 x i8] }
+; CHECK-DAG: %abb = type { %abc }
+; CHECK-DAG: %abc = type { [4 x i8] }
+
+; CHECK-LABEL: define void @foo1(%fum.1** %a, %fum.1.2** %b) {
+; CHECK-NEXT:   %b.copy = call %fum.1.2** @llvm.ssa.copy.p0p0s_fum.1.2s(%fum.1.2** %b)
+; CHECK-NEXT:   %a.copy = call %fum.1** @llvm.ssa.copy.p0p0s_fum.1s(%fum.1** %a)
+; CHECK-NEXT:  ret void
+
+; CHECK: declare %fum.1.2** @llvm.ssa.copy.p0p0s_fum.1.2s(%fum.1.2** returned)
+
+; CHECK: declare %fum.1** @llvm.ssa.copy.p0p0s_fum.1s(%fum.1** returned)
+
+; CHECK-LABEL: define void @foo2(%fum.1.2** %b, %fum.1** %a) {
+; CHECK-NEXT:   %a.copy = call %fum.1** @llvm.ssa.copy.p0p0s_fum.1s(%fum.1** %a)
+; CHECK-NEXT:  %b.copy = call %fum.1.2** @llvm.ssa.copy.p0p0s_fum.1.2s(%fum.1.2** %b)
+; CHECK-NEXT:  ret void
+
+