[ELF] Postpone more linker script errors

[MaskRay]

Since assignAddresses is executed more than once, error reporting
during assignAddresses would be duplicated. Generalize #66854 to cover
more errors.

Note: address-related errors exposed in one invocation might not be
errors in another invocation.

[llvmbot]

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Fangrui Song (MaskRay)

Changes

Since assignAddresses is executed more than once, error reporting
during assignAddresses would be duplicated. Generalize #66854 to cover
more errors.

---

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

5 Files Affected:

• (modified) lld/ELF/LinkerScript.cpp (+11-8)
• (modified) lld/ELF/LinkerScript.h (+6-1)
• (modified) lld/ELF/ScriptParser.cpp (+3-2)
• (modified) lld/test/ELF/linkerscript/addr.test (+2-4)
• (modified) lld/test/ELF/linkerscript/locationcountererr-arm-exidx.test (+3-1)

diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 68f5240ddc690..90824300c3b73 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -177,11 +177,10 @@ void LinkerScript::setDot(Expr e, const Twine &loc, bool inSec) {
   // report it if this is the last assignAddresses iteration. dot may be smaller
   // if there is another assignAddresses iteration.
   if (val < dot && inSec) {
-    backwardDotErr =
-        (loc + ": unable to move location counter (0x" + Twine::utohexstr(dot) +
-         ") backward to 0x" + Twine::utohexstr(val) + " for section '" +
-         state->outSec->name + "'")
-            .str();
+    recordError(loc + ": unable to move location counter (0x" +
+                Twine::utohexstr(dot) + ") backward to 0x" +
+                Twine::utohexstr(val) + " for section '" + state->outSec->name +
+                "'");
   }
 
   // Update to location counter means update to section size.
@@ -1411,7 +1410,7 @@ LinkerScript::assignAddresses() {
   state = &st;
   errorOnMissingSection = true;
   st.outSec = aether;
-  backwardDotErr.clear();
+  pendingErrors.clear();
 
   SymbolAssignmentMap oldValues = getSymbolAssignmentValues(sectionCommands);
   for (SectionCommand *cmd : sectionCommands) {
@@ -1661,6 +1660,10 @@ void LinkerScript::printMemoryUsage(raw_ostream& os) {
   }
 }
 
+void LinkerScript::recordError(const Twine &msg) {
+  pendingErrors.push_back(msg.str());
+}
+
 static void checkMemoryRegion(const MemoryRegion *region,
                               const OutputSection *osec, uint64_t addr) {
   uint64_t osecEnd = addr + osec->size;
@@ -1673,8 +1676,8 @@ static void checkMemoryRegion(const MemoryRegion *region,
 }
 
 void LinkerScript::checkFinalScriptConditions() const {
-  if (backwardDotErr.size())
-    errorOrWarn(backwardDotErr);
+  for (StringRef err : pendingErrors)
+    errorOrWarn(err);
   for (const OutputSection *sec : outputSections) {
     if (const MemoryRegion *memoryRegion = sec->memRegion)
       checkMemoryRegion(memoryRegion, sec, sec->addr);
diff --git a/lld/ELF/LinkerScript.h b/lld/ELF/LinkerScript.h
index 36feab36e26ba..dd0924a757493 100644
--- a/lld/ELF/LinkerScript.h
+++ b/lld/ELF/LinkerScript.h
@@ -350,6 +350,11 @@ class LinkerScript final {
   // Describe memory region usage.
   void printMemoryUsage(raw_ostream &os);
 
+  // Record a pending error during an assignAddresses invocation.
+  // assignAddresses is executed more than once. Therefore, lld::error should be
+  // avoided to not report duplicate errors.
+  void recordError(const Twine &msg);
+
   // Check backward location counter assignment and memory region/LMA overflows.
   void checkFinalScriptConditions() const;
 
@@ -375,7 +380,7 @@ class LinkerScript final {
   bool seenDataAlign = false;
   bool seenRelroEnd = false;
   bool errorOnMissingSection = false;
-  std::string backwardDotErr;
+  SmallVector<std::string, 0> pendingErrors;
 
   // List of section patterns specified with KEEP commands. They will
   // be kept even if they are unused and --gc-sections is specified.
diff --git a/lld/ELF/ScriptParser.cpp b/lld/ELF/ScriptParser.cpp
index f90ce6fa74075..db46263115242 100644
--- a/lld/ELF/ScriptParser.cpp
+++ b/lld/ELF/ScriptParser.cpp
@@ -158,7 +158,8 @@ static void moveAbsRight(ExprValue &a, ExprValue &b) {
   if (a.sec == nullptr || (a.forceAbsolute && !b.isAbsolute()))
     std::swap(a, b);
   if (!b.isAbsolute())
-    error(a.loc + ": at least one side of the expression must be absolute");
+    script->recordError(
+        a.loc + ": at least one side of the expression must be absolute");
 }
 
 static ExprValue add(ExprValue a, ExprValue b) {
@@ -1384,7 +1385,7 @@ StringRef ScriptParser::readParenLiteral() {
 
 static void checkIfExists(const OutputSection &osec, StringRef location) {
   if (osec.location.empty() && script->errorOnMissingSection)
-    error(location + ": undefined section " + osec.name);
+    script->recordError(location + ": undefined section " + osec.name);
 }
 
 static bool isValidSymbolName(StringRef s) {
diff --git a/lld/test/ELF/linkerscript/addr.test b/lld/test/ELF/linkerscript/addr.test
index c997525e3dc3f..53466dda26791 100644
--- a/lld/test/ELF/linkerscript/addr.test
+++ b/lld/test/ELF/linkerscript/addr.test
@@ -13,13 +13,11 @@
 # CHECK-NEXT: 0000000000001001    0 NOTYPE GLOBAL DEFAULT   1 x2
 # CHECK-NEXT: 0000000000001000    0 NOTYPE GLOBAL DEFAULT   1 x3
 
-## TODO Fix duplicate errors
 # RUN: not ld.lld a.o -T absent.lds 2>&1 | FileCheck %s --check-prefix=ABSENT --implicit-check-not=error:
-# ABSENT-COUNT-2: error: absent.lds:3: undefined section .aaa
+# ABSENT: error: absent.lds:3: undefined section .aaa
 
-## TODO Fix duplicate errors
 # RUN: not ld.lld a.o -T absolute.lds 2>&1 | FileCheck %s --check-prefix=ABSOLUTE --implicit-check-not=error:
-# ABSOLUTE-COUNT-4: error: absolute.lds:2: at least one side of the expression must be absolute
+# ABSOLUTE: error: absolute.lds:2: at least one side of the expression must be absolute
 
 #--- a.s
 .globl _start
diff --git a/lld/test/ELF/linkerscript/locationcountererr-arm-exidx.test b/lld/test/ELF/linkerscript/locationcountererr-arm-exidx.test
index 791a9ae178fa9..2ce35494ca148 100644
--- a/lld/test/ELF/linkerscript/locationcountererr-arm-exidx.test
+++ b/lld/test/ELF/linkerscript/locationcountererr-arm-exidx.test
@@ -8,7 +8,9 @@
 # RUN: not ld.lld -z norelro -z max-page-size=4096 -T a.t a.o -o /dev/null --no-merge-exidx-entries 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=ERR --implicit-check-not=error:
 
-# ERR:      error: a.t:14: unable to move location counter (0x4104) backward to 0x4070 for section 'code.unused_space'
+# ERR:      error: a.t:9: unable to move location counter (0x1000) backward to 0xf6c for section 'dummy1'
+# ERR-NEXT: error: a.t:10: unable to move location counter (0x2000) backward to 0x1f6c for section 'dummy2'
+# ERR-NEXT: error: a.t:14: unable to move location counter (0x4104) backward to 0x4070 for section 'code.unused_space'
 # ERR-NEXT: error: section '.ARM.exidx' will not fit in region 'CODE': overflowed by 148 bytes
 # ERR-NEXT: error: section dummy1 at 0x1000 of size 0xFFFFFFFFFFFFFF6C exceeds available address space
 # ERR-NEXT: error: section dummy2 at 0x2000 of size 0xFFFFFFFFFFFFFF6C exceeds available address space

[mysterymath]

LGTM!

As an aside, seeing progress towards a deferred error abstraction is interesting. This might be useful (if tied with a condition) for phase ordering issues that come up, where the linker is forced to emit an error early, but it wouldn't have to if the link ends up going a particular way later. (We hit something like this in Fuchsia in https://fxbug.dev/334990275; the linker is forced to contend with an output section conflict early on, but the offending synthetic section ends up empty.)

[mysterymath]

nit: recordedError is about the same length as pendingError and has similar information content, but the association with recordError is more obvious and greppable. deferError and deferredErrors have similar properties.

[smithp35]

LGTM too, no additional comments.