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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

This looks pretty close to me. I'm happy with the code as I understand sans one thing and a couple TODOs. After fixing the getSectionOfType thing, adding the TODOs, if you get the LGTM on tests and everything from James I'm happy with this.



================
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 "
----------------
alexshap wrote:
> jhenderson wrote:
> > 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?
> yes, that's what clang-format currently does
> (clang-format -i -style=llvm Object.cpp)
I was running clang-format without specifying the style so the default style might not quite be llvm style on my system. Thanks for catching this!


================
Comment at: tools/llvm-objcopy/Object.cpp:363-368
+  auto S = SecTable.getSection(Link, "Link field value " + Twine(Link) +
+                                         " in section " + Name + " is invalid");
+  SymTab = dyn_cast<SymbolTableSection>(S);
+  if (!SymTab)
+    error("Link field value " + Twine(Link) + " in section " + Name +
+          " is not a symbol table");
----------------
We have getSectionOfType for this purpose.


================
Comment at: tools/llvm-objcopy/Object.h:436
 
-class SectionWithStrTab : public Section {
-private:
-  const SectionBase *StrTab = nullptr;
+class GroupSection : public SectionBase {
+  MAKE_SEC_WRITER_FRIEND
----------------
Can you add a TODO that says something like "The way stripping and groups interact is complicated and still needs to be worked on".


================
Comment at: tools/llvm-objcopy/Object.h:439
+
+  ArrayRef<uint8_t> Contents;
+  ELF::Elf32_Word FlagWord;
----------------
Can you add a TODO here about getting rid of Contents once initialize is refactored?


Repository:
  rL LLVM

https://reviews.llvm.org/D43996