[llvm-link] Add the --ignore-if-conflict option

[DianQK]

Adding the --ignore-if-conflict option supports the following usage case.

The user code (user.ll):

define void __addsf3() { ... }
define void foo() { ... }

The builtin function provided by the compiler (builtin.ll):

define void __addsf3() { ... }

I want to allow user to override builtin functions under LTO. I can't use --override, which might cause the foo symbol to accidentally override a symbol of the same name.

[llvmbot]

@llvm/pr-subscribers-lto

@llvm/pr-subscribers-llvm-binary-utilities

Author: DianQK (DianQK)

Changes

Adding the --ignore-if-conflict option supports the following usage case.

The user code (user.ll):

define void __addsf3() { ... }
define void foo() { ... }

The builtin function provided by the compiler (builtin.ll):

define void __addsf3() { ... }

I want to allow user to override builtin functions under LTO. I can't use --override, which might cause the foo symbol to accidentally override a symbol of the same name.

---

Full diff: https://github.com/llvm/llvm-project/pull/75696.diff

7 Files Affected:

• (modified) llvm/docs/CommandGuide/llvm-link.rst (+8)
• (modified) llvm/include/llvm/Linker/Linker.h (+1)
• (modified) llvm/lib/Linker/LinkModules.cpp (+8)
• (added) llvm/test/tools/llvm-link/Inputs/f_1.ll (+9)
• (added) llvm/test/tools/llvm-link/ignore.test (+8)
• (modified) llvm/tools/llvm-link/llvm-link.cpp (+13-1)
• (modified) llvm/unittests/Linker/LinkModulesTest.cpp (+10)

diff --git a/llvm/docs/CommandGuide/llvm-link.rst b/llvm/docs/CommandGuide/llvm-link.rst
index 1cc1376becf9df..dd06cf4a99e5aa 100644
--- a/llvm/docs/CommandGuide/llvm-link.rst
+++ b/llvm/docs/CommandGuide/llvm-link.rst
@@ -57,6 +57,14 @@ OPTIONS
   a symbol is declared more than twice, the definition from the file declared
   last takes precedence.
 
+.. option:: --ignore-if-conflict <filename>
+
+  Adds the passed-in file to the link and ignores symbols that have already
+  been declared with the definitions in the file that is passed in. This flag
+  can be specified multiple times to have multiple files act as ignores. If
+  a symbol is declared more than twice, the definition from the file declared
+  first takes precedence.
+
 .. option:: --import <function:filename>
 
   Specify a function that should be imported from the specified file for
diff --git a/llvm/include/llvm/Linker/Linker.h b/llvm/include/llvm/Linker/Linker.h
index ac8041d8df1afa..5556e863c15884 100644
--- a/llvm/include/llvm/Linker/Linker.h
+++ b/llvm/include/llvm/Linker/Linker.h
@@ -27,6 +27,7 @@ class Linker {
     None = 0,
     OverrideFromSrc = (1 << 0),
     LinkOnlyNeeded = (1 << 1),
+    IgnoreFromSrcIfConflict = (1 << 2),
   };
 
   Linker(Module &M);
diff --git a/llvm/lib/Linker/LinkModules.cpp b/llvm/lib/Linker/LinkModules.cpp
index 4fe1f1a0f51833..8195e68539e652 100644
--- a/llvm/lib/Linker/LinkModules.cpp
+++ b/llvm/lib/Linker/LinkModules.cpp
@@ -52,6 +52,9 @@ class ModuleLinker {
 
   bool shouldOverrideFromSrc() { return Flags & Linker::OverrideFromSrc; }
   bool shouldLinkOnlyNeeded() { return Flags & Linker::LinkOnlyNeeded; }
+  bool shouldIgnoreFromSrcIfConflict() {
+    return Flags & Linker::IgnoreFromSrcIfConflict;
+  }
 
   bool shouldLinkFromSource(bool &LinkFromSrc, const GlobalValue &Dest,
                             const GlobalValue &Src);
@@ -317,6 +320,11 @@ bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,
     return false;
   }
 
+  if (shouldIgnoreFromSrcIfConflict()) {
+    LinkFromSrc = false;
+    return false;
+  }
+
   assert(!Src.hasExternalWeakLinkage());
   assert(!Dest.hasExternalWeakLinkage());
   assert(Dest.hasExternalLinkage() && Src.hasExternalLinkage() &&
diff --git a/llvm/test/tools/llvm-link/Inputs/f_1.ll b/llvm/test/tools/llvm-link/Inputs/f_1.ll
new file mode 100644
index 00000000000000..a0b85a4563dc49
--- /dev/null
+++ b/llvm/test/tools/llvm-link/Inputs/f_1.ll
@@ -0,0 +1,9 @@
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+define void @f() {
+entry:
+  call void @f_1();
+  ret void
+}
+
+declare void @f_1();
diff --git a/llvm/test/tools/llvm-link/ignore.test b/llvm/test/tools/llvm-link/ignore.test
new file mode 100644
index 00000000000000..eac9c3b548afc5
--- /dev/null
+++ b/llvm/test/tools/llvm-link/ignore.test
@@ -0,0 +1,8 @@
+# RUN: not llvm-link %S/Inputs/f.ll %S/Inputs/f_1.ll 2>&1 | FileCheck %s
+# RUN: llvm-link %S/Inputs/f.ll --ignore-if-conflict %S/Inputs/f_1.ll -S | FileCheck -check-prefix=IGNORE %s
+
+# CHECK: error: Linking globals named 'f': symbol multiply defined!
+
+# IGNORE: define void @f() {
+# IGNORE-NEXT: entry:
+# IGNORE-NEXT: ret void
diff --git a/llvm/tools/llvm-link/llvm-link.cpp b/llvm/tools/llvm-link/llvm-link.cpp
index a476b50a1ed90b..33d39e6baf8eff 100644
--- a/llvm/tools/llvm-link/llvm-link.cpp
+++ b/llvm/tools/llvm-link/llvm-link.cpp
@@ -53,6 +53,12 @@ static cl::list<std::string> OverridingInputs(
         "input bitcode file which can override previously defined symbol(s)"),
     cl::cat(LinkCategory));
 
