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

[Jez Ng via Phabricator via llvm-commits]

int3 created this revision.
int3 added a reviewer: lld-macho.
Herald added a reviewer: ributzka.
Herald added projects: lld-macho, All.
int3 requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

So @keith observed
here <https://reviews.llvm.org/D128108#inline-1263900> 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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133728

Files:
  lld/MachO/InputSection.cpp
  lld/MachO/InputSection.h
  lld/MachO/MapFile.cpp
  lld/MachO/SyntheticSections.cpp


Index: lld/MachO/SyntheticSections.cpp
===================================================================
--- lld/MachO/SyntheticSections.cpp
+++ lld/MachO/SyntheticSections.cpp
@@ -752,11 +752,7 @@
 
 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>(
@@ -1566,7 +1562,7 @@
       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;
@@ -1639,7 +1635,8 @@
       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;
     }
Index: lld/MachO/MapFile.cpp
===================================================================
--- lld/MachO/MapFile.cpp
+++ lld/MachO/MapFile.cpp
@@ -88,10 +88,8 @@
           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));
+      assert(*str.end() == '\000');
+      (os << "literal string: ").write_escaped(str);
       break;
     }
     case InputSection::ConcatKind:
Index: lld/MachO/InputSection.h
===================================================================
--- lld/MachO/InputSection.h
+++ lld/MachO/InputSection.h
@@ -208,8 +208,10 @@
   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));
   }
 
Index: lld/MachO/InputSection.cpp
===================================================================
--- lld/MachO/InputSection.cpp
+++ lld/MachO/InputSection.cpp
@@ -242,9 +242,9 @@
     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;
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D133728.459563.patch
Type: text/x-patch
Size: 3469 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220912/af155fd5/attachment.bin>