[PATCH] D36560: [llvm-objcopy] Add support for .dynamic, .dynsym, and .dynstr

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I take it yaml2obj cannot create dynamic section/symbol table information? If it could, it would mean that no pre-built binaries would be needed for the tests.

A thought on the duplication of DynamicSymbolTableSection and DynamicSection: what would you think of having a shared class called something like "SectionWithStringTable", and then implementing these two section types as inheriting from that one. The only logic needed in DynamicSection/DynamicSymbolTableSection would then be the classof method.



================
Comment at: test/tools/llvm-objcopy/dynsym.test:3
+# RUN: llvm-readobj -dyn-symbols %t | FileCheck %s
+
+#CHECK: DynamicSymbols [
----------------
Please also check the section header data like in the other tests.


================
Comment at: tools/llvm-objcopy/Object.cpp:347
+    // mean altering the memory image. There are no speical link types or
+    // anything so we can just use a Section
+    if (Shdr.sh_flags & SHF_ALLOC) {
----------------
Full stop.


================
Comment at: tools/llvm-objcopy/Object.cpp:356
+    // Hash tables should refer to SHT_DYNSYM which we're not going to change.
+    // Because of this we don't need to mess with the hash tables anyway
+    Data = unwrapOrError(ElfFile.getSectionContents(&Shdr));
----------------
Ditto. I'd also replace the last word "anyway" with "either".


================
Comment at: tools/llvm-objcopy/Object.cpp:364
+    Data = unwrapOrError(ElfFile.getSectionContents(&Shdr));
+    return make_unique<DynamicSymbolTableSection>(Data);
   case SHT_SYMTAB: {
----------------
DynamicSection not DynamicSymbolTableSection!


================
Comment at: tools/llvm-objcopy/Object.cpp:415
+      DynamicSec->setStrTab(
+          dyn_cast<StringTableSection>(Sections[DynamicSec->Link - 1].get()));
+    }
----------------
As with other cases, we need to check that the Link value is actually a valid section index, or we will get a crash. Since this is a recurring theme, perhaps Object should gain a getSection() method that does the error checking?


================
Comment at: tools/llvm-objcopy/Object.cpp:419
+      DynSymTab->setStrTab(
+          dyn_cast<StringTableSection>(Sections[DynSymTab->Link - 1].get()));
+    }
----------------
Ditto.


================
Comment at: tools/llvm-objcopy/Object.h:180-183
+// Oddly this seems to be a complete duplication of DynamicSection because they
+// both need their sh_link to be changed if sections get moved around at all.
+// Still logically these are two different things so they have two different
+// classes.
----------------
As the DynamicSection class currently appears after this class, I think that either a) the comment should be moved to the second class to appear in order (i.e. DynamicSection), or b) the DynamicSection should be moved to before the DynamicSymbolTableSection. This means that the comment refers to something that a user reading top-down would have already read about.


Repository:
  rL LLVM

https://reviews.llvm.org/D36560