[llvm-cov] Enforce alignment of function records

[serge-sans-paille]

Summary

Function Records are required to be aligned on 8 bits. This is enforced for each
records except the first, when one relies on the default alignment within and
std::string. There's no such guarantee, and indeed on 32 bits for some
implementation of std::string this is not enforced.

Provide a portable implementation based on llvm's MemoryBuffer.

[vsk]

The requirement is that the advancing by sizeof(CovMapFunctionRecordV3) bytes in the record buffer advances to the start of the next record. The address of the buffer doesn't need the same alignment.

Are you seeing an alignment-related crash, and if so, have you tried addressing it by marking CovMapFunctionRecordV3 with LLVM_PACKED?

[serge-sans-paille]

In D104745#2834796, @vsk wrote:

The requirement is that the advancing by sizeof(CovMapFunctionRecordV3) bytes in the record buffer advances to the start of the next record. The address of the buffer doesn't need the same alignment.

The function advanceByOne(...) starts with the following assert: assert(isAddrAligned(Align(8), this) && "Function record not aligned"); so I tend to think each record needs to be aligned, including the first.

Take the case of the first record created, and assume it has a size multiple of 8. In that case, upon creation no padding is added (independently of it's start address). That's what the snippet below does

while (FuncRecords.size() % 8 != 0)
  FuncRecords += '\0';

Now let's assume the start address is not a multiple of 8. In that case next record won't have an address aligned to 8, etc.

A potential patch would be yo compute padding based on the start address instead of the size, but then when the std::string grows, the alignment of the data may change upon reallocation. Thus my patch.
I also tend to think that using a MemoryBuffer conveys more meaning than an std::string for what is actually a view on raw data.

[vsk]

In D104745#2835126, @serge-sans-paille wrote:

In D104745#2834796, @vsk wrote:

The requirement is that the advancing by sizeof(CovMapFunctionRecordV3) bytes in the record buffer advances to the start of the next record. The address of the buffer doesn't need the same alignment.

The function advanceByOne(...) starts with the following assert: assert(isAddrAligned(Align(8), this) && "Function record not aligned"); so I tend to think each record needs to be aligned, including the first.

I see, thanks for highlighting the inconsistency there. It seems like assert(isAddrAligned(Align(8), this) && ...) is overzealous: the important thing is that this points to the start of a function record, *not* that this has some particular alignment. Removing the assert would make sense to me.

Take the case of the first record created, and assume it has a size multiple of 8. In that case, upon creation no padding is added (independently of it's start address). That's what the snippet below does

while (FuncRecords.size() % 8 != 0)
  FuncRecords += '\0';

Now let's assume the start address is not a multiple of 8. In that case next record won't have an address aligned to 8, etc.

Yes, and that's fine. If that runs afoul of alignment rules (and as far as I know it doesn't, since we go through endian::byte_swap to read fields from the struct), marking the function record struct 'packed' should appease the compiler.

Do you feel I'm missing something here, e.g. are you seeing an alignment-related crash in this code?

A potential patch would be yo compute padding based on the start address instead of the size, but then when the std::string grows, the alignment of the data may change upon reallocation. Thus my patch.
I also tend to think that using a MemoryBuffer conveys more meaning than an std::string for what is actually a view on raw data.

I'm completely happy to see refactors/cleanups in this area. I just wanted to make sure we're all on the same page about whether or not there's an alignment bug here. E.g. if there is such a bug that can lead to a crash, it would make sense to add a regression test.

[serge-sans-paille]

I'm completely happy to see refactors/cleanups in this area. I just wanted to make sure we're all on the same page about whether or not there's an alignment bug here. E.g. if there is such a bug that can lead to a crash, it would make sense to add a regression test.

Actually, the test already exist: current code crashes the test suite on llvm-conv/*branch* tests on RHEL7 on i686 machine. Let me try to explain the bug in a different way:

Assume the buffer allocation starts at address 0x04. Then we have a record of, say, 0x10 bytes. That size is a multiple of 8 so no padding is added, and the second record starts at address 0x14.

Now let's consider the record loading. Loading starts at address 0x04 and that's fine. It loads the expected 0x10 bytes and that's fine too. Then it wants to load the second record, and to do so it computes it's alignment *based on the address*. 0x14 is not aligned on 8, the aligned address for the second record is computed as 0x18, and that's wrong.

Why does this happen? The padding for the record buffer construction is computed based on *size* while the padding for the record loading is computed based on *address*. What my patch does is basically make sure that both abstraction are in sync: size multiple of 8 means address multiple of 8.

[vsk]

In D104745#2837127, @serge-sans-paille wrote:

I'm completely happy to see refactors/cleanups in this area. I just wanted to make sure we're all on the same page about whether or not there's an alignment bug here. E.g. if there is such a bug that can lead to a crash, it would make sense to add a regression test.

Actually, the test already exist: current code crashes the test suite on llvm-conv/*branch* tests on RHEL7 on i686 machine. Let me try to explain the bug in a different way:

My apologies, I wasn't aware of the breakage. Thanks for taking the time to patiently explain the situation.

Assume the buffer allocation starts at address 0x04. Then we have a record of, say, 0x10 bytes. That size is a multiple of 8 so no padding is added, and the second record starts at address 0x14.

Now let's consider the record loading. Loading starts at address 0x04 and that's fine. It loads the expected 0x10 bytes and that's fine too. Then it wants to load the second record, and to do so it computes it's alignment *based on the address*. 0x14 is not aligned on 8, the aligned address for the second record is computed as 0x18, and that's wrong.
Why does this happen? The padding for the record buffer construction is computed based on *size* while the padding for the record loading is computed based on *address*. What my patch does is basically make sure that both abstraction are in sync: size multiple of 8 means address multiple of 8.

I think I see the issue now:

template <support::endianness Endian>
std::pair<const char *, const CovMapFunctionRecordV3 *>
advanceByOne(const char *) const {
  assert(isAddrAligned(Align(8), this) && "Function record not aligned");
  const char *Next = ((const char *)this) + sizeof(CovMapFunctionRecordV3) -
                     sizeof(char) + getDataSize<Endian>();
  // Each function record has an alignment of 8, so we need to adjust
  // alignment before reading the next record.
  Next += offsetToAlignedAddr(Next, Align(8)); //<< HERE (offset to next aligned record calculated from potentially unaligned address)
  return {nullptr, reinterpret_cast<const CovMapFunctionRecordV3 *>(Next)};
}

The proposed fix lgtm.

[MaskRay]

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

1017

worth a comment that this block computes the size taking into account of padding.

1039–1046

[MaskRay]

Function Records are required to be aligned on 8 bits.

8 bytes.

within and std::string

and -> an

llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h

struct ProfileMappingRecord {
     ProfileMappingRecord(CovMapVersion Version, StringRef FunctionName,
                          uint64_t FunctionHash, StringRef CoverageMapping,
                          size_t FilenamesBegin, size_t FilenamesSize)
         : Version(Version), FunctionName(FunctionName),
           FunctionHash(FunctionHash), CoverageMapping(CoverageMapping),
           FilenamesBegin(FilenamesBegin), FilenamesSize(FilenamesSize) {}
   };
 
+  using FuncRecordsStorage = std::unique_ptr<MemoryBuffer>;
+
 private:
   std::vector<std::string> Filenames;
   std::vector<ProfileMappingRecord> MappingRecords;
   InstrProfSymtab ProfileNames;
   size_t CurrentRecord = 0;
   std::vector<StringRef> FunctionsFilenames;
   std::vector<CounterExpression> Expressions;
   std::vector<CounterMappingRegion> MappingRegions;
 
   // Used to tie the lifetimes of coverage function records to the lifetime of
   // this BinaryCoverageReader instance. Needed to support the format change in
   // D69471, which can split up function records into multiple sections on ELF.
-  std::string FuncRecords;
+  FuncRecordsStorage FuncRecords;
 
-  BinaryCoverageReader(std::string &&FuncRecords)
+  BinaryCoverageReader(FuncRecordsStorage &&FuncRecords)
       : FuncRecords(std::move(FuncRecords)) {}
 
 public:
   BinaryCoverageReader(const BinaryCoverageReader &) = delete;
   BinaryCoverageReader &operator=(const BinaryCoverageReader &) = delete;
 
   static Expected<std::vector<std::unique_ptr<BinaryCoverageReader>>>
   create(MemoryBufferRef ObjectBuffer, StringRef Arch,
          SmallVectorImpl<std::unique_ptr<MemoryBuffer>> &ObjectFileBuffers,
          StringRef CompilationDir = "");
 
   static Expected<std::unique_ptr<BinaryCoverageReader>>
-  createCoverageReaderFromBuffer(StringRef Coverage, std::string &&FuncRecords,
+  createCoverageReaderFromBuffer(StringRef Coverage,
+                                 FuncRecordsStorage &&FuncRecords,
                                  InstrProfSymtab &&ProfileNames,
                                  uint8_t BytesInAddress,
                                  support::endianness Endian,
                                  StringRef CompilationDir = "");
 
   Error readNextRecord(CoverageMappingRecord &Record) override;
 };
 

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

return Reader->readFunctionRecords(FuncRecBuf, FuncRecBufEnd, None, nullptr,
                                        nullptr);
   return Error::success();
 }
 
 static const char *TestingFormatMagic = "llvmcovmtestdata";
 
 Expected<std::unique_ptr<BinaryCoverageReader>>
 BinaryCoverageReader::createCoverageReaderFromBuffer(
-    StringRef Coverage, std::string &&FuncRecords,
+    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);
-  StringRef FuncRecordsRef = Reader->FuncRecords;
+  StringRef FuncRecordsRef = Reader->FuncRecords->getBuffer();
   if (BytesInAddress == 4 && Endian == support::endianness::little) {
     if (Error E =
             readCoverageMappingData<uint32_t, support::endianness::little>(
                 Reader->ProfileNames, Coverage, FuncRecordsRef,
                 Reader->MappingRecords, CompilationDir, Reader->Filenames))
       return std::move(E);
   } else if (BytesInAddress == 4 && Endian == support::endianness::big) {
     if (Error E = readCoverageMappingData<uint32_t, support::endianness::big>(
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, CoverageRecords;
+  StringRef CoverageMapping;
+  BinaryCoverageReader::FuncRecordsStorage CoverageRecords;
   if (Version < CovMapVersion::Version4) {
     CoverageMapping = Data;
     if (CoverageMapping.empty())
       return make_error<CoverageMapError>(coveragemap_error::truncated);
+    CoverageRecords = MemoryBuffer::getMemBuffer("");
   } else {
     uint32_t 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);
     // 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 = Data.substr(Pad);
-    if (CoverageRecords.empty())
+    CoverageRecords = MemoryBuffer::getMemBuffer(Data.substr(Pad));
+    if (CoverageRecords->getBufferSize() == 0)
       return make_error<CoverageMapError>(coveragemap_error::truncated);
   }
   return BinaryCoverageReader::createCoverageReaderFromBuffer(
-      CoverageMapping, CoverageRecords.str(), std::move(ProfileNames),
+      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,
                                                         StringRef Name) {
   // On COFF, the object file section name may end in "$M". This tells the
loadBinaryFormat(std::unique_ptr<Binary> Bin, StringRef Arch,
   InstrProfSymtab ProfileNames;
   std::vector<SectionRef> NamesSectionRefs = *NamesSection;
   if (NamesSectionRefs.size() != 1)
     return make_error<CoverageMapError>(coveragemap_error::malformed);
   if (Error E = ProfileNames.create(NamesSectionRefs.back()))
     return std::move(E);
 
   // Look for the coverage records section (Version4 only).
-  std::string FuncRecords;
   auto CoverageRecordsSections =
       lookupSections(*OF, getInstrProfSectionName(IPSK_covfun, ObjFormat,
                                                   /*AddSegmentInfo=*/false));
