[PATCH] D81986: [ELF] Refine LMA offset propagation rule in D76995

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/LinkerScript.cpp:854
 
+  bool sameMemRegion = ctx->memRegion == sec->memRegion;
   bool prevLMARegionIsDefault = ctx->lmaRegion == nullptr;
----------------
grimar wrote:
> 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 = ...
> ```
> 
Good idea.


================
Comment at: lld/test/ELF/linkerscript/lma-offset2.s:17
+  DDR : o = 0xfb00000, l = 185M
+  TCM : o = 0x1f400000, l = 128K
+}
----------------
grimar wrote:
> I'd rename "DDR", "TCM" to something more clear like "TEXT", "DATA"
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.


================
Comment at: lld/test/ELF/linkerscript/lma-offset2.s:22
+  .text : { *(.text) } > DDR
+  .mdata : AT(0xfb01000) { *(.data); } > TCM
+  .bss : { *(.bss) } > DDR
----------------
grimar wrote:
> `.mdata` -> `.data`?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81986/new/

https://reviews.llvm.org/D81986