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

[Asaf Fisher via Phabricator via llvm-commits]

AsafFisher added a comment.

What's up with this PR?



================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:783
+         Segment.Offset <= Section.OriginalOffset &&
+         Segment.Offset + Segment.MemSize >= Section.OriginalOffset + SecSize;
 }
----------------
pcc wrote:
> evgeny777 wrote:
> > 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.
> For sections such as bss, sh_offset can have an arbitrary value (because there is no file data), so this can end up returning the wrong result. To properly classify bss sections, sh_addr must be used as in D58426.
Seems like D58426 is done, rebase!


================
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;
----------------
evgeny777 wrote:
> 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
Lets make this work, GNU binutils implements it.


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

https://reviews.llvm.org/D59351