Add Header::MinimumProfileReaderVersion to detect incompatible changes

Author: mingmingl-llvm

llvm/include/llvm/ProfileData/InstrProf.h

@@ -368,6 +368,7 @@ enum class instrprof_error {
   zlib_unavailable,
   raw_profile_version_mismatch,
   counter_value_too_large,
+  unsupported_incompatible_future_version,
 };
 
 /// An ordered list of functions identified by their NameRef found in
@@ -1146,7 +1147,13 @@ enum ProfVersion {
   Version11 = 11,
   // VTable profiling,
   Version12 = 12,
-  // Record the on-disk byte size of header.
+  // Two additional header fields to add partial forward compatibility.
+  // Indexed profile reader compares the latest version it can parse with the
+  // minimum version required by (and recorded in) the profile; if the reader
+  // can reasonably interprets the payload, it proceeds to parse known sections
+  // and skip unknown sections; otherwise it stops reading and throws error
+  // (users should update compilers and/or command line tools to parse profiles
+  // with newer versions).
   Version13 = 13,
   // The current version is 13.
   CurrentVersion = INSTR_PROF_INDEX_VERSION
@@ -1177,9 +1184,24 @@ struct Header {
   uint64_t TemporalProfTracesOffset;
   uint64_t VTableNamesOffset;
 
-  // The on-disk byte size of the header. As with other fields, do not read its
-  // value before calling readFromBuffer.
-  uint64_t Size = 0;
+  // Records the on-disk byte size of the header.
+  uint64_t OnDiskByteSize = 0;
+
+  // Indexed profile writer will records the minimum profile reader version
+  // required to parse this profile. If a profile reader's supported version is
+  // smaller than what's recorded in this field, the profile reader (in
+  // compilers or command line tools) should be updated.
+  //
+  // Example scenario:
+  // The semantics of an existing section change starting from version V + 1
+  // (with readers supporting both versions to preserve backward compatibility),
+  // indexed profile writer should record `MinimumProfileReaderVersion` as V
+  // + 1. Previously-built profile readers won't know how to interpret the
+  // existing section correctly; these readers will find the
+  // `MinimumProfileReaderVersion` recorded in the profile is higher than the
+  // readers' supported version (a constant baked in the library), and stop the
+  // read process.
+  uint64_t MinimumProfileReaderVersion = 0;
 
   // 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
@@ -1192,6 +1214,7 @@ struct Header {
   // the version.
   size_t size() const;
 
+  // Returns the end byte offset of all known fields by the reader.
   Expected<size_t> knownFieldsEndByteOffset() const;
 
   // Returns the format version in little endian. The header retains the

llvm/include/llvm/ProfileData/InstrProfWriter.h

@@ -40,6 +40,7 @@ class InstrProfWriter {
 
 private:
   bool Sparse;
+
   StringMap<ProfilingData> FunctionData;
   /// The maximum length of a single temporal profile trace.
   uint64_t MaxTemporalProfTraceLength;
@@ -81,6 +82,25 @@ class InstrProfWriter {
   // The MemProf version we should write.
   memprof::IndexedVersion MemProfVersionRequested;
 
+  // Returns the profile version in uint32_t.
+  // Header.Version is uint64_t with the lowest 32 bits specifying profile
+  // version and the most significant bits specyfing profile flavors.
+  uint32_t profileVersion() const;
+
+  // Returns the minimum profile reader version required to parse this profile.
+  uint64_t minProfileReaderVersion() const;
+
+  // The following fields are used in unit tests only.
+  // If not std::nullopt, this field overwrites the lowest 32 bits of
+  // Header::Version in the generated profile.
+  std::optional<uint32_t> ProfileVersion = std::nullopt;
+  // If true, profile writer will append one 64-bit dummy value as unknown
+  // header fields.
+  bool AppendAdditionalHeaderFields = false;
+  // If not std::nullopt, this field overwrites
+  // Header::MinimumProfileReaderVersion in the generated profile.
+  std::optional<int> MinCompatibleReaderProfileVersion = std::nullopt;
+
 public:
   InstrProfWriter(
       bool Sparse = false, uint64_t TemporalProfTraceReservoirSize = 0,
@@ -187,15 +207,20 @@ class InstrProfWriter {
     return static_cast<bool>(ProfileKind & InstrProfKind::SingleByteCoverage);
   }
 
-  // Internal interface for testing purpose only.
-  void setValueProfDataEndianness(llvm::endianness Endianness);
-  void setOutputSparse(bool Sparse);
   // Compute the overlap b/w this object and Other. Program level result is
   // stored in Overlap and function level result is stored in FuncLevelOverlap.
   void overlapRecord(NamedInstrProfRecord &&Other, OverlapStats &Overlap,
                      OverlapStats &FuncLevelOverlap,
                      const OverlapFuncFilters &FuncFilter);
 
+  // Internal interface for testing purpose only.
+  void setValueProfDataEndianness(llvm::endianness Endianness);
+  void setOutputSparse(bool Sparse);
+  void setProfileVersion(uint32_t Version);
+  void setMinCompatibleReaderProfileVersion(uint32_t Version);
+  void setAppendAdditionalHeaderFields();
+  void resetTestOnlyStatesForHeaderSection();
+
 private:
   void addRecord(StringRef Name, uint64_t Hash, InstrProfRecord &&I,
                  uint64_t Weight, function_ref<void(Error)> Warn);

llvm/lib/ProfileData/InstrProf.cpp

@@ -38,6 +38,7 @@
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Path.h"
@@ -165,6 +166,11 @@ static std::string getInstrProfErrString(instrprof_error Err,
   case instrprof_error::counter_value_too_large:
     OS << "excessively large counter value suggests corrupted profile data";
     break;
+  case instrprof_error::unsupported_incompatible_future_version:
+    OS << "unsupported incompatible future version. The profile is likely "
+          "generated from newer released compilers/tools and not parsable by "
+          "current reader.";
+    break;
   }
 
   // If optional error message is not empty, append it to the message.
@@ -1612,37 +1618,49 @@ Expected<Header> Header::readFromBuffer(const unsigned char *Buffer) {
 
   // Read the version.
   H.Version = read(Buffer, sizeof(uint64_t));
-  uint64_t ProfileVersion = GET_VERSION(H.formatVersion());
-
-  if (ProfileVersion > IndexedInstrProf::ProfVersion::CurrentVersion) {
-    if (CurrentVersion < ProfVersion::Version13)
-      return make_error<InstrProfError>(instrprof_error::unsupported_version);
-
-    // FIXME: When the semantic of an existing payload section changes, allowing
-    // profile reader to interpret new profiles using the old code in the
-    // current way is too error-prone.
-    // Should the payload section record its own version and fail the profile
-    // reader to require a re-build of the profile reader in this case?
+  uint64_t ProfileRecordedProfileVersion = GET_VERSION(H.formatVersion());
+
+  if (ProfileRecordedProfileVersion >= ProfVersion::Version13) {
+    // Starting from version 13, profiles should records header's on-disk byte
+    // size and the minimum profile reader version required.
+    H.OnDiskByteSize = read(Buffer, kOnDiskSizeOffset);
+    H.MinimumProfileReaderVersion =
+        read(Buffer, kOnDiskSizeOffset + sizeof(uint64_t));
+  } else {
+    // Prior to version 13, the largest version supported by the reader
+    // (ProfVersion::CurrentVersion) must be greater than or equal to the
+    // profile-recorded version.
+    H.MinimumProfileReaderVersion = ProfileRecordedProfileVersion;
   }
 
-  if (ProfileVersion >= ProfVersion::Version13)
-    H.Size = read(Buffer, kOnDiskSizeOffset);
+  // Stop reading and return error if the largest version supported by the
+  // reader falls behind the minimum reader version required by the profiles.
+  if (IndexedInstrProf::ProfVersion::CurrentVersion <
+      H.MinimumProfileReaderVersion)
+    return make_error<InstrProfError>(
+        instrprof_error::unsupported_incompatible_future_version,
+        formatv("Profile reader should support {0} to parse profile of version "
+                "{1} ",
+                H.MinimumProfileReaderVersion, ProfileRecordedProfileVersion));
 
   // `FieldByteOffset` represents the end byte of the last known header field.
   auto FieldByteOffsetOrErr = H.knownFieldsEndByteOffset();
-  if (FieldByteOffsetOrErr)
+  if (Error E = FieldByteOffsetOrErr.takeError())
+    return E;
+
+  auto FieldByteOffset = FieldByteOffsetOrErr.get();
 
   assert(FieldByteOffset != 0 &&
          "FieldByteOffset specifies the byte offset of the last known field in "
          "header and should not be zero");
 
   // If the version from profile is higher than the currently supported version,
   // read the known defined header fields and discard the rest.
-  if (ProfileVersion > ProfVersion::CurrentVersion)
-    ProfileVersion = ProfVersion::CurrentVersion;
+  if (ProfileRecordedProfileVersion > ProfVersion::CurrentVersion)
+    ProfileRecordedProfileVersion = ProfVersion::CurrentVersion;
 
   // Initialize header fields based on the currently supported version.
-  switch (ProfileVersion) {
+  switch (ProfileRecordedProfileVersion) {
     // When a new field is added in the header add a case statement here to
     // populate it.
     static_assert(
@@ -1651,7 +1669,8 @@ Expected<Header> Header::readFromBuffer(const unsigned char *Buffer) {
         "if not add a case statement to fall through to the latest version.");
   case 13ull:
     // Size field is already read.
-    FieldByteOffset -= sizeof(Header::Size);
+    FieldByteOffset -= sizeof(Header::OnDiskByteSize);
+    FieldByteOffset -= sizeof(Header::MinimumProfileReaderVersion);
     [[fallthrough]];
   case 12ull:
     FieldByteOffset -= sizeof(Header::VTableNamesOffset);
@@ -1682,21 +1701,23 @@ Expected<Header> Header::readFromBuffer(const unsigned char *Buffer) {
 }
 
 Expected<size_t> Header::knownFieldsEndByteOffset() const {
-  const uint64_t ProfileVersion = GET_VERSION(formatVersion());
-  if (ProfileVersion <= ProfVersion::Version12)
+  // All header fields are known before, so the end byte offset of known fields
+  // is the same as header byte size.
+  const uint64_t ProfileRecordedProfileVersion = GET_VERSION(formatVersion());
+  if (ProfileRecordedProfileVersion <= ProfVersion::Version12)
     return this->size();
 
-  // Starting from version 13, the known field end byte offset is inferred based
-  // on the currently supported version.
+  // Starting from version 13, the known field end byte offset is calculated
+  // based on the currently supported version recorded in the reader.
   switch (IndexedInstrProf::ProfVersion::CurrentVersion) {
     // When a new field is added in the header add a case statement here to
     // populate it.
-    static_assert(
-        IndexedInstrProf::ProfVersion::CurrentVersion == Version13,
-        "Please update the reading code below if a new field has been added, "
-        "if not add a case statement to fall through to the latest version.");
+    static_assert(IndexedInstrProf::ProfVersion::CurrentVersion == Version13,
+                  "If current version bumps, please update the case below to "
+                  "calculate the known field end byte offset.");
   case 13ull:
-    return kOnDiskSizeOffset + sizeof(Header::Size);
+    return kOnDiskSizeOffset + sizeof(Header::OnDiskByteSize) +
+           sizeof(Header::MinimumProfileReaderVersion);
   default:
     break;
   }
@@ -1705,16 +1726,17 @@ Expected<size_t> Header::knownFieldsEndByteOffset() const {
 }
 
 size_t Header::size() const {
-  const uint64_t ProfileVersion = GET_VERSION(formatVersion());
+  const uint64_t ProfileRecordedProfileVersion = GET_VERSION(formatVersion());
   // Starting from version 13, the indexed profile records the byte size of
   // header.
-  if (ProfileVersion >= ProfVersion::Version13) {
-    assert(Size != 0 && "User can call Header::size() only after reading it "
-                        "from readMemoryBuffer");
-    return Size;
+  if (ProfileRecordedProfileVersion >= ProfVersion::Version13) {
+    assert(OnDiskByteSize != 0 &&
+           "User can call Header::size() only after reading it "
+           "from readMemoryBuffer");
+    return OnDiskByteSize;
   }
-  switch (ProfileVersion) {
-    assert(ProfileVersion <= ProfVersion::Version12 &&
+  switch (ProfileRecordedProfileVersion) {
+    assert(ProfileRecordedProfileVersion <= ProfVersion::Version12 &&
            "Do not infer header size field for newer version");
   case 12ull:
     return offsetOf(&Header::VTableNamesOffset) +

llvm/lib/ProfileData/InstrProfWriter.cpp

@@ -36,9 +36,6 @@
 
 using namespace llvm;
 
-static cl::opt<bool> WriteFutureVersion("write-future-version",
-                                        cl::init(false));
-
 // A struct to define how the data stream should be patched. For Indexed
 // profiling, only uint64_t data type is needed.
 struct PatchItem {
@@ -196,7 +193,7 @@ InstrProfWriter::InstrProfWriter(
 
 InstrProfWriter::~InstrProfWriter() { delete InfoObj; }
 
-// Internal interface for testing purpose only.
+// Begin: Internal interface for testing purpose only.
 void InstrProfWriter::setValueProfDataEndianness(llvm::endianness Endianness) {
   InfoObj->ValueProfDataEndianness = Endianness;
 }
@@ -205,6 +202,25 @@ void InstrProfWriter::setOutputSparse(bool Sparse) {
   this->Sparse = Sparse;
 }
 
+void InstrProfWriter::setProfileVersion(uint32_t Version) {
+  ProfileVersion = std::make_optional(Version);
+}
+
+void InstrProfWriter::setMinCompatibleReaderProfileVersion(uint32_t Version) {
+  MinCompatibleReaderProfileVersion = std::make_optional(Version);
+}
+
+void InstrProfWriter::setAppendAdditionalHeaderFields() {
+  AppendAdditionalHeaderFields = true;
+}
+
+void InstrProfWriter::resetTestOnlyStatesForHeaderSection() {
+  ProfileVersion = std::nullopt;
+  MinCompatibleReaderProfileVersion = std::nullopt;
+  AppendAdditionalHeaderFields = false;
+}
+// End: Internal interface for testing purpose only.
+
 void InstrProfWriter::addRecord(NamedInstrProfRecord &&I, uint64_t Weight,
                                 function_ref<void(Error)> Warn) {
   auto Name = I.Name;
@@ -555,6 +571,24 @@ static Error writeMemProf(
               memprof::MaximumSupportedVersion));
 }
 
+uint32_t InstrProfWriter::profileVersion() const {
+  // ProfileVersion is set in tests only.
+  if (this->ProfileVersion)
+    return *(this->ProfileVersion);
+
+  uint64_t CurVersion = IndexedInstrProf::ProfVersion::CurrentVersion;
+
+  return WritePrevVersion ? CurVersion - 1 : CurVersion;
+}
+
+uint64_t InstrProfWriter::minProfileReaderVersion() const {
+  // MinCompatibleReaderProfileVersion is set in tests only.
+  if (this->MinCompatibleReaderProfileVersion)
+    return *(this->MinCompatibleReaderProfileVersion);
+
+  return IndexedInstrProf::ProfVersion::Version13;
+}
+
 Error InstrProfWriter::writeImpl(ProfOStream &OS) {
   using namespace IndexedInstrProf;
   using namespace support;
@@ -578,19 +612,8 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
   // Write the header.
   IndexedInstrProf::Header Header;
   Header.Magic = IndexedInstrProf::Magic;
-  Header.Version = WritePrevVersion
-                       ? IndexedInstrProf::ProfVersion::Version12
-                       : IndexedInstrProf::ProfVersion::CurrentVersion;
-  if (WriteFutureVersion)
-    Header.Version = 14;
-  // The WritePrevVersion handling will either need to be removed or updated
-  // if the version is advanced beyond 13.
-  // Starting from version 13, an indexed profile records the on-disk
-  // byte size of header at a fixed byte offset. Compilers or tools built
-  // starting from this version can read the on-disk byte size (as opposed to
-  // relying on struct definition of Header) to locate the start of payload
-  // sections.
-  // FIXME: Clean up WritePrevVersion later.
+  Header.Version = profileVersion();
+
   assert(IndexedInstrProf::ProfVersion::CurrentVersion ==
          IndexedInstrProf::ProfVersion::Version13);
   if (static_cast<bool>(ProfileKind & InstrProfKind::IRInstrumentation))
@@ -647,24 +670,26 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
   uint64_t VTableNamesOffset = OS.tell();
   OS.write(0);
 
-  // Record the offset of 'Header::Size' field and reserve the space to allow
-  // back patching later.
+  // Record the offset of 'Header::Size' field.
   const uint64_t OnDiskHeaderSizeOffset = OS.tell();
-  if (!WritePrevVersion)
+
+  if (!WritePrevVersion) {
+    // Reserve the space for `OnDiskByteSize` to allow back patching later.
     OS.write(0);
 
-  // Write a dummy value
-  if (WriteFutureVersion)
+    // Write the minimum compatible version required.
+    OS.write(minProfileReaderVersion());
+  }
+
+  // This is a test-only path to append dummy header fields.
+  // NOTE: please write all other header fields before this one.
+  if (AppendAdditionalHeaderFields)
     OS.write(0);
 
   // Record the OnDiskHeader Size after each header field either gets written or
   // gets its space reserved.
   uint64_t OnDiskHeaderSize = OS.tell() - StartOffset;
 
-  llvm::errs() << "OnDiskHeaderSize is " << OnDiskHeaderSize << "\n";
-  fflush(stdout);
-  fflush(stderr);
-
   // Reserve space to write profile summary data.
   uint32_t NumEntries = ProfileSummaryBuilder::DefaultCutoffs.size();
   uint32_t SummarySize = Summary::getSize(Summary::NumKinds, NumEntries);