[Object] Refactor build ID parsing into Object lib.

[mysterymath]

Summary

This makes parsing for build IDs in the markup filter slightly more
permissive, in line with fromHex.

It also removes the distinction between missing build ID and empty build
ID; empty build IDs aren't a useful concept, since their purpose is to
uniquely identify a binary. This removes a layer of indirection wherever
build IDs are obtained.

[mysterymath]

Rebase off of change stack onto main.

[jhenderson]

llvm/lib/Object/BuildID.cpp

21–23

Why have you changed this?

llvm/test/DebugInfo/symbolize-filter-markup-parse-fields.test

19–20

This check feels a bit fragile. Also, why has there been a behaviour change?

[mysterymath]

Update test case.

llvm/lib/Object/BuildID.cpp

21–23

To make the new code comply with the style guide: https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
Since this is a refactoring change, I altered the existing definitions so the file would be internally consistent.

llvm/test/DebugInfo/symbolize-filter-markup-parse-fields.test

19–20

Different places that parsed build IDs disagreed on whether or not this was an error. I chose a semantics for build IDs consistent with the behavior of fromHex, since that appears to be the semantics generally used for hex->binary conversion.

[mysterymath]

Correct llvm-cov usage; update commit message.

[jhenderson]

Looks good, with a couple of nits.

llvm/include/llvm/DebugInfo/Symbolize/MarkupFilter.h

17–18

Whilst you're working on this file, please delete the blank lines between the include lists. These prevent clang-format from reordering them automatically to match the LLVM style guide. (Either in this patch, or in a separate NFC commit, no need for review).

llvm/lib/Object/BuildID.cpp

21–23

Huh, I didn't know about that style guide entry. I have some issues with it, but that's a separate topic.

llvm/test/DebugInfo/symbolize-filter-markup-parse-fields.test

4–5

As as suggestion, now that you've expanded the CHECK-NOT lines, it would be good to indent the CHECK lines so that the error messages line up with the CHECK-NOT line errors, as demonstrated by the inline edit (you'd apply it to each CHECK line). This way, it'll be easier to spot by eye anywhere which differs unexpectedly.

[mysterymath]

Update header and test style.

llvm/include/llvm/DebugInfo/Symbolize/MarkupFilter.h

 //===- MarkupFilter.h -------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file declares a filter that replaces symbolizer markup with
 /// human-readable expressions.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_DEBUGINFO_SYMBOLIZE_MARKUPFILTER_H
 #define LLVM_DEBUGINFO_SYMBOLIZE_MARKUPFILTER_H
 
