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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, after a few nits mentioned inline fixed.



================
Comment at: test/tools/llvm-objcopy/group-unchanged.test:5-7
+# In this test the section .comment is getting removed,
+# since this section goes after all the sections comprising a group, 
+# the content of the section .group doesn't change.
----------------
jhenderson wrote:
> Nit: the wrapping on this comment looks weird.
Wrapping on the comment still looks weird ("the" on line 7, at least, should go on the previous line).


================
Comment at: test/tools/llvm-objcopy/group.test:7
+# the indices of the sections which go after .comment will change, 
+# thus the content of .group should be updated: 
+# .text.foo (3) is replaced with .text.foo (2).
----------------
Please also point out that the signature symbol index (i.e. sh_info) was updated.


================
Comment at: tools/llvm-objcopy/Object.h:436
 
-class SectionWithStrTab : public Section {
-private:
-  const SectionBase *StrTab = nullptr;
+class GroupSection : public SectionBase {
+  MAKE_SEC_WRITER_FRIEND
----------------
jakehehrlich wrote:
> Can you add a TODO that says something like "The way stripping and groups interact is complicated and still needs to be worked on".
I can't find this TODO anywhere?


================
Comment at: tools/llvm-objcopy/Object.h:439
+
+  ArrayRef<uint8_t> Contents;
+  ELF::Elf32_Word FlagWord;
----------------
jakehehrlich wrote:
> Can you add a TODO here about getting rid of Contents once initialize is refactored?
Nits: "hierarchy, this" -> "hierarchy. This" and "to to" -> "to" 


Repository:
  rL LLVM

https://reviews.llvm.org/D43996