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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I'm not listed as a reviewer, but it looks good to me.



================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1331
+  // Assign a name to the function record. This is used to merge duplicates.
+  std::string FuncRecordName = llvm::utohexstr(Info.NameHash);
+
----------------
I think this needs to be namespaced somehow, something like `__covrec_0afeec...u`. The meaning is more obvious, and there is a (admittedly small) risk of colliding with other random 64-bit hex symbols without a double-underscore prefix.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1372
+  // Make sure the data doesn't get deleted.
+  CGM.addUsedGlobal(FuncRecord);
+}
----------------
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.


================
Comment at: clang/test/Profile/def-assignop.cpp:21
 
-  // Check that coverage mapping includes 6 function records including the
   // defaulted copy and move operators: A::operator=
----------------
I guess 6 was just stale/wrong.


================
Comment at: llvm/docs/CoverageMappingFormat.rst:228
 
-* Coverage mapping data which is an array of bytes. Zero paddings are added at the end to force 8 byte alignment.
+The variable has 8-byte alignment because ld64 cannot always pack symbols from different object files tightly (the word-level alignment assumption is baked in too deeply).
 
----------------
This section of the document isn't word wrapped to 80 cols, but the rest is.


================
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.
+
----------------
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.


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

https://reviews.llvm.org/D69471