[PATCH] D83103: [lld-macho] Support binding dysyms to any section

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/SyntheticSections.cpp:105
+struct Binding {
+  OutputSegment *segment{nullptr};
+  uint64_t offset{0};
----------------
smeenai wrote:
> Super nit: can we use `=` initialization instead, for consistency with the rest of the code? (At least, I think `=` initialization is standard in LLD.)
ah yeah I'm just in the habit of using {} normally. will change


================
Comment at: lld/MachO/SyntheticSections.cpp:177
   raw_svector_ostream os{contents};
-  os << static_cast<uint8_t>(MachO::BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB |
-                             in.got->parent->index);
-  encodeULEB128(in.got->getSegmentOffset(), os);
-  uint32_t entries_to_skip = 0;
-  for (const Symbol *sym : in.got->getEntries()) {
-    if (const auto *dysym = dyn_cast<DylibSymbol>(sym)) {
-      if (entries_to_skip != 0) {
-        os << static_cast<uint8_t>(MachO::BIND_OPCODE_ADD_ADDR_ULEB);
-        encodeULEB128(WordSize * entries_to_skip, os);
-        entries_to_skip = 0;
-      }
-
-      // TODO: Implement compact encoding -- we only need to encode the
-      // differences between consecutive symbol entries.
-      if (dysym->file->ordinal <= MachO::BIND_IMMEDIATE_MASK) {
-        os << static_cast<uint8_t>(MachO::BIND_OPCODE_SET_DYLIB_ORDINAL_IMM |
-                                   dysym->file->ordinal);
-      } else {
-        error("TODO: Support larger dylib symbol ordinals");
-        continue;
-      }
-      os << static_cast<uint8_t>(
-                MachO::BIND_OPCODE_SET_SYMBOL_TRAILING_FLAGS_IMM)
-         << dysym->getName() << '\0'
-         << static_cast<uint8_t>(MachO::BIND_OPCODE_SET_TYPE_IMM |
-                                 MachO::BIND_TYPE_POINTER)
-         << static_cast<uint8_t>(MachO::BIND_OPCODE_DO_BIND);
-    } else {
-      // We have a defined symbol with a pre-populated address; skip over it.
-      ++entries_to_skip;
+  Binding lastBinding;
+  bool did_encode{false};
----------------
smeenai wrote:
> We'll initialize e.g. addend to 0 here, and for the GOT entries, the addend is always 0, so the `lastBinding.addend != addend` condition will never be taken and we won't emit an initial `BIND_OPCODE_SET_ADDEND_SLEB`. Will dyld assume 0 in that case, or should we initialize it explicitly?
> 
> Seems like we were never setting the addend at all before, so maybe assuming dyld will default to 0 is okay? Should probably double-check though.
yes I'm pretty sure this is okay. ld64 doesn't emit `SET_ADDEND` unless we have a non-zero addend.


================
Comment at: lld/MachO/SyntheticSections.cpp:179
+  bool did_encode{false};
+  SetVector<const Symbol *> entries = in.got->getEntries();
+  for (size_t i = 0, n = entries.size(); i < n; ++i) {
----------------
smeenai wrote:
> Are we creating a copy of all the entries here?
oops


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83103