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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

+1 on moving small improvements and such to another change. I left some comments on those sorts of changes here since I was thinking about them. In a nutshell I think a) GroupSection shouldn't inherit from anything b) SecitonWithSymTab should be merged into GroupSection since nothing else uses it and c) we should use yaml2obj for testing the groups here. SecitonWithStrTab is a very strange special case that I'd like to avoid in other endeavors.

the sh_info part can easily be handled using getSectionByIndex in initialization and adding a Symbol pointer in the group. Section indexes stay up to date as the symbol table changes so you're allowed to

Handeling section removal with this properly is very complicated and should be handled in another change.



================
Comment at: test/tools/llvm-objcopy/test-groups.test:1
+# RUN: cp %p/Inputs/groups.o %t
+# RUN: llvm-objcopy -strip-dwo %t
----------------
jhenderson wrote:
> The test file should probably just be called "groups.test" rather than "test-groups.test". The "test" part is implicit!
groups are supported in yaml2obj and obj2yaml. Please use those for testing rather than using this. For relocatable files yaml2obj is actually pretty good. It's awful for executables.


================
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
----------------
jhenderson wrote:
> Why -strip-dwo? What do you expect to happen here.
nit: Whatever this tests it can be tested by using -R directly. I'd prefer if -R was used since it's the most direct way of removing something. Other forms of stripping have subtle edge cases.


================
Comment at: test/tools/llvm-objcopy/test-groups.test:3
+# RUN: llvm-objcopy -strip-dwo %t
+# RUN: llvm-readobj -section-data -sections %t | FileCheck %s
+
----------------
instead of using section data can you use elf-section-groups?


================
Comment at: tools/llvm-objcopy/Object.cpp:84
+void BinarySectionWriter::visit(const GroupSection &Sec) {
+  error("Cannot write '.group' out to binary");
+}
----------------
Please use the section name here. This isn't a generated section like .gnu_debuglink...also I should make the above use the section name now that I think about it.


================
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 "
----------------
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.


================
Comment at: tools/llvm-objcopy/Object.cpp:396
+  FlagWord = *Word++;
+  for (; Word != End; ++Word) {
+    GroupMembers.push_back(
----------------
I'm not in love with the existence of initlizeSymbolTable and initlizeRelocations but I think you'll have to do something similar here. Having owned data isn't really what we want. This might be an indication that we need to also pass section data to the initialize method.


================
Comment at: tools/llvm-objcopy/Object.h:261
 
-private:
+protected:
   ArrayRef<uint8_t> Contents;
----------------
I don't think this should be protected. The point of Section it just writes it's data out and nothing else happens. It has no classof because no extra features of it are ever exposed. If you need access to the underlying data then you shouldn't inherit from this, you should be rolling your own section. It should really be called "PlainDataSection" or "RawDataSection" or something like that. I named it terribly.


================
Comment at: tools/llvm-objcopy/Object.h:437
 
-class SectionWithStrTab : public Section {
-private:
-  const SectionBase *StrTab = nullptr;
+class SectionWithSymTab : public Section {
+  const SectionBase *SymTab = nullptr;
----------------
I only see this used one place and this is the last major section type we might ever need to add. I think this should just all be rolled inside of a GroupSection.


Repository:
  rL LLVM

https://reviews.llvm.org/D43996