[PATCH] D76252: [lld-macho] Add basic support for linking against dylibs

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/MachO/Arch/X86_64.cpp:38
   default:
-    error("TODO: Unhandled relocation type");
+    error("TODO: Unhandled relocation type " + std::to_string(type));
     return 0;
----------------
MaskRay wrote:
> This message enhancement should be moved to the initial patch.
Yeah, but I'm not worried about the exact order and the contents of initial patch series, as this is going to be submitted as soon as the first patch is submitted, and no one will be using the feature only with the first patch, so I think I'm fine with this.


================
Comment at: lld/MachO/Target.h:18
 enum {
+  WordSize = 8,
   PageSize = 4096,
----------------
Are you going to make it work only for 64-bit platforms? IIRC, both macOS and iOS are dropping 32-bit app support, so it is probably a good choice to not support 32-bit, but just to confirm.


================
Comment at: lld/MachO/Writer.cpp:297
+    for (Reloc &r : sect->relocs) {
+      if (auto *s = r.target.dyn_cast<Symbol *>()) {
+        if (auto dylibSymbol = dyn_cast<DylibSymbol>(s))
----------------
I wonder if you directly dyn_cast to `DylibSymbol *` as `r.target.dyn_cast<DylibSymbol *>()`.


================
Comment at: lld/MachO/Writer.cpp:332
 
+void Writer::createDyldInfoContents() {
+  uint64_t sectionStart = linkEditSeg->getOffset();
----------------
Can you add a comment as to what dyld info contains?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76252