[PATCH] D71035: [llvm-objcopy][ELF] -O binary: use LMA instead of sh_offset to decide where to write section contents

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test:23
 Sections:
   - Name:            .text
     Type:            SHT_PROGBITS
----------------
This section isn't n a segment. Is that deliberate? If so, it probably needs a comment saying this.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test:32
     Flags:           [ SHF_ALLOC ]
+    ## The computed LMA is sh_address-p_vaddr+p_paddr = 0x2000-0x2000+0x4000 = 0x4000.
     Address:         0x2000
----------------
Does this comment still make sense? The numbers don't line up with the p_vaddr below, and the comment along with it (Also sh_address -> sh_addr).


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test:68
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
----------------
Same comment as above: is this section deliberately not in a segment?

Assuming it is, there may be scope for a test where both sections are in segments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2257
+  // Compute the section LMA based on its sh_offset and the containing segment's
+  // p_offset and p_paddr. Also compute the minimum LMA. In the output, the
+  // contents between address 0 and MinAddr will be skipped.
----------------
The minimum LMA of what? Please specify that in the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71035