[InstrProf] Make the IndexedInstrProf header backwards compatible.

[snehasish]

Summary

While the contents of the profile are backwards compatible the header
itself is not. For example, when adding new fields to the header results
in significant issues. This change adds allows for portable
instantiation of the header across indexed format versions.

[davidxl]

The refactoring looks good. However in what situation do we need to modify the main header? There might be other ways to achieve the functionality.

[snehasish]

In D118390#3276782, @davidxl wrote:

The refactoring looks good. However in what situation do we need to modify the main header? There might be other ways to achieve the functionality.

We discussed some alternatives offline, summarizing here:

The motivation behind this change is the desire to add another (optional) ondiskhashtable to the indexed profile format. This will hold memory profile information which will be used to guide the memory allocator [1]. However the phase ordering of memory instrumentation (envisioned to be enabled at the same time as FDO instrumentation) implies that the current "function name & CFG hash" based indexing for the existing on disk hashtable is incompatible for memprof profile data. While it may be possible to refactor out the has computation to reuse, the approach is brittle and there are unknowns along with a larger radius of potentially affected users. Adding a new field to the header allows us provision another memprof profile data section. This section will contain metadata and an on disk hash table with the profile data.

Why not use the Unused field?
Existing profiles may break, at least the compat tests in the repo have old profiles which have this field set.

Why not add indirection to the existing on disk table header?
The on disk table structure allows for metadata before the actual payload and this approach is feasible. However, it makes the logic in IndexedInstrProfReader even more complicated and harder to follow.

I'll add some more static checks to ensure that users don't inadvertently break the ordering based computation here.

[1] https://groups.google.com/g/llvm-dev/c/h1DvHguLpxU/m/yGE-zyrhAAAJ

[davidxl]

lgtm

[snehasish]

Rebase.

[snehasish]

This patch broke on big endian systems due to the interpretation of the FormatVersion variable. While refactoring the code, I replaced uses of the FormatVersion local variable with Header::Version in all but one check. For little endian reader and writer setup this passed since we always check the format version after a byte_swap to little endian (so in this case it was a no-op). I added a new method to the Header struct to return the format version in little endian instead of a local var. This passed all the instrprof unit tests on a local big endian setup. I would have liked to update this diff but arc got confused so if you have any comments please add them to https://reviews.llvm.org/rGa3beb34015fc. Thanks!

