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

Vedant Kumar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 3 16:00:36 PST 2019


vsk added inline comments.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1372
+  // Make sure the data doesn't get deleted.
+  CGM.addUsedGlobal(FuncRecord);
+}
----------------
rnk wrote:
> I guess we really do want llvm.used, we really want these records to be retained in the binary by the linker even though nothing references them.
Yes, this ensures that the records can be read back when generating a report. The record names can be stripped out post-link to reduce bloat in the linkedit section.


================
Comment at: llvm/docs/CoverageMappingFormat.rst:246
 
-The function record layout has evolved since version 1. In version 1, the function record for *foo* is defined as follows:
+* Function records are now marked *linkonce_odr*. This allows linkers to merge duplicate *dummy* records (emitted for functions included-but-not-used in a translation unit). As part of this change, region mapping information for a function is now included within the function record, instead of being affixed to the coverage header.
+
----------------
rnk wrote:
> Do you need to highlight "dummy" records? I thought all function records are linkonce_odr, so that any duplicate records can be deduplicated, not just the dummy ones for unused inline functions.
I think it's helpful to highlight "dummy" records, as these motivate the linkonce_odr idea. I'll point out that all function records are linkonce_odr.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:39
 
-void CoverageFilenamesSectionWriter::write(raw_ostream &OS) {
+Error CoverageFilenamesSectionWriter::write(raw_ostream &OS, bool Compress) {
+  std::string FilenamesStr;
----------------
hans wrote:
> It seems the only way this can fail is if zlib::compress fails, which should not be possible except if it runs out of memory, which we generally don't diagnose.
> 
> I think we could assert that zlib::compress succeeds, simplify the interface of this function, and remove the need for the new err_code_coverage_mapping_failed diag, as well as the compression_failed enumerator.
Thanks, sgtm.


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

https://reviews.llvm.org/D69471





More information about the llvm-commits mailing list