[PATCH] D80568: [llvm-objcopy][ELF] Fix removing a group member section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/remove-section-in-group.test:6
+
+## This checks that the group section is shrunk when its member is removed.
+
----------------
I'd probably move this comment above the RUN bit, since it describes the test as a whole.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/remove-section-in-group.test:17
+    Type:            SHT_GROUP
+    Link:            .symtab
+    Info:            foo_bar_grp
----------------
I suspect (don't know for sure) that you can omit the Link line here. I think yaml2obj adds it automatically by default for these sort of sections.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/remove-section-in-group.test:33
+
+# CHECK:      - Name:            .group
+# CHECK:        Members:
----------------
I personally find it easier to follow a test if the CHECK is closer to where it is used by FileCheck. I'd layout the file (with optional new lines at appropriate spots):

```
## Comments
# RUN: ...
# CHECK:

<yaml>
```


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:973
+Error GroupSection::removeSectionReferences(
+    bool AllowBrokenLinks, function_ref<bool(const SectionBase *)> ToRemove) {
+  llvm::erase_if(GroupMembers, ToRemove);
----------------
What happens if you remove the symbol table itself? I'm slightly concerned that it could leave the sh_link field invalid as presumably `Section::removeSectionReferences` won't be called for the GroupSection.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80568