[llvm-cov] Support multi-source object files for `convert-for-testing`

[AtomicGu]

Summary

llvm-cov convert-for-testing only functions properly when the input binary contains a single source file. When the binary has multiple source files, a Malformed coverage data error will occur when the generated .covmapping is read back. This is because the testing format lacks support for indicating the size of its file records, and current implementation just assumes there's only one record in it. This patch fixes this problem by introducing a new testing format version.

Changes to the code:

• Add a new format version. The version number is stored in the the last 8 bytes of the orignial magic number field to be backward-compatible.
• Output a LEB128 number before the file records section to indicate its size in the new version.
• Change the format parsing code correspondingly.
• Update the document to formalize the testing format.
• Additionally, fix the bug when converting COFF binaries.

[phosek]

This is a breaking change since it modifies the .covmapping format in a backwards incompatible way. For .profdata format we always try to avoid breaking changes and provide backwards compatibility, but I don't know if the same holds for the .covmapping format which is used only for testing. @davidxl do you have any thoughts on that?

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

908–910

It looks like in the current implementation, we're using getFilenameSize to compute the CoverageMappingSize, but CovMapHeader also contains CoverageMappingSize, accessible through getCoverageSize(). Shouldn't we be using that instead? Isn't that the issue with the current implementation?

921–922

Can you preserve this check in the new implementation?

[AtomicGu]

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

908–910

I have inspected this variable. The value is not right. It is always zero, as the documentation says:

The length of the string in the third field of __llvm_coverage_mapping that contains any encoded coverage mapping data affixed to the coverage header. Always 0, but present for backwards compatibility.

[AtomicGu]

• [llvm-cov] Add two checks to readCoverageMappingData.

[AtomicGu]

• [llvm-cov] Add truncated checking to loadTestingFormat.

[phosek]

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

908–910

If it's not being used, could we reuse it rather than introducing a new field?

[AtomicGu]

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

908–910

There is an old test llvm-project/llvm/test/tools/llvm-cov/Inputs/combine_expansions.covmapping which is Version1. In this case, this variable is used and the value is still not right. If we reuse this field, at least we will lose the compatibility for Version1. However I think we can add a version check and only support the multi-source covmapping after Version4. So... I think this may work!

[AtomicGu]

This comment was removed by AtomicGu.

[AtomicGu]

• [llvm-cov] use getCoverageSize() to store the CoverageMappingSize.
• [llvm-cov] Update existing covmapping files.

[AtomicGu]

llvm/docs/CoverageMappingFormat.rst

327

It seems that the convert-for-testing command of llvm-cov does not show up in any public documentation. So I decide not to mention it here.

[AtomicGu]

This comment was removed by AtomicGu.

[AtomicGu]

@phosek I finished the revision. The x64 debian pre-merge checks passed. However the x64 windows pre-merge checks failed because of timeout. But I think the code is OK because It happened a few times before. Do I need to retry the pre-merge checks?

[gulfem]

llvm/tools/llvm-cov/TestingSupport.cpp

142

I don't think this is the right way of fixing the issue. There are two sizes that I want to differentiate:

1. CoverageMappingSize which is per module in CovMapHeader
2. TotalCoverageMappingSize, which is the sum of CoverageMappingSize in all modules

CoverageMappingData.size() at line 135 gives you TotalCoverageMappingSize, and this code overwrites the first module's CoverageMappingSize with TotalCoverageMappingSize. This is the first incorrect thing here. Second incorrect thing is that even if we reuse the CoverageMappingSize field in CovMapHeader, this file is not the right place to write it. This code overwrites a value instead of writing the correct value in the first place. This is where CoverageMappingSize is written in the first place in CodeGen.
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CoverageMappingGen.cpp#L1785

We need to encode TotalCoverageMappingSize to fix this issue, and that's why I suggest using the earlier version that you implemented which encodes CoverageMappingData.size(). I don't think using CoverageMappingSize per module would be helpful here because in order to calculate TotalCoverageMappingSize, you need to read CoverageMappingSize by reading every CovMapHeader here, which we probably do not want to do.
https://github.com/llvm/llvm-project/blob/main/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp#L910

[gulfem]

In D156611#4555830, @phosek wrote:

This is a breaking change since it modifies the .covmapping format in a backwards incompatible way. For .profdata format we always try to avoid breaking changes and provide backwards compatibility, but I don't know if the same holds for the .covmapping format which is used only for testing. @davidxl do you have any thoughts on that?

