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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/Inputs/various-symbols.s:2
+# static int local1, local2; // Local Symbols.
+# int global1;        // A coomon symmbol.
+# char global2 = 123; // A extern symbol.
----------------
NIT: `common` is misspelt


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/Inputs/various-symbols.s:4
+# char global2 = 123; // A extern symbol.
+# int global3 = 456;  // A extern symbol.
+# extern int extern1, extern2, extern3, extern4; // Undefined symbols.
----------------
`global3` is unreferenced.  Is that intended?


================
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
+
----------------
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; }
```


================
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')
----------------
Isn't this just `strnlen`?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOReader.cpp:43
+  S.Sectname = StringRef(Sec.sectname, strlenOrMaxLen(Sec.sectname, 16)).str();
+  S.Segname = StringRef(Sec.segname, strlenOrMaxLen(Sec.segname, 16)).str();
   S.Addr = Sec.addr;
----------------
I believe that the `16` here is supposed to be the maximum length of the section/segment name in MachO.  Can you use `sizeof(llvm::MachO::section::sectname)` and `sizeof(llvm::MachO::section::segname)` instead please?


================
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));
----------------
I wish that we had the structs built using `packed_endian` types


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:149
+
+      for (auto &Sec : LC.Sections) {
+        struct MachO::section Temp;
----------------
Can the `Sec` be const?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:152
+        copyStringWithPadding(Temp.sectname, Sec.Sectname, 16);
+        copyStringWithPadding(Temp.segname, Sec.Segname, 16);
+        Temp.addr = Sec.Addr;
----------------
I think that you can drop `copyStringWithPadding`.  Instead zero initialise the section that you are constructing, which also gives you more reproducible output.


================
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);
----------------
Can't `Sec` be const?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:486
+      }
+      auto VmPaddingSize = OffsetToAlignment(VmOffsetInSegment, Align);
+      VmOffsetInSegment += VmPaddingSize + Sec.Size;
----------------
`VM` is more appropriate - it is an initialism for Virtual Memory.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:517
+  // Lay out relocations.
+  for (auto &LC : O.LoadCommands)
+    for (auto &Sec : LC.Sections) {
----------------
Can't `LC` be const?


================
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;
----------------
Can't `Sec` be const?


================
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;
----------------
Can't `LC` be const?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.h:32
 
+  void updateDysymtab(MachO::macho_load_command &MLC);
+  void updateSizeOfCmds();
----------------
Might be more readable as `updateDySymTab`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62652