[llvm][profile] Add padding after binary IDs

[leonardchan]

Summary

Some tests with binary IDs would fail with error: no profile can be merged. This is because raw profiles could have unaligned headers when emitting binary IDs. This means padding should be emitted after binary IDs are emitted to ensure everything else is aligned. This patch adds padding after each binary ID to ensure the next binary ID size is 8-byte aligned. This also adds extra checks to ensure we aren't reading corrupted data when printing binary IDs.

[mcgrathr]

llvm/lib/ProfileData/InstrProfReader.cpp

556

The following code is still not robust to invalid BinaryIdLen values.
It needs to compare against Remaining (accounting for the size field just read) before using the value in arithmetic to avoid overflow risks. Computing a pointer from untrusted input and then comparing it to another pointer is never robust.

565–567

What is the packing protocol? It seems wise to pad after *each* ID to make the next size field naturally-aligned, rather than having the second size field be misaligned if the first build ID length is not a multiple of 8.

[leonardchan]

D110188 which originally added the padding was reverted. I'm merging that change into this since they're both tightly coupled. Rather than adding padding after all the build IDs, we instead add padding after *each* build ID such that the build ID sizes are naturally aligned.

llvm/lib/ProfileData/InstrProfReader.cpp

565–567

Right now there's no alignment and the size just seems to be added after the previous build ID. I updated __llvm_write_build_ids to take this suggestion of adding padding after each ID. This should also prevent us from needing to add padding after all the build IDs if there's padding in between each.

I'm wondering though if maybe adding padding after each ID would lead to some bloat compared to just adding padding once.

[leonardchan]

llvm/lib/ProfileData/InstrProfReader.cpp

565–567

What is the packing protocol?

Oh, I may have misunderstood at first. In terms of packing, the only requirement seems to be that each header be 8-byte aligned (https://github.com/llvm/llvm-project/blob/dcadd64986b8a84dc244d4e7faa848fb4c18cea6/llvm/lib/ProfileData/InstrProfReader.cpp#L337). I believe all the other sections like counters, data, and names are aligned as a whole with padding coming after each section to make sure the next is aligned. So if we want to follow suite, we could stick to just padding after the binary IDs section unless padding in between IDs is more desireable.

[mcgrathr]

llvm/lib/ProfileData/InstrProfReader.cpp

565–567

The reason that other sections are aligned is so that normal, aligned word access can be used on their fields. The same logic applies to the size field in the BinaryIds section. There is a) no reason to expect multiple IDs in raw profiles in practice and b) no reason to worry about a few bytes per build ID in the context of the size of the data overall. IMHO it is far more important to maintain universal invariants like arranging that fields with natural alignment requirements are all kept aligned in the data so that code doesn't need to use memcpy to access them safely.

[leonardchan]

Added tests to generate corrupted headers that we check errors for.

[phosek]

LGTM the only question is whether these tests shouldn't rather live in compiler-rt/test/profile?

[leonardchan]

In D110365#3026182, @phosek wrote:

LGTM the only question is whether these tests shouldn't rather live in compiler-rt/test/profile?

I think llvm might be better since they're more for checking that the reader operates properly on some profdata rather than explicitly checking what the profdata is. That said, I do think there should be perhaps a compiler-rt test here for checking that the profile runtime generates the correct data. I'll add one shortly.

[leonardchan]

Added a compiler-rt test to check for padding. I'll leave it up for a bit in case others have comments on the test. Otherwise I'll commit this either by EOD or tomorrow.

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

static size_t RoundUp(size_t size, size_t align) {
   return (size + align - 1) & ~(align - 1);
 }
 
 /*
  * Write binary id length and then its data, because binary id does not
  * have a fixed length.
  */
 static int WriteOneBinaryId(ProfDataWriter *Writer, uint64_t BinaryIdLen,
-                            const uint8_t *BinaryIdData) {
+                            const uint8_t *BinaryIdData,
+                            uint64_t BinaryIdPadding) {
   ProfDataIOVec BinaryIdIOVec[] = {
       {&BinaryIdLen, sizeof(uint64_t), 1, 0},
-      {BinaryIdData, sizeof(uint8_t), BinaryIdLen, 0}};
+      {BinaryIdData, sizeof(uint8_t), BinaryIdLen, 0},
+      {NULL, sizeof(uint8_t), BinaryIdPadding, 1},
+  };
   if (Writer->Write(Writer, BinaryIdIOVec,
                     sizeof(BinaryIdIOVec) / sizeof(*BinaryIdIOVec)))
     return -1;
 
   /* Successfully wrote binary id, report success. */
   return 0;
 }
 