@davidxl @AtomicGu is doing a GSoC project this summer, and ran into this issue in the test that he wrote in https://reviews.llvm.org/D151283. If you do not have any concern about compatibility of .covmapping format, can we land this change?

[AtomicGu]

llvm/tools/llvm-cov/TestingSupport.cpp

142

Yes, that's what I thought at first. But @phosek wants the backward compatibility, and this is the only way. So do I need to revert to the previous version? Both version has passed the pre-merge checks.

[AtomicGu]

Revert to the previous version.

[gulfem]

Please remove the following in the commit message:

Reuse the CoverageSize field of the first file record to store the section size after Version4.

[AtomicGu]

@gulfem It's done. I'm waiting the pre-merge checks to be passed.

[AtomicGu]

@gulfem The pre-merge checks have been passed. How to commit this review? I think I don't have the commit access.

[gulfem]

In D156611#4581842, @AtomicGu wrote:

@gulfem The pre-merge checks have been passed. How to commit this review? I think I don't have the commit access.

I'll land it for you. You can request a commit access via https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access when you have a good track record.

[gulfem]

In D156611#4555830, @phosek wrote:

This is a breaking change since it modifies the .covmapping format in a backwards incompatible way. For .profdata format we always try to avoid breaking changes and provide backwards compatibility, but I don't know if the same holds for the .covmapping format which is used only for testing. @davidxl do you have any thoughts on that?

I was going to suggest bumping the coverage mapping format version if there is a concern about backwards compatibility. But, I don't think this is really changing the coverage mapping format itself. To me, it fixes the issue where we convert objects files to covmapping files, and the way we read coverage via using object files should not be impacted by that.

[phosek]

I still have some concerns, but I think the risk is fairly low, and since no other reviewer responded, I think we can go ahead and land the change. We can always revert the change if it turns out that it broke some downstream users.

[dyung]

Hi @AtomicGu, we have some internal tests that use covmapping files that started failing after this change. I notice as part of your commit message you stated that you "update(d) existing .covmapping files". How exactly did you do that? I'm guessing that we just need to update our private test files to work with your new format, but not being familiar with this area at all, I'm not sure how to convert our "old" .covmapping file to the "new" format that is expected. Any tips?

[AtomicGu]

In D156611#4587396, @dyung wrote:

Hi @AtomicGu, we have some internal tests that use covmapping files that started failing after this change. I notice as part of your commit message you stated that you "update(d) existing .covmapping files". How exactly did you do that? I'm guessing that we just need to update our private test files to work with your new format, but not being familiar with this area at all, I'm not sure how to convert our "old" .covmapping file to the "new" format that is expected. Any tips?

I'm so sorry to hear about that. I updated those old .covmapping file by a special temporary modified version of llvm-cov. I will upload it at once for your ergency.

I thought this file format is private and shouldn't be used in any project outside LLVM, as it's not mentioned in any public document. Well, it turns out I was wrong. @gulfem @phosek Do you think we need to add a new option to covert-for-testing for potential others to update their .covmapping files?

Maybe we should have changed the magic number of it. So that it can preserve the backward compatibility.

[AtomicGu]

@dyung Here is a patch for converting. Use this patch by git apply. As I said, it's very rudimentary and for temporary use.

To use it, set an environment variable LLVM_COV_NEW_TESTING_FORMAT_CVT with an output path of the new .covmapping file. Then run llvm-cov report on previous .covmapping files. You need to specify the option --instr-profile as well to make llvm-cov start successfully, although it won't affect the output.

For example:

LLVM_COV_NEW_TESTING_FORMAT_CVT=path/to/main.covmapping2 \
llvm-cov report --instr-profile any/profdata/will/do main.covmapping

diff.patch4 KBDownload

However, use it cautiously. I don't know whether we will revert this path or not. How do you think? As an actual downstream user, do you think we should keep the backward compatibility, or provide a formal tool (like a new option to convert-for-testing) to convert the old format files?

Please reply to me as soon as possible. I will fix this at once when the decision is made.

[AtomicGu]

This comment was removed by AtomicGu.

[AtomicGu]

This comment was removed by AtomicGu.

[AtomicGu]

Use another backward-compatible way to fix the bug.

[AtomicGu]

We can keep backward compatibility this time. Let’s revert the previous commit. I hope it’s not too late.
@phosek @gulfem @dyung I don't know how to revert this patch. I don't think I have the privilege to do this.

[AtomicGu]

• Support COFF binaries for convert-for-testing.

[AtomicGu]

• Now it can pass the tests.

[dyung]

In D156611#4587837, @AtomicGu wrote:

