Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lld][ELF] Error when deplibs adds new input file after LTO #98565

Merged
merged 3 commits into from
Jul 12, 2024

Conversation

mysterymath
Copy link
Contributor

Parsing the new input file's symbols might invalidate LTO codegen, but the semantics of deplibs require them to be parsed. Accordingly, report an error unless the file had already been added to the link.

Fixes #56070

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 11, 2024

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Daniel Thornburgh (mysterymath)

Changes

Parsing the new input file's symbols might invalidate LTO codegen, but the semantics of deplibs require them to be parsed. Accordingly, report an error unless the file had already been added to the link.

Fixes #56070


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

2 Files Affected:

  • (modified) lld/ELF/Driver.cpp (+15)
  • (added) lld/test/ELF/deplibs-lto.s (+39)
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index ed773f5e69f77..18029cb71f074 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -2956,6 +2956,7 @@ template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
   // With this the symbol table should be complete. After this, no new names
   // except a few linker-synthesized ones will be added to the symbol table.
   const size_t numObjsBeforeLTO = ctx.objectFiles.size();
+  const size_t numInputFilesBeforeLTO = ctx.driver.files.size();
   compileBitcodeFiles<ELFT>(skipLinkedOutput);
 
   // Symbol resolution finished. Report backward reference problems,
@@ -2980,6 +2981,20 @@ template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
   for (const DuplicateSymbol &d : ctx.duplicates)
     reportDuplicate(*d.sym, d.file, d.section, d.value);
 
+  // ELF dependent libraries may have introduced new input files after LTO has
+  // completed. This is an error if the file hadn't already been parsed, since
+  // it's no longer legal to change the symbol table by parsing it.
+  auto newInputFiles = ArrayRef(ctx.driver.files).slice(numInputFilesBeforeLTO);
+  if (!newInputFiles.empty()) {
+    DenseSet<StringRef> oldFilenames;
+    for (InputFile *f :
+         ArrayRef(ctx.driver.files).slice(0, numInputFilesBeforeLTO))
+      oldFilenames.insert(f->getName());
+    for (InputFile *newFile : newInputFiles)
+      if (!oldFilenames.contains(newFile->getName()))
+        error("input file '" + newFile->getName() + "' added after LTO");
+  }
+
   // Handle --exclude-libs again because lto.tmp may reference additional
   // libcalls symbols defined in an excluded archive. This may override
   // versionId set by scanVersionScript().
diff --git a/lld/test/ELF/deplibs-lto.s b/lld/test/ELF/deplibs-lto.s
new file mode 100644
index 0000000000000..5c909197b445f
--- /dev/null
+++ b/lld/test/ELF/deplibs-lto.s
@@ -0,0 +1,39 @@
+# REQUIRES: x86
+
+# RUN: rm -rf %t && split-file %s %t && cd %t
+# RUN: llvm-mc -filetype=obj -triple=aarch64 -o deplibs.o deplibs.s
+# RUN: llvm-mc -filetype=obj -triple=aarch64 -o foo.o foo.s
+# RUN: llvm-as -o lto.o lto.ll
+# RUN: llvm-ar rc libdeplibs.a deplibs.o
+# RUN: llvm-ar rc libfoo.a foo.o
+
+## LTO emits a libcall (`__aarch64_ldadd4_relax`) that is resolved using a
+## library (libdeplibc.a) that contains a `.deplibs` section pointing to a file
+## not yet added to the link.
+
+# RUN: not ld.lld lto.o -u a -L. -ldeplibs 2>&1 | FileCheck %s
+# CHECK: error: input file 'foo.o' added after LTO
+
+## Including the file before LTO prevents the issue.
+
+# RUN: ld.lld lto.o -u a -L. -ldeplibs -lfoo
+
+#--- foo.s
+.global foo
+foo:
+#--- deplibs.s
+.global __aarch64_ldadd4_relax
+__aarch64_ldadd4_relax:
+    b foo
+.section ".deplibs","MS",@llvm_dependent_libraries,1
+    .asciz "foo"
+#--- lto.ll
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64"
+
+define void @a(i32* nocapture %0) #0 {
+  %2 = atomicrmw add i32* %0, i32 1 monotonic, align 4
+  ret void
+}
+
+attributes #0 = { "target-features"="+outline-atomics" }

@@ -0,0 +1,39 @@
# REQUIRES: x86
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aarch64

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

# RUN: llvm-ar rc libfoo.a foo.o

## LTO emits a libcall (`__aarch64_ldadd4_relax`) that is resolved using a
## library (libdeplibc.a) that contains a `.deplibs` section pointing to a file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libdeplibs.a

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

# CHECK: error: input file 'foo.o' added after LTO

## Including the file before LTO prevents the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the blank line. The convention is to have ## immediately precede associated RUN lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -2980,6 +2981,20 @@ template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
for (const DuplicateSymbol &d : ctx.duplicates)
reportDuplicate(*d.sym, d.file, d.section, d.value);

// ELF dependent libraries may have introduced new input files after LTO has
// completed. This is an error if the files haven't already been parsed, since
// it's no longer legal to change the symbol table by parsing them.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"no longer legal ..." => changing the symbol table could break the semantic assumption of LTO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Parsing the new input file's symbols might invalidate LTO codegen, but
the semantics of deplibs require them to be parsed. Accordingly, report
an error unless the file had already been added to the link.

Fixes llvm#56070
 - Consistent plurality in comment
 - errorOrWarn, since an output file may still producible
- Require aarch64
- Fix typo
- Remove blank lines in test
- Include reason for illegality in comment

Misc:

- Remove backticks around file and section names
@mysterymath mysterymath merged commit 5b82741 into llvm:main Jul 12, 2024
3 of 5 checks passed
@mysterymath mysterymath deleted the lld-deplibs branch July 12, 2024 19:44
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
Parsing the new input file's symbols might invalidate LTO codegen, but
the semantics of deplibs require them to be parsed. Accordingly, report
an error unless the file had already been added to the link.

Fixes llvm#56070
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LTO] Objects brought in by LTO-generated libcalls do not handle deplibs.
3 participants