[PATCH] D74286: [ELF] Respect output section alignment for AT> (non-null lmaRegion)

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

It looks fine to me. Perhaps please wait a bit to see if other people have an opinion on this.
Have a few minor nits/questions though.



================
Comment at: lld/test/ELF/linkerscript/lma-align.test:3
+# RUN: echo 'ret; .data.rel.ro; .balign 16; .byte 0;.data; .byte 0; .bss; .space 1' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %t.o
+# RUN: ld.lld -T %s %t.o -o %t
----------------
3 nits:
1) The space is missing `.byte 0;.data;` -> `.byte 0; .data;`
2) It is probably might be a matter of taste or may be related to my personal workflow, but still:
    the constructions in test cases which use `|` like here do make things a bit more difficult to debug.
    I.e. the asm file is not created, but sometimes it is useful to have it. Up to you probably as it is not
    that hard to change when needed anyways.
3) Pardon my ignorance, but why `.space 1` for `.bss` and `.byte 0` for asm? Does it have a different meaning?


================
Comment at: lld/test/ELF/linkerscript/lma-align.test:18
+## FIXME .data and .bss should be placed in different PT_LOAD segments
+## because their LMA regions are different.
+# CHECK-NEXT: LOAD  0x002010 0x0000000000011010 0x0000000000001020 0x000001 0x000031 RW  0x1000
----------------
If you have a plan to fix it soon, it is fine, otherwise may be worth to create a bug and add a link here?

(nit: add a `:` after `FIXME`)


================
Comment at: lld/test/ELF/linkerscript/lma-align.test:29
+  .data.rel.ro 0x11000 : { *(.data.rel.ro) } >RAM AT>ROM
+  ## ALIGN decides output section alignment
+  .data . : ALIGN(16) { *(.data*) } >RAM AT>ROM
----------------
Missing a full stop here and in the comment above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74286