@dyung Here is a patch for converting. Use this patch by git apply. As I said, it's very rudimentary and for temporary use.

To use it, set an environment variable LLVM_COV_NEW_TESTING_FORMAT_CVT with an output path of the new .covmapping file. Then run llvm-cov report on previous .covmapping files. You need to specify the option --instr-profile as well to make llvm-cov start successfully, although it won't affect the output.

For example:

LLVM_COV_NEW_TESTING_FORMAT_CVT=path/to/main.covmapping2 \
llvm-cov report --instr-profile any/profdata/will/do main.covmapping

diff.patch4 KBDownload

However, use it cautiously. I don't know whether we will revert this path or not. How do you think? As an actual downstream user, do you think we should keep the backward compatibility, or provide a formal tool (like a new option to convert-for-testing) to convert the old format files?

Please reply to me as soon as possible. I will fix this at once when the decision is made.

Thanks for this @AtomicGu, I'll give that a try. We have private changes downstream that when it was implemented a test was added which used .covmapping files and that is what I need to update. It's not really something we ship to customers, so really all I am doing is what you did here, updating the tests.

[AtomicGu]

@gulfem All the tests have been passed. But this time we change no existing .covmapping files. I think we can land this patch with confidence now. I haven't been granted the commit access yet. So I need your help to do this.

[AtomicGu]

• Pass the new tests in D151283.

[phosek]

If we're going to change the test data format to encode a version, which I think is the right direction, I'd like to do it in a more systematic way. Specifically, I think we should change the magic to be just "llvmcovm" (which is 8 characters so it could be represented as uint64_t) followed by a uint64_t used for the version. We could then treat "testdata" (8387236824784925793 as uint64_t) as one of the recognized versions (ideally it would be interchangeable with 1).

[AtomicGu]

• Formalize the testing format of llvm-cov.

[AtomicGu]

@phosek Hi. I took your advice and reimplemented the new format. I added a new class for writing testing format and introduced the format in the document to make it more formal for the later maintainers. I think this is the best we can do now.

[gulfem]

I added a few nit-picks. Other than that, looks good to me.

llvm/docs/CoverageMappingFormat.rst

620–621

just say for testing purposes and remove the part about including binary in the repository.

657

which is explained later.

686

Just say Version 2 is the current version, and please remove the rest (no need to explain why Version 1 is kept, etc.).

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

986

Some code is duplicated between the case statements, and each case statement is long to read. I think for checking versions if might be a better fit than switch because there might be several versions later. So, I would suggest this: when the TestingFormatVersion is bigger than Version1, use decodeULEB128 to read the size. The rest of the code can mostly be shared up to this point:

uint32_t FilenamesSize =
CovHeader->getFilenamesSize<support::endianness::little>();
CoverageMappingSize = sizeof(CovMapHeader) + FilenamesSize;

You can put the code above in a if statement that checks TestingFormatVersion, and I think the rest can be shared.

[gulfem]

llvm/docs/CoverageMappingFormat.rst

630

I think it will useful to document convert-for-testing in llvm-cov command guide documentation, and remove "You can't see it in any other public documents or in the --help output of llvm-cov". The details about covmapping format can stay here.

[AtomicGu]

• Took advice from gulfem.

[AtomicGu]

@gulfem @phosek Hi. The patch passed all tests. It's ready for landing now.

[gulfem]

llvm/docs/CommandGuide/llvm-cov.rst

569

I think there is no need for this example, so you can remove this code-block.

llvm/docs/CoverageMappingFormat.rst

621

for testing purposes.

llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp

273

You can turn this switch into an if, as the first case statement is not doing anything.

[AtomicGu]

• Took advice from gulfem.

llvm/docs/CommandGuide/llvm-cov.rst

 
  Provides a directory to search for objects corresponding to binary IDs in the
  profile.
 
 .. option:: -check-binary-ids
 
  Fail if an object file cannot be found for a binary ID present in the profile,
  neither on the command line nor via binary ID lookup.
+
+CONVERT-FOR-TESTING COMMAND
+---------------------------
+
+.. warning::
+  This command is for the LLVM developers who are working on ``llvm-cov`` only.
+
+SYNOPSIS
+^^^^^^^^
+
+:program:`llvm-cov convert-for-testing` *BIN* -o *OUT*
+
+DESCRIPTION
+^^^^^^^^^^^
+
+The :program:`llvm-cov convert-for-testing` command serves the purpose of
+testing `llvm-cov` itself. It can extract all code coverage data from the
+binary *BIN* to the file *OUT*, thereby reducing the size of test files. The
+output file typically bears the :program:`.covmapping` extension.
+
+The :program:`.covmapping` files can be read back by ``llvm-cov`` just as
+ordinary binary files.

