[PATCH] D62652: [llvm-objcopy][MachO] Recompute and update offset/size fields in the writer

[Seiya Nuta via Phabricator via llvm-commits]

seiya marked 22 inline comments as done.
seiya added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/real-world-input-copy.test:13
+# RUN: llvm-objcopy %t.some-symbols.o %t.some-symbols.copy.o
+# RUN: cmp %t.some-symbols.o %t.some-symbols.copy.o
+
----------------
alexshap wrote:
> compnerd wrote:
> > The test input seems oddly complicated for testing that.  It seems that anything that contains a `LC_DYNSYMTAB` which means anything with an externally visible symbol should be sufficient.  The BSS section would be emitted by any static.  Something like this seems much more concise which is better for testing the cases:
> > 
> > ```
> > static int i;
> > int f(void) { return i; }
> > ```
> +1
Indeed. Simplified the test case.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOReader.cpp:22
+// strlen(Str), otherwise return MaxLen.
+size_t strlenOrMaxLen(const char *Str, size_t MaxLen) {
+  if (Str[MaxLen - 1] != '\0')
----------------
compnerd wrote:
> Isn't this just `strnlen`?
Quite so :)


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:145
+      if (IsLittleEndian != sys::IsLittleEndianHost)
+        MachO::swapStruct(MLC.segment_command_data);
+      memcpy(Begin, &MLC.segment_command_data, sizeof(MachO::segment_command));
----------------
compnerd wrote:
> I wish that we had the structs built using `packed_endian` types
I wonder if we could do this better too. Could you tell me what `packed_endian` is? You mean  `llvm/Support/Endian.h`? I'd like to take a look at it.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:478
+    uint64_t FileOffsetInSegment = 0;
+    for (auto &Sec : LC.Sections) {
+      auto Align = pow(2, Sec.Align);
----------------
compnerd wrote:
> Can't `Sec` be const?
No. It's mutated in the loop.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:517
+  // Lay out relocations.
+  for (auto &LC : O.LoadCommands)
+    for (auto &Sec : LC.Sections) {
----------------
compnerd wrote:
> Can't `LC` be const?
It can't be const since its sections are mutated in the loop.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:518
+  for (auto &LC : O.LoadCommands)
+    for (auto &Sec : LC.Sections) {
+      Sec.RelOff = Sec.Relocations.empty() ? 0 : Offset;
----------------
compnerd wrote:
> Can't `Sec` be const?
No. It's mutated in the loop.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:526
+  auto NListSize = Is64Bit ? sizeof(MachO::nlist_64) : sizeof(MachO::nlist);
+  for (auto &LC : O.LoadCommands) {
+    auto &MLC = LC.MachOLoadCommand;
----------------
compnerd wrote:
> Can't `LC` be const?
No. `LC. MachOLoadCommand ` is mutated in the loop.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:487
+
+      VMSize = std::max(VMSize, Sec.Addr + Sec.Size);
+    }
----------------
I've replaced the calculation of VMSize by the algorithm used in `MachObjectWriter::writeObject`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62652/new/

https://reviews.llvm.org/D62652