[ELF] Postpone "unable to move location counter backward" error #66854

MaskRay · 2023-09-20T03:14:56Z

The size of .ARM.exidx may shrink across assignAddress calls. It is possible
that the initial iteration has a larger location counter, causing __code_size = __code_end - .; osec : { . += __code_size; } to report an error, while the error would
have been suppressed for subsequent assignAddress iterations.

Other sections like .relr.dyn may change sizes across assignAddress calls as
well. However, their initial size is zero, so it is difficiult to trigger a
similar error.

Similar to https://reviews.llvm.org/D152170, postpone the error reporting.
Fix #66836. While here, add more information to the error message.

llvmbot · 2023-09-20T03:16:05Z

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Changes

The size of .ARM.exidx may shrink across assignAddress calls. It is possible
that the initial iteration has a larger location counter, causing __code_size = __code_end - .; . += __code_size; to report an error, while the error would
have been suppressed for subsequent assignAddress iterations.

Other sections like .relr.dyn may change sizes across assignAddress calls as
well. However, their initial size is zero, so it is difficiult to trigger a
similar error.

Similar to https://reviews.llvm.org/D152170, postpone the error reporting.
Fix #66836. While here, add more information to the error message.

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

5 Files Affected:

(modified) lld/ELF/LinkerScript.cpp (+14-4)
(modified) lld/ELF/LinkerScript.h (+3-2)
(modified) lld/ELF/Writer.cpp (+1-1)
(added) lld/test/ELF/linkerscript/locationcountererr-arm-exidx.test (+53)
(modified) lld/test/ELF/linkerscript/locationcountererr.test (+1-1)

diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 943fd865ae0c15c..14a89c79a65ecf8 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -169,9 +169,16 @@ void LinkerScript::expandOutputSection(uint64_t size) {
 
 void LinkerScript::setDot(Expr e, const Twine &loc, bool inSec) {
   uint64_t val = e().getValue();
-  if (val < dot && inSec)
-    error(loc + ": unable to move location counter backward for: " +
-          state->outSec->name);
+  // If val is smaller and we are in an output section, record the error and
+  // 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 (0x" + Twine::utohexstr(val) + ") for section '" +
+         state->outSec->name + "'")
+            .str();
+  }
 
   // Update to location counter means update to section size.
   if (inSec)
