[PATCH] D43996: [llvm-objcopy] Implement support for .group sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D43996#1025058, @alexshap wrote:

> it's not about stripping the .group itself, it's about the validity of the binary (even if we strip smth else/unrelated). Let me try to explain what i mean:  the thing is that if we remove a section from a binary the indices of other sections (which go after the removed one) will change. The content of the section .group is essentially an array of indices of some set of sections, thus we need to recalculate them (because they could have changed). The same issue is with sh_link and sh_info (so without this diff llvm-objcopy creates invalid binaries and the linker either crashes or doesn't work properly).


Thanks, that makes sense to me. I think to make this a little clearer, it would be good to do a more explicit test of this. Specifically, don't use a checked-in binary, and instead explicitly create an ELF object with one or more group sections from assembly, and strip a non-group related section from somewhere, to show that the group sections are updated accordingly. I'm thinking something like this as the input, possibly with more groups before and after the removed section to show that only those that should be adjusted are adjusted:

  .section to_be_stripped,"", at progbits
  .byte 0xFF
  
  .section group_member1,"G", at progbits,a_sym,comdat
  a_sym:
  .long 0x0badbeef
  
  .section group_member2,"G", at progbits,a_sym,comdat
  .long 0xcafebabe

Also, on the note of testing, at lest one of the new error messages (attempting to strip .symtab) can, and therefore should, be tested.

In https://reviews.llvm.org/D43996#1025065, @alexshap wrote:

> (regarding the test - when we strip out dwo  the indices change and we check that they are correct (now) by dumping those .group sections)
>  Fixing sh_info might be a little bit more involved and since for me this worked ( in the sense that i was able to build/run a large project) even with invalid sh_info, i wanted to push this change first.
>  Looking at the current code and the documentation (for sh_link and sh_info https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-94076/index.html) - if i'm not mistaken there are some other cases which need to be fixed, but i'd prefer to do it incrementally.


Okay, yes that should be fine to me. @jakehehrlich might have a different opinion though.


Repository:
  rL LLVM

https://reviews.llvm.org/D43996