[PATCH] D66409: [llvm-objcopy][MachO] Implement -Obinary

[Seiya Nuta via Phabricator via llvm-commits]

seiya added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:503
+  // Sort sections by their addresses.
+  for (LoadCommand &LC : O.LoadCommands)
+    for (Section &Sec : LC.Sections)
----------------
grimar wrote:
> `LC` -> `Cmd`?
`LC` is the common abbreviation for LoadCommand under `llvm-objcopy/MachO`.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:518
+      Sec->Addr -= AddrBase;
+  }
+
----------------
grimar wrote:
> This can be:
> 
> ```
> // Remove ....
> for (Section *Sec : OrderedSections)
>   Sec->Addr -= OrderedSections[0]->Addr;
> ```
I don't think it works. This for loop subtracts the //original// value of the first section's address (`OrderedSections[0]->Addr`). In the code you suggested,  in the first iteration, `Sec` points to `OrderedSections[0]` and as a result `OrderedSections[0]->Addr` becomes 0 in remaining iterations.



================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.h:72
+  Object &O;
+  Buffer &B;
+  MachOLayoutBuilder LayoutBuilder;
----------------
grimar wrote:
> What do you think of using probably more traditional/clear `Obj` and `Buf` names?
> 
I'd like to keep them because `O` and `B` are widely used under `llvm-objcopy/MachO`. That said, `Obj` and `Buf` looks better to me. I'll submit another NFC patch to rename them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66409