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

Tue May 11 18:49:20 PDT 2021

int3 added inline comments.

================
Comment at: lld/MachO/MergedOutputSection.cpp:131
+  size_t maxPotentialThunks = 0;
+  for (ThunkMapPair &tp : thunkMap) {
+    ThunkInfo &ti = tp.second;
----------------
gkm wrote:
> int3 wrote:
> > hm so we are calling `estimateStubsInRangeVA` itself in a loop... is this potentially expensive? I guess `thunkMap` and `endIdx - callIdx` aren't typically large, so the nested loops are probably fine... but might be worth calling out explicitly in a comment
> Although we call `estimateStubsInRangeVA()` within a loop, it is predicated by condition that is only true once. The call needs to happen within the loop in order to happen at the proper time: as soon as all input sections are finalized, i.e., when the end of __text is within forward-branch range of the current call site. I will add a comment to highlight that feature.
oh I see. Thanks for adding the comment!

================
Comment at: lld/MachO/MergedOutputSection.cpp:263
+        // unfinalized inputs[finalIdx].
+        fatal(Twine(__FUNCTION__) + ": FIXME: thunk range overrun");
+      }
----------------
gkm wrote:
> int3 wrote:
> > I think this should work
> Sadly, it doesn't. Unlike old C compilers, `__FUNCTION__` is not expanded by the preprocessor into a string literal. It is a compiler-internal variable, and thus not amenable to concat via adjacent string literals. 
huh, TIL. I *did* vaguely wonder when C preprocessors became smart enough to parse function declarations :p makes more sense that the compiler is doing it

================
Comment at: lld/MachO/Target.h:93-95
+  // We contrive this value as sufficiently far from any valid address
+  // that it will always be out-of-range for any architecture.
+  static constexpr uint64_t outOfRangeVA = 0xfull << 60;
----------------
gkm wrote:
> int3 wrote:
> > This seems unnecessarily cute... Can we just use `numeric_limits<uint64_t>::max()`? Also it's not really target-specific so I'd prefer it not be put in Target
> FYI, this wasn't intended to be cute at all. `numeric_limits<uint64_t>::max()` is not viable since it is the tombstone value for `DenseMap<>` and induces weird assertions. Another disqualifier is that `max()` is only one increment away from wrapping to 0. My chosen value `0xf000'0000'0000'0000` is VERY FAR away from 0. Perhaps MachO guarantees that `__text` addresses will never be within even 4 GiB of 0, so I am unnecessarily cautious?
> 
> Regarding target-specificity: it is OS/runtime specific (mostly Darwin & iOS), modulo CPU-arch variations. It so happens all are Apple creations, and common enough that we can choose a single constant that works for all. If not `Target.h`, where do you propose we define this? I don't see anywhere that seems a better fit ... `Config.h`? `OutputSection.h`? `Relocations.h`? What looks good to you?
> since it is the tombstone value for DenseMap<> and induces weird assertions

ohh okay. That makes sense... please add that to the comment :)

I thought you were trying to make a constant with the string "full" in it... :p

I just was thinking of having it as a global. It can still be in Target.h. It's runtime-specific, but as you said it works uniformly for all the targets we support, and that's not quite obvious from seeing `target->outOfRangeVA`. Though if you don't want to pollute the global namespace further I guess changing all the use sites to `Target::outOfRangeVA` would work too.

================
Comment at: lld/test/MachO/tools/generate-thunkable-program.py:15-17
+# This list comes from libSystem.tbd and contains a sizeable subset
+# of dylib calls available for all MacOS target archs.
+libSystem_calls = (
----------------
int3 wrote:
> Couldn't we just generate a bunch of random strings for the symbols? This list isn't really helping us exercise new code paths in LLD...
not yet addressed

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100818