[llvm] c723cc2 - [Remarks] BitstreamRemarkParser: Refactor error handling (#156511)

via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 15 07:22:05 PDT 2025


Author: Tobias Stadler
Date: 2025-09-15T16:22:00+02:00
New Revision: c723cc2a041d6e7e741b0ce6abc1f18d4ada9b4a

URL: https://github.com/llvm/llvm-project/commit/c723cc2a041d6e7e741b0ce6abc1f18d4ada9b4a
DIFF: https://github.com/llvm/llvm-project/commit/c723cc2a041d6e7e741b0ce6abc1f18d4ada9b4a.diff

LOG: [Remarks] BitstreamRemarkParser: Refactor error handling (#156511)

In preparation of larger changes to the bitstream remark format,
refactor the error handling code in the BitstreamRemarkParser.

Main change: move the various static helper methods into the parser
helper classes, so we don't need to pass around as many args. Calling
`error(...)` inside the helper classes now automatically prepends the
current block being parsed to the error message.

NFCI (except for error messages on invalid bitstream files).

Pull Request: https://github.com/llvm/llvm-project/pull/156511

Added: 
    

Modified: 
    llvm/lib/Remarks/BitstreamRemarkParser.cpp
    llvm/lib/Remarks/BitstreamRemarkParser.h

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Remarks/BitstreamRemarkParser.cpp b/llvm/lib/Remarks/BitstreamRemarkParser.cpp
index 86a6c6dffb187..d40b40dfb2ba0 100644
--- a/llvm/lib/Remarks/BitstreamRemarkParser.cpp
+++ b/llvm/lib/Remarks/BitstreamRemarkParser.cpp
@@ -12,7 +12,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "BitstreamRemarkParser.h"
-#include "llvm/Remarks/Remark.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include <optional>
@@ -20,27 +19,68 @@
 using namespace llvm;
 using namespace llvm::remarks;
 
-static Error unknownRecord(const char *BlockName, unsigned RecordID) {
-  return createStringError(
-      std::make_error_code(std::errc::illegal_byte_sequence),
-      "Error while parsing %s: unknown record entry (%lu).", BlockName,
-      RecordID);
+namespace {
+
+template <typename... Ts> Error error(char const *Fmt, const Ts &...Vals) {
+  std::string Buffer;
+  raw_string_ostream OS(Buffer);
+  OS << formatv(Fmt, Vals...);
+  return make_error<StringError>(
+      std::move(Buffer),
+      std::make_error_code(std::errc::illegal_byte_sequence));
+}
+
+} // namespace
+
+Error BitstreamBlockParserHelperBase::unknownRecord(unsigned AbbrevID) {
+  return error("Unknown record entry ({}).", AbbrevID);
+}
+
+Error BitstreamBlockParserHelperBase::unexpectedRecord(StringRef RecordName) {
+  return error("Unexpected record entry ({}).", RecordName);
+}
+
+Error BitstreamBlockParserHelperBase::malformedRecord(StringRef RecordName) {
+  return error("Malformed record entry ({}).", RecordName);
+}
+
+Error BitstreamBlockParserHelperBase::unexpectedBlock(unsigned Code) {
+  return error("Unexpected subblock ({}).", Code);
 }
 
-static Error malformedRecord(const char *BlockName, const char *RecordName) {
-  return createStringError(
-      std::make_error_code(std::errc::illegal_byte_sequence),
-      "Error while parsing %s: malformed record entry (%s).", BlockName,
-      RecordName);
+static Expected<unsigned> expectSubBlock(BitstreamCursor &Stream) {
+  Expected<BitstreamEntry> Next = Stream.advance();
+  if (!Next)
+    return Next.takeError();
+  switch (Next->Kind) {
+  case BitstreamEntry::SubBlock:
+    return Next->ID;
+  case BitstreamEntry::Record:
+  case BitstreamEntry::EndBlock:
+    return error("Expected subblock, but got unexpected record.");
+  case BitstreamEntry::Error:
+    return error("Expected subblock, but got unexpected end of bitstream.");
+  }
+  llvm_unreachable("Unexpected BitstreamEntry");
 }
 
-BitstreamMetaParserHelper::BitstreamMetaParserHelper(
-    BitstreamCursor &Stream, BitstreamBlockInfo &BlockInfo)
-    : Stream(Stream), BlockInfo(BlockInfo) {}
+Error BitstreamBlockParserHelperBase::expectBlock() {
+  auto MaybeBlockID = expectSubBlock(Stream);
+  if (!MaybeBlockID)
+    return MaybeBlockID.takeError();
+  if (*MaybeBlockID != BlockID)
+    return error("Expected {} block, but got unexpected block ({}).", BlockName,
+                 *MaybeBlockID);
+  return Error::success();
+}
 
-/// Parse a record and fill in the fields in the parser.
-static Error parseRecord(BitstreamMetaParserHelper &Parser, unsigned Code) {
-  BitstreamCursor &Stream = Parser.Stream;
+Error BitstreamBlockParserHelperBase::enterBlock() {
+  if (Stream.EnterSubBlock(BlockID))
+    return error("Error while entering {} block.", BlockName);
+  return Error::success();
+}
+
+Error BitstreamMetaParserHelper::parseRecord(unsigned Code) {
   // Note: 2 is used here because it's the max number of fields we have per
   // record.
   SmallVector<uint64_t, 2> Record;
@@ -52,171 +92,132 @@ static Error parseRecord(BitstreamMetaParserHelper &Parser, unsigned Code) {
   switch (*RecordID) {
   case RECORD_META_CONTAINER_INFO: {
     if (Record.size() != 2)
-      return malformedRecord("BLOCK_META", "RECORD_META_CONTAINER_INFO");
-    Parser.ContainerVersion = Record[0];
-    Parser.ContainerType = Record[1];
+      return malformedRecord(MetaContainerInfoName);
+    Container = {Record[0], Record[1]};
+    // Error immediately if container version is outdated, so the user sees an
+    // explanation instead of a parser error.
+    if (Container->Version != CurrentContainerVersion) {
+      return ::error(
+          "Unsupported remark container version (expected: {}, read: {}). "
+          "Please upgrade/downgrade your toolchain to read this container.",
+          CurrentContainerVersion, Container->Version);
+    }
     break;
   }
   case RECORD_META_REMARK_VERSION: {
     if (Record.size() != 1)
-      return malformedRecord("BLOCK_META", "RECORD_META_REMARK_VERSION");
-    Parser.RemarkVersion = Record[0];
+      return malformedRecord(MetaRemarkVersionName);
+    RemarkVersion = Record[0];
+    // Error immediately if remark version is outdated, so the user sees an
+    // explanation instead of a parser error.
+    if (*RemarkVersion != CurrentRemarkVersion) {
+      return ::error(
+          "Unsupported remark version in container (expected: {}, read: {}). "
+          "Please upgrade/downgrade your toolchain to read this container.",
+          CurrentRemarkVersion, *RemarkVersion);
+    }
     break;
   }
   case RECORD_META_STRTAB: {
     if (Record.size() != 0)
-      return malformedRecord("BLOCK_META", "RECORD_META_STRTAB");
-    Parser.StrTabBuf = Blob;
+      return malformedRecord(MetaStrTabName);
+    StrTabBuf = Blob;
     break;
   }
   case RECORD_META_EXTERNAL_FILE: {
     if (Record.size() != 0)
-      return malformedRecord("BLOCK_META", "RECORD_META_EXTERNAL_FILE");
-    Parser.ExternalFilePath = Blob;
+      return malformedRecord(MetaExternalFileName);
+    ExternalFilePath = Blob;
     break;
   }
   default:
-    return unknownRecord("BLOCK_META", *RecordID);
+    return unknownRecord(*RecordID);
   }
   return Error::success();
 }
 
-BitstreamRemarkParserHelper::BitstreamRemarkParserHelper(
-    BitstreamCursor &Stream)
-    : Stream(Stream) {}
-
-/// Parse a record and fill in the fields in the parser.
-static Error parseRecord(BitstreamRemarkParserHelper &Parser, unsigned Code) {
-  BitstreamCursor &Stream = Parser.Stream;
-  // Note: 5 is used here because it's the max number of fields we have per
-  // record.
-  SmallVector<uint64_t, 5> Record;
-  StringRef Blob;
-  Expected<unsigned> RecordID = Stream.readRecord(Code, Record, &Blob);
-  if (!RecordID)
-    return RecordID.takeError();
+Error BitstreamRemarkParserHelper::parseRecord(unsigned Code) {
+  Record.clear();
+  Expected<unsigned> MaybeRecordID =
+      Stream.readRecord(Code, Record, &RecordBlob);
+  if (!MaybeRecordID)
+    return MaybeRecordID.takeError();
+  RecordID = *MaybeRecordID;
+  return handleRecord();
+}
 
-  switch (*RecordID) {
+Error BitstreamRemarkParserHelper::handleRecord() {
+  switch (RecordID) {
   case RECORD_REMARK_HEADER: {
     if (Record.size() != 4)
-      return malformedRecord("BLOCK_REMARK", "RECORD_REMARK_HEADER");
-    Parser.Type = Record[0];
-    Parser.RemarkNameIdx = Record[1];
-    Parser.PassNameIdx = Record[2];
-    Parser.FunctionNameIdx = Record[3];
+      return malformedRecord(RemarkHeaderName);
+    Type = Record[0];
+    RemarkNameIdx = Record[1];
+    PassNameIdx = Record[2];
+    FunctionNameIdx = Record[3];
     break;
   }
   case RECORD_REMARK_DEBUG_LOC: {
     if (Record.size() != 3)
-      return malformedRecord("BLOCK_REMARK", "RECORD_REMARK_DEBUG_LOC");
-    Parser.SourceFileNameIdx = Record[0];
-    Parser.SourceLine = Record[1];
-    Parser.SourceColumn = Record[2];
+      return malformedRecord(RemarkDebugLocName);
+    Loc = {Record[0], Record[1], Record[2]};
     break;
   }
   case RECORD_REMARK_HOTNESS: {
     if (Record.size() != 1)
-      return malformedRecord("BLOCK_REMARK", "RECORD_REMARK_HOTNESS");
-    Parser.Hotness = Record[0];
+      return malformedRecord(RemarkHotnessName);
+    Hotness = Record[0];
     break;
   }
   case RECORD_REMARK_ARG_WITH_DEBUGLOC: {
     if (Record.size() != 5)
-      return malformedRecord("BLOCK_REMARK", "RECORD_REMARK_ARG_WITH_DEBUGLOC");
-    // Create a temporary argument. Use that as a valid memory location for this
-    // argument entry.
-    Parser.TmpArgs.emplace_back();
-    Parser.TmpArgs.back().KeyIdx = Record[0];
-    Parser.TmpArgs.back().ValueIdx = Record[1];
-    Parser.TmpArgs.back().SourceFileNameIdx = Record[2];
-    Parser.TmpArgs.back().SourceLine = Record[3];
-    Parser.TmpArgs.back().SourceColumn = Record[4];
-    Parser.Args =
-        ArrayRef<BitstreamRemarkParserHelper::Argument>(Parser.TmpArgs);
+      return malformedRecord(RemarkArgWithDebugLocName);
+    auto &Arg = Args.emplace_back(Record[0], Record[1]);
+    Arg.Loc = {Record[2], Record[3], Record[4]};
     break;
   }
   case RECORD_REMARK_ARG_WITHOUT_DEBUGLOC: {
     if (Record.size() != 2)
-      return malformedRecord("BLOCK_REMARK",
-                             "RECORD_REMARK_ARG_WITHOUT_DEBUGLOC");
-    // Create a temporary argument. Use that as a valid memory location for this
-    // argument entry.
-    Parser.TmpArgs.emplace_back();
-    Parser.TmpArgs.back().KeyIdx = Record[0];
-    Parser.TmpArgs.back().ValueIdx = Record[1];
-    Parser.Args =
-        ArrayRef<BitstreamRemarkParserHelper::Argument>(Parser.TmpArgs);
+      return malformedRecord(RemarkArgWithoutDebugLocName);
+    Args.emplace_back(Record[0], Record[1]);
     break;
   }
   default:
-    return unknownRecord("BLOCK_REMARK", *RecordID);
+    return unknownRecord(RecordID);
   }
   return Error::success();
 }
 
-template <typename T>
-static Error parseBlock(T &ParserHelper, unsigned BlockID,
-                        const char *BlockName) {
-  BitstreamCursor &Stream = ParserHelper.Stream;
-  Expected<BitstreamEntry> Next = Stream.advance();
-  if (!Next)
-    return Next.takeError();
-  if (Next->Kind != BitstreamEntry::SubBlock || Next->ID != BlockID)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing %s: expecting [ENTER_SUBBLOCK, %s, ...].",
-        BlockName, BlockName);
-  if (Stream.EnterSubBlock(BlockID))
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while entering %s.", BlockName);
-
-  // Stop when there is nothing to read anymore or when we encounter an
-  // END_BLOCK.
-  while (!Stream.AtEndOfStream()) {
-    Next = Stream.advance();
-    if (!Next)
-      return Next.takeError();
-    switch (Next->Kind) {
-    case BitstreamEntry::EndBlock:
-      return Error::success();
-    case BitstreamEntry::Error:
-    case BitstreamEntry::SubBlock:
-      return createStringError(
-          std::make_error_code(std::errc::illegal_byte_sequence),
-          "Error while parsing %s: expecting records.", BlockName);
-    case BitstreamEntry::Record:
-      if (Error E = parseRecord(ParserHelper, Next->ID))
-        return E;
-      continue;
-    }
-  }
-  // If we're here, it means we didn't get an END_BLOCK yet, but we're at the
-  // end of the stream. In this case, error.
-  return createStringError(
-      std::make_error_code(std::errc::illegal_byte_sequence),
-      "Error while parsing %s: unterminated block.", BlockName);
-}
-
-Error BitstreamMetaParserHelper::parse() {
-  return parseBlock(*this, META_BLOCK_ID, "META_BLOCK");
-}
+Error BitstreamRemarkParserHelper::parseNext() {
+  Type.reset();
+  RemarkNameIdx.reset();
+  PassNameIdx.reset();
+  FunctionNameIdx.reset();
+  Hotness.reset();
+  Loc.reset();
+  Args.clear();
 
-Error BitstreamRemarkParserHelper::parse() {
-  return parseBlock(*this, REMARK_BLOCK_ID, "REMARK_BLOCK");
+  if (Error E = expectBlock())
+    return E;
+  return parseBlock();
 }
 
 BitstreamParserHelper::BitstreamParserHelper(StringRef Buffer)
     : Stream(Buffer) {}
 
-Expected<std::array<char, 4>> BitstreamParserHelper::parseMagic() {
+Error BitstreamParserHelper::expectMagic() {
   std::array<char, 4> Result;
-  for (unsigned i = 0; i < 4; ++i)
+  for (unsigned I = 0; I < 4; ++I)
     if (Expected<unsigned> R = Stream.Read(8))
-      Result[i] = *R;
+      Result[I] = *R;
     else
       return R.takeError();
-  return Result;
+
+  StringRef MagicNumber{Result.data(), Result.size()};
+  if (MagicNumber != remarks::ContainerMagic)
+    return error("Unknown magic number: expecting {}, got {}.",
+                 remarks::ContainerMagic, MagicNumber);
+  return Error::success();
 }
 
 Error BitstreamParserHelper::parseBlockInfoBlock() {
@@ -225,8 +226,7 @@ Error BitstreamParserHelper::parseBlockInfoBlock() {
     return Next.takeError();
   if (Next->Kind != BitstreamEntry::SubBlock ||
       Next->ID != llvm::bitc::BLOCKINFO_BLOCK_ID)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
+    return error(
         "Error while parsing BLOCKINFO_BLOCK: expecting [ENTER_SUBBLOCK, "
         "BLOCKINFO_BLOCK, ...].");
 
@@ -236,9 +236,7 @@ Error BitstreamParserHelper::parseBlockInfoBlock() {
     return MaybeBlockInfo.takeError();
 
   if (!*MaybeBlockInfo)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCKINFO_BLOCK.");
+    return error("Missing BLOCKINFO_BLOCK.");
 
   BlockInfo = **MaybeBlockInfo;
 
@@ -246,77 +244,17 @@ Error BitstreamParserHelper::parseBlockInfoBlock() {
   return Error::success();
 }
 
-static Expected<bool> isBlock(BitstreamCursor &Stream, unsigned BlockID) {
-  bool Result = false;
-  uint64_t PreviousBitNo = Stream.GetCurrentBitNo();
-  Expected<BitstreamEntry> Next = Stream.advance();
-  if (!Next)
-    return Next.takeError();
-  switch (Next->Kind) {
-  case BitstreamEntry::SubBlock:
-    // Check for the block id.
-    Result = Next->ID == BlockID;
-    break;
-  case BitstreamEntry::Error:
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Unexpected error while parsing bitstream.");
-  default:
-    Result = false;
-    break;
-  }
-  if (Error E = Stream.JumpToBit(PreviousBitNo))
-    return std::move(E);
-  return Result;
-}
-
-Expected<bool> BitstreamParserHelper::isMetaBlock() {
-  return isBlock(Stream, META_BLOCK_ID);
-}
-
-Expected<bool> BitstreamParserHelper::isRemarkBlock() {
-  return isBlock(Stream, META_BLOCK_ID);
-}
-
-static Error validateMagicNumber(StringRef MagicNumber) {
-  if (MagicNumber != remarks::ContainerMagic)
-    return createStringError(std::make_error_code(std::errc::invalid_argument),
-                             "Unknown magic number: expecting %s, got %.4s.",
-                             remarks::ContainerMagic.data(), MagicNumber.data());
-  return Error::success();
-}
-
-static Error advanceToMetaBlock(BitstreamParserHelper &Helper) {
-  Expected<std::array<char, 4>> MagicNumber = Helper.parseMagic();
-  if (!MagicNumber)
-    return MagicNumber.takeError();
-  if (Error E = validateMagicNumber(
-          StringRef(MagicNumber->data(), MagicNumber->size())))
+Error BitstreamParserHelper::advanceToMetaBlock() {
+  if (Error E = expectMagic())
     return E;
-  if (Error E = Helper.parseBlockInfoBlock())
+  if (Error E = parseBlockInfoBlock())
     return E;
-  Expected<bool> isMetaBlock = Helper.isMetaBlock();
-  if (!isMetaBlock)
-    return isMetaBlock.takeError();
-  if (!*isMetaBlock)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Expecting META_BLOCK after the BLOCKINFO_BLOCK.");
   return Error::success();
 }
 
 Expected<std::unique_ptr<BitstreamRemarkParser>>
 remarks::createBitstreamParserFromMeta(
     StringRef Buf, std::optional<StringRef> ExternalFilePrependPath) {
-  BitstreamParserHelper Helper(Buf);
-  Expected<std::array<char, 4>> MagicNumber = Helper.parseMagic();
-  if (!MagicNumber)
-    return MagicNumber.takeError();
-
-  if (Error E = validateMagicNumber(
-          StringRef(MagicNumber->data(), MagicNumber->size())))
-    return std::move(E);
-
   auto Parser = std::make_unique<BitstreamRemarkParser>(Buf);
 
   if (ExternalFilePrependPath)
@@ -339,13 +277,13 @@ Expected<std::unique_ptr<Remark>> BitstreamRemarkParser::next() {
 }
 
 Error BitstreamRemarkParser::parseMeta() {
-  // Advance and to the meta block.
-  if (Error E = advanceToMetaBlock(ParserHelper))
+  if (Error E = ParserHelper.advanceToMetaBlock())
     return E;
 
-  BitstreamMetaParserHelper MetaHelper(ParserHelper.Stream,
-                                       ParserHelper.BlockInfo);
-  if (Error E = MetaHelper.parse())
+  BitstreamMetaParserHelper MetaHelper(ParserHelper.Stream);
+  if (Error E = MetaHelper.expectBlock())
+    return E;
+  if (Error E = MetaHelper.parseBlock())
     return E;
 
   if (Error E = processCommonMeta(MetaHelper))
@@ -364,59 +302,41 @@ Error BitstreamRemarkParser::parseMeta() {
 
 Error BitstreamRemarkParser::processCommonMeta(
     BitstreamMetaParserHelper &Helper) {
-  if (std::optional<uint64_t> Version = Helper.ContainerVersion)
-    ContainerVersion = *Version;
-  else
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCK_META: missing container version.");
-
-  if (std::optional<uint8_t> Type = Helper.ContainerType) {
-    // Always >= BitstreamRemarkContainerType::First since it's unsigned.
-    if (*Type > static_cast<uint8_t>(BitstreamRemarkContainerType::Last))
-      return createStringError(
-          std::make_error_code(std::errc::illegal_byte_sequence),
-          "Error while parsing BLOCK_META: invalid container type.");
-
-    ContainerType = static_cast<BitstreamRemarkContainerType>(*Type);
-  } else
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCK_META: missing container type.");
-
+  if (!Helper.Container)
+    return Helper.error("Missing container info.");
+  auto &Container = *Helper.Container;
+  ContainerVersion = Container.Version;
+  // Always >= BitstreamRemarkContainerType::First since it's unsigned.
+  if (Container.Type > static_cast<uint8_t>(BitstreamRemarkContainerType::Last))
+    return Helper.error("Invalid container type.");
+  ContainerType = static_cast<BitstreamRemarkContainerType>(Container.Type);
   return Error::success();
 }
 
-static Error processStrTab(BitstreamRemarkParser &P,
-                           std::optional<StringRef> StrTabBuf) {
-  if (!StrTabBuf)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCK_META: missing string table.");
+Error BitstreamRemarkParser::processStrTab(BitstreamMetaParserHelper &Helper) {
+  if (!Helper.StrTabBuf)
+    return Helper.error("Missing string table.");
   // Parse and assign the string table.
-  P.StrTab.emplace(*StrTabBuf);
+  StrTab.emplace(*Helper.StrTabBuf);
   return Error::success();
 }
 
-static Error processRemarkVersion(BitstreamRemarkParser &P,
-                                  std::optional<uint64_t> RemarkVersion) {
-  if (!RemarkVersion)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCK_META: missing remark version.");
-  P.RemarkVersion = *RemarkVersion;
+Error BitstreamRemarkParser::processRemarkVersion(
+    BitstreamMetaParserHelper &Helper) {
+  if (!Helper.RemarkVersion)
+    return Helper.error("Missing remark version.");
+  RemarkVersion = *Helper.RemarkVersion;
   return Error::success();
 }
 
 Error BitstreamRemarkParser::processExternalFilePath(
-    std::optional<StringRef> ExternalFilePath) {
-  if (!ExternalFilePath)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCK_META: missing external file path.");
+    BitstreamMetaParserHelper &Helper) {
+  if (!Helper.ExternalFilePath)
+    return Helper.error("Missing external file path.");
+  StringRef ExternalFilePath = *Helper.ExternalFilePath;
 
   SmallString<80> FullPath(ExternalFilePrependPath);
-  sys::path::append(FullPath, *ExternalFilePath);
+  sys::path::append(FullPath, ExternalFilePath);
 
   // External file: open the external file, parse it, check if its metadata
   // matches the one from the separate metadata, then replace the current parser
@@ -435,32 +355,22 @@ Error BitstreamRemarkParser::processExternalFilePath(
   // Create a separate parser used for parsing the separate file.
   ParserHelper = BitstreamParserHelper(TmpRemarkBuffer->getBuffer());
   // Advance and check until we can parse the meta block.
-  if (Error E = advanceToMetaBlock(ParserHelper))
+  if (Error E = ParserHelper.advanceToMetaBlock())
     return E;
   // Parse the meta from the separate file.
   // Note: here we overwrite the BlockInfo with the one from the file. This will
   // be used to parse the rest of the file.
-  BitstreamMetaParserHelper SeparateMetaHelper(ParserHelper.Stream,
-                                               ParserHelper.BlockInfo);
-  if (Error E = SeparateMetaHelper.parse())
+  BitstreamMetaParserHelper SeparateMetaHelper(ParserHelper.Stream);
+  if (Error E = SeparateMetaHelper.expectBlock())
+    return E;
+  if (Error E = SeparateMetaHelper.parseBlock())
     return E;
 
-  uint64_t PreviousContainerVersion = ContainerVersion;
   if (Error E = processCommonMeta(SeparateMetaHelper))
     return E;
 
   if (ContainerType != BitstreamRemarkContainerType::SeparateRemarksFile)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing external file's BLOCK_META: wrong container "
-        "type.");
-
-  if (PreviousContainerVersion != ContainerVersion)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing external file's BLOCK_META: mismatching versions: "
-        "original meta: %lu, external file meta: %lu.",
-        PreviousContainerVersion, ContainerVersion);
+    return SeparateMetaHelper.error("Wrong container type in external file.");
 
   // Process the meta from the separate file.
   return processSeparateRemarksFileMeta(SeparateMetaHelper);
@@ -468,26 +378,26 @@ Error BitstreamRemarkParser::processExternalFilePath(
 
 Error BitstreamRemarkParser::processStandaloneMeta(
     BitstreamMetaParserHelper &Helper) {
-  if (Error E = processStrTab(*this, Helper.StrTabBuf))
+  if (Error E = processStrTab(Helper))
     return E;
-  return processRemarkVersion(*this, Helper.RemarkVersion);
+  return processRemarkVersion(Helper);
 }
 
 Error BitstreamRemarkParser::processSeparateRemarksFileMeta(
     BitstreamMetaParserHelper &Helper) {
-  return processRemarkVersion(*this, Helper.RemarkVersion);
+  return processRemarkVersion(Helper);
 }
 
 Error BitstreamRemarkParser::processSeparateRemarksMetaMeta(
     BitstreamMetaParserHelper &Helper) {
-  if (Error E = processStrTab(*this, Helper.StrTabBuf))
+  if (Error E = processStrTab(Helper))
     return E;
-  return processExternalFilePath(Helper.ExternalFilePath);
+  return processExternalFilePath(Helper);
 }
 
 Expected<std::unique_ptr<Remark>> BitstreamRemarkParser::parseRemark() {
   BitstreamRemarkParserHelper RemarkHelper(ParserHelper.Stream);
-  if (Error E = RemarkHelper.parse())
+  if (Error E = RemarkHelper.parseNext())
     return std::move(E);
 
   return processRemark(RemarkHelper);
@@ -498,28 +408,20 @@ BitstreamRemarkParser::processRemark(BitstreamRemarkParserHelper &Helper) {
   std::unique_ptr<Remark> Result = std::make_unique<Remark>();
   Remark &R = *Result;
 
-  if (StrTab == std::nullopt)
-    return createStringError(
-        std::make_error_code(std::errc::invalid_argument),
-        "Error while parsing BLOCK_REMARK: missing string table.");
+  if (!StrTab)
+    return Helper.error("Missing string table.");
 
   if (!Helper.Type)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCK_REMARK: missing remark type.");
+    return Helper.error("Missing remark type.");
 
   // Always >= Type::First since it's unsigned.
   if (*Helper.Type > static_cast<uint8_t>(Type::Last))
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCK_REMARK: unknown remark type.");
+    return Helper.error("Unknown remark type.");
 
   R.RemarkType = static_cast<Type>(*Helper.Type);
 
   if (!Helper.RemarkNameIdx)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCK_REMARK: missing remark name.");
+    return Helper.error("Missing remark name.");
 
   if (Expected<StringRef> RemarkName = (*StrTab)[*Helper.RemarkNameIdx])
     R.RemarkName = *RemarkName;
@@ -527,9 +429,7 @@ BitstreamRemarkParser::processRemark(BitstreamRemarkParserHelper &Helper) {
     return RemarkName.takeError();
 
   if (!Helper.PassNameIdx)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCK_REMARK: missing remark pass.");
+    return Helper.error("Missing remark pass.");
 
   if (Expected<StringRef> PassName = (*StrTab)[*Helper.PassNameIdx])
     R.PassName = *PassName;
@@ -537,61 +437,53 @@ BitstreamRemarkParser::processRemark(BitstreamRemarkParserHelper &Helper) {
     return PassName.takeError();
 
   if (!Helper.FunctionNameIdx)
-    return createStringError(
-        std::make_error_code(std::errc::illegal_byte_sequence),
-        "Error while parsing BLOCK_REMARK: missing remark function name.");
+    return Helper.error("Missing remark function name.");
+
   if (Expected<StringRef> FunctionName = (*StrTab)[*Helper.FunctionNameIdx])
     R.FunctionName = *FunctionName;
   else
     return FunctionName.takeError();
 
-  if (Helper.SourceFileNameIdx && Helper.SourceLine && Helper.SourceColumn) {
-    Expected<StringRef> SourceFileName = (*StrTab)[*Helper.SourceFileNameIdx];
+  if (Helper.Loc) {
+    Expected<StringRef> SourceFileName =
+        (*StrTab)[Helper.Loc->SourceFileNameIdx];
     if (!SourceFileName)
       return SourceFileName.takeError();
     R.Loc.emplace();
     R.Loc->SourceFilePath = *SourceFileName;
-    R.Loc->SourceLine = *Helper.SourceLine;
-    R.Loc->SourceColumn = *Helper.SourceColumn;
+    R.Loc->SourceLine = Helper.Loc->SourceLine;
+    R.Loc->SourceColumn = Helper.Loc->SourceColumn;
   }
 
   if (Helper.Hotness)
     R.Hotness = *Helper.Hotness;
 
-  if (!Helper.Args)
-    return std::move(Result);
-
-  for (const BitstreamRemarkParserHelper::Argument &Arg : *Helper.Args) {
+  for (const BitstreamRemarkParserHelper::Argument &Arg : Helper.Args) {
     if (!Arg.KeyIdx)
-      return createStringError(
-          std::make_error_code(std::errc::illegal_byte_sequence),
-          "Error while parsing BLOCK_REMARK: missing key in remark argument.");
+      return Helper.error("Missing key in remark argument.");
     if (!Arg.ValueIdx)
-      return createStringError(
-          std::make_error_code(std::errc::illegal_byte_sequence),
-          "Error while parsing BLOCK_REMARK: missing value in remark "
-          "argument.");
+      return Helper.error("Missing value in remark argument.");
 
     // We have at least a key and a value, create an entry.
-    R.Args.emplace_back();
+    auto &RArg = R.Args.emplace_back();
 
     if (Expected<StringRef> Key = (*StrTab)[*Arg.KeyIdx])
-      R.Args.back().Key = *Key;
+      RArg.Key = *Key;
     else
       return Key.takeError();
 
     if (Expected<StringRef> Value = (*StrTab)[*Arg.ValueIdx])
-      R.Args.back().Val = *Value;
+      RArg.Val = *Value;
     else
       return Value.takeError();
 
-    if (Arg.SourceFileNameIdx && Arg.SourceLine && Arg.SourceColumn) {
+    if (Arg.Loc) {
       if (Expected<StringRef> SourceFileName =
-              (*StrTab)[*Arg.SourceFileNameIdx]) {
-        R.Args.back().Loc.emplace();
-        R.Args.back().Loc->SourceFilePath = *SourceFileName;
-        R.Args.back().Loc->SourceLine = *Arg.SourceLine;
-        R.Args.back().Loc->SourceColumn = *Arg.SourceColumn;
+              (*StrTab)[Arg.Loc->SourceFileNameIdx]) {
+        RArg.Loc.emplace();
+        RArg.Loc->SourceFilePath = *SourceFileName;
+        RArg.Loc->SourceLine = Arg.Loc->SourceLine;
+        RArg.Loc->SourceColumn = Arg.Loc->SourceColumn;
       } else
         return SourceFileName.takeError();
     }

diff  --git a/llvm/lib/Remarks/BitstreamRemarkParser.h b/llvm/lib/Remarks/BitstreamRemarkParser.h
index cba805dc24b59..257ac46eb9495 100644
--- a/llvm/lib/Remarks/BitstreamRemarkParser.h
+++ b/llvm/lib/Remarks/BitstreamRemarkParser.h
@@ -13,14 +13,15 @@
 #ifndef LLVM_LIB_REMARKS_BITSTREAM_REMARK_PARSER_H
 #define LLVM_LIB_REMARKS_BITSTREAM_REMARK_PARSER_H
 
-#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Bitstream/BitstreamReader.h"
 #include "llvm/Remarks/BitstreamRemarkContainer.h"
+#include "llvm/Remarks/Remark.h"
 #include "llvm/Remarks/RemarkFormat.h"
 #include "llvm/Remarks/RemarkParser.h"
+#include "llvm/Remarks/RemarkStringTable.h"
 #include "llvm/Support/Error.h"
-#include <array>
+#include "llvm/Support/FormatVariadic.h"
 #include <cstdint>
 #include <memory>
 #include <optional>
@@ -28,66 +29,156 @@
 namespace llvm {
 namespace remarks {
 
-struct Remark;
+class BitstreamBlockParserHelperBase {
+protected:
+  BitstreamCursor &Stream;
+
+  StringRef BlockName;
+  unsigned BlockID;
+
+public:
+  BitstreamBlockParserHelperBase(BitstreamCursor &Stream, unsigned BlockID,
+                                 StringRef BlockName)
+      : Stream(Stream), BlockName(BlockName), BlockID(BlockID) {}
+
+  template <typename... Ts> Error error(char const *Fmt, const Ts &...Vals) {
+    std::string Buffer;
+    raw_string_ostream OS(Buffer);
+    OS << "Error while parsing " << BlockName << " block: ";
+    OS << formatv(Fmt, Vals...);
+    return make_error<StringError>(
+        std::move(Buffer),
+        std::make_error_code(std::errc::illegal_byte_sequence));
+  }
+
+  Error expectBlock();
+
+protected:
+  Error enterBlock();
+
+  Error unknownRecord(unsigned AbbrevID);
+  Error unexpectedRecord(StringRef RecordName);
+  Error malformedRecord(StringRef RecordName);
+  Error unexpectedBlock(unsigned Code);
+};
+
+template <typename Derived>
+class BitstreamBlockParserHelper : public BitstreamBlockParserHelperBase {
+protected:
+  using BitstreamBlockParserHelperBase::BitstreamBlockParserHelperBase;
+  Derived &derived() { return *static_cast<Derived *>(this); }
+
+  /// Parse a record and fill in the fields in the parser.
+  /// The subclass can statically override this method.
+  Error parseRecord(unsigned Code) { return unexpectedRecord(Code); }
+
+  /// Parse a subblock and fill in the fields in the parser.
+  /// The subclass can statically override this method.
+  Error parseSubBlock(unsigned Code) { return unexpectedBlock(Code); }
+
+public:
+  /// Enter, parse, and leave this bitstream block. This expects the
+  /// BitstreamCursor to be right after the SubBlock entry (i.e. after calling
+  /// expectBlock).
+  Error parseBlock() {
+    if (Error E = enterBlock())
+      return E;
+
+    // Stop when there is nothing to read anymore or when we encounter an
+    // END_BLOCK.
+    while (true) {
+      Expected<BitstreamEntry> Next = Stream.advance();
+      if (!Next)
+        return Next.takeError();
+      switch (Next->Kind) {
+      case BitstreamEntry::SubBlock:
+        if (Error E = derived().parseSubBlock(Next->ID))
+          return E;
+        continue;
+      case BitstreamEntry::EndBlock:
+        return Error::success();
+      case BitstreamEntry::Record:
+        if (Error E = derived().parseRecord(Next->ID))
+          return E;
+        continue;
+      case BitstreamEntry::Error:
+        return error("Unexpected end of bitstream.");
+      }
+      llvm_unreachable("Unexpected BitstreamEntry");
+    }
+  }
+};
 
 /// Helper to parse a META_BLOCK for a bitstream remark container.
-struct BitstreamMetaParserHelper {
-  /// The Bitstream reader.
-  BitstreamCursor &Stream;
-  /// Reference to the storage for the block info.
-  BitstreamBlockInfo &BlockInfo;
-  /// The parsed content: depending on the container type, some fields might be
-  /// empty.
-  std::optional<uint64_t> ContainerVersion;
-  std::optional<uint8_t> ContainerType;
-  std::optional<StringRef> StrTabBuf;
-  std::optional<StringRef> ExternalFilePath;
+class BitstreamMetaParserHelper
+    : public BitstreamBlockParserHelper<BitstreamMetaParserHelper> {
+  friend class BitstreamBlockParserHelper;
+
+public:
+  struct ContainerInfo {
+    uint64_t Version;
+    uint64_t Type;
+  };
+
+  /// The parsed content: depending on the container type, some fields might
+  /// be empty.
+  std::optional<ContainerInfo> Container;
   std::optional<uint64_t> RemarkVersion;
+  std::optional<StringRef> ExternalFilePath;
+  std::optional<StringRef> StrTabBuf;
 
-  /// Continue parsing with \p Stream. \p Stream is expected to contain a
-  /// ENTER_SUBBLOCK to the META_BLOCK at the current position.
-  /// \p Stream is expected to have a BLOCKINFO_BLOCK set.
-  BitstreamMetaParserHelper(BitstreamCursor &Stream,
-                            BitstreamBlockInfo &BlockInfo);
+  BitstreamMetaParserHelper(BitstreamCursor &Stream)
+      : BitstreamBlockParserHelper(Stream, META_BLOCK_ID, MetaBlockName) {}
 
-  /// Parse the META_BLOCK and fill the available entries.
-  /// This helper does not check for the validity of the fields.
-  Error parse();
+protected:
+  Error parseRecord(unsigned Code);
 };
 
 /// Helper to parse a REMARK_BLOCK for a bitstream remark container.
-struct BitstreamRemarkParserHelper {
-  /// The Bitstream reader.
-  BitstreamCursor &Stream;
+class BitstreamRemarkParserHelper
+    : public BitstreamBlockParserHelper<BitstreamRemarkParserHelper> {
+  friend class BitstreamBlockParserHelper;
+
+protected:
+  SmallVector<uint64_t, 5> Record;
+  StringRef RecordBlob;
+  unsigned RecordID;
+
+public:
+  struct RemarkLoc {
+    uint64_t SourceFileNameIdx;
+    uint64_t SourceLine;
+    uint64_t SourceColumn;
+  };
+
+  struct Argument {
+    std::optional<uint64_t> KeyIdx;
+    std::optional<uint64_t> ValueIdx;
+    std::optional<RemarkLoc> Loc;
+
+    Argument(std::optional<uint64_t> KeyIdx, std::optional<uint64_t> ValueIdx)
+        : KeyIdx(KeyIdx), ValueIdx(ValueIdx) {}
+  };
+
   /// The parsed content: depending on the remark, some fields might be empty.
   std::optional<uint8_t> Type;
   std::optional<uint64_t> RemarkNameIdx;
   std::optional<uint64_t> PassNameIdx;
   std::optional<uint64_t> FunctionNameIdx;
-  std::optional<uint64_t> SourceFileNameIdx;
-  std::optional<uint32_t> SourceLine;
-  std::optional<uint32_t> SourceColumn;
   std::optional<uint64_t> Hotness;
-  struct Argument {
-    std::optional<uint64_t> KeyIdx;
-    std::optional<uint64_t> ValueIdx;
-    std::optional<uint64_t> SourceFileNameIdx;
-    std::optional<uint32_t> SourceLine;
-    std::optional<uint32_t> SourceColumn;
-  };
-  std::optional<ArrayRef<Argument>> Args;
-  /// Avoid re-allocating a vector every time.
-  SmallVector<Argument, 8> TmpArgs;
-
-  /// Continue parsing with \p Stream. \p Stream is expected to contain a
-  /// ENTER_SUBBLOCK to the REMARK_BLOCK at the current position.
-  /// \p Stream is expected to have a BLOCKINFO_BLOCK set and to have already
-  /// parsed the META_BLOCK.
-  BitstreamRemarkParserHelper(BitstreamCursor &Stream);
-
-  /// Parse the REMARK_BLOCK and fill the available entries.
-  /// This helper does not check for the validity of the fields.
-  Error parse();
+  std::optional<RemarkLoc> Loc;
+
+  SmallVector<Argument, 8> Args;
+
+  BitstreamRemarkParserHelper(BitstreamCursor &Stream)
+      : BitstreamBlockParserHelper(Stream, REMARK_BLOCK_ID, RemarkBlockName) {}
+
+  /// Clear helper state and parse next remark block.
+  Error parseNext();
+
+protected:
+  Error parseRecord(unsigned Code);
+  Error handleRecord();
 };
 
 /// Helper to parse any bitstream remark container.
@@ -98,21 +189,15 @@ struct BitstreamParserHelper {
   BitstreamBlockInfo BlockInfo;
   /// Start parsing at \p Buffer.
   BitstreamParserHelper(StringRef Buffer);
-  /// Parse the magic number.
-  Expected<std::array<char, 4>> parseMagic();
+  /// Parse and validate the magic number.
+  Error expectMagic();
+  /// Advance to the meta block
+  Error advanceToMetaBlock();
   /// Parse the block info block containing all the abbrevs.
   /// This needs to be called before calling any other parsing function.
   Error parseBlockInfoBlock();
-  /// Return true if the next block is a META_BLOCK. This function does not move
-  /// the cursor.
-  Expected<bool> isMetaBlock();
-  /// Return true if the next block is a REMARK_BLOCK. This function does not
-  /// move the cursor.
-  Expected<bool> isRemarkBlock();
   /// Return true if the parser reached the end of the stream.
   bool atEndOfStream() { return Stream.AtEndOfStream(); }
-  /// Jump to the end of the stream, skipping everything.
-  void skipToEnd() { return Stream.skipToEnd(); }
 };
 
 /// Parses and holds the state of the latest parsed remark.
@@ -149,14 +234,16 @@ struct BitstreamRemarkParser : public RemarkParser {
   Expected<std::unique_ptr<Remark>> parseRemark();
 
 private:
-  /// Helper functions.
   Error processCommonMeta(BitstreamMetaParserHelper &Helper);
   Error processStandaloneMeta(BitstreamMetaParserHelper &Helper);
   Error processSeparateRemarksFileMeta(BitstreamMetaParserHelper &Helper);
   Error processSeparateRemarksMetaMeta(BitstreamMetaParserHelper &Helper);
+  Error processExternalFilePath(BitstreamMetaParserHelper &Helper);
+  Error processStrTab(BitstreamMetaParserHelper &Helper);
+  Error processRemarkVersion(BitstreamMetaParserHelper &Helper);
+
   Expected<std::unique_ptr<Remark>>
   processRemark(BitstreamRemarkParserHelper &Helper);
-  Error processExternalFilePath(std::optional<StringRef> ExternalFilePath);
 };
 
 Expected<std::unique_ptr<BitstreamRemarkParser>> createBitstreamParserFromMeta(


        


More information about the llvm-commits mailing list