[lld-macho][nfci] Don't include null terminator in StringRefs

[int3]

Summary

So @keith observed
here that the
StringRefs we were returning from CStringInputSection::getStringRef()
included the null terminator in their total length, but regular
StringRefs do not. Let's fix that so these StringRefs are less confusing
to use.

[int3]

lld/MachO/MapFile.cpp

91

dereferencing end iterators from the STL is undefined, but fortunately a StringRef isn't part of the STL...

[keith]

thanks! I started on this by had some off by one breaking some other test, but I assume your case ironed that out!

[Roger]

Generally looks good :)

lld/MachO/SyntheticSections.cpp

1552

It looks to me that we used to copy over a null terminator here but we no longer will do that. Am I correct?

[thakis]

lld/MachO/InputSection.cpp

244–245

There's a bunch of places that assume that the trailing \0 byte is still there, but that's generally not the case for StringRefs. Maybe we should use ArrayRefs in the places you touched?

lld/MachO/MapFile.cpp

91

Looks very fishy though. See above :)

[int3]

lld/MachO/InputSection.cpp

244–245

actually I think it's only that one assert that assumes this, and that assert is no longer actually necessary

all the other places that "skip the null terminator" are basically just skipping a byte in the output buffer. they don't assume that the StringRef itself has a null terminator

lld/MachO/MapFile.cpp

91

I realize that assert was only done because we were trimming off the null terminator in the (now deleted) substr call. So we can remove the assert

lld/MachO/SyntheticSections.cpp

1552

indeed. Though not guaranteed by a spec, it appears that our mmaped output buffer is zero-initialized -- at least, a good deal of the code already assumes that this is the case (and tests would fail otherwise)

[int3]

remove assert

[int3]

@thakis I'm gonna land this now, will do a follow-up if you have further comments :)

[thakis]

I should probably put an explicit "LGTM" here: LGTM :)

