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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:161
+template <class ELFT> void Object<ELFT>::sortSections() {
+  // Put allocated sections in address order. Maintain ordering as closely as
+  // possible while meeting that demand however.
----------------
address order -> offset order


================
Comment at: tools/llvm-objcopy/Object.cpp:185
+      // We should respect interstitial gaps of allocated sections
+      Offset = FirstInSeg->Offset + Section->Addr - FirstInSeg->Addr;
+    }
----------------
Could you explain why we are using a mix of offsets and addresses here, as it is surprising to me.


================
Comment at: tools/llvm-objcopy/Object.cpp:196
+  // section header table offset to be exactly here. This spot might not be
+  // aligned properlly however so we should align it as needed. This only takes
+  // a little bit of tweaking to ensure that the sh_name is 4 byte aligned.
----------------
properlly -> properly


================
Comment at: tools/llvm-objcopy/Object.cpp:197-198
+  // aligned properlly however so we should align it as needed. This only takes
+  // a little bit of tweaking to ensure that the sh_name is 4 byte aligned.
+  Offset = alignTo(Offset, sizeof(typename ELFT::Word));
+  SHOffset = Offset;
----------------
Why only the sh_name field? 4-byte alignment could cause other struct members to be misaligned (e.g. the sh_addr and sh_offset fields), because they are 8-bytes in size.


Repository:
  rL LLVM

https://reviews.llvm.org/D33964