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

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked 3 inline comments as done.
vsk added inline comments.


================
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);
----------------
rnk wrote:
> 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.
Thanks for the suggestion, I'll keep this in mind for future refactors.


================
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");
----------------
rnk wrote:
> I think the shorter pattern for this is `cantFail(zlib::compress(...))`.
Both sound reasonable, I'll pick the shorter spelling.


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

https://reviews.llvm.org/D69471