llvm/docs/CoverageMappingFormat.rst

.. code-block:: llvm
      { i64, i32, i64 } {
        i64 0x5cf8c24cdb18bdac, ; Function's name MD5
        i32 9, ; Function's encoded coverage mapping data string length
        i64 0  ; Function's structural hash
 
 Coverage Mapping Header:
 ------------------------
 
-The coverage mapping header has the following fields:
+As shown above, the coverage mapping header has the following fields:
 
 * The number of function records affixed to the coverage header. Always 0, but present for backwards compatibility.
 
 * The length of the string in the third field of *__llvm_coverage_mapping* that contains the encoded translation unit filenames.
 
 * The length of the string in the third field of *__llvm_coverage_mapping* that contains any encoded coverage mapping data affixed to the coverage header. Always 0, but present for backwards compatibility.
 
-* The format version. The current version is 4 (encoded as a 3).
+* The format version. The current version is 6 (encoded as a 5).
 
 .. _function records:
 
 Function record:
 ----------------
 
 A function record is a structure of the following type:
 

 * *columnStart*: The starting column of the mapping region.
 
 * *numLines*: The difference between the ending line and the starting line
   of the current mapping region.
 
 * *columnEnd*: The ending column of the mapping region. If the high bit is set,
   the current mapping region is a gap area. A count for a gap area is only used
   as the line execution count if there are no other regions on a line.
+
+Testing Format
+==============
+
+.. warning::
+  This section is for the LLVM developers who are working on ``llvm-cov`` only.
+
+``llvm-cov`` uses a special file format (called ``.covmapping`` below) for
+testing purposes. This format is private and should have no use for general
+users. As a developer, you can get such files by the ``convert-for-testing``
+subcommand of ``llvm-cov``.
+
+The structure of the ``.covmapping`` files follows:
+
+``[magicNumber : u64, version : u64, profileNames, coverageMapping, coverageRecords]``
+
+Magic Number and Version
+------------------------
+
+The magic is ``0x6d766f636d766c6c``, which is the ASCII string
+``llvmcovm`` in little-endian.
+
+There are two versions for now:
+
+- Version1, encoded as ``0x6174616474736574`` (ASCII string ``testdata``).
+- Version2, encoded as 1.
+
+The only difference between Version1 and Version2 is in the encoding of the
+``coverageMapping`` fields, which is explained later.
+
+Profile Names
+------------
+
+``profileNames``, ``coverageMapping`` and ``coverageRecords`` are 3 sections
+extracted from the original binary file.
+
+``profileNames`` encodes the size, address and the raw data of the section:
+
+``[profileNamesSize : LEB128, profileNamesAddr : LEB128, profileNamesData : bytes]``
+
+Coverage Mapping
+---------------
+
+This field is padded with zero bytes to make it 8-byte aligned.
+
+``coverageMapping`` contains the records of the source files. In version 1,
+only one record is stored:
+
+``[padding : bytes, coverageMappingData : bytes]``
+
+Version 2 relaxes this restriction by encoding the size of
+``coverageMappingData`` as a LEB128 number before the data:
+
+``[coverageMappingSize : LEB128, padding : bytes, coverageMappingData : bytes]``
+
+The current version is 2.
+
+Coverage Records
+---------------
+
+This field is padded with zero bytes to make it 8-byte aligned.
+
+``coverageRecords`` is encoded as:
+
+``[padding : bytes, coverageRecordsData : bytes]``
+
+The rest data in the file is considered as the ``coverageRecordsData``.

llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h

enum CovMapVersion {
   Version5 = 4,
   // Compilation directory is stored separately and combined with relative
   // filenames to produce an absolute file path.
   Version6 = 5,
   // The current version is Version6.
   CurrentVersion = INSTR_PROF_COVMAP_VERSION
 };
 
+// Correspond to "llvmcovm", in little-endian.
+constexpr uint64_t TestingFormatMagic = 0x6d766f636d766c6c;
+
+enum class TestingFormatVersion : uint64_t {
+  // The first version's number corresponds to the string "testdata" in
+  // little-endian. This is for a historical reason.
+  Version1 = 0x6174616474736574,
+  // Version1 has a defect that it can't store multiple file records. Version2
+  // fix this problem by adding a new field before the file records section.
+  Version2 = 1,
+  // The current testing format version is Version2.
+  CurrentVersion = Version2
+};
+
 template <int CovMapVersion, class IntPtrT> struct CovMapTraits {
   using CovMapFuncRecordType = CovMapFunctionRecordV3;
   using NameRefType = uint64_t;
 };
 
 template <class IntPtrT> struct CovMapTraits<CovMapVersion::Version3, IntPtrT> {
   using CovMapFuncRecordType = CovMapFunctionRecordV2;
   using NameRefType = uint64_t;

llvm/include/llvm/ProfileData/Coverage/CoverageMappingWriter.h

CoverageMappingWriter(ArrayRef<unsigned> VirtualFileMapping,
                         MutableArrayRef<CounterMappingRegion> MappingRegions)
       : VirtualFileMapping(VirtualFileMapping), Expressions(Expressions),
         MappingRegions(MappingRegions) {}
 
   /// Write encoded coverage mapping data to the given output stream.
   void write(raw_ostream &OS);
 };
 
+/// Writer for the coverage mapping testing format.
+class TestingFormatWriter {
+  uint64_t ProfileNamesAddr;
+  StringRef ProfileNamesData;
+  StringRef CoverageMappingData;
+  StringRef CoverageRecordsData;
+
+public:
+  TestingFormatWriter(uint64_t ProfileNamesAddr, StringRef ProfileNamesData,
+                      StringRef CoverageMappingData,
+                      StringRef CoverageRecordsData)
+      : ProfileNamesAddr(ProfileNamesAddr), ProfileNamesData(ProfileNamesData),
+        CoverageMappingData(CoverageMappingData),
+        CoverageRecordsData(CoverageRecordsData) {}
+
+  /// Encode to the given output stream.
+  void
+  write(raw_ostream &OS,
+        TestingFormatVersion Version = TestingFormatVersion::CurrentVersion);
+};
+
 } // end namespace coverage
 
 } // end namespace llvm
 
 #endif // LLVM_PROFILEDATA_COVERAGE_COVERAGEMAPPINGWRITER_H

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

