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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/group-unchanged.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy -remove-section=.comment.2 %t %t2
----------------
Please add a brief comment explaining the purpose of this test, and in particular, how it is different from group.test (and also a corresponding comment in group.test).


================
Comment at: test/tools/llvm-objcopy/group-unchanged.test:10
+# CHECK-NEXT:      Type: COMDAT (0x1)
+# CHECK-NEXT:      Signature: _ZN1SIiE4getXEv
+# CHECK:           .text._ZN1SIiE4getXEv (5)
----------------
Since this isn't a regression test (that job is for strip-dwo-groups.test), let's keep the names and section contents simpler, i.e. let's rename the symbols to simple C-style symbols (e.g. foo, bar etc), along with their corresponding section(s).


================
Comment at: test/tools/llvm-objcopy/group-unchanged.test:24
+    AddressAlign:    0x0000000000000010
+    Content:         554889E54883EC10488D7DF8E8000000008945F44883C4105DC3
+  - Name:            .rela.text
----------------
We don't need this or the relocation section at all, I believe.


================
Comment at: test/tools/llvm-objcopy/group-unchanged.test:43
+      - SectionOrType:   .text._ZN1SIiE4getXEv
+  - Name:            .comment
+    Type:            SHT_PROGBITS
----------------
Do we need this section?


================
Comment at: test/tools/llvm-objcopy/group-unchanged.test:53
+    Content:         554889E5B80100000048897DF85DC3
+  - Name:            .note.GNU-stack
+    Type:            SHT_PROGBITS
----------------
I don't think we need this section either.


================
Comment at: test/tools/llvm-objcopy/group-unchanged.test:57
+    Content:         ''
+  - Name:            .eh_frame
+    Type:            SHT_X86_64_UNWIND
----------------
Or this section.


================
Comment at: test/tools/llvm-objcopy/group-unchanged.test:95
+  Weak:            
+    - Name:            _ZN1SIiE4getXEv
+      Type:            STT_FUNC
----------------
We only need this symbol, and all the others can be removed. The symbol size can be zero as well.


================
Comment at: test/tools/llvm-objcopy/group-unchanged.test:99
+      Size:            0x000000000000000F
+DynamicSymbols:  
+...
----------------
I don't think we need this.


================
Comment at: test/tools/llvm-objcopy/group.test:1
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy -remove-section=.comment %t %t2
----------------
Most of the comments I made in group-unchanged.test also apply here.


================
Comment at: test/tools/llvm-objcopy/strip-dwo-groups.test:1
+# RUN: cp %p/Inputs/groups.o %t
+# RUN: llvm-objcopy -strip-dwo %t
----------------
As this is effectively a regression test, it would be good to comment as such in the test file.


================
Comment at: tools/llvm-objcopy/Object.cpp:362
 
+void GroupSection::finalize() { 
+  this->Info = Symbol->Index;
----------------
To match the class declaration, please put this function below initialize().


Repository:
  rL LLVM

https://reviews.llvm.org/D43996