llvm/include/llvm/ProfileData/InstrProf.h

 // This structure defines the file header of the LLVM profile
 // data file in indexed-format.
 struct Header {
   uint64_t Magic;
   uint64_t Version;
   uint64_t Unused; // Becomes unused since version 4
   uint64_t HashType;
   uint64_t HashOffset;
+  // New fields should only be added at the end to ensure that the size
+  // computation is correct. The methods below need to be updated to ensure that
+  // the new field is read correctly.
+
+  // Reads a header struct from the buffer.
+  static Expected<Header> readFromBuffer(const unsigned char *Buffer);
+
+  // Returns the size of the header in bytes for all valid fields based on the
+  // version. I.e a older version header will return a smaller size.
+  size_t size() const;
 };
 
 // Profile summary data recorded in the profile data file in indexed
 // format. It is introduced in version 4. The summary data follows
 // right after the profile file header.
 struct Summary {
   struct Entry {
     uint64_t Cutoff; ///< The required percentile of total execution count.

llvm/lib/ProfileData/InstrProf.cpp

 #include <algorithm>
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
 #include <cstring>
 #include <memory>
 #include <string>
 #include <system_error>
+#include <type_traits>
 #include <utility>
 #include <vector>
 
 using namespace llvm;
 
 static cl::opt<bool> StaticFuncFullModulePrefix(
     "static-func-full-module-prefix", cl::init(true), cl::Hidden,
     cl::desc("Use full module build paths in the profile counter names for "
OS << "  " << ProfileKindName
        << " profile base count sum: " << format("%.0f", Base.ValueCounts[I])
        << "\n"
        << "  " << ProfileKindName
        << " profile test count sum: " << format("%.0f", Test.ValueCounts[I])
        << "\n";
   }
 }
 
+namespace IndexedInstrProf {
+// A C++14 compatible version of the offsetof macro.
+template <typename T1, typename T2>
+inline size_t constexpr offsetOf(T1 T2::*Member) {
+  constexpr T2 Object{};
+  return size_t(&(Object.*Member)) - size_t(&Object);
+}
+
+static inline uint64_t read(const unsigned char *Buffer, size_t Offset) {
+  return *reinterpret_cast<const uint64_t *>(Buffer + Offset);
+}
+
+Expected<Header> Header::readFromBuffer(const unsigned char *Buffer) {
+  using namespace support;
+  static_assert(std::is_standard_layout<Header>::value,
+                "The header should be standard layout type since we use offset "
+                "of fields to read.");
+  Header H;
+
+  H.Magic = read(Buffer, offsetOf(&Header::Magic));
+  // Check the magic number.
+  uint64_t Magic = endian::byte_swap<uint64_t, little>(H.Magic);
+  if (Magic != IndexedInstrProf::Magic)
+    return make_error<InstrProfError>(instrprof_error::bad_magic);
+
+  // Read the version.
+  H.Version = read(Buffer, offsetOf(&Header::Version));
+  uint64_t FormatVersion = endian::byte_swap<uint64_t, little>(H.Version);
+  if (GET_VERSION(FormatVersion) >
+      IndexedInstrProf::ProfVersion::CurrentVersion)
+    return make_error<InstrProfError>(instrprof_error::unsupported_version);
+
+  switch (GET_VERSION(FormatVersion)) {
+  // When a new field is added in the header add a case statement here to
+  // populate it.
+  default:
+    H.HashType = read(Buffer, offsetOf(&Header::HashType));
+    H.HashOffset = read(Buffer, offsetOf(&Header::HashOffset));
+  }
+
+  return H;
+}
+
+size_t Header::size() const {
+  switch (GET_VERSION(Version)) {
+  // When a new field is added to the header add a case statement here to
+  // compute the size as offset of the new field + size of the new field. This
+  // relies on the field being added to the end of the list.
+  default:
+    return offsetOf(&Header::HashOffset) + sizeof(Header::HashOffset);
+  }
+}
+
+} // namespace IndexedInstrProf
+
 } // end namespace llvm

llvm/lib/ProfileData/InstrProfReader.cpp

Error IndexedInstrProfReader::readHeader() {
   using namespace support;
 
   const unsigned char *Start =
       (const unsigned char *)DataBuffer->getBufferStart();
   const unsigned char *Cur = Start;
   if ((const unsigned char *)DataBuffer->getBufferEnd() - Cur < 24)
     return error(instrprof_error::truncated);
 
-  auto *Header = reinterpret_cast<const IndexedInstrProf::Header *>(Cur);
-  Cur += sizeof(IndexedInstrProf::Header);
+  auto HeaderOr = IndexedInstrProf::Header::readFromBuffer(Start);
+  if (!HeaderOr)
+    return HeaderOr.takeError();
 
-  // Check the magic number.
-  uint64_t Magic = endian::byte_swap<uint64_t, little>(Header->Magic);
-  if (Magic != IndexedInstrProf::Magic)
-    return error(instrprof_error::bad_magic);
-
-  // Read the version.
-  uint64_t FormatVersion = endian::byte_swap<uint64_t, little>(Header->Version);
-  if (GET_VERSION(FormatVersion) >
-      IndexedInstrProf::ProfVersion::CurrentVersion)
-    return error(instrprof_error::unsupported_version);
+  const IndexedInstrProf::Header *Header = &HeaderOr.get();
+  Cur += Header->size();
 
-  Cur = readSummary((IndexedInstrProf::ProfVersion)FormatVersion, Cur,
+  Cur = readSummary((IndexedInstrProf::ProfVersion)Header->Version, Cur,
                     /* UseCS */ false);
-  if (FormatVersion & VARIANT_MASK_CSIR_PROF)
-    Cur = readSummary((IndexedInstrProf::ProfVersion)FormatVersion, Cur,
+  if (Header->Version & VARIANT_MASK_CSIR_PROF)
+    Cur = readSummary((IndexedInstrProf::ProfVersion)Header->Version, Cur,
                       /* UseCS */ true);
 
   // Read the hash type and start offset.
   IndexedInstrProf::HashT HashType = static_cast<IndexedInstrProf::HashT>(
       endian::byte_swap<uint64_t, little>(Header->HashType));
   if (HashType > IndexedInstrProf::HashT::Last)
     return error(instrprof_error::unsupported_hash_type);
 
   uint64_t HashOffset = endian::byte_swap<uint64_t, little>(Header->HashOffset);
 
   // The rest of the file is an on disk hash table.
-  auto IndexPtr =
-      std::make_unique<InstrProfReaderIndex<OnDiskHashTableImplV3>>(
-          Start + HashOffset, Cur, Start, HashType, FormatVersion);
+  auto IndexPtr = std::make_unique<InstrProfReaderIndex<OnDiskHashTableImplV3>>(
+      Start + HashOffset, Cur, Start, HashType, Header->Version);
 
   // Load the remapping table now if requested.
   if (RemappingBuffer) {
     Remapper = std::make_unique<
         InstrProfReaderItaniumRemapper<OnDiskHashTableImplV3>>(
         std::move(RemappingBuffer), *IndexPtr);
     if (Error E = Remapper->populateRemappings())
       return E;