-  if (auto E = CoverageRecordsSections.takeError())
+
+  BinaryCoverageReader::FuncRecordsStorage FuncRecords;
+  if (auto E = CoverageRecordsSections.takeError()) {
     consumeError(std::move(E));
-  else {
+    FuncRecords = MemoryBuffer::getMemBuffer("");
+  } else {
+    // Compute the FuncRecordsBuffer of the buffer, taking into account the
+    // padding between each record, and making sure the first block is aligned
+    // in memory to maintain consistency between buffer address and size
+    // alignment.
+    const Align RecordAlignment(8);
+    uint64_t FuncRecordsSize = 0;
     for (SectionRef Section : *CoverageRecordsSections) {
       auto CoverageRecordsOrErr = Section.getContents();
       if (!CoverageRecordsOrErr)
         return CoverageRecordsOrErr.takeError();
-      FuncRecords += CoverageRecordsOrErr.get();
-      while (FuncRecords.size() % 8 != 0)
-        FuncRecords += '\0';
+      FuncRecordsSize += alignTo(CoverageRecordsOrErr->size(), RecordAlignment);
     }
+    auto WritableBuffer =
+        WritableMemoryBuffer::getNewUninitMemBuffer(FuncRecordsSize);
+    char *FuncRecordsBuffer = WritableBuffer->getBufferStart();
+    assert(isAddrAligned(RecordAlignment, FuncRecordsBuffer) &&
+           "Allocated memory is correctly aligned");
+
+    for (SectionRef Section : *CoverageRecordsSections) {
+      auto CoverageRecordsOrErr = Section.getContents();
+      if (!CoverageRecordsOrErr)
+        return CoverageRecordsOrErr.takeError();
+      const auto &CoverageRecords = CoverageRecordsOrErr.get();
+      FuncRecordsBuffer = std::copy(CoverageRecords.begin(),
+                                    CoverageRecords.end(), FuncRecordsBuffer);
+      FuncRecordsBuffer =
+          std::fill_n(FuncRecordsBuffer,
+                      alignAddr(FuncRecordsBuffer, RecordAlignment) -
+                          (uintptr_t)FuncRecordsBuffer,
+                      '\0');
+    }
+    assert(FuncRecordsBuffer == WritableBuffer->getBufferEnd() &&
+           "consistent init");
+    FuncRecords = std::move(WritableBuffer);
   }
 
   return BinaryCoverageReader::createCoverageReaderFromBuffer(
       CoverageMapping, std::move(FuncRecords), std::move(ProfileNames),
       BytesInAddress, Endian, CompilationDir);
 }
 
 /// Determine whether \p Arch is invalid or empty, given \p Bin.