[ELF] Refine LMA offset propagation rule in D76995

[MaskRay]

Summary

If neither AT(lma) nor AT>lma_region is specified,
D76995 keeps lmaOffset (LMA - VMA) if the previous section is in the
default LMA region.

This patch additionally checks that the two sections are in the same
memory region.

Add a test case derived from https://bugs.llvm.org/show_bug.cgi?id=45313

.mdata : AT(0xfb01000) { *(.data); } > TCM
// It is odd to make .bss inherit lmaOffset, because the two sections
// are in different memory regions.
.bss : { *(.bss) } > DDR

With this patch, section VMA/LMA match GNU ld. Note, GNU ld supports
out-of-order (w.r.t sh_offset) sections and places .text and .bss in the
same PT_LOAD. We don't have that behavior.

[grimar]

Sounds reasonable to me.

lld/ELF/LinkerScript.cpp

854

I know we did not do it in LLD before. But what do you think about starting using
the const for variables that are not suppored to change?

const bool sameMemRegion = ...

lld/test/ELF/linkerscript/lma-offset2.s

9

Perhaps a description about what this test case does wouldn't hurt.

18

I'd rename "DDR", "TCM" to something more clear like "TEXT", "DATA"

23

.mdata -> .data?

[MaskRay]

Fix reference: it should be D76995

Add comments.

[MaskRay]

lld/ELF/LinkerScript.cpp

854

Good idea.

lld/test/ELF/linkerscript/lma-offset2.s

18

I don't know DDR but TCM but I think renaming to TEXT&DATA will make the test less reasonable.

.text and .bss are in the same memory region. .bss in TEXT will be weird.

23

I don't know .mdata but I think renaming it will make the test less reasonable.

It is weird for .data and .bss to be placed in different DATA memory regions.
.mdata is special and the distinction may be justified.

[grimar]

LGTM

lld/ELF/LinkerScript.cpp

 }
 
 // This function assigns offsets to input sections and an output section
 // for a single sections command (e.g. ".text { *(.text); }").
 void LinkerScript::assignOffsets(OutputSection *sec) {
   if (!(sec->flags & SHF_ALLOC))
     dot = 0;
 
-  bool prevLMARegionIsDefault = ctx->lmaRegion == nullptr;
+  const bool sameMemRegion = ctx->memRegion == sec->memRegion;
+  const bool prevLMARegionIsDefault = ctx->lmaRegion == nullptr;
   ctx->memRegion = sec->memRegion;
   ctx->lmaRegion = sec->lmaRegion;
   if (ctx->memRegion)
     dot = ctx->memRegion->curPos;
 
   if ((sec->flags & SHF_ALLOC) && sec->addrExpr)
     setDot(sec->addrExpr, sec->location, false);
 
   // 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);
 
   switchTo(sec);
 
   // ctx->lmaOffset is LMA minus VMA. If LMA is explicitly specified via AT() or
   // AT>, recompute ctx->lmaOffset; otherwise, if both previous/current LMA
-  // region is the default, reuse previous lmaOffset; otherwise, reset lmaOffset
-  // to 0. This emulates heuristics described in
+  // region is the default, and the two sections are in the same memory region,
+  // reuse previous lmaOffset; otherwise, reset lmaOffset to 0. This emulates
+  // heuristics described in
   // https://sourceware.org/binutils/docs/ld/Output-Section-LMA.html
   if (sec->lmaExpr)
     ctx->lmaOffset = sec->lmaExpr().getValue() - dot;
   else if (MemoryRegion *mr = sec->lmaRegion)
     ctx->lmaOffset = alignTo(mr->curPos, sec->alignment) - dot;
-  else if (!prevLMARegionIsDefault)
+  else if (!sameMemRegion || !prevLMARegionIsDefault)
     ctx->lmaOffset = 0;
 
   // Propagate ctx->lmaOffset to the first "non-header" section.
   if (PhdrEntry *l = ctx->outSec->ptLoad)
     if (sec == findFirstSection(l))
       l->lmaOffset = ctx->lmaOffset;
 
   // We can call this method multiple times during the creation of

lld/docs/ELF/linker_script.rst

- ``AT>lma_region`` specifies the LMA region. The lack of ``AT>lma_region``
   If the linker script does not have a PHDRS command, then if
   ``lma_region`` is different from the ``lma_region`` for
   the previous OutputSection a new loadable segment will be generated.
 
 The two keywords cannot be specified at the same time.
 
 If neither ``AT(lma)`` nor ``AT>lma_region`` is specified:
 
-- If the previous section is also in the default LMA region, the difference
-  between the LMA and the VMA is computed to be the same as the previous
-  difference.
+- If the previous section is also in the default LMA region, and the two
+  section have the same memory regions, the difference between the LMA and the
+  VMA is computed to be the same as the previous difference.
 - Otherwise, the LMA is set to the VMA.

lld/test/ELF/linkerscript/lma-offset2.s

+# REQUIRES: x86
+## If neither AT(lma) nor AT>lma_region is specified, don't propagate
+## lmaOffset if the section and the previous section are in different memory
+## regions.
+
+# RUN: echo '.globl _start; _start: ret; \
+# RUN:   .data; .byte 0; \
+# RUN:   .bss; .byte 0' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %t.o
+# RUN: ld.lld -T %s %t.o -o %t
+# RUN: llvm-readelf -l %t | FileCheck %s
+
+## GNU ld places .text and .bss in the same RWX PT_LOAD.
+# CHECK:      Type Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg
+# CHECK-NEXT: LOAD 0x001000 0x000000000fb00000 0x000000000fb00000 0x000001 0x000001 R E
+# CHECK-NEXT: LOAD 0x002000 0x000000001f400000 0x000000000fb01000 0x000001 0x000001 RW
+# CHECK-NEXT: LOAD 0x002001 0x000000000fb00001 0x000000000fb00001 0x000000 0x000001 RW
+
+MEMORY  {
+  DDR : o = 0xfb00000, l = 185M
+  TCM : o = 0x1f400000, l = 128K
+}
+
+SECTIONS {
+  .text : { *(.text) } > DDR
+  .mdata : AT(0xfb01000) { *(.data); } > TCM
+  ## .mdata and .bss are in different memory regions. Start a new PT_LOAD for
+  ## .bss, even if .mdata does not set a LMA region.
+  .bss : { *(.bss) } > DDR
+}