[ELF] Warn changed output section address

[MaskRay]

Summary

When the output section address (addrExpr) is specified, GNU ld warns if
sh_addr is different. This patch implements the warning.

Note, LinkerScript::assignAddresses can be called more than once. We
need to record the changed section addresses, and only report the
warnings when the addresses are finalized.

[MaskRay]

lld/test/ELF/linkerscript/section-align2.test

1–3

AArch64 needs thunks. assignAddresses is called at least twice.

This enhances the test - it can detect duplicate diagnostic issue (https://sourceware.org/bugzilla/show_bug.cgi?id=25570 )

[psmith]

Just had some thoughts on what values we warn, and what value we give as the original.

lld/test/ELF/linkerscript/lma-align.test

8

While the warnings look correct from the LLD last known update perspective, are they what a user would want to see? Looking at the script:

• As a user I set the address of .data to 0x11000, why is LLD reporting 0x11001? Perhaps it is worth reporting the value of the original addrexpr.
• Although "." is an addrexpr it is no different to not using an addrexpr. Could we consider addrexpr involving "." as relative to previous dot expressions and not warn? I'm not 100% behind not warning as there could be a context like . = 0x10000 ; before the addrexpr involving "." although I expect that is rare.

SECTIONS {
  .text 0x1000 : { *(.text*) } >ROM
  ## Input section alignment decides output section alignment.
  .data.rel.ro 0x11000 : { *(.data.rel.ro) } >RAM AT>ROM
  ## ALIGN decides output section alignment.
  .data . : ALIGN(16) { *(.data*) } >RAM AT>ROM
  ## Start a new PT_LOAD because the LMA region is different from the previous one.
  .bss . : ALIGN(64) { *(.bss*) } >RAM
}

[MaskRay]

lld/test/ELF/linkerscript/lma-align.test

8

This warning is reported only when addrExpr is set (., not ALIGN(16)).

section-align2.test has a case where addrExpr is set while alignExpr is not:

.data2 . : { *(.data2) }

As a user I set the address of .data to 0x11000, why is LLD reporting 0x11001? Perhaps it is worth reporting the value of the original addrexpr.

When lld tries to layout .data, dot is 0x11001.

For comparison, GNU ld reports ld.bfd: warning: changing start of section .data2 by 7 bytes

I believe this patch matches GNU ld behavior: warn if the actual address is not equal to addrExpr. This can be caused by either ALIGN or input section alignments.

[MaskRay]

Delete the warning of .data2

It exposed another discrepancy which will be fixed by D74736

[grimar]

lld/ELF/LinkerScript.h

275

What about making this public and removing and inlining the warnChangedSectionAddresses code?

[MaskRay]

Inline warnChangedSectionAddresses()

[psmith]

Thanks for clarifying, I don't have any more comments, happy if George is.

[grimar]

LGTM

lld/ELF/LinkerScript.h

class LinkerScript final {
   // This should remain a plain pointer as its lifetime is smaller than
   // LinkerScript.
   AddressState *ctx = nullptr;
 
   OutputSection *aether;
 
   uint64_t dot;
 
+  // Sections whose addresses are not equal to their addrExpr values.
+  std::vector<std::pair<const OutputSection *, uint64_t>>
+      changedSectionAddresses;
+
 public:
   OutputSection *createOutputSection(StringRef name, StringRef location);
   OutputSection *getOrCreateOutputSection(StringRef name);
 
   bool hasPhdrsCommands() { return !phdrsCommands.empty(); }
   uint64_t getDot() { return dot; }
   void discard(InputSectionBase *s);
 
   ExprValue getSymbolValue(StringRef name, const Twine &loc);
 
   void addOrphanSections();
   void adjustSectionsBeforeSorting();
   void adjustSectionsAfterSorting();
 
   std::vector<PhdrEntry *> createPhdrs();
   bool needsInterpSection();
 
   bool shouldKeep(InputSectionBase *s);
   const Defined *assignAddresses();
+  void warnChangedSectionAddresses();
   void allocateHeaders(std::vector<PhdrEntry *> &phdrs);
   void processSectionCommands();
   void processSymbolAssignments();
   void declareSymbols();
 
   // Used to handle INSERT AFTER statements.
   void processInsertCommands();
 

lld/ELF/LinkerScript.cpp

void LinkerScript::assignOffsets(OutputSection *sec) {
   // If the address of the section has been moved forward by an explicit
   // expression so that it now starts past the current curPos of the enclosing
   // region, we need to expand the current region to account for the space
   // between the previous section, if any, and the start of this section.
   if (ctx->memRegion && ctx->memRegion->curPos < dot)
     expandMemoryRegion(ctx->memRegion, dot - ctx->memRegion->curPos,
                        ctx->memRegion->name, sec->name);
 
+  uint64_t oldDot = dot;
   switchTo(sec);
+  if (sec->addrExpr && oldDot != dot)
+    changedSectionAddresses.push_back({sec, oldDot});
 
   ctx->lmaOffset = 0;
 
   if (sec->lmaExpr)
     ctx->lmaOffset = sec->lmaExpr().getValue() - dot;
   if (MemoryRegion *mr = sec->lmaRegion)
     ctx->lmaOffset = alignTo(mr->curPos, sec->alignment) - dot;
 
if (script->hasSectionsCommand) {
     Out::elfHeader->addr = dot;
     Out::programHeaders->addr = dot + Out::elfHeader->size;
     dot += getHeaderSize();
   }
 
   auto deleter = std::make_unique<AddressState>();
   ctx = deleter.get();
   errorOnMissingSection = true;
+  changedSectionAddresses.clear();
   switchTo(aether);
 
   SymbolAssignmentMap oldValues = getSymbolAssignmentValues(sectionCommands);
   for (BaseCommand *base : sectionCommands) {
     if (auto *cmd = dyn_cast<SymbolAssignment>(base)) {
       cmd->addr = dot;
       assignSymbol(cmd, false);
       cmd->size = dot - cmd->addr;
       continue;
     }
     assignOffsets(cast<OutputSection>(base));
   }
 
   ctx = nullptr;
   return getChangedSymbolAssignment(oldValues);
 }
 
+// If a SECTIONS command is given, addrExpr is the specified output section
+// address. Warn if the computed value is different from the actual address.
+void LinkerScript::warnChangedSectionAddresses() {
+  for (auto changed : changedSectionAddresses) {
+    const OutputSection *os = changed.first;
+    warn("start of section " + os->name + " changes from 0x" +
+         utohexstr(changed.second) + " to 0x" + utohexstr(os->addr));
+  }
+}
+
 // Creates program headers as instructed by PHDRS linker script command.
 std::vector<PhdrEntry *> LinkerScript::createPhdrs() {
   std::vector<PhdrEntry *> ret;
 
   // Process PHDRS and FILEHDR keywords because they are not
   // real output sections and cannot be added in the following loop.
   for (const PhdrsCommand &cmd : phdrsCommands) {
     PhdrEntry *phdr = make<PhdrEntry>(cmd.type, cmd.flags ? *cmd.flags : PF_R);

lld/ELF/Writer.cpp

if (!changed) {
         break;
       if (++assignPasses == 5) {
         errorOrWarn("assignment to symbol " + toString(*changedSym) +
                     " does not converge");
         break;
       }
     }
   }
+
+  if (script->hasSectionsCommand)
+    script->warnChangedSectionAddresses();
 }
 
 static void finalizeSynthetic(SyntheticSection *sec) {
   if (sec && sec->isNeeded() && sec->getParent())
     sec->finalizeContents();
 }
 
 // In order to allow users to manipulate linker-synthesized sections,

lld/test/ELF/linkerscript/lma-align.test

 # REQUIRES: x86
 # RUN: echo '.globl _start; _start: ret; .data.rel.ro; .balign 16; .byte 0; \
 # RUN:   .data; .balign 32; .byte 0; .bss; .byte 0' | \
 # RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %t.o
-# RUN: ld.lld -T %s %t.o -o %t
+# RUN: ld.lld -T %s %t.o -o %t 2>&1 | FileCheck --check-prefix=WARN %s --implicit-check-not=warning:
 # RUN: llvm-readelf -S -l %t | FileCheck %s
 
+# WARN: warning: start of section .data changes from 0x11001 to 0x11010
+# WARN: warning: start of section .bss changes from 0x11021 to 0x11040
+
 # CHECK:      Name         Type     Address          Off    Size   ES Flg Lk Inf Al
 # CHECK-NEXT:              NULL     0000000000000000 000000 000000 00      0   0  0
 # CHECK-NEXT: .text        PROGBITS 0000000000001000 001000 000001 00  AX  0   0  4
 # CHECK-NEXT: .data.rel.ro PROGBITS 0000000000011000 002000 000001 00  WA  0   0 16
 # CHECK-NEXT: .data        PROGBITS 0000000000011010 002010 000011 00  WA  0   0 32
 # CHECK-NEXT: .bss         NOBITS   0000000000011040 002040 000001 00  WA  0   0 64
 
 # CHECK:      Type  Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align

lld/test/ELF/linkerscript/section-align2.test

 # REQUIRES: aarch64
 ## Test ALIGN and its interaction with explicit output section addresses.
 
 # RUN: echo '.globl _start; _start: ret; .data.rel.ro; .balign 8; .byte 0; .data; .byte 0; \
 # RUN:   .section .data2,"aw"; .balign 8; .byte 0; .bss; .balign 32; .byte 0' | \
 # RUN:   llvm-mc -filetype=obj -triple=aarch64 - -o %t.o
-# RUN: ld.lld -T %s %t.o -o %t 2>&1 | count 0
+# RUN: ld.lld -T %s %t.o -o %t 2>&1 | FileCheck --check-prefix=WARN %s --implicit-check-not=warning:
 # RUN: llvm-readelf -S %t | FileCheck %s
 
+## Check we don't warn in the absence of SECTIONS.
+# RUN: ld.lld --fatal-warnings -Ttext=0x10000 %t.o -o /dev/null
+
+# WARN: warning: start of section .data.rel.ro changes from 0x10004 to 0x10010
+# WARN: warning: start of section .bss changes from 0x20009 to 0x20010
+
 # CHECK:      Name         Type     Address          Off    Size   ES Flg Lk Inf Al
 # CHECK-NEXT:              NULL     0000000000000000 000000 000000 00      0   0  0
 # CHECK-NEXT: .text        PROGBITS 0000000000010000 010000 000004 00  AX  0   0  4
 # CHECK-NEXT: .data.rel.ro PROGBITS 0000000000010010 010010 000001 00  WA  0   0 16
 # CHECK-NEXT: .data        PROGBITS 0000000000020000 020000 000001 00  WA  0   0  1
 # CHECK-NEXT: .data2       PROGBITS 0000000000020001 020001 000008 00  WA  0   0  8
 # CHECK-NEXT: .bss         NOBITS   0000000000020010 020009 000011 00  WA  0   0 32