[PATCH] D71647: [llvm-objcopy][MachO] Handle relocation entries where r_extern is 0

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/copy-relocations.s:12-16
+## $ llvm-objdump --macho --reloc copy-relocations.o
+## Relocation information (__DATA,__bar) 2 entries
+## address  pcrel length extern type    scattered symbolnum/value
+## 00000000 False quad   False  SUB     False     3 (__DATA,__bar)
+## 00000000 False quad   False  UNSIGND False     1 (__TEXT,__text)
----------------
I don't think you need this comment.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/copy-relocations.s:27-30
+# CHECK:      Relocation information (__DATA,__bar) 2 entries
+# CHECK-NEXT: address  pcrel length extern type    scattered symbolnum/value
+# CHECK-NEXT: 00000000 False quad   False  SUB     False     2 (__DATA,__bar)
+# CHECK-NEXT: 00000000 False quad   False  UNSIGND False     1 (__TEXT,__text)
----------------
I think this should go in place of the comment above.

Is it possible to use yaml to create this situation instead of assembly? Assembly is less obvious as to what you want to achieve.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOReader.cpp:209-210
+  for (auto &LC : O.LoadCommands)
+    for (std::unique_ptr<Section> &Sec : LC.Sections)
+      Sections.push_back(Sec.get());
+
----------------
If you don't change the Sections to unique_ptrs, you could change this to:
```
    for (const Section &Sec : LC.Sections)
      Sections.push_back(&Sec);
```


================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.h:93
   // corresponding content inside the binary.
-  std::vector<Section> Sections;
+  std::vector<std::unique_ptr<Section>> Sections;
 
----------------
Is this really necessary? As far as I can tell, the only place it's actually important is where you create a vector of section pointers, but you can do that without needing these to be unique_ptrs. I've commented above.


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

https://reviews.llvm.org/D71647