[ELF] Make shouldAddProvideSym return values consistent when demoted to Undefined

[MaskRay]

Case: PROVIDE(f1 = bar); when both f1 and bar are in separate
sections that would be discarded by GC.

Due to demoteDefined, shouldAddProvideSym(f1) may initially return
false (when Defined) and then return true (been demoted to Undefined).

addScriptReferencedSymbolsToSymTable
  shouldAddProvideSym(f1): false
  // the RHS (bar) is not added to `referencedSymbols` and may be GCed
declareSymbols
  shouldAddProvideSym(f1): false
markLive
demoteSymbolsAndComputeIsPreemptible
  // demoted f1 to Undefined
processSymbolAssignments
  addSymbol
    shouldAddProvideSym(f1): true

The inconsistency can cause cmd->expression() in addSymbol to be
evaluated, leading to symbol not found: bar errors (since bar in the
RHS is not in referencedSymbols and is GCed) (#111478).

Fix this by adding a sym->isUsedInRegularObj condition, making
shouldAddProvideSym(f1) values consistent. In addition, we need a
sym->exportDynamic condition to keep provide-shared.s working.

Fixes: ebb326a

[llvmbot]

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Fangrui Song (MaskRay)

Changes

Case: PROVIDE(f1 = bar); when both f1 and bar are in separate sections
that would be discarded by GC.

Due to demoteDefined, shouldAddProvideSym(f1) may initially return
false (when Defined) and then return true (been demoted to Undefined).

addScriptReferencedSymbolsToSymTable
  shouldAddProvideSym(f1): false
  // the RHS (bar) is not added to `referencedSymbols` and may be GCed
declareSymbols
  shouldAddProvideSym(f1): false
markLive
demoteSymbolsAndComputeIsPreemptible
  // demoted f1 to Undefined
processSymbolAssignments
  addSymbol
    shouldAddProvideSym(f1): true

When cmd->expression() in addSymbol is evaluated, this could lead to
symbol not found: bar errors (#111478).

Fix this by adding a sym->isUsedInRegularObj condition, making
shouldAddProvideSym(f1) values consistent. In addition, we need a
sym->exportDynamic condition to keep provide-shared.s working.

---

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

2 Files Affected:

• (modified) lld/ELF/LinkerScript.cpp (+6-1)
• (added) lld/test/ELF/linkerscript/provide-defined.s (+33)

diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index f3f95ec589bd82..2247d592c8bca3 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -1811,6 +1811,11 @@ void LinkerScript::addScriptReferencedSymbolsToSymTable() {
 }
 
 bool LinkerScript::shouldAddProvideSym(StringRef symName) {
+  // A Defined may be demoted to Undefined but the return value must stay the
+  // same. Use isUsedInRegularObj to ensure this. The exportDynamic condition,
+  // while not so accurate, allows PROVIDE to define a symbol referenced by a
+  // DSO.
   Symbol *sym = elf::ctx.symtab->find(symName);
-  return sym && !sym->isDefined() && !sym->isCommon();
+  return sym && !sym->isDefined() && !sym->isCommon() &&
+         (sym->isUsedInRegularObj || sym->exportDynamic);
 }
diff --git a/lld/test/ELF/linkerscript/provide-defined.s b/lld/test/ELF/linkerscript/provide-defined.s
new file mode 100644
index 00000000000000..6ebfd706c16e1b
--- /dev/null
+++ b/lld/test/ELF/linkerscript/provide-defined.s
@@ -0,0 +1,33 @@
+# REQUIRES: x86
+## Test the GC behavior when the PROVIDE symbol is defined by a relocatable file.
+
+# RUN: rm -rf %t && split-file %s %t && cd %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 a.s -o a.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 b.s -o b.o
+# RUN: ld.lld -T a.t --gc-sections a.o b.o -o a
+# RUN: llvm-readelf -s a | FileCheck %s
+
+# CHECK:     1: {{.*}}               0 NOTYPE  GLOBAL DEFAULT     1 _start
+# CHECK-NEXT:2: {{.*}}               0 NOTYPE  GLOBAL DEFAULT     2 f3
+# CHECK-NOT: {{.}}
+
+#--- a.s
+.global _start, f1, f2, f3, bar
+_start:
+  call f3
+
+.section .text.f1,"ax"; f1:
+.section .text.f2,"ax"; f2:
+.section .text.f3,"ax"; f3:
+.section .text.bar,"ax"; bar:
+
+#--- b.s
+  call f2
+
+#--- a.t
+SECTIONS {
+  . = . + SIZEOF_HEADERS;
+  PROVIDE(f1 = bar+1);
+  PROVIDE(f2 = bar+2);
+  PROVIDE(f3 = bar+3);
+}

[smithp35]

LGTM. One small suggestion for the comment.

[smithp35]

I got a bit confused about whether the return value was this function's return value or the symbol's value. After thinking it through it is definitely the former.

A suggestion:

This function is called before and after garbage collection. To prevent undefined references the result of this function for a symbol must be the same for each call. We use isUsedInRegularObj to
prevent garbage collection demoting a Defined to an Undefnined. The exportDynamic condition, while not so accurate, allows PROVIDE to define a symbol referenced by a DSO.

[MaskRay]

Thanks for the review and comment suggestion. Adopted

[aeubanks]

hi, I've bisected an lld crash to this PR, reproducer tar is in https://crbug.com/373447237 if you could take a look

[MaskRay]

hi, I've bisected an lld crash to this PR, reproducer tar is in https://crbug.com/373447237 if you could take a look

This is a -shared link that uses PROVIDE(linker_script_start_of_text = ...) with references only from LLVM bitcode files.
The current PROVIDE mechanism cannot easily support both #111478 and https://crbug.com/373447237 .

As a workaround, you can remove PROVIDE or add -u

% ~/Stable/bin/ld.lld @response.txt -y linker_script_start_of_text -u linker_script_start_of_text
<internal>: reference to linker_script_start_of_text
repos/chromium/src/out/clang/obj/base/libbase.a(base/anchor_functions.o): reference to linker_script_start_of_text
<internal>: definition of linker_script_start_of_text
repos/chromium/src/out/clang/obj/base/libfast_float_fast_int_test__library.so.lto.libbase.a(anchor_functions.o at 1277214).o: reference to linker_script_start_of_text
repos/chromium/src/base/android/library_loader/anchor_functions.lds:6: definition of linker_script_start_of_text

[aeubanks]

#112386 fixes the crash, so we don't need a workaround?

[MaskRay]

#112386 fixes the crash, so we don't need a workaround?

workaround not needed:)