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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Not a major point, and I'm not asking you to do this for this change, but I'd prefer it if you keep mass unimportant changes (adding explicit, moving private/public around etc) to a separate change, to make it easier to see the bulk of the interesting change. One or two odd changes are fine.

It's not clear to me what you mean when you say that you plan to add proper sh_info support later?

What do you expect the behaviour of .group sections to be when either they or their members are attempted to be stripped?

I briefly did some very non-comprehensive playing around with GNU objcopy, and how it strips .group sections if requested. If you strip a group member, only that member is stripped. If you strip the group section itself, its members are not stripped, and instead, the SHF_GROUP flag is removed from them. If the section containing the symbol that the group refers to via sh_info is stripped, the sh_info field appears to be redirected to the .group's section symbol. Some of this behaviour seems a bit weird to me. According to the ELF specification, the whole group should be kept or discarded as one, so I'd expect either objcopy to refuse to strip any of them unless all are stripped, or implicitly strip the lot. It's not clear from my reading of the specification what should be done if the signature symbol is stripped (it's theoretically possible for it to belong to a non-group-member section), but I'm not sure I see the benefit of GNU's approach here. I think being able to strip the .group sections themselves is reasonable, as it effectively says "I definitely want this copy of this group's contents, and none other".



================
Comment at: test/tools/llvm-objcopy/test-groups.test:1
+# RUN: cp %p/Inputs/groups.o %t
+# RUN: llvm-objcopy -strip-dwo %t
----------------
The test file should probably just be called "groups.test" rather than "test-groups.test". The "test" part is implicit!


================
Comment at: test/tools/llvm-objcopy/test-groups.test:2
+# RUN: cp %p/Inputs/groups.o %t
+# RUN: llvm-objcopy -strip-dwo %t
+# RUN: llvm-readobj -section-data -sections %t | FileCheck %s
----------------
Why -strip-dwo? What do you expect to happen here.


================
Comment at: tools/llvm-objcopy/Object.cpp:390-391
+  SectionWithSymTab::initialize(SecTable);
+  assert(Contents.size() % sizeof(ELF::Elf32_Word) == 0);
+  assert(!Contents.empty());
+  const ELF::Elf32_Word *Word =
----------------
These shouldn't be asserts, as it is theoretically possible for a malformed section to have SHT_GROUP, but not conform to these requirements. They should be errors.


Repository:
  rL LLVM

https://reviews.llvm.org/D43996