static int WriteBinaryIdForNote(ProfDataWriter *Writer,
 
   const char *NoteName = (const char *)Note + sizeof(ElfW(Nhdr));
   if (Note->n_type == NT_GNU_BUILD_ID && Note->n_namesz == 4 &&
       memcmp(NoteName, "GNU\0", 4) == 0) {
 
     uint64_t BinaryIdLen = Note->n_descsz;
     const uint8_t *BinaryIdData =
         (const uint8_t *)(NoteName + RoundUp(Note->n_namesz, 4));
-    if (Writer != NULL &&
-        WriteOneBinaryId(Writer, BinaryIdLen, BinaryIdData) == -1)
+    uint8_t BinaryIdPadding = __llvm_profile_get_num_padding_bytes(BinaryIdLen);
+    if (Writer != NULL && WriteOneBinaryId(Writer, BinaryIdLen, BinaryIdData,
+                                           BinaryIdPadding) == -1)
       return -1;
 
-    BinaryIdSize = sizeof(BinaryIdLen) + BinaryIdLen;
+    BinaryIdSize = sizeof(BinaryIdLen) + BinaryIdLen + BinaryIdPadding;
   }
 
   return BinaryIdSize;
 }
 
 /*
  * Helper function that iterates through notes section and find build ids.
  * If writer is given, write binary ids into profiles.

compiler-rt/test/profile/Linux/binary-id-padding.c

+// Set these requirements to ensure that we have an 8-byte binary ID.
+// REQUIRES: linux
+//
+// This will generate a 20-byte build ID, which requires 4-byte padding.
+// RUN: %clang_profgen -Wl,--build-id=sha1 -o %t %s
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: %run %t %t.profraw
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+
+enum ValueKind {
+#define VALUE_PROF_KIND(Enumerator, Value, Descr) Enumerator = Value,
+#include "profile/InstrProfData.inc"
+};
+
+typedef struct __llvm_profile_header {
+#define INSTR_PROF_RAW_HEADER(Type, Name, Initializer) Type Name;
+#include "profile/InstrProfData.inc"
+} __llvm_profile_header;
+
+typedef void *IntPtrT;
+typedef struct __llvm_profile_data {
+#define INSTR_PROF_DATA(Type, LLVMType, Name, Initializer) Type Name;
+#include "profile/InstrProfData.inc"
+} __llvm_profile_data;
+
+void bail(const char* str) {
+  fprintf(stderr, "%s %s\n", str, strerror(errno));
+  exit(1);
+}
+
+void func() {}
+
+int main(int argc, char** argv) {
+  if (argc == 2) {
+    int fd = open(argv[1], O_RDONLY);
+    if (fd == -1)
+      bail("open");
+
+    struct stat st;
+    if (stat(argv[1], &st))
+      bail("stat");
+    uint64_t FileSize = st.st_size;
+
+    char* Buf = (char *)mmap(NULL, FileSize, PROT_READ, MAP_SHARED, fd, 0);
+    if (Buf == MAP_FAILED)
+      bail("mmap");
+
+    __llvm_profile_header *Header = (__llvm_profile_header *)Buf;
+    if (Header->BinaryIdsSize != 32)
+      bail("Invalid binary ID size");
+
+    char *BinaryIdsStart = Buf + sizeof(__llvm_profile_header);
+
+    uint64_t BinaryIdSize = *((uint64_t *)BinaryIdsStart);
+    if (BinaryIdSize != 20)
+      bail("Expected a binary ID of size 20");
+
+    // Skip the size and the binary ID itself to check padding.
+    BinaryIdsStart += 8 + 20;
+    if (*((uint32_t *)BinaryIdsStart))
+      bail("Found non-zero binary ID padding");
+
+    if (munmap(Buf, FileSize))
+      bail("munmap");
+
+    if (close(fd))
+      bail("close");
+  } else {
+    func();
+  }
+  return 0;
+}

llvm/lib/ProfileData/InstrProfReader.cpp

 template <class IntPtrT>
 Error RawInstrProfReader<IntPtrT>::readHeader(
     const RawInstrProf::Header &Header) {
   Version = swap(Header.Version);
   if (GET_VERSION(Version) != RawInstrProf::Version)
     return error(instrprof_error::unsupported_version);
 
   BinaryIdsSize = swap(Header.BinaryIdsSize);
+  if (BinaryIdsSize % sizeof(uint64_t))
+    return error(instrprof_error::bad_header);
+
   CountersDelta = swap(Header.CountersDelta);
   NamesDelta = swap(Header.NamesDelta);
   auto DataSize = swap(Header.DataSize);
   auto PaddingBytesBeforeCounters = swap(Header.PaddingBytesBeforeCounters);
   auto CountersSize = swap(Header.CountersSize);
   auto PaddingBytesAfterCounters = swap(Header.PaddingBytesAfterCounters);
   NamesSize = swap(Header.NamesSize);
   ValueKindLast = swap(Header.ValueKindLast);
Error RawInstrProfReader<IntPtrT>::readHeader(
 
   // Binary ids start just after the header.
   BinaryIdsStart =
       reinterpret_cast<const uint8_t *>(&Header) + sizeof(RawInstrProf::Header);
   CountersStart = reinterpret_cast<const uint64_t *>(Start + CountersOffset);
   NamesStart = Start + NamesOffset;
   ValueDataStart = reinterpret_cast<const uint8_t *>(Start + ValueDataOffset);
 
+  const uint8_t *BufferEnd = (const uint8_t *)DataBuffer->getBufferEnd();
+  if (BinaryIdsStart + BinaryIdsSize > BufferEnd)
+    return error(instrprof_error::bad_header);
+
   std::unique_ptr<InstrProfSymtab> NewSymtab = std::make_unique<InstrProfSymtab>();
   if (Error E = createSymtab(*NewSymtab.get()))
     return E;
 
   Symtab = std::move(NewSymtab);
   return success();
 }
 
Error RawInstrProfReader<IntPtrT>::readNextRecord(NamedInstrProfRecord &Record) {
   if (Error E = readValueProfilingData(Record))
     return error(std::move(E));
 
   // Iterate.
   advanceData();
   return success();
 }
 
+static size_t RoundUp(size_t size, size_t align) {
+  return (size + align - 1) & ~(align - 1);
+}
+
 template <class IntPtrT>
 Error RawInstrProfReader<IntPtrT>::printBinaryIds(raw_ostream &OS) {
   if (BinaryIdsSize == 0)
     return success();
 
   OS << "Binary IDs: \n";
   const uint8_t *BI = BinaryIdsStart;
-  while (BI < BinaryIdsStart + BinaryIdsSize) {
+  const uint8_t *BIEnd = BinaryIdsStart + BinaryIdsSize;
+  while (BI < BIEnd) {
+    size_t Remaining = BIEnd - BI;
+
+    // There should be enough left to read the binary ID size field.
+    if (Remaining < sizeof(uint64_t))
+      return make_error<InstrProfError>(instrprof_error::malformed);
+
     uint64_t BinaryIdLen = swap(*reinterpret_cast<const uint64_t *>(BI));
+
+    // There should be enough left to read the binary ID size field, and the
+    // binary ID.
+    if (Remaining < sizeof(BinaryIdLen) + BinaryIdLen)
+      return make_error<InstrProfError>(instrprof_error::malformed);
+
     // Increment by binary id length data type size.
     BI += sizeof(BinaryIdLen);
     if (BI > (const uint8_t *)DataBuffer->getBufferEnd())
       return make_error<InstrProfError>(instrprof_error::malformed);
 
     for (uint64_t I = 0; I < BinaryIdLen; I++)
       OS << format("%02x", BI[I]);
     OS << "\n";
 
-    // Increment by binary id data length.
-    BI += BinaryIdLen;
+    // Increment by binary id data length, rounded to the next 8 bytes. This
+    // accounts for the zero-padding after each build ID.
+    BI += RoundUp(BinaryIdLen, sizeof(uint64_t));
     if (BI > (const uint8_t *)DataBuffer->getBufferEnd())
       return make_error<InstrProfError>(instrprof_error::malformed);
   }
 
   return success();
 }
 
 namespace llvm {

llvm/test/tools/llvm-profdata/binary-ids-padding.test

+// Header
+//
+// INSTR_PROF_RAW_HEADER(uint64_t, Magic, __llvm_profile_get_magic())
+// INSTR_PROF_RAW_HEADER(uint64_t, Version, __llvm_profile_get_version())
+// INSTR_PROF_RAW_HEADER(uint64_t, BinaryIdsSize, __llvm_write_binary_ids(NULL))
+// INSTR_PROF_RAW_HEADER(uint64_t, DataSize, DataSize)
+// INSTR_PROF_RAW_HEADER(uint64_t, CountersSize, CountersSize)
+// INSTR_PROF_RAW_HEADER(uint64_t, NamesSize,  NamesSize)
+// INSTR_PROF_RAW_HEADER(uint64_t, CountersDelta, (uintptr_t)CountersBegin)
+// INSTR_PROF_RAW_HEADER(uint64_t, NamesDelta, (uintptr_t)NamesBegin)
+// INSTR_PROF_RAW_HEADER(uint64_t, ValueKindLast, IPVK_Last)
+
+RUN: printf '\201rforpl\377' > %t.profraw
+RUN: printf '\7\0\0\0\0\0\0\0' >> %t.profraw
+// There will be 2 20-byte binary IDs, so the total Binary IDs size will be 64 bytes.
+//   2 * 8  binary ID sizes
+// + 2 * 20 binary IDs (of size 20)
+// + 2 * 4  binary ID paddings
+// --------
+// = 64     bytes
+RUN: printf '\100\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\2\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\3\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\20\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\4\0\1\0\0\0' >> %t.profraw
+RUN: printf '\0\0\4\0\2\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+
+// Binary IDs - There are only two in this case that are 20 bytes.
+RUN: printf '\24\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\1\2\3\4\5\6\7' >> %t.profraw
+RUN: printf '\0\1\2\3\4\5\6\7' >> %t.profraw
+RUN: printf '\0\1\2\3\0\0\0\0' >> %t.profraw
+RUN: printf '\24\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\1\1\1\1\1\1\1\1' >> %t.profraw
+RUN: printf '\2\2\2\2\2\2\2\2' >> %t.profraw
+RUN: printf '\3\3\3\3\0\0\0\0' >> %t.profraw
+
+// Data Section
+//
+// struct ProfData {
+// #define INSTR_PROF_DATA(Type, LLVMType, Name, Initializer) \
+//    Type Name;
+// #include "llvm/ProfileData/InstrProfData.inc"
+// };
+
+RUN: printf '\254\275\030\333\114\302\370\134' >> %t.profraw
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\4\0\1\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\1\0\0\0\0\0\0\0' >> %t.profraw
+
+RUN: printf '\067\265\035\031\112\165\023\344' >> %t.profraw
+RUN: printf '\02\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\xd8\xff\3\0\1\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\02\0\0\0\0\0\0\0' >> %t.profraw
+
+RUN: printf '\023\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\067\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\101\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\7\0foo\1bar\0\0\0\0\0\0\0' >> %t.profraw
+
+// RUN: llvm-profdata show --binary-ids  %t.profraw | FileCheck %s
+// CHECK: Instrumentation level: Front-end
+// CHECK: Binary IDs:
+// CHECK-NEXT: 0001020304050607000102030405060700010203
+// CHECK-NEXT: 0101010101010101020202020202020203030303

llvm/test/tools/llvm-profdata/insufficient-binary-ids-size.test

+RUN: printf '\201rforpl\377' > %t.profraw
+RUN: printf '\7\0\0\0\0\0\0\0' >> %t.profraw
+// We should fail on this because the data buffer (profraw file) is not long
+// enough to hold this binary IDs size. NOTE that this (combined with the 8-byte
+// alignment requirement for binary IDs size) will ensure we can at least read one
+// 8-byte size if the binary IDs are provided.
+RUN: printf '\8\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+
+RUN: printf '\0\0\0\0\0\0\0' >> %t.profraw
+
+// RUN: not llvm-profdata show --binary-ids  %t.profraw 2>&1 | FileCheck %s
+// CHECK: invalid instrumentation profile data (file header is corrupt)

llvm/test/tools/llvm-profdata/large-binary-id-size.test

+RUN: printf '\201rforpl\377' > %t.profraw
+RUN: printf '\7\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\40\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+
+// Check for a corrupted size being too large past the end of the file.
+RUN: printf '\7\7\7\7\7\7\7\7' >> %t.profraw
+RUN: printf '\0\1\2\3\4\5\6\7' >> %t.profraw
+RUN: printf '\0\1\2\3\4\5\6\7' >> %t.profraw
+RUN: printf '\0\1\2\3\0\0\0\0' >> %t.profraw
+
+// RUN: not llvm-profdata show --binary-ids  %t.profraw 2>&1 | FileCheck %s
+// CHECK: malformed instrumentation profile data

llvm/test/tools/llvm-profdata/misaligned-binary-ids-size.test

+RUN: printf '\201rforpl\377' > %t.profraw
+RUN: printf '\7\0\0\0\0\0\0\0' >> %t.profraw
+// We should fail on this because the binary IDs is not a multiple of 8 bytes.
+RUN: printf '\77\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\0\0\0\0\0\0\0' >> %t.profraw
+
+// Binary IDs - There are only two in this case that are 20 bytes.
+RUN: printf '\24\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\0\1\2\3\4\5\6\7' >> %t.profraw
+RUN: printf '\0\1\2\3\4\5\6\7' >> %t.profraw
+RUN: printf '\0\1\2\3\0\0\0\0' >> %t.profraw
+RUN: printf '\24\0\0\0\0\0\0\0' >> %t.profraw
+RUN: printf '\1\1\1\1\1\1\1\1' >> %t.profraw
+RUN: printf '\2\2\2\2\2\2\2\2' >> %t.profraw
+RUN: printf '\3\3\3\3\0\0\0\0' >> %t.profraw
+
+// RUN: not llvm-profdata show --binary-ids  %t.profraw 2>&1 | FileCheck %s
+// CHECK: invalid instrumentation profile data (file header is corrupt)