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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

looks pretty good. Let's add some tests :)



================
Comment at: lld/MachO/Arch/ARM64.cpp:113
+  thunkSize = stubSize;
+  branchRange = llvm::maxIntN(28) - thunkSize;
   stubHelperHeaderSize = sizeof(stubHelperHeaderCode);
----------------
remove llvm::


================
Comment at: lld/MachO/MergedOutputSection.cpp:87
+//   prepareSymbolRelocation() and prepareBranchTarget() dig into
+//   Reloc records. Relocs::isCallSite, Inputsection::callSiteCount,
+//   and MergedOutputSection::callSiteCount memoize paths to call
----------------



================
Comment at: lld/MachO/MergedOutputSection.cpp:222
+
+  if (config->verbose)
+    warn("thunks for " + parent->name + "," + name +
----------------
gkm wrote:
> int3 wrote:
> > LLD-ELF handles `--verbose` by assigning to `errorHandler().verbose`, I think we should do likewise
> Done. Note that ELF has no other use of `OPT_verbose`. COFF assigns to `errorHandler().verbose`, and also has `config->verbose` to enable other output.
I don't think this line below should be a warning though... `log()` would suffice (and obviate the need for Config::verbose)


================
Comment at: lld/MachO/MergedOutputSection.h:20
 
+class Defined;
+
----------------
leftover?


================
Comment at: lld/MachO/Writer.cpp:36
 #include <algorithm>
+#include <cmath>
 
----------------
leftover?


================
Comment at: lld/MachO/Writer.cpp:515-522
+// Add stubs and bindings where necessary (e.g. if the symbol is a
+// DylibSymbol). Return TRUE if this is a call that might need a
+// branch-range-extension thunk. This is something of a kludge for
+// sake of performance: We can't process thunks until address
+// assigment in output-sections finalize() time, at which time we must
+// iterate over all relocs again. Here, we dig deeply enough into the
+// attributes of a branch reloc that we can determine its eligibility
----------------
seems outdated now


================
Comment at: lld/MachO/Writer.cpp:545
     }
+  } else if (sym != config->entry) {
+    llvm_unreachable("invalid branch target symbol type");
----------------
why is this `config->entry` check necessary?


================
Comment at: lld/MachO/Writer.cpp:564
     prepareBranchTarget(sym);
+    r.isCallSite = true;
+    isec->callSiteCount++;
----------------
can't we just check for `relocAttrs.hasAttr(RelocAttrBits::BRANCH)` instead of adding a new property on Reloc?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100818