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

[Jez Ng via Phabricator via llvm-commits]

int3 marked 2 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/InputSection.cpp:244
     if (end == StringRef::npos)
       fatal(getLocation(off) + ": string is not null terminated");
+    uint32_t hash = deduplicateLiterals ? xxHash64(s.take_front(end)) : 0;
----------------
thakis wrote:
> 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?
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


================
Comment at: lld/MachO/MapFile.cpp:91
       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);
----------------
thakis wrote:
> int3 wrote:
> > dereferencing end iterators from the STL is undefined, but fortunately a StringRef isn't part of the STL...
> Looks very fishy though. See above :)
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


================
Comment at: lld/MachO/SyntheticSections.cpp:1546
       StringRef string = isec->getStringRef(i);
       memcpy(buf + isec->pieces[i].outSecOff, string.data(), string.size());
     }
----------------
Roger wrote:
> It looks to me that we used to copy over a null terminator here but we no longer will do that. Am I correct?
indeed. Though not guaranteed by a spec, it appears that our `mmap`ed 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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133728