[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

I think it looks good, but I'd wait for @hans too.

BTW, after this we should remove -limited-coverage-experimental:
https://github.com/llvm/llvm-project/blob/bbc328c/clang/lib/CodeGen/CodeGenModule.cpp#L67



================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:572-574
+    if (CovBuf + sizeof(CovMapHeader) > CovBufEnd)
       return make_error<CoverageMapError>(coveragemap_error::malformed);
+    auto CovHeader = reinterpret_cast<const CovMapHeader *>(CovBuf);
----------------
While working on CodeView, I think we came up with a good code pattern that could be used here to make this easier to read and less error prone. Check out llvm::codeview::consume:
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/DebugInfo/CodeView/RecordSerialization.h#L64

Ignore the use of BinaryStreamReader, that exists to abstract over discontiguous inputs, and isn't relevant to this code here.

You could make a helper that looks like this and use it in various places to avoid all the reinterpret_casts:
```
template <typename T>
bool consumeObject(StringRef &Buf, const T *&Obj) {
  if (Buf.size() < sizeof(T))
    return false;
  Obj = reinterpret_cast<const T*>(Buf.data());
  Buf = Buf.drop_front(sizeof(T));
  return true;
}
```

And then instead of dealing in pointers and pointer arithmetic, you slice up StringRef buffers into sub buffers. It becomes much harder to create array OOB bugs.

This is just a suggestion for future work, it seems out of scope for this patch.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:53-56
+    Error E =
+        zlib::compress(FilenamesStr, CompressedStr, zlib::BestSizeCompression);
+    if (E)
+      llvm_unreachable("Unexpected failure in zlib::compress");
----------------
I think the shorter pattern for this is `cantFail(zlib::compress(...))`.


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

https://reviews.llvm.org/D69471