[PATCH] D33964: [LLVM][llvm-objcopy] Added basic plumbing to get things started

Michael Spencer via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 21 17:20:42 PDT 2017


Bigcheese requested changes to this revision.
Bigcheese added a comment.
This revision now requires changes to proceed.

Thanks for working on this! It's been a low priority on my todo list for years now.

I'm a bit concerned about the section layout algorithm. It ignores the section offsets from the input sections and just computes new ones derived from the address and size. This will fail when given an ELF which has a gap in the sections in a given segment. I think the hardest part of objcopy will be making sure to keep the file in a consistent state, and I'd like to see a design that makes it hard to get wrong.



================
Comment at: tools/llvm-objcopy/Object.cpp:171
+  Flags = Ehdr.e_flags;
+  SectionNames = new StringTableSection();
+  SectionNames->Name = ".shstrtab";
----------------
Memory leak. Use unique_ptr.


================
Comment at: tools/llvm-objcopy/Object.cpp:193
+
+uint64_t align(uint64_t Value, uint64_t Multiple) {
+  if (!Multiple || Value % Multiple == 0)
----------------
Use alignTo from llvm/Support/MathExtras.h


================
Comment at: tools/llvm-objcopy/Object.cpp:224
+  // a little bit of tweaking to ensure that the sh_name is 4 byte aligned
+  Offset += 4 - Offset % 4;
+  SHOffset = Offset;
----------------
Replace with alignTo?


================
Comment at: tools/llvm-objcopy/Object.h:3
+//
+//                             The LLVM Linker
+//
----------------
`The LLVM Compiler Infrastructure`


================
Comment at: tools/llvm-objcopy/Object.h:90
+
+class StringTableSection : public SectionBase {
+private:
----------------
This should probably use llvm/MC/StringTableBuilder.h


================
Comment at: tools/llvm-objcopy/Object.h:100
+  void addString(llvm::StringRef Name);
+  void removeString(llvm::StringRef Name);
+  uint32_t findIndex(llvm::StringRef Name) const;
----------------
dead code


================
Comment at: tools/llvm-objcopy/Object.h:104-106
+  static bool classof(const SectionBase *S) {
+    return S->Type == llvm::ELF::SHT_STRTAB;
+  }
----------------
dead code


Repository:
  rL LLVM

https://reviews.llvm.org/D33964





More information about the llvm-commits mailing list