[PATCH] D128108: [WIP][lld-macho] Add support for objc_msgSend stubs

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/SyntheticSections.cpp:1634
+      if (trackOffsets)
+        duplicateStringOffsetMap.insert(
+            std::make_pair(isec->getStringRef(i), offsetInfo.outSecOff));
----------------
keith wrote:
> int3 wrote:
> > Why can't we just use `stringMapOffset` itself? Is it because it isn't available in the non-deduplicated `CStringSection`?
> > 
> > If that's the case, I wonder if we should just always have `__objc_methname` be deduplicated...
> > Why can't we just use `stringMapOffset` itself? Is it because it isn't available in the non-deduplicated `CStringSection`?
> 
> Correct
> 
> > If that's the case, I wonder if we should just always have `__objc_methname` be deduplicated...
> 
> Yea we could definitely do that if you think the performance downside wouldn't be a big deal. I didn't feel great adding this here either, but figured the time savings on larger codebases might be significant enough to warrant the complexity.
> 
yeah initially I wanted to keep the default output of LLD 100% unoptimized but I'm having second thoughts... firstly the behavior difference wrt ld64 is not ideal, secondly we haven't parallelized this yet, thirdly the extra data we have to write for `__cstring` probably offsets the perf savings we get from not dedup'ing (just as I've found some builds to be faster with `-dead_strip`), and fourthly, the code complexity...

would be nice to pick an objC-intensive app and run a benchmark here. but I don't think a minor regression here should block this, we can work on parallelizing this later instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128108