static Error readCoverageMappingData(
   // In Version4, function records are not affixed to coverage headers. Read
   // the records from their dedicated section.
   if (Version >= CovMapVersion::Version4)
     return Reader->readFunctionRecords(FuncRecBuf, FuncRecBufEnd, std::nullopt,
                                        nullptr, nullptr);
   return Error::success();
 }
 
-static const char *TestingFormatMagic = "llvmcovmtestdata";
-
 Expected<std::unique_ptr<BinaryCoverageReader>>
 BinaryCoverageReader::createCoverageReaderFromBuffer(
     StringRef Coverage, FuncRecordsStorage &&FuncRecords,
     InstrProfSymtab &&ProfileNames, uint8_t BytesInAddress,
     support::endianness Endian, StringRef CompilationDir) {
   std::unique_ptr<BinaryCoverageReader> Reader(
       new BinaryCoverageReader(std::move(FuncRecords)));
   Reader->ProfileNames = std::move(ProfileNames);
BinaryCoverageReader::createCoverageReaderFromBuffer(
   return std::move(Reader);
 }
 
 static Expected<std::unique_ptr<BinaryCoverageReader>>
 loadTestingFormat(StringRef Data, StringRef CompilationDir) {
   uint8_t BytesInAddress = 8;
   support::endianness Endian = support::endianness::little;
 
-  Data = Data.substr(StringRef(TestingFormatMagic).size());
+  // Read the magic and version.
+  Data = Data.substr(sizeof(TestingFormatMagic));
+  if (Data.size() < sizeof(uint64_t))
+    return make_error<CoverageMapError>(coveragemap_error::malformed);
+  auto TestingVersion =
+      support::endian::byte_swap<uint64_t, support::endianness::little>(
+          *reinterpret_cast<const uint64_t *>(Data.data()));
+  Data = Data.substr(sizeof(uint64_t));
+
+  // Read the ProfileNames data.
   if (Data.empty())
     return make_error<CoverageMapError>(coveragemap_error::truncated);
   unsigned N = 0;
   uint64_t ProfileNamesSize = decodeULEB128(Data.bytes_begin(), &N);
   if (N > Data.size())
     return make_error<CoverageMapError>(coveragemap_error::malformed);
   Data = Data.substr(N);
   if (Data.empty())
     return make_error<CoverageMapError>(coveragemap_error::truncated);
   N = 0;
   uint64_t Address = decodeULEB128(Data.bytes_begin(), &N);
   if (N > Data.size())
     return make_error<CoverageMapError>(coveragemap_error::malformed);
   Data = Data.substr(N);
   if (Data.size() < ProfileNamesSize)
     return make_error<CoverageMapError>(coveragemap_error::malformed);
   InstrProfSymtab ProfileNames;
   if (Error E = ProfileNames.create(Data.substr(0, ProfileNamesSize), Address))
     return std::move(E);
   Data = Data.substr(ProfileNamesSize);
+
+  // In Version2, the size of CoverageMapping is stored directly.
+  uint64_t CoverageMappingSize;
+  if (TestingVersion == uint64_t(TestingFormatVersion::Version2)) {
+    N = 0;
+    CoverageMappingSize = decodeULEB128(Data.bytes_begin(), &N);
+    if (N > Data.size())
+      return make_error<CoverageMapError>(coveragemap_error::malformed);
+    Data = Data.substr(N);
+    if (CoverageMappingSize < sizeof(CovMapHeader))
+      return make_error<CoverageMapError>(coveragemap_error::malformed);
+  } else if (TestingVersion != uint64_t(TestingFormatVersion::Version1)) {
+    return make_error<CoverageMapError>(coveragemap_error::unsupported_version);
+  }
+
   // Skip the padding bytes because coverage map data has an alignment of 8.
-  size_t Pad = offsetToAlignedAddr(Data.data(), Align(8));
+  auto Pad = offsetToAlignedAddr(Data.data(), Align(8));
   if (Data.size() < Pad)
     return make_error<CoverageMapError>(coveragemap_error::malformed);
   Data = Data.substr(Pad);
   if (Data.size() < sizeof(CovMapHeader))
     return make_error<CoverageMapError>(coveragemap_error::malformed);
   auto const *CovHeader = reinterpret_cast<const CovMapHeader *>(
       Data.substr(0, sizeof(CovMapHeader)).data());
-  CovMapVersion Version =
-      (CovMapVersion)CovHeader->getVersion<support::endianness::little>();
-  StringRef CoverageMapping;
-  BinaryCoverageReader::FuncRecordsStorage CoverageRecords;
+  auto Version =
+      CovMapVersion(CovHeader->getVersion<support::endianness::little>());
+
+  // In Version1, the size of CoverageMapping is calculated.
+  if (TestingVersion == uint64_t(TestingFormatVersion::Version1)) {
     if (Version < CovMapVersion::Version4) {
-    CoverageMapping = Data;
-    if (CoverageMapping.empty())
-      return make_error<CoverageMapError>(coveragemap_error::truncated);
-    CoverageRecords = MemoryBuffer::getMemBuffer("");
+      CoverageMappingSize = Data.size();
     } else {
-    uint32_t FilenamesSize =
+      auto FilenamesSize =
           CovHeader->getFilenamesSize<support::endianness::little>();
-    uint32_t CoverageMappingSize = sizeof(CovMapHeader) + FilenamesSize;
-    CoverageMapping = Data.substr(0, CoverageMappingSize);
-    if (CoverageMapping.empty())
-      return make_error<CoverageMapError>(coveragemap_error::truncated);
-    Data = Data.substr(CoverageMappingSize);
+      CoverageMappingSize = sizeof(CovMapHeader) + FilenamesSize;
+    }
+  }
+
+  auto CoverageMapping = Data.substr(0, CoverageMappingSize);
+
+  // Read the CoverageRecords data.
+  if (Version < CovMapVersion::Version4) {
+    if (!Data.empty())
+      return make_error<CoverageMapError>(coveragemap_error::malformed);
+  } else {
     // Skip the padding bytes because coverage records data has an alignment
     // of 8.
     Pad = offsetToAlignedAddr(Data.data(), Align(8));
     if (Data.size() < Pad)
       return make_error<CoverageMapError>(coveragemap_error::malformed);
-    CoverageRecords = MemoryBuffer::getMemBuffer(Data.substr(Pad));
-    if (CoverageRecords->getBufferSize() == 0)
-      return make_error<CoverageMapError>(coveragemap_error::truncated);
+    Data = Data.substr(Pad);
   }
+  BinaryCoverageReader::FuncRecordsStorage CoverageRecords =
+      MemoryBuffer::getMemBuffer(Data);
+
   return BinaryCoverageReader::createCoverageReaderFromBuffer(
       CoverageMapping, std::move(CoverageRecords), std::move(ProfileNames),
       BytesInAddress, Endian, CompilationDir);
 }
 
 /// Find all sections that match \p Name. There may be more than one if comdats
 /// are in use, e.g. for the __llvm_covfun section on ELF.
 static Expected<std::vector<SectionRef>> lookupSections(ObjectFile &OF,
static Expected<std::vector<SectionRef>> lookupSections(ObjectFile &OF,
   if (Sections.empty())
     return make_error<CoverageMapError>(coveragemap_error::no_data_found);
   return Sections;
 }
 
 static Expected<std::unique_ptr<BinaryCoverageReader>>
 loadBinaryFormat(std::unique_ptr<Binary> Bin, StringRef Arch,
                  StringRef CompilationDir = "",
                  object::BuildIDRef *BinaryID = nullptr) {
   std::unique_ptr<ObjectFile> OF;
   if (auto *Universal = dyn_cast<MachOUniversalBinary>(Bin.get())) {
     // If we have a universal binary, try to look up the object for the
     // appropriate architecture.
     auto ObjectFileOrErr = Universal->getMachOObjectForArch(Arch);
     if (!ObjectFileOrErr)
       return ObjectFileOrErr.takeError();
     OF = std::move(ObjectFileOrErr.get());

 
 Expected<std::vector<std::unique_ptr<BinaryCoverageReader>>>
 BinaryCoverageReader::create(
     MemoryBufferRef ObjectBuffer, StringRef Arch,
     SmallVectorImpl<std::unique_ptr<MemoryBuffer>> &ObjectFileBuffers,
     StringRef CompilationDir, SmallVectorImpl<object::BuildIDRef> *BinaryIDs) {
   std::vector<std::unique_ptr<BinaryCoverageReader>> Readers;
 
-  if (ObjectBuffer.getBuffer().startswith(TestingFormatMagic)) {
+  if (ObjectBuffer.getBuffer().size() > sizeof(TestingFormatMagic)) {
+    uint64_t Magic =
+        support::endian::byte_swap<uint64_t, support::endianness::little>(
+            *reinterpret_cast<const uint64_t *>(ObjectBuffer.getBufferStart()));
+    if (Magic == TestingFormatMagic) {
       // This is a special format used for testing.
       auto ReaderOrErr =
           loadTestingFormat(ObjectBuffer.getBuffer(), CompilationDir);
       if (!ReaderOrErr)
         return ReaderOrErr.takeError();
       Readers.push_back(std::move(ReaderOrErr.get()));
       return std::move(Readers);
     }
+  }
 
   auto BinOrErr = createBinary(ObjectBuffer);
   if (!BinOrErr)
     return BinOrErr.takeError();
   std::unique_ptr<Binary> Bin = std::move(BinOrErr.get());
 
   if (isArchSpecifierInvalidOrMissing(Bin.get(), Arch))
     return make_error<CoverageMapError>(

llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp

for (auto I = MappingRegions.begin(), E = MappingRegions.end(); I != E; ++I) {
     assert(I->LineEnd >= I->LineStart);
     encodeULEB128(I->LineEnd - I->LineStart, OS);
     encodeULEB128(I->ColumnEnd, OS);
     PrevLineStart = I->LineStart;
   }
   // Ensure that all file ids have at least one mapping region.
   assert(CurrentFileID == (VirtualFileMapping.size() - 1));
 }
+
+void TestingFormatWriter::write(raw_ostream &OS, TestingFormatVersion Version) {
+  auto ByteSwap = [](uint64_t N) {
+    return support::endian::byte_swap<uint64_t, support::endianness::little>(N);
+  };
+
+  // Output a 64bit magic number.
+  auto Magic = ByteSwap(TestingFormatMagic);
+  OS.write(reinterpret_cast<char *>(&Magic), sizeof(Magic));
+
+  // Output a 64bit version field.
+  auto VersionLittle = ByteSwap(uint64_t(Version));
+  OS.write(reinterpret_cast<char *>(&VersionLittle), sizeof(VersionLittle));
+
+  // Output the ProfileNames data.
+  encodeULEB128(ProfileNamesData.size(), OS);
+  encodeULEB128(ProfileNamesAddr, OS);
+  OS << ProfileNamesData;
+
+  // Version2 adds an extra field to indicate the size of the
+  // CoverageMappingData.
+  if (Version == TestingFormatVersion::Version2)
+    encodeULEB128(CoverageMappingData.size(), OS);
+
+  // Coverage mapping data is expected to have an alignment of 8.
+  for (unsigned Pad = offsetToAlignment(OS.tell(), Align(8)); Pad; --Pad)
+    OS.write(uint8_t(0));
+  OS << CoverageMappingData;
+
+  // Coverage records data is expected to have an alignment of 8.
+  for (unsigned Pad = offsetToAlignment(OS.tell(), Align(8)); Pad; --Pad)
+    OS.write(uint8_t(0));
+  OS << CoverageRecordsData;
+}

llvm/tools/llvm-cov/TestingSupport.cpp

 //===- TestingSupport.cpp - Convert objects files into test files --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/Object/COFF.h"
 #include "llvm/Object/ObjectFile.h"
+#include "llvm/ProfileData/Coverage/CoverageMappingWriter.h"
 #include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/Alignment.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/raw_ostream.h"
 #include <functional>
if (BytesInAddress != 8) {
     errs() << "error: 64 bit binary expected\n";
     return 1;
   }
 
   // Look for the sections that we are interested in.
   int FoundSectionCount = 0;
   SectionRef ProfileNames, CoverageMapping, CoverageRecords;
   auto ObjFormat = OF->getTripleObjectFormat();
+
+  auto ProfileNamesSection = getInstrProfSectionName(IPSK_name, ObjFormat,
+                                                     /*AddSegmentInfo=*/false);
+  auto CoverageMappingSection =
+      getInstrProfSectionName(IPSK_covmap, ObjFormat, /*AddSegmentInfo=*/false);
+  auto CoverageRecordsSection =
+      getInstrProfSectionName(IPSK_covfun, ObjFormat, /*AddSegmentInfo=*/false);
+  if (isa<object::COFFObjectFile>(OF)) {
+    // On COFF, the object file section name may end in "$M". This tells the
+    // linker to sort these sections between "$A" and "$Z". The linker removes
+    // the dollar and everything after it in the final binary. Do the same to
+    // match.
+    auto Strip = [](std::string &Str) {
+      auto Pos = Str.find('$');
+      if (Pos != std::string::npos)
+        Str.resize(Pos);
+    };
+    Strip(ProfileNamesSection);
+    Strip(CoverageMappingSection);
+    Strip(CoverageRecordsSection);
+  }
+
   for (const auto &Section : OF->sections()) {
     StringRef Name;
     if (Expected<StringRef> NameOrErr = Section.getName()) {
       Name = *NameOrErr;
     } else {
       consumeError(NameOrErr.takeError());
       return 1;
     }
 
-    if (Name == llvm::getInstrProfSectionName(IPSK_name, ObjFormat,
-                                              /*AddSegmentInfo=*/false)) {
+    if (Name == ProfileNamesSection)
       ProfileNames = Section;
-    } else if (Name == llvm::getInstrProfSectionName(
-                           IPSK_covmap, ObjFormat, /*AddSegmentInfo=*/false)) {
+    else if (Name == CoverageMappingSection)
       CoverageMapping = Section;
-    } else if (Name == llvm::getInstrProfSectionName(
-                           IPSK_covfun, ObjFormat, /*AddSegmentInfo=*/false)) {
+    else if (Name == CoverageRecordsSection)
       CoverageRecords = Section;
-    } else
+    else
       continue;
     ++FoundSectionCount;
   }
   if (FoundSectionCount != 3)
     return 1;
 
   // Get the contents of the given sections.
   uint64_t ProfileNamesAddress = ProfileNames.getAddress();
int convertForTestingMain(int argc, const char *argv[]) {
   }
 
   int FD;
   if (auto Err = sys::fs::openFileForWrite(OutputFilename, FD)) {
     errs() << "error: " << Err.message() << "\n";
     return 1;
   }
 
+  coverage::TestingFormatWriter Writer(ProfileNamesAddress, ProfileNamesData,
+                                       CoverageMappingData,
+                                       CoverageRecordsData);
   raw_fd_ostream OS(FD, true);
-  OS << "llvmcovmtestdata";
-  encodeULEB128(ProfileNamesData.size(), OS);
-  encodeULEB128(ProfileNamesAddress, OS);
-  OS << ProfileNamesData;
-  // Coverage mapping data is expected to have an alignment of 8.
-  for (unsigned Pad = offsetToAlignment(OS.tell(), Align(8)); Pad; --Pad)
-    OS.write(uint8_t(0));
-  OS << CoverageMappingData;
-  // Coverage records data is expected to have an alignment of 8.
-  for (unsigned Pad = offsetToAlignment(OS.tell(), Align(8)); Pad; --Pad)
-    OS.write(uint8_t(0));
-  OS << CoverageRecordsData;
+  Writer.write(OS);
 
   return 0;
 }