(You landed this at the right time, should've approved with my original comment.)

lld/MachO/InputSection.h

public:
   StringPiece &getStringPiece(uint64_t off);
   const StringPiece &getStringPiece(uint64_t off) const;
   // Split at each null byte.
   void splitIntoPieces();
 
   LLVM_ATTRIBUTE_ALWAYS_INLINE
   StringRef getStringRef(size_t i) const {
     size_t begin = pieces[i].inSecOff;
+    // The endpoint should be *at* the null terminator, not after. This matches
+    // the behavior of StringRef(const char *Str).
     size_t end =
-        (pieces.size() - 1 == i) ? data.size() : pieces[i + 1].inSecOff;
+        ((pieces.size() - 1 == i) ? data.size() : pieces[i + 1].inSecOff) - 1;
     return toStringRef(data.slice(begin, end - begin));
   }
 
   // Returns i'th piece as a CachedHashStringRef. This function is very hot when
   // string merging is enabled, so we want to inline.
   LLVM_ATTRIBUTE_ALWAYS_INLINE
   llvm::CachedHashStringRef getCachedHashStringRef(size_t i) const {
     assert(deduplicateLiterals);

lld/MachO/InputSection.cpp

 }
 
 void CStringInputSection::splitIntoPieces() {
   size_t off = 0;
   StringRef s = toStringRef(data);
   while (!s.empty()) {
     size_t end = s.find(0);
     if (end == StringRef::npos)
       fatal(getLocation(off) + ": string is not null terminated");
-    size_t size = end + 1;
-    uint32_t hash = deduplicateLiterals ? xxHash64(s.substr(0, size)) : 0;
+    uint32_t hash = deduplicateLiterals ? xxHash64(s.take_front(end)) : 0;
     pieces.emplace_back(off, hash);
+    size_t size = end + 1; // include null terminator
     s = s.substr(size);
     off += size;
   }
 }
 
 StringPiece &CStringInputSection::getStringPiece(uint64_t off) {
   if (off >= data.size())
     fatal(toString(this) + ": offset is outside the section");

lld/MachO/MapFile.cpp

parallelFor(0, syms.size(), [&](size_t i) {
     case InputSection::CStringLiteralKind: {
       // Output "literal string: <string literal>"
       const auto *isec = cast<CStringInputSection>(sym->isec);
       const StringPiece &piece = isec->getStringPiece(sym->value);
       assert(
           sym->value == piece.inSecOff &&
           "We expect symbols to always point to the start of a StringPiece.");
       StringRef str = isec->getStringRef(&piece - &(*isec->pieces.begin()));
-      assert(str.back() == '\000');
-      (os << "literal string: ")
-          // Remove null sequence at the end
-          .write_escaped(str.substr(0, str.size() - 1));
+      (os << "literal string: ").write_escaped(str);
       break;
     }
     case InputSection::ConcatKind:
     case InputSection::WordLiteralKind:
       os << toString(*sym);
     }
   });
 

lld/MachO/SyntheticSections.cpp

 ObjCStubsSection::ObjCStubsSection()
     : SyntheticSection(segment_names::text, section_names::objcStubs) {
   flags = S_ATTR_SOME_INSTRUCTIONS | S_ATTR_PURE_INSTRUCTIONS;
   align = target->objcStubsAlignment;
 }
 
 void ObjCStubsSection::addEntry(Symbol *sym) {
   assert(sym->getName().startswith(symbolPrefix) && "not an objc stub");
-  // Ensure our lookup string has the length of the actual string + the null
-  // terminator to mirror
-  StringRef methname =
-      StringRef(sym->getName().data() + symbolPrefix.size(),
-                sym->getName().size() - symbolPrefix.size() + 1);
+  StringRef methname = sym->getName().drop_front(symbolPrefix.size());
   offsets.push_back(
       in.objcMethnameSection->getStringOffset(methname).outSecOff);
   Defined *newSym = replaceSymbol<Defined>(
       sym, sym->getName(), nullptr, isec,
       /*value=*/symbols.size() * target->objcStubsFastSize,
       /*size=*/target->objcStubsFastSize,
       /*isWeakDef=*/false, /*isExternal=*/true, /*isPrivateExtern=*/true,
       /*includeInSymtab=*/true, /*isThumb=*/false,

 }
 
 void CStringSection::writeTo(uint8_t *buf) const {
   for (const CStringInputSection *isec : inputs) {
     for (size_t i = 0, e = isec->pieces.size(); i != e; ++i) {
       if (!isec->pieces[i].live)
         continue;
       StringRef string = isec->getStringRef(i);
       memcpy(buf + isec->pieces[i].outSecOff, string.data(), string.size());
     }
   }
 }
 
 void CStringSection::finalizeContents() {
   uint64_t offset = 0;
   for (CStringInputSection *isec : inputs) {
     for (size_t i = 0, e = isec->pieces.size(); i != e; ++i) {
       if (!isec->pieces[i].live)
         continue;
       // See comment above DeduplicatedCStringSection for how alignment is
       // handled.
       uint32_t pieceAlign =
           1 << countTrailingZeros(isec->align | isec->pieces[i].inSecOff);
       offset = alignTo(offset, pieceAlign);
       isec->pieces[i].outSecOff = offset;
       isec->isFinal = true;
       StringRef string = isec->getStringRef(i);
-      offset += string.size();
+      offset += string.size() + 1; // account for null terminator
     }
   }
   size = offset;
 }
 
 // Mergeable cstring literals are found under the __TEXT,__cstring section. In
 // contrast to ELF, which puts strings that need different alignments into
 // different sections, clang's Mach-O backend puts them all in one section.
for (size_t i = 0, e = isec->pieces.size(); i != e; ++i) {
       if (!isec->pieces[i].live)
         continue;
       auto s = isec->getCachedHashStringRef(i);
       auto it = stringOffsetMap.find(s);
       assert(it != stringOffsetMap.end());
       StringOffset &offsetInfo = it->second;
       if (offsetInfo.outSecOff == UINT64_MAX) {
         offsetInfo.outSecOff = alignTo(size, 1ULL << offsetInfo.trailingZeros);
-        size = offsetInfo.outSecOff + s.size();
+        size =
+            offsetInfo.outSecOff + s.size() + 1; // account for null terminator
       }
       isec->pieces[i].outSecOff = offsetInfo.outSecOff;
     }
     isec->isFinal = true;
   }
 }
 
 void DeduplicatedCStringSection::writeTo(uint8_t *buf) const {