@@ -1343,6 +1350,7 @@ const Defined *LinkerScript::assignAddresses() {
   state = &st;
   errorOnMissingSection = true;
   st.outSec = aether;
+  backwardDotErr.clear();
 
   SymbolAssignmentMap oldValues = getSymbolAssignmentValues(sectionCommands);
   for (SectionCommand *cmd : sectionCommands) {
@@ -1494,7 +1502,9 @@ static void checkMemoryRegion(const MemoryRegion *region,
   }
 }
 
-void LinkerScript::checkMemoryRegions() const {
+void LinkerScript::checkDotAndMemoryRegions() const {
+  if (backwardDotErr.size())
+    errorOrWarn(backwardDotErr);
   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 89780bb60f48244..a731610e3fdd504 100644
--- a/lld/ELF/LinkerScript.h
+++ b/lld/ELF/LinkerScript.h
@@ -345,8 +345,8 @@ class LinkerScript final {
   // Describe memory region usage.
   void printMemoryUsage(raw_ostream &os);
 
-  // Verify memory/lma overflows.
-  void checkMemoryRegions() const;
+  // Check backward location counter assignment and memory region/LMA overflows.
+  void checkDotAndMemoryRegions() const;
 
   // SECTIONS command list.
   SmallVector<SectionCommand *, 0> sectionCommands;
@@ -358,6 +358,7 @@ class LinkerScript final {
   bool seenDataAlign = false;
   bool seenRelroEnd = false;
   bool errorOnMissingSection = false;
+  std::string backwardDotErr;
 
   // 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/Writer.cpp b/lld/ELF/Writer.cpp
index 40f7d7981d9d441..4c82967b0a47d25 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -2187,7 +2187,7 @@ template <class ELFT> void Writer<ELFT>::finalizeSections() {
   for (OutputSection *sec : outputSections)
     sec->finalize();
 
-  script->checkMemoryRegions();
+  script->checkDotAndMemoryRegions();
 
   if (config->emachine == EM_ARM && !config->isLE && config->armBe8) {
     addArmInputSectionMappingSymbols();
diff --git a/lld/test/ELF/linkerscript/locationcountererr-arm-exidx.test b/lld/test/ELF/linkerscript/locationcountererr-arm-exidx.test
new file mode 100644
index 000000000000000..cea9d3de2264c96
--- /dev/null
+++ b/lld/test/ELF/linkerscript/locationcountererr-arm-exidx.test
@@ -0,0 +1,53 @@
+# REQUIRES: arm
+# RUN: rm -rf %t && split-file %s %t && cd %t
+# RUN: llvm-mc -filetype=obj -triple=armv7-linux-gnueabi a.s -o a.o
+# RUN: llvm-mc -filetype=obj -triple=armv7-linux-gnueabi a.s -o a.o
+
+## If we don't merge adjacent duplicate entries, __code_size will be negative and
+## . += __code_size will trigger an error.
+# RUN: not ld.lld -z norelro -T a.t a.o -o /dev/null --no-merge-exidx-entries 2>&1 | FileCheck %s --check-prefix=ERR
+
+# ERR: a.t:10: unable to move location counter (0x4104) backward (0x4070) for section 'code.unused_space'
+
+## If we merge adjacent duplicate entries, we will have enough space.
+## https://github.com/llvm/llvm-project/issues/66836
+# RUN: ld.lld -z norelro -T a.t a.o -o a
+# RUN: llvm-readelf -S a | FileCheck %s
+
+# CHECK:       [Nr] Name              Type            Address  Off    Size   ES Flg Lk Inf Al
+# CHECK-NEXT:  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
+# CHECK-NEXT:  [ 1] .text             PROGBITS        00004000 004000 000054 00  AX  0   0  4
+# CHECK-NEXT:  [ 2] .ARM.exidx        ARM_EXIDX       00004054 004054 000010 00  AL  1   0  4
+# CHECK-NEXT:  [ 3] code.unused_space NOBITS          00004064 004064 00000c 00   A  0   0  1
+
+#--- a.s
+.globl _start
+_start:
+  bx lr
+
+.macro A
+.section .text.f\@,"ax"
+.globl f\@
+f\@:
+.fnstart
+  bx lr
+.cantunwind
+.fnend
+.endm
+
+.rept 20
+  A
+.endr
+
+#--- a.t
+MEMORY {
+  CODE (RX) : ORIGIN = 0x4000, LENGTH = 0x70
+}
+__code_end = ORIGIN(CODE) + LENGTH(CODE);
+
+SECTIONS {
+  .text : { *(.text .text.*) } > CODE
+  .ARM.exidx : { *(.ARM.exidx .ARM.exidx.*) } > CODE
+  __code_size = __code_end - .;
+  code.unused_space (NOLOAD) : { . += __code_size; } > CODE
+}
diff --git a/lld/test/ELF/linkerscript/locationcountererr.test b/lld/test/ELF/linkerscript/locationcountererr.test
index 086bfeba1e3d455..181bc1f8ea00fa4 100644
--- a/lld/test/ELF/linkerscript/locationcountererr.test
+++ b/lld/test/ELF/linkerscript/locationcountererr.test
@@ -1,7 +1,7 @@
 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux /dev/null -o %t
 # RUN: not ld.lld %t --script %s -o /dev/null 2>&1 | FileCheck %s
-# CHECK: {{.*}}.test:8: unable to move location counter backward for: .text
+# CHECK: {{.*}}.test:8: unable to move location counter (0x2000) backward (0x10) for section '.text'
 
 SECTIONS {
   .text 0x2000 : {

arichardson

Looks sensible to me but probably a good idea to get another review.

arichardson · 2023-09-20T05:10:00Z

lld/ELF/LinkerScript.cpp

+  if (val < dot && inSec) {
+    backwardDotErr =
+        (loc + ": unable to move location counter (0x" + Twine::utohexstr(dot) +
+         ") backward (0x" + Twine::utohexstr(val) + ") for section '" +


Much prefer the new error message but I noticed that it's ambiguous if this means we are moving it backwards by a number of bytes or to given address. How about the following:

Suggested change

") backward (0x" + Twine::utohexstr(val) + ") for section '" +

") backward to (0x" + Twine::utohexstr(val) + ") for section '" +

Updated in 54daea8e15f7e5e95e6531d34616062af81c4d84 to backward to 0x... for section 'xxx'

arichardson · 2023-09-20T05:16:00Z

lld/test/ELF/linkerscript/locationcountererr.test

@@ -1,7 +1,7 @@
 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux /dev/null -o %t
 # RUN: not ld.lld %t --script %s -o /dev/null 2>&1 | FileCheck %s
-# CHECK: {{.*}}.test:8: unable to move location counter backward for: .text
+# CHECK: {{.*}}.test:8: unable to move location counter (0x2000) backward (0x10) for section '.text'


Maybe worth adding a test with more than one backwards error? Previously we'd get multiple reports now just the first one. There probably isn't much value in reporting further errors, but I think it does make sense to document this behaviour in a test?

Thanks for the suggestion. 54daea8e15f7e5e95e6531d34616062af81c4d84 added two more memory regions to demonstrate the full set of errors.

smithp35

LGTM too.

smithp35 · 2023-09-20T08:29:56Z

lld/ELF/LinkerScript.h

-  // Verify memory/lma overflows.
-  void checkMemoryRegions() const;
+  // Check backward location counter assignment and memory region/LMA overflows.
+  void checkDotAndMemoryRegions() const;


It may be better to have a more general name such as checkStableAddressScript() or checkFinalScriptConditions() rather than detail each check.

Reason I mention it is that there may be more checks that are worth delaying. For example in https://github.com/kernkonzept/fiasco/blob/master/src/kernel.arm.ld#L60 the assert will fail in LLD but not in GNU ld because the assert is evaluated eagerly before any addresses are assigned, wheras GNU ld evaluates them once at the end. In this case the assert can be moved to the end of the script.

Not sure that this particular function would be the place to do that though.

Thanks for the suggestion. I remember that I've seen similar ASSERT issues. We can postpone the check when the problem arises...

Renamed to checkFinalScriptConditions in 2bf25dc88bfa848ca520578ce2c0e6bb15d1a1a9

The size of .ARM.exidx may shrink across `assignAddress` calls. It is possible that the initial iteration has a larger location counter, causing `__code_size = __code_end - .; osec : { . += __code_size; }` to report an error, while the error would have been suppressed for subsequent `assignAddress` iterations. Other sections like .relr.dyn may change sizes across `assignAddress` calls as well. However, their initial size is zero, so it is difficiult to trigger a similar error. Similar to https://reviews.llvm.org/D152170, postpone the error reporting. Fix llvm#66836. While here, add more information to the error message.

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. Pull Request: #96361

Since `assignAddresses` is executed more than once, error reporting during `assignAddresses` would be duplicated. Generalize llvm#66854 to cover more errors. Note: address-related errors exposed in one invocation might not be errors in another invocation. Pull Request: llvm#96361

MaskRay requested review from smithp35, arichardson and PiJoules September 20, 2023 03:14

llvmbot added lld lld:ELF labels Sep 20, 2023

MaskRay force-pushed the lld/dot branch 2 times, most recently from 5871e26 to 363c4f3 Compare September 20, 2023 03:16

arichardson approved these changes Sep 20, 2023

View reviewed changes

MaskRay force-pushed the lld/dot branch from 363c4f3 to 54daea8 Compare September 20, 2023 05:47

smithp35 approved these changes Sep 20, 2023

View reviewed changes

MaskRay force-pushed the lld/dot branch from 54daea8 to 2bf25dc Compare September 20, 2023 15:52

MaskRay added 3 commits September 20, 2023 09:05

improve test

45423a4

Rename checkDotAndMemoryRegions to checkFinalScriptConditions

0f78cd2

MaskRay force-pushed the lld/dot branch from 2bf25dc to 0f78cd2 Compare September 20, 2023 16:06

MaskRay merged commit 0de0b6d into llvm:main Sep 20, 2023
1 check passed

MaskRay deleted the lld/dot branch September 20, 2023 16:07

This was referenced Sep 20, 2023

sve gather scatter offset sinking #66932

Closed

sve gather scatter offset sinking #66994

Closed

kstoimenov mentioned this pull request Sep 22, 2023

Add memcpm test kstoimenov/llvm-project#13

Closed

MaskRay mentioned this pull request Jun 21, 2024

[ELF] Postpone more linker script errors #96361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ELF] Postpone "unable to move location counter backward" error #66854

[ELF] Postpone "unable to move location counter backward" error #66854

MaskRay commented Sep 20, 2023 •

edited

Loading

llvmbot commented Sep 20, 2023 •

edited

Loading

arichardson left a comment

arichardson Sep 20, 2023

MaskRay Sep 20, 2023

arichardson Sep 20, 2023

MaskRay Sep 20, 2023

smithp35 left a comment

smithp35 Sep 20, 2023

MaskRay Sep 20, 2023

MaskRay Sep 20, 2023

	") backward (0x" + Twine::utohexstr(val) + ") for section '" +
	") backward to (0x" + Twine::utohexstr(val) + ") for section '" +

[ELF] Postpone "unable to move location counter backward" error #66854

[ELF] Postpone "unable to move location counter backward" error #66854

Conversation

MaskRay commented Sep 20, 2023 • edited Loading

llvmbot commented Sep 20, 2023 • edited Loading

arichardson left a comment

Choose a reason for hiding this comment

arichardson Sep 20, 2023

Choose a reason for hiding this comment

MaskRay Sep 20, 2023

Choose a reason for hiding this comment

arichardson Sep 20, 2023

Choose a reason for hiding this comment

MaskRay Sep 20, 2023

Choose a reason for hiding this comment

smithp35 left a comment

Choose a reason for hiding this comment

smithp35 Sep 20, 2023

Choose a reason for hiding this comment

MaskRay Sep 20, 2023

Choose a reason for hiding this comment

MaskRay Sep 20, 2023

Choose a reason for hiding this comment

MaskRay commented Sep 20, 2023 •

edited

Loading

llvmbot commented Sep 20, 2023 •

edited

Loading