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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Nearly there from my point of view!



================
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.
----------------
Nit: the wrapping on this comment looks weird.


================
Comment at: test/tools/llvm-objcopy/group-unchanged.test:43-44
+  Local:           
+    - Name:            main.cpp
+      Type:            STT_FILE
+    - Name:            .text.foo
----------------
You can get rid of the STT_FILE symbol as well.


================
Comment at: test/tools/llvm-objcopy/group.test:5-9
+# In this test the section .comment is getting removed,
+# as a result, the indices of all 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).
----------------
Wapping again here.


================
Comment at: test/tools/llvm-objcopy/group.test:34
+      - SectionOrType:   .text.foo
+  - Name:            .comment
+    Type:            SHT_PROGBITS
----------------
Thinking about it, it would be good if there were a symbol in this section, that appears before .text.foo, so that we can show that sh_info gets updated too (same applies to the other test, except the inverse - the removal of a symbol won't cause a change in sh_info).


================
Comment at: test/tools/llvm-objcopy/strip-dwo-groups.test:21-24
+// In this test `llvm-objcopy -strip-dwo` strips out dwo sections,
+// as a result the index of the symbol table, the indices of the symbols
+// and the indices of the sections which go after the removed ones will change.
+// Consequently, the fields Link, Info and the content of .group need to be updated. 
----------------
Sorry, I guess I should have been clearer: I think it would be good to explicitly state why this test case exists at all (i.e. why aren't the other two sufficient). The main point is, by my understanding, this is a "real world" use case that was previously failing.


================
Comment at: tools/llvm-objcopy/Object.cpp:281
   if (Symbols == Sec) {
-    error("Symbol table " + Symbols->Name + " cannot be removed because it is "
-                                            "referenced by the relocation "
-                                            "section " +
+    error("Symbol table " + Symbols->Name +
+          " cannot be removed because it is "
----------------
jakehehrlich wrote:
> I realize this might look a little strange but it's what clang-format does if I recall correctly. I want this to be whatever clang-format makes it not what actually looks nice. Changing how this error message is generated to make it look nice would be an improvement though.
@alexshap, can you confirm that you've clang-formatted this bit here, like @jakehehrlich pointed out?


Repository:
  rL LLVM

https://reviews.llvm.org/D43996