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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich marked 5 inline comments as done.
jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:185
+      // We should respect interstitial gaps of allocated sections
+      Offset = FirstInSeg->Offset + Section->Addr - FirstInSeg->Addr;
+    }
----------------
jhenderson wrote:
> Could you explain why we are using a mix of offsets and addresses here, as it is surprising to me.
The first an main point is that the Offsets change in this loop and what we want to respect is the difference between the section offsets in the original file. FirstInSeg->Offset no longer respects that information. This is changeable however by adding fields that keep track of the original file offset. This might make sense to use elsewhere as well like in the sorting algorithm where we're trying to respect the original offset as much as possible. This would free up Offset to be messed with (in what way isn't clear) before sorting occurs. So this is changeable but only by adding another field. Perhaps that is not a bad idea however. 

This said when I was thinking about this I never actually even thought to use Offset. I was thinking about respecting the memory image not the file image. I think as a guiding principal "respect the memory image" is a a better guiding principal but only slightly. The memory image and file image should agree for all valid files however so I'm not really opposed to changing it. I have a slight opposition but it's based on some ideas I have for changing the file layout algorithm and I don't want to justify code here based on code that hasn't been written yet. The general idea is that it is possible to enforce "respect the memory image" via const correctness/encapsulation but it is not possible to do the same for the file image because Offset will need to be changed.

My personal opinion is that adding a new Offset like field that isn't quite Offset and using that field to compute the new offset here isn't really any clearer that what I have here. What do you think?


================
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;
----------------
jhenderson wrote:
> 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.
The next field (sh_type) is also a 4-byte word and thus anything after those two will be 8-byte aligned. So this is the minimal alignment I can choose which ensures that all the fields of the section header are aligned. I updated the comment to reflect this fact.


Repository:
  rL LLVM

https://reviews.llvm.org/D33964