+static cl::list<std::string> IgnoreIfConflictInputs(
+    "ignore-if-conflict", cl::value_desc("filename"),
+    cl::desc("defined symbol(s) of input bitcode file which can be ignore if "
+             "previously same defined symbol(s)"),
+    cl::cat(LinkCategory));
+
 // Option to simulate function importing for testing. This enables using
 // llvm-link to simulate ThinLTO backend processes.
 static cl::list<std::string> Imports(
@@ -382,7 +388,8 @@ static bool importFunctions(const char *argv0, Module &DestModule) {
 static bool linkFiles(const char *argv0, LLVMContext &Context, Linker &L,
                       const cl::list<std::string> &Files, unsigned Flags) {
   // Filter out flags that don't apply to the first file we load.
-  unsigned ApplicableFlags = Flags & Linker::Flags::OverrideFromSrc;
+  unsigned ApplicableFlags = Flags & (Linker::Flags::OverrideFromSrc |
+                                      Linker::Flags::IgnoreFromSrcIfConflict);
   // Similar to some flags, internalization doesn't apply to the first file.
   bool InternalizeLinkedSymbols = false;
   for (const auto &File : Files) {
@@ -488,6 +495,11 @@ int main(int argc, char **argv) {
                  Flags | Linker::Flags::OverrideFromSrc))
     return 1;
 
+  // Next the -ignore-if-conflict ones.
+  if (!linkFiles(argv[0], Context, L, IgnoreIfConflictInputs,
+                 Flags | Linker::Flags::IgnoreFromSrcIfConflict))
+    return 1;
+
   // Import any functions requested via -import
   if (!importFunctions(argv[0], *Composite))
     return 1;
diff --git a/llvm/unittests/Linker/LinkModulesTest.cpp b/llvm/unittests/Linker/LinkModulesTest.cpp
index 182ce73178c1d4..25adf74db8c1db 100644
--- a/llvm/unittests/Linker/LinkModulesTest.cpp
+++ b/llvm/unittests/Linker/LinkModulesTest.cpp
@@ -233,6 +233,16 @@ TEST_F(LinkModuleTest, NewCAPIFailure) {
   EXPECT_EQ("Linking globals named 'foo': symbol multiply defined!", Err);
 }
 
+TEST_F(LinkModuleTest, IgnoreFromSrcIfConflict) {
+  LLVMContext Ctx;
+  std::unique_ptr<Module> DestM(getExternal(Ctx, "foo"));
+  std::unique_ptr<Module> SourceM(getExternal(Ctx, "foo"));
+  Ctx.setDiagnosticHandlerCallBack(expectNoDiags);
+  bool Failed = Linker::linkModules(*DestM, std::move(SourceM),
+                                    Linker::Flags::IgnoreFromSrcIfConflict);
+  ASSERT_FALSE(Failed);
+}
+
 TEST_F(LinkModuleTest, MoveDistinctMDs) {
   LLVMContext C;
   SMDiagnostic Err;

[teresajohnson]

I'm confused by the motivation. The description says it is about overriding compiler builtins, but the source changes don't do anything specific regarding builtins. The source changes just say ignore any already defined symbols in the given input files. Normally one would give symbols that can be overridden weak linkage.

[DianQK]

I'm confused by the motivation. The description says it is about overriding compiler builtins, but the source changes don't do anything specific regarding builtins. The source changes just say ignore any already defined symbols in the given input files. Normally one would give symbols that can be overridden weak linkage.

I just used ... in the description to ignore the diff code. The rust doesn't seem to be set to weak by default, I'm not sure of the historical reasons here.

But I found a new issue, even adding this new argument does not solve the issue. For example,

user.ll:

target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
target triple = "thumbv6m-none-unknown-eabi"

module asm ".global __aeabi_uidivmod"
module asm ".type __aeabi_uidivmod, %function"
module asm "__aeabi_uidivmod:"
module asm "str    r0, [r2, #0x060]" ; It's just an example.
module asm "str    r1, [r2, #0x064]"

builtin.ll:

target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
target triple = "thumbv6m-none-unknown-eabi"

; Function Attrs: naked nocf_check noinline nounwind
define weak hidden void @__aeabi_uidivmod() unnamed_addr {
  tail call void asm sideeffect alignstack "push {lr}\0Asub sp, sp, #4\0Amov r2, sp\0Abl __udivmodsi4\0Aldr r1, [sp]\0Aadd sp, sp, #4\0Apop {pc}", "~{cc},~{memory}"()
  unreachable
}

I guess I should add the naked function to user.ll to solve it.