[PATCH] D59351: [llvm-objcopy] Add --update-section

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 marked 2 inline comments as done.
evgeny777 added inline comments.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:783
+         Segment.Offset <= Section.OriginalOffset &&
+         Segment.Offset + Segment.MemSize >= Section.OriginalOffset + SecSize;
 }
----------------
jakehehrlich wrote:
> We need to be *very* careful here. Can you explain why you did this?
This patch introduces segment fixup procedure which works incorrectly without this change
Besides that this change has some sense, because:

- Non-allocatable section can't be in segment
- Using MemSize instead of FileSize seems correct, because some allocatable sections (like .bss) have file size of 0.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1489
+// layoutSections based on segment virtual address and previous segment size.
+void Segment::fixupSectionOffsets() {
+  SectionBase *Prev = nullptr;
----------------
jakehehrlich wrote:
> We should not be changing segment layout. This is a huge bag of worms and this function does not correctly handle this. You basically have to re-link an executable to do this. Changing the size of a section that's within a segment should just simply be an error. I'm highly skeptical of use cases that require this.
One of use cases is adding some initialization to existing binary, e.g you dump text section and your own function, change start address, write section back to the module. There are other use cases as well:

https://github.com/hioa-cs/IncludeOS/blob/master/cmake/post.service.cmake
https://github.com/torvalds/linux/blob/master/arch/mips/Kconfig
https://github.com/abelromeroperez/underc0rerk/blob/master/patch-lkm.py

Also see RFC


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

https://reviews.llvm.org/D59351