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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.
Herald added a reviewer: ributzka.


================
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:
> keith wrote:
> > int3 wrote:
> > > 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
> > I pushed a new option here which I believe avoids the copies. The core issue is that here https://github.com/llvm/llvm-project/blob/1b349bdaaa540e97d90318c3706527f6ca084987/lld/MachO/InputSection.h#L206-L212 we manually construct a StringRef with the actual data, and with the length of the actual data + 1, since `pieces[i + 1].inSecOff` is after the null terminator. But any other StringRef initialization, like this one https://github.com/llvm/llvm-project/blob/1b349bdaaa540e97d90318c3706527f6ca084987/lld/MachO/InputFiles.cpp#L943 goes through an initializer that computes the length, which naturally excludes the null terminator https://github.com/llvm/llvm-project/blob/1b349bdaaa540e97d90318c3706527f6ca084987/llvm/include/llvm/ADT/StringRef.h#L83-L85
> > 
> > I'm now mirroring this StringRef initialization, but maybe we should correct that string length off by 1 instead?
> ooh. Yeah we should fix that off-by-one. I'm fine if you want to do it in a separate commit though
fix is here: {D133728}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128108