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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Have few general comments.



================
Comment at: test/tools/llvm-objcopy/basic-copy.test:3
+# RUN: llvm-objcopy %t %t2
+# RUN: llvm-readobj -sections %t2 | FileCheck %s
+!ELF
----------------
I would add empty line here.


================
Comment at: test/tools/llvm-objcopy/basic-copy.test:15
+    AddressAlign:    0x0000000000000010
+    Content:         "00000000"
+# CHECK:      Name: .text
----------------
And here. Just to separate execution flow from check in a more obvious way.


================
Comment at: tools/llvm-objcopy/Object.cpp:36
+  auto MinElem =
+      std::min_element(std::begin(Sections), std::end(Sections), CompOffset);
+  Offset = (**MinElem).Offset;
----------------
I would inline CompOffset, it is trivial operation that does not probably require addition comparator.


================
Comment at: tools/llvm-objcopy/Object.cpp:76
+void StringTableSection::removeString(StringRef Name) {
+  size_t count = Strings.erase(Name);
+  // We need to account for the null character as well
----------------
Count


================
Comment at: tools/llvm-objcopy/Object.cpp:121
+    Seg.Index = Index;
+    Index++;
+    for (auto &Section : Sections) {
----------------
  Seg.Index = Index++;


================
Comment at: tools/llvm-objcopy/Object.cpp:189
+  // possible while meeting that demand however.
+  auto CompareSections = [](const SecPtr &a, const SecPtr &b) {
+    if(a->Type == SHT_NULL) return true;
----------------
A, B


================
Comment at: tools/llvm-objcopy/Object.cpp:198
+
+uint64_t align(uint64_t value, uint64_t multiple) {
+  if(!multiple || value % multiple == 0) return value;
----------------
LLVM code style says variable names should be uppercase. Value, Multiple, so on.


================
Comment at: tools/llvm-objcopy/Object.cpp:199
+uint64_t align(uint64_t value, uint64_t multiple) {
+  if(!multiple || value % multiple == 0) return value;
+  return value + multiple - value % multiple;
----------------
Is it clang-formatted ?


================
Comment at: tools/llvm-objcopy/Object.cpp:269
+  Ehdr.e_phnum = Segments.size();
+  Ehdr.e_shentsize = sizeof(typename ELFT::Shdr);
+  Ehdr.e_shnum = Sections.size();
----------------
Consider using typedefs in header
(example from LLD code)

```
template <class ELFT> class Writer {
public:
  typedef typename ELFT::Shdr Elf_Shdr;
  typedef typename ELFT::Ehdr Elf_Ehdr;
  typedef typename ELFT::Phdr Elf_Phdr;
```


================
Comment at: tools/llvm-objcopy/Object.h:43
+  uint64_t Align;
+  uint32_t EntrySize;
+
----------------
Sort by Name may be ?

And looks you can do initialization inline instead of doing that in constructor below.


================
Comment at: tools/llvm-objcopy/Object.h:94
+private:
+  std::map<llvm::StringRef, uint32_t> Strings;
+
----------------
Consider using llvm::StringMap may be ?


================
Comment at: tools/llvm-objcopy/Object.h:104
+    Align = 1;
+    EntrySize = 0;
+  }
----------------
I think you can inline initialization,


================
Comment at: tools/llvm-objcopy/Object.h:132
+  void writeSectionData(llvm::FileOutputBuffer &) const;
+  void writeSectionHeaders(llvm::FileOutputBuffer &) const;
+
----------------
Not sure how much is in common not to include argument names in definitions.
(in LLD where I spend most of time I am working on, we always do that, but may be it is ok, just noticable thing for me).


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:53
+                                    cl::init("-"));
+void CopyBinary(const ELFObjectFile<ELF64LE> &ObjFile) {
+  std::unique_ptr<FileOutputBuffer> Buffer;
----------------
ELF64LE ? I guess you want to template it.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:94
+  } else
+    reportError(InputFilename, object_error::invalid_file_type);
+  return 0;
----------------
You either want to remove brackets here:

```
  if (ELFObjectFile<ELF64LE> *o = dyn_cast<ELFObjectFile<ELF64LE>>(&Binary))
    CopyBinary(*o);
   else
    reportError(InputFilename, object_error::invalid_file_type);
```

Or probably better just to early return:

```
  if (ELFObjectFile<ELF64LE> *o = dyn_cast<ELFObjectFile<ELF64LE>>(&Binary))
    return CopyBinary(*o);

  reportError(InputFilename, object_error::invalid_file_type);
   return X;
```


https://reviews.llvm.org/D33964