-#include "Markup.h"
-
-#include <map>
-
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/DebugInfo/Symbolize/Markup.h"
+#include "llvm/Object/BuildID.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
+#include <map>
 
 namespace llvm {
 namespace symbolize {
 
 class LLVMSymbolizer;
 
 /// Filter to convert parsed log symbolizer markup elements into human-readable
 /// text.
private:
   void printValue(Twine Value);
 
   std::optional<Module> parseModule(const MarkupNode &Element) const;
   std::optional<MMap> parseMMap(const MarkupNode &Element) const;
 
   std::optional<uint64_t> parseAddr(StringRef Str) const;
   std::optional<uint64_t> parseModuleID(StringRef Str) const;
   std::optional<uint64_t> parseSize(StringRef Str) const;
-  std::optional<SmallVector<uint8_t>> parseBuildID(StringRef Str) const;
+  object::BuildID parseBuildID(StringRef Str) const;
   std::optional<std::string> parseMode(StringRef Str) const;
   std::optional<PCType> parsePCType(StringRef Str) const;
   std::optional<uint64_t> parseFrameNumber(StringRef Str) const;
 
   bool checkTag(const MarkupNode &Node) const;
   bool checkNumFields(const MarkupNode &Element, size_t Size) const;
   bool checkNumFieldsAtLeast(const MarkupNode &Element, size_t Size) const;
   bool checkNumFieldsAtMost(const MarkupNode &Element, size_t Size) const;

llvm/include/llvm/Object/BuildID.h

 /// A build ID in binary form.
 typedef SmallVector<uint8_t, 10> BuildID;
 
 /// A reference to a BuildID in binary form.
 typedef ArrayRef<uint8_t> BuildIDRef;
 
 class ObjectFile;
 
+/// Parses a build ID from a hex string.
+BuildID parseBuildID(StringRef Str);
+
 /// Returns the build ID, if any, contained in the given object file.
-std::optional<BuildIDRef> getBuildID(const ObjectFile *Obj);
+BuildIDRef getBuildID(const ObjectFile *Obj);
 
 /// BuildIDFetcher searches local cache directories for debug info.
 class BuildIDFetcher {
 public:
   BuildIDFetcher(std::vector<std::string> DebugFileDirectories)
       : DebugFileDirectories(std::move(DebugFileDirectories)) {}
   virtual ~BuildIDFetcher() = default;
 

llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp

MarkupFilter::parseModule(const MarkupNode &Element) const {
   StringRef Type = Element.Fields[2];
   if (Type != "elf") {
     WithColor::error() << "unknown module type\n";
     reportLocation(Type.begin());
     return std::nullopt;
   }
   if (!checkNumFields(Element, 4))
     return std::nullopt;
-  ASSIGN_OR_RETURN_NONE(SmallVector<uint8_t>, BuildID,
-                        parseBuildID(Element.Fields[3]));
+  SmallVector<uint8_t> BuildID = parseBuildID(Element.Fields[3]);
+  if (BuildID.empty())
+    return std::nullopt;
   return Module{ID, Name.str(), std::move(BuildID)};
 }
 
 std::optional<MarkupFilter::MMap>
 MarkupFilter::parseMMap(const MarkupNode &Element) const {
   if (!checkNumFieldsAtLeast(Element, 3))
     return std::nullopt;
   ASSIGN_OR_RETURN_NONE(uint64_t, Addr, parseAddr(Element.Fields[0]));
std::optional<uint64_t> MarkupFilter::parseFrameNumber(StringRef Str) const {
   if (Str.getAsInteger(10, ID)) {
     reportTypeError(Str, "frame number");
     return std::nullopt;
   }
   return ID;
 }
 
 // Parse a build ID (%x in the spec).
-std::optional<SmallVector<uint8_t>>
-MarkupFilter::parseBuildID(StringRef Str) const {
-  std::string Bytes;
-  if (Str.empty() || Str.size() % 2 || !tryGetFromHex(Str, Bytes)) {
+object::BuildID MarkupFilter::parseBuildID(StringRef Str) const {
+  object::BuildID BID = llvm::object::parseBuildID(Str);
+  if (BID.empty())
     reportTypeError(Str, "build ID");
-    return std::nullopt;
-  }
-  ArrayRef<uint8_t> BuildID(reinterpret_cast<const uint8_t *>(Bytes.data()),
-                            Bytes.size());
-  return SmallVector<uint8_t>(BuildID.begin(), BuildID.end());
+  return BID;
 }
 
 // Parses the mode string for an mmap element.
 std::optional<std::string> MarkupFilter::parseMode(StringRef Str) const {
   if (Str.empty()) {
     reportTypeError(Str, "mode");
     return std::nullopt;
   }

llvm/lib/DebugInfo/Symbolize/Symbolize.cpp

ObjectFile *LLVMSymbolizer::lookUpDebuglinkObject(const std::string &Path,
   }
   return DbgObjOrErr.get();
 }
 
 ObjectFile *LLVMSymbolizer::lookUpBuildIDObject(const std::string &Path,
                                                 const ELFObjectFileBase *Obj,
                                                 const std::string &ArchName) {
   auto BuildID = getBuildID(Obj);
-  if (!BuildID)
-    return nullptr;
-  if (BuildID->size() < 2)
+  if (BuildID.size() < 2)
     return nullptr;
   std::string DebugBinaryPath;
-  if (!getOrFindDebugBinary(*BuildID, DebugBinaryPath))
+  if (!getOrFindDebugBinary(BuildID, DebugBinaryPath))
     return nullptr;
   auto DbgObjOrErr = getOrCreateObject(DebugBinaryPath, ArchName);
   if (!DbgObjOrErr) {
     consumeError(DbgObjOrErr.takeError());
     return nullptr;
   }
   return DbgObjOrErr.get();
 }

llvm/lib/Debuginfod/Debuginfod.cpp

IteratorGroup.async([&, this]() -> void {
           continue;
 
         // TODO: Support non-ELF binaries
         object::ELFObjectFileBase *Object =
             dyn_cast<object::ELFObjectFileBase>(Bin);
         if (!Object)
           continue;
 
-        std::optional<BuildIDRef> ID = getBuildID(Object);
-        if (!ID)
+        BuildIDRef ID = getBuildID(Object);
+        if (ID.empty())
           continue;
 
-        std::string IDString = buildIDToString(*ID);
+        std::string IDString = buildIDToString(ID);
         if (Object->hasDebugInfo()) {
           std::lock_guard<sys::RWMutex> DebugBinariesGuard(DebugBinariesMutex);
           (void)DebugBinaries.try_emplace(IDString, std::move(FilePath));
         } else {
           std::lock_guard<sys::RWMutex> BinariesGuard(BinariesMutex);
           (void)Binaries.try_emplace(IDString, std::move(FilePath));
         }
       }

llvm/lib/Object/BuildID.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/Object/BuildID.h"
 
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
-namespace llvm {
-namespace object {
+using namespace llvm;
+using namespace llvm::object;
 
 namespace {
 
-template <typename ELFT>
-std::optional<BuildIDRef> getBuildID(const ELFFile<ELFT> &Obj) {
+template <typename ELFT> BuildIDRef getBuildID(const ELFFile<ELFT> &Obj) {
   auto PhdrsOrErr = Obj.program_headers();
   if (!PhdrsOrErr) {
     consumeError(PhdrsOrErr.takeError());
     return {};
   }
   for (const auto &P : *PhdrsOrErr) {
     if (P.p_type != ELF::PT_NOTE)
       continue;
     Error Err = Error::success();
     for (auto N : Obj.notes(P, Err))
       if (N.getType() == ELF::NT_GNU_BUILD_ID &&
           N.getName() == ELF::ELF_NOTE_GNU)
         return N.getDesc();
     consumeError(std::move(Err));
   }
   return {};
 }
 
 } // namespace
 
-std::optional<BuildIDRef> getBuildID(const ObjectFile *Obj) {
+BuildID llvm::object::parseBuildID(StringRef Str) {
+  std::string Bytes;
+  if (!tryGetFromHex(Str, Bytes))
+    return {};
+  ArrayRef<uint8_t> BuildID(reinterpret_cast<const uint8_t *>(Bytes.data()),
+                            Bytes.size());
+  return SmallVector<uint8_t>(BuildID.begin(), BuildID.end());
+}
+
+BuildIDRef llvm::object::getBuildID(const ObjectFile *Obj) {
   if (auto *O = dyn_cast<ELFObjectFile<ELF32LE>>(Obj))
-    return getBuildID(O->getELFFile());
+    return ::getBuildID(O->getELFFile());
   if (auto *O = dyn_cast<ELFObjectFile<ELF32BE>>(Obj))
-    return getBuildID(O->getELFFile());
+    return ::getBuildID(O->getELFFile());
   if (auto *O = dyn_cast<ELFObjectFile<ELF64LE>>(Obj))
-    return getBuildID(O->getELFFile());
+    return ::getBuildID(O->getELFFile());
   if (auto *O = dyn_cast<ELFObjectFile<ELF64BE>>(Obj))
-    return getBuildID(O->getELFFile());
+    return ::getBuildID(O->getELFFile());
   return std::nullopt;
 }
 
 std::optional<std::string> BuildIDFetcher::fetch(BuildIDRef BuildID) const {
   auto GetDebugPath = [&](StringRef Directory) {
     SmallString<128> Path{Directory};
     sys::path::append(Path, ".build-id",
                       llvm::toHex(BuildID[0], /*LowerCase=*/true),
for (const auto &Directory : DebugFileDirectories) {
       // Try <debug-file-directory>/.build-id/../...
       SmallString<128> Path = GetDebugPath(Directory);
       if (llvm::sys::fs::exists(Path))
         return std::string(Path);
     }
   }
   return std::nullopt;
 }
-
-} // namespace object
-} // namespace llvm

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

static Expected<std::vector<SectionRef>> lookupSections(ObjectFile &OF,
   if (Sections.empty())
     return make_error<CoverageMapError>(coveragemap_error::no_data_found);
   return Sections;
 }
 
 static Expected<std::unique_ptr<BinaryCoverageReader>>
 loadBinaryFormat(std::unique_ptr<Binary> Bin, StringRef Arch,
                  StringRef CompilationDir = "",
-                 std::optional<object::BuildIDRef> *BinaryID = nullptr) {
+                 object::BuildIDRef *BinaryID = nullptr) {
   std::unique_ptr<ObjectFile> OF;
   if (auto *Universal = dyn_cast<MachOUniversalBinary>(Bin.get())) {
     // If we have a universal binary, try to look up the object for the
     // appropriate architecture.
     auto ObjectFileOrErr = Universal->getMachOObjectForArch(Arch);
     if (!ObjectFileOrErr)
       return ObjectFileOrErr.takeError();
     OF = std::move(ObjectFileOrErr.get());
if (auto *Ar = dyn_cast<Archive>(Bin.get())) {
     // to the caller.
     if (Ar->isThin())
       for (auto &Buffer : Ar->takeThinBuffers())
         ObjectFileBuffers.push_back(std::move(Buffer));
 
     return std::move(Readers);
   }
 
-  std::optional<object::BuildIDRef> BinaryID;
+  object::BuildIDRef BinaryID;
   auto ReaderOrErr = loadBinaryFormat(std::move(Bin), Arch, CompilationDir,
                                       BinaryIDs ? &BinaryID : nullptr);
   if (!ReaderOrErr)
     return ReaderOrErr.takeError();
   Readers.push_back(std::move(ReaderOrErr.get()));
-  if (BinaryID)
-    BinaryIDs->push_back(*BinaryID);
+  if (!BinaryID.empty())
+    BinaryIDs->push_back(BinaryID);
   return std::move(Readers);
 }
 
 Error BinaryCoverageReader::readNextRecord(CoverageMappingRecord &Record) {
   if (CurrentRecord >= MappingRecords.size())
     return make_error<CoverageMapError>(coveragemap_error::eof);
 
   FunctionsFilenames.clear();

llvm/lib/ProfileData/RawMemProfReader.cpp

Error RawMemProfReader::initialize(std::unique_ptr<MemoryBuffer> DataBuffer) {
   if (Error E = symbolizeAndFilterStackFrames())
     return E;
 
   return mapRawProfileToRecords();
 }
 
 Error RawMemProfReader::setupForSymbolization() {
   auto *Object = cast<object::ObjectFile>(Binary.getBinary());
-  auto BuildIdOr = object::getBuildID(Object);
-  if (!BuildIdOr.has_value())
+  object::BuildIDRef BinaryId = object::getBuildID(Object);
+  if (BinaryId.empty())
     return make_error<StringError>(Twine("No build id found in binary ") +
                                        Binary.getBinary()->getFileName(),
                                    inconvertibleErrorCode());
-  llvm::ArrayRef<uint8_t> BinaryId = BuildIdOr.value();
 
   int NumMatched = 0;
   for (const auto &Entry : SegmentInfo) {
     llvm::ArrayRef<uint8_t> SegmentId(Entry.BuildId, Entry.BuildIdSize);
     if (BinaryId == SegmentId) {
       // We assume only one text segment in the main binary for simplicity and
       // reduce the overhead of checking multiple ranges during symbolization.
       if (++NumMatched > 1) {

llvm/test/DebugInfo/symbolize-filter-markup-parse-fields.test

 RUN: split-file %s %t
 RUN: llvm-symbolizer --filter-markup < %t/log 2> %t.err
 RUN: FileCheck %s -input-file=%t.err --match-full-lines
 
-CHECK-NOT: '0x4f'
-CHECK-NOT: '00'
-CHECK: error: expected address; found ''
-CHECK: error: expected address; found '42'
-CHECK: error: expected address; found '0xgg'
+CHECK-NOT: error: expected address; found '0x4f'
+CHECK-NOT: error: expected address; found '00'
+CHECK:     error: expected address; found ''
+CHECK:     error: expected address; found '42'
+CHECK:     error: expected address; found '0xgg'
 
-CHECK-NOT: '0'
-CHECK: error: expected module ID; found ''
-CHECK: error: expected module ID; found '-1'
-CHECK-NOT: '077'
-CHECK: error: expected module ID; found '079'
-CHECK-NOT: '0xff'
-CHECK: error: expected module ID; found '0xfg'
-CHECK: error: expected module ID; found '0x'
+CHECK-NOT: error: expected module ID; found '0'
+CHECK:     error: expected module ID; found ''
+CHECK:     error: expected module ID; found '-1'
+CHECK-NOT: error: expected module ID; found '077'
+CHECK:     error: expected module ID; found '079'
+CHECK-NOT: error: expected module ID; found '0xff'
+CHECK:     error: expected module ID; found '0xfg'
+CHECK:     error: expected module ID; found '0x'
 
-CHECK: error: expected build ID; found ''
-CHECK: error: expected build ID; found '0'
-CHECK-NOT: '0xff'
-CHECK: error: expected build ID; found 'fg'
+CHECK:     error: expected build ID; found ''
+CHECK-NOT: error: expected build ID; found '0'
+CHECK-NOT: error: expected build ID; found '0xff'
+CHECK:     error: expected build ID; found 'fg'
 
 ;--- log
 {{{mmap:0x4f:1:unknown}}}
 {{{mmap:00:1:unknown}}}
 {{{mmap::1:unknown}}}
 {{{mmap:42:1:unknown}}}
 {{{mmap:0xgg:1:unknown}}}
 

llvm/tools/llvm-objdump/llvm-objdump.cpp

static void createFakeELFSections(ObjectFile &Obj) {
   else
     llvm_unreachable("Unsupported binary format");
 }
 
 // Tries to fetch a more complete version of the given object file using its
 // Build ID. Returns std::nullopt if nothing was found.
 static std::optional<OwningBinary<Binary>>
 fetchBinaryByBuildID(const ObjectFile &Obj) {
-  std::optional<object::BuildIDRef> BuildID = getBuildID(&Obj);
-  if (!BuildID)
+  object::BuildIDRef BuildID = getBuildID(&Obj);
+  if (BuildID.empty())
     return std::nullopt;
-  std::optional<std::string> Path = BIDFetcher->fetch(*BuildID);
+  std::optional<std::string> Path = BIDFetcher->fetch(BuildID);
   if (!Path)
     return std::nullopt;
   Expected<OwningBinary<Binary>> DebugBinary = createBinary(*Path);
   if (!DebugBinary) {
     reportWarning(toString(DebugBinary.takeError()), *Path);
     return std::nullopt;
   }
   return std::move(*DebugBinary);
if (!llvm::to_integer(V, Value, 0)) {
                          ": expected a non-negative integer, but got '" + V +
                          "'");
     }
   }
 }
 
 static object::BuildID parseBuildIDArg(const opt::Arg *A) {
   StringRef V(A->getValue());
-  std::string Bytes;
-  if (!tryGetFromHex(V, Bytes))
+  object::BuildID BID = parseBuildID(V);
+  if (BID.empty())
     reportCmdLineError(A->getSpelling() + ": expected a build ID, but got '" +
                        V + "'");
-  ArrayRef<uint8_t> BuildID(reinterpret_cast<const uint8_t *>(Bytes.data()),
-                            Bytes.size());
-  return object::BuildID(BuildID.begin(), BuildID.end());
+  return BID;
 }
 
 void objdump::invalidArgValue(const opt::Arg *A) {
   reportCmdLineError("'" + StringRef(A->getValue()) +
                      "' is not a valid value for '" + A->getSpelling() + "'");
 }
 
 static std::vector<std::string>

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

static void enableDebuginfod(LLVMSymbolizer &Symbolizer,
   IsEnabled = true;
   // Look up symbols using the debuginfod client.
   Symbolizer.setBuildIDFetcher(std::make_unique<DebuginfodFetcher>(
       Args.getAllArgValues(OPT_debug_file_directory_EQ)));
   // The HTTPClient must be initialized for use by the debuginfod client.
   HTTPClient::initialize();
 }
 
-static object::BuildID parseBuildID(StringRef Str) {
-  std::string Bytes;
-  if (!tryGetFromHex(Str, Bytes))
-    return {};
-  ArrayRef<uint8_t> BuildID(reinterpret_cast<const uint8_t *>(Bytes.data()),
-                            Bytes.size());
-  return object::BuildID(BuildID.begin(), BuildID.end());
-}
-
 static bool parseCommand(StringRef BinaryName, bool IsAddr2Line,
                          StringRef InputString, Command &Cmd,
                          std::string &ModuleName, object::BuildID &BuildID,
                          uint64_t &ModuleOffset) {
   const char kDelimiters[] = " \n\r";
   ModuleName = "";
   if (InputString.consume_front("CODE ")) {
     Cmd = Command::Code;