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

[Keith Smiley via Phabricator via llvm-commits]

keith added inline comments.


================
Comment at: lld/MachO/SyntheticSections.cpp:731
+  std::string methname = sym->getName().drop_front(symbolPrefixLength).str();
+  methname.push_back('\0');
+  auto map = in.objcMethnameSection->getDuplicateStringOffsetMap();
----------------
int3 wrote:
> isn't the string already null-terminated? why do we have to add another `\0`?
I didn't dig into this, but this is how I got here:

In `CStringSection`:

```
      StringRef string = isec->getStringRef(i);
      llvm::outs() << "'" << string << "'" << " len: " << string.size() << "\n";
```

This prints: `'foo' size 4`.

In `ObjCStubsSection::addEntry`

```
  llvm::outs() << "'" << sym->getName() << "' size " << sym->getName().size() << "\n";
  std::string methname = sym->getName().drop_front(symbolPrefixLength).str();
```

This prints `'foo' size 3`.


================
Comment at: lld/MachO/SyntheticSections.cpp:780-781
+      ConcatOutputSection::getOrCreateForInput(in.objcSelrefs);
+  // NOTE: This has to come before any selrefs sections from inputs since
+  // the offset from the start of the section is used in the stubs.
+  inputSections.insert(inputSections.begin(), in.objcSelrefs);
----------------
int3 wrote:
> I'm not sure I understand how InputSection ordering matters... also, which offset are you referring to here?
> 
> Plus inserting like this means that we are copying the entire vector, which is pretty large... would prefer if we didn't have to do this
Sorry I didn't go back to check this. I originally added it before I started using `getVA` for the selrefs section, with that change this isn't required. Removed.


================
Comment at: lld/MachO/SyntheticSections.cpp:1634
+      if (trackOffsets)
+        duplicateStringOffsetMap.insert(
+            std::make_pair(isec->getStringRef(i), offsetInfo.outSecOff));
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128108