[PATCH] D100818: [lld-macho] Implement branch-range-extension thunks

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Arch/ARM64.cpp:112
   stubSize = sizeof(stubCode);
+  thunkSize = llvm::alignTo<16>(stubSize);
+  branchRange = llvm::maxIntN(28) - thunkSize;
----------------
could you comment on why the alignTo is needed?

also remove llvm::


================
Comment at: lld/MachO/Arch/ARM64.cpp:120
+  thunk->align = 4;
+  // FIXME: populate data
+  thunk->data = {reinterpret_cast<const uint8_t *>(stubCode), sizeof(stubCode)};
----------------
stale comment? (since data seems like it's being populated)


================
Comment at: lld/MachO/InputSection.cpp:39
   const RelocAttrs &relocAttrs = target->getRelocAttrs(type);
   if (relocAttrs.hasAttr(RelocAttrBits::BRANCH)) {
+    return sym->resolveBranchVA();
----------------
I guess we can do away with the braces now


================
Comment at: lld/MachO/InputSection.h:48
+  bool isThunkable = false;
+  bool isFinal = false;
 
----------------
would be good to have a comment; the link to `finalize()` isn't immediately obvious


================
Comment at: lld/MachO/MergedOutputSection.cpp:37-38
   input->parent = this;
+  if (input->isThunkable)
+    isThunkable = true;
 }
----------------
nit: `isThunkable |= input->isThunkable;`


================
Comment at: lld/MachO/MergedOutputSection.cpp:88
+//   prepareSymbolRelocation() and prepareBranchTarget() dig into
+//   Reloc records. They flip the new booleans Reloc::isThunkable,
+//   Inputsection::isThunkable, and MergedOutputSection::isThunkable
----------------
it won't be 'new' after this lands :)


================
Comment at: lld/MachO/MergedOutputSection.cpp:102
+// * MergedInputSection::finalize() and MergedInputSection::writeTo()
+//   merge the inputs and thunks vectors (both ordered by asending
+//   address), which is simple and cheap.
----------------



================
Comment at: lld/MachO/MergedOutputSection.cpp:127
+  size_t thunkSize = target->thunkSize;
+  if (thunkSize == 0) {
+    for (InputSection *isec : inputs)
----------------
seems more readable


================
Comment at: lld/MachO/MergedOutputSection.cpp:144-146
+  size_t ic = 0;
+  size_t ix = 0;
+  size_t ie = inputs.size();
----------------
I find these kind of hard to read, could we use slightly more verbose names?


================
Comment at: lld/MachO/MergedOutputSection.cpp:147
+  size_t ie = inputs.size();
+  assert(ie > 0);
+  // Kick-off by ensuring that the first input section has an address
----------------
doesn't seem like a useful assert


================
Comment at: lld/MachO/MergedOutputSection.cpp:149
+  // Kick-off by ensuring that the first input section has an address
+  finalizeOne(inputs[ix++]);
+  for (; ic < ie; ic++) {
----------------
tschuett wrote:
> LLVM prefers preincrement:
> https://llvm.org/docs/CodingStandards.html#prefer-preincrement
> 
> There a couple of postincrements.
I mean this line actually does want to return a copy of the original value, so I think it's fine... but yeah the increments-for-effect-only should be preincrements


================
Comment at: lld/MachO/MergedOutputSection.cpp:158
+      continue;
+    assert(isec->getSize() > 0ull);
+    // Process relocs by ascending address
----------------
redundant assert


================
Comment at: lld/MachO/MergedOutputSection.cpp:160
+    // Process relocs by ascending address
+    for (Reloc &r : reverse(isec->relocs)) {
+      relocCount++;
----------------
I believe llvm-mc emits relocs in reverse order, but I don't think that's guaranteed anywhere in the format... we should probably sort it ourselves


================
Comment at: lld/MachO/MergedOutputSection.cpp:170-171
+      // Calculate our call referent address
+      auto *funcSym = r.referent.dyn_cast<Symbol *>();
+      assert(funcSym);
+      assert(isa<Defined>(funcSym) || isa<DylibSymbol>(funcSym));
----------------
(`get<>()` will assert)


================
Comment at: lld/MachO/MergedOutputSection.cpp:208
+      thunkInfo.isec->parent = this;
+      Twine thunkName =
+          funcSym->getName() + ".thunk." + std::to_string(thunkInfo.sequence++);
----------------
I think the lint is right here... the return value of `to_string` is going to be unowned


================
Comment at: lld/MachO/MergedOutputSection.cpp:222
+
+  if (config->verbose)
+    warn("thunks for " + parent->name + "," + name +
----------------
LLD-ELF handles `--verbose` by assigning to `errorHandler().verbose`, I think we should do likewise


================
Comment at: lld/MachO/MergedOutputSection.cpp:233-244
+  size_t i = 0, ie = inputs.size();
+  size_t t = 0, te = thunks.size();
+  while (i < ie || t < te) {
+    while (i < ie && (t == te || inputs[i]->getSize() == 0 ||
+                      inputs[i]->outSecOff < thunks[t]->outSecOff)) {
+      inputs[i]->writeTo(buf + inputs[i]->outSecFileOff);
+      i++;
----------------
instead of two sorted arrays, would it be simpler to create a map of regular InputSection to an array of thunks that immediately follow it?


================
Comment at: lld/MachO/MergedOutputSection.h:22
+
+using InputsVector = std::vector<InputSection *>;
+
----------------
nit: this type seems too simple to be worth an alias. Also since other places in the codebase (like the global `inputSections`) don't use it, we now have two different ways to refer to the same type in the codebase...


================
Comment at: lld/MachO/MergedOutputSection.h:41-42
   void finalize() override;
+  void finalizeWithThunks();
+  void finalizeWithoutThunks();
 
----------------
doesn't seem like these are implemented


================
Comment at: lld/MachO/MergedOutputSection.h:50-53
+  size_t createThunks();
+  Defined *makeThunk(StringRef name, uint64_t maxAddr);
+  uint64_t placeThunk(uint64_t callSiteAddr, uint64_t nearestThunkI) const;
+  bool needsThunk(Symbol funcSym, uint64_t callSiteAddr) const;
----------------
nor any of these


================
Comment at: lld/MachO/Symbols.h:251-257
+#define X(T) alignas(T) char u_##T[sizeof(T)]
+  X(Defined);
+  X(Undefined);
+  X(CommonSymbol);
+  X(DylibSymbol);
+  X(LazySymbol);
+#undef X
----------------
this can be committed on its own as an NFC commit... try to keep the diff to relevant changes only


================
Comment at: lld/MachO/Target.h:67
 
+  virtual void populateThunk(InputSection *thunk, Symbol *funcSym) {}
+
----------------
nit: insert `llvm_unreachable()` in the body?


================
Comment at: lld/MachO/Writer.cpp:545-546
       }
+    } else {
+      return target->thunkSize > 0;
     }
----------------
why aren't externalWeakDefs thunkable?


================
Comment at: lld/MachO/Writer.cpp:549
   }
+  return false;
 }
----------------
do we actually get here in practice? I would assume that `prepareBranchTarget` is only ever called on DylibSymbols and Defined symbols...


================
Comment at: lld/MachO/Writer.cpp:603-605
+        if (isa<LazySymbol>(sym))
+          warn("relocation to lazy symbol: " + toString(*sym) +
+               "\n>>> defined in " + toString(sym->getFile()));
----------------
I thought we discussed that this case should be impossible; how does it arise?


================
Comment at: lld/MachO/Writer.cpp:1070
+  // After this point, we create no new segments; HOWEVER, we might
+  // yet create branch-range extention thunks for architectures whose
+  // hardware call instructions have limited range, e.g., ARM(64)
----------------



================
Comment at: lld/MachO/Writer.cpp:1073
+  // Since the thunks are interspersed with text input sections,
+  // we create the t
   sortSegmentsAndSections();
----------------
seems like you forgot to finish this sentence :p 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100818