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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

file naming consistency nits:

x86_64-objcstubs.s -> x86-64-objc-stubs.s
arm64-objcstubs.s -> arm64-objc-stubs.s
methname.s -> objc-methname.s
selrefs.s -> objc-selrefs.s



================
Comment at: lld/MachO/Arch/ARM64.cpp:114-116
+    0xd4200020, // brk   #0
+    0xd4200020, // brk   #0
+    0xd4200020, // brk   #0
----------------
What's the purpose of these `brk #0` instructions? Also, any idea why they are rendered as `brk #0x1` in the `arm64-objcstubs.s` test?


================
Comment at: lld/MachO/Arch/ARM64.cpp:159
+  objcStubsFastSize = sizeof(objcStubsFastCode);
+  objcStubsSmallSize = 0; /* TODO */
+  objcStubsAlignment = 32;
----------------
are you planning to implement this in the near future? otherwise I think it's cleaner to drop it for now


================
Comment at: lld/MachO/Arch/X86_64.cpp:192
+                   selrefsVA + selectorIndex * LP64::wordSize);
+  writeRipRelative(d, buf, stubAddr, 0xD,
+                   gotAddr + msgSendIndex * LP64::wordSize);
----------------
consistency nit 


================
Comment at: lld/MachO/Driver.cpp:792-801
+  switch (arg->getOption().getID()) {
+  case OPT_objc_stubs_fast:
+    return ObjCStubsMode::fast;
+  case OPT_objc_stubs_small:
+    warn("-objc_stubs_small is not yet implemented, defaulting to "
+         "-objc_stubs_fast");
+    return ObjCStubsMode::fast;
----------------
the switch statement seems a bit overkill given that we know `arg` can only take two values... how about just `if (arg->getOption().getID() == OPT_objc_stubs_small) { ... }`?


================
Comment at: lld/MachO/Driver.cpp:1183
+  in.objcMethnameSection->addInput(isec);
+  in.objcMethnameSection->isec->markLive(0);
+}
----------------
is this necessary? isn't marking the pieces as `live` above sufficient?


================
Comment at: lld/MachO/InputSection.h:197
+      : InputSection(CStringLiteralKind, section, data, align) {
+    deduplicateLiterals = dedupLiterals;
+  }
----------------
use member initialization list


================
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();
----------------
keith wrote:
> 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`.
could you dig into this? I would really prefer we avoid copying the string if possible


================
Comment at: lld/MachO/SyntheticSections.h:314
 
+class ObjCStubsSection final : public SyntheticSection {
+public:
----------------
Could we have a block comment here explaining how ObjC stubs are laid out? (Similar to what we have for `StubsSection`.)


================
Comment at: lld/MachO/SyntheticSections.h:324
+
+  static constexpr auto symbolPrefix = "_objc_msgSend$";
+
----------------
could make this a `StringLiteral` (then we wouldn't need a separate `symbolPrefixLength` either)


================
Comment at: lld/MachO/SyntheticSections.h:564
+
+  llvm::DenseMap<llvm::CachedHashStringRef, StringOffset>
+  getStringOffsetMap() const {
----------------
we should be returning a const ref instead of a copy... but I think it would be even cleaner to just expose a `getStringOffset(StringRef)` method instead of the whole map interface. I think having the 31-bit hashing logic encapsulated within `DeduplicatedCStringSection` is also cleaner


================
Comment at: lld/MachO/Writer.cpp:1215
   in.header = make<MachHeaderSection>();
-  if (config->dedupLiterals)
-    in.cStringSection = make<DeduplicatedCStringSection>();
-  else
-    in.cStringSection = make<CStringSection>();
+  if (config->dedupLiterals) {
+    in.cStringSection =
----------------
nit: braces unnecessary


================
Comment at: lld/test/MachO/arm64-objcstubs.s:36
+# CHECK-NEXT: brk     #0x1
+
+.section  __TEXT,__objc_methname,cstring_literals
----------------
Should we have a `CHECK-EMPTY` here just to verify that `_objc_msgSend$bar` isn't generated? Otherwise I'm not sure what the purposes of `selref2` is :)


================
Comment at: lld/test/MachO/methname.s:30-34
+  .asciz "incstr"
+
+.section __TEXT,__objc_methname,cstring_literals
+_some_methname:
+  .asciz  "inmethname"
----------------
can we spell out "in" -> "internal"? I spent a while trying to figure out what "inc str" meant heh

also I don't think the `_some_cstr` and `_some_methname` symbols are needed


================
Comment at: lld/test/MachO/x86_64-objcstubs.s:18
+.section  __TEXT,__objc_methname,cstring_literals
+selref1:
+  .asciz  "foo"
----------------
just curious -- does `clang -S` normally generate regular symbols for strings in `__objc_methname`? AFAIK it usually uses private `LFoo` symbols for `__cstring`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128108