[lldb][NFC] Refactor getUUID functionality

[zequanwu]

include correct header

[labath]

Thanks for cleaning this up. A couple of comments inline.

lldb/include/lldb/Utility/UUID.h

41

I'd name this like fromCvRecord or something. Also, replace the pointer by a reference, please.

lldb/source/Plugins/Process/minidump/MinidumpParser.cpp

76

This is the only place which passes false, right?
If that's true, I think it would be better to drop this argument, and do something special here. That would give as an opportunity to call out the fact that breakpad (used to) put elf build-ids into this field. This is a sufficiently weird thing to merit drawing extra attention to it. Maybe something like:

if (isBinFormatELF()) {
  // Older versions of breakpad used to write the (first 16 bytes of) ELF build into this field.
  return UUID::fromOptionalData(pdb70_uuid->Uuid, sizeof(pdb70_uuid->Uuid))
}
return UUID::fromCvRecord(*pdb70_uuid);

lldb/source/Utility/UUID.cpp

38

You could just take the argument by value, and then byte-swap it in-place.

[zequanwu]

Address comments.

[labath]

lgtm, modulo the comment.

lldb/source/Plugins/Process/minidump/MinidumpParser.cpp

75

Once the reference is gone, you also don't need to make a copy here.

lldb/source/Utility/UUID.cpp

38

My previous comments were a bit confusing, but taking the argument by a mutable reference, was not what I intended. :(
I first wanted to use a const reference as that's better than a pointer, but then I realized that you're also making a copy, so it would be better if we just had the compiler do that. So you can just drop the reference now.

Sorry.

[zequanwu]

Address comments.

lldb/include/lldb/Utility/UUID.h

 //===-- UUID.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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_UUID_H
 #define LLDB_UTILITY_UUID_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Endian.h"
 #include <stddef.h>
 #include <stdint.h>
 #include <string>
 
 namespace lldb_private {
 
   class Stream;
 
 class UUID {
 public:
   UUID() = default;
 
+  // Reference:
+  // https://crashpad.chromium.org/doxygen/structcrashpad_1_1CodeViewRecordPDB70.html
+  struct CvRecordPdb70 {
+    struct {
+      llvm::support::ulittle32_t Data1;
+      llvm::support::ulittle16_t Data2;
+      llvm::support::ulittle16_t Data3;
+      uint8_t Data4[8];
+    } Uuid;
+    llvm::support::ulittle32_t Age;
+    // char PDBFileName[];
+  };
+
+  /// Create a UUID from CvRecordPdb70.
+  static UUID fromCvRecord(CvRecordPdb70 debug_info);
+
   /// Creates a UUID from the data pointed to by the bytes argument. No special
   /// significance is attached to any of the values.
   static UUID fromData(const void *bytes, uint32_t num_bytes) {
     if (bytes)
       return fromData({reinterpret_cast<const uint8_t *>(bytes), num_bytes});
     return UUID();
   }
 

lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp

 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm::pdb;
 using namespace llvm::codeview;
 
 LLDB_PLUGIN_DEFINE(ObjectFilePDB)
 
-struct CVInfoPdb70 {
-  // 16-byte GUID
-  struct _Guid {
-    llvm::support::ulittle32_t Data1;
-    llvm::support::ulittle16_t Data2;
-    llvm::support::ulittle16_t Data3;
-    uint8_t Data4[8];
-  } Guid;
-
-  llvm::support::ulittle32_t Age;
-};
-
 static UUID GetPDBUUID(InfoStream &IS) {
-  // This part is similar with what has done in ObjectFilePECOFF.
-  using llvm::support::endian::read16be;
-  using llvm::support::endian::read32;
-  using llvm::support::endian::read32be;
-
-  GUID guid = IS.getGuid();
-  const uint8_t *guid_p = guid.Guid;
-  struct CVInfoPdb70 info;
-  info.Guid.Data1 = read32be(guid_p);
-  guid_p += 4;
-  info.Guid.Data2 = read16be(guid_p);
-  guid_p += 2;
-  info.Guid.Data3 = read16be(guid_p);
-  guid_p += 2;
-  memcpy(info.Guid.Data4, guid_p, 8);
-
-  // Return 20-byte UUID if the Age is not zero
-  uint32_t age = IS.getAge();
-  if (age) {
-    info.Age = read32(&age, llvm::support::big);
-    return UUID::fromOptionalData(&info, sizeof(info));
-  }
-  // Otherwise return 16-byte GUID
-  return UUID::fromOptionalData(&info.Guid, sizeof(info.Guid));
+  UUID::CvRecordPdb70 debug_info;
+  memcpy(&debug_info.Uuid, IS.getGuid().Guid, sizeof(debug_info.Uuid));
+  debug_info.Age = IS.getAge();
+  return UUID::fromCvRecord(debug_info);
 }
 
 char ObjectFilePDB::ID;
 
 void ObjectFilePDB::Initialize() {
   PluginManager::RegisterPlugin(GetPluginNameStatic(),
                                 GetPluginDescriptionStatic(), CreateInstance,
                                 CreateMemoryInstance, GetModuleSpecifications);

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

 #define OPT_HEADER_MAGIC_PE32 0x010b
 #define OPT_HEADER_MAGIC_PE32_PLUS 0x020b
 
 using namespace lldb;
 using namespace lldb_private;
 
 LLDB_PLUGIN_DEFINE(ObjectFilePECOFF)
 
-struct CVInfoPdb70 {
-  // 16-byte GUID
-  struct _Guid {
-    llvm::support::ulittle32_t Data1;
-    llvm::support::ulittle16_t Data2;
-    llvm::support::ulittle16_t Data3;
-    uint8_t Data4[8];
-  } Guid;
-
-  llvm::support::ulittle32_t Age;
-};
-
 static UUID GetCoffUUID(llvm::object::COFFObjectFile &coff_obj) {
   const llvm::codeview::DebugInfo *pdb_info = nullptr;
   llvm::StringRef pdb_file;
 
-  // This part is similar with what has done in minidump parser.
   if (!coff_obj.getDebugPDBInfo(pdb_info, pdb_file) && pdb_info) {
     if (pdb_info->PDB70.CVSignature == llvm::OMF::Signature::PDB70) {
-      using llvm::support::endian::read16be;
-      using llvm::support::endian::read32be;
-
-      const uint8_t *sig = pdb_info->PDB70.Signature;
-      struct CVInfoPdb70 info;
-      info.Guid.Data1 = read32be(sig);
-      sig += 4;
-      info.Guid.Data2 = read16be(sig);
-      sig += 2;
-      info.Guid.Data3 = read16be(sig);
-      sig += 2;
-      memcpy(info.Guid.Data4, sig, 8);
-
-      // Return 20-byte UUID if the Age is not zero
-      if (pdb_info->PDB70.Age) {
-        info.Age = read32be(&pdb_info->PDB70.Age);
-        return UUID::fromOptionalData(&info, sizeof(info));
-      }
-      // Otherwise return 16-byte GUID
-      return UUID::fromOptionalData(&info.Guid, sizeof(info.Guid));
+      UUID::CvRecordPdb70 info;
+      memcpy(&info.Uuid, pdb_info->PDB70.Signature, sizeof(info.Uuid));
+      info.Age = pdb_info->PDB70.Age;
+      return UUID::fromCvRecord(info);
     }
   }
 
   return UUID();
 }
 
 char ObjectFilePECOFF::ID;
 

lldb/source/Plugins/Process/minidump/MinidumpParser.cpp

UUID MinidumpParser::GetModuleUUID(const minidump::Module *module) {
   Status error = consumeObject(cv_record, signature);
   if (error.Fail())
     return UUID();
 
   const CvSignature cv_signature =
       static_cast<CvSignature>(static_cast<uint32_t>(*signature));
 
   if (cv_signature == CvSignature::Pdb70) {
-    const CvRecordPdb70 *pdb70_uuid = nullptr;
+    const UUID::CvRecordPdb70 *pdb70_uuid = nullptr;
     Status error = consumeObject(cv_record, pdb70_uuid);
     if (error.Fail())
       return UUID();
-
-    CvRecordPdb70 swapped;
-    if (!GetArchitecture().GetTriple().isOSBinFormatELF()) {
-      // LLDB's UUID class treats the data as a sequence of bytes, but breakpad
-      // interprets it as a sequence of little-endian fields, which it converts
-      // to big-endian when converting to text. Swap the bytes to big endian so
-      // that the string representation comes out right.
-      swapped = *pdb70_uuid;
-      llvm::sys::swapByteOrder(swapped.Uuid.Data1);
-      llvm::sys::swapByteOrder(swapped.Uuid.Data2);
-      llvm::sys::swapByteOrder(swapped.Uuid.Data3);
-      llvm::sys::swapByteOrder(swapped.Age);
-      pdb70_uuid = &swapped;
-    }
+    if (GetArchitecture().GetTriple().isOSBinFormatELF()) {
       if (pdb70_uuid->Age != 0)
         return UUID::fromOptionalData(pdb70_uuid, sizeof(*pdb70_uuid));
-    return UUID::fromOptionalData(&pdb70_uuid->Uuid, sizeof(pdb70_uuid->Uuid));
+      return UUID::fromOptionalData(&pdb70_uuid->Uuid,
+                                    sizeof(pdb70_uuid->Uuid));
+    }
+    return UUID::fromCvRecord(*pdb70_uuid);
   } else if (cv_signature == CvSignature::ElfBuildId)
     return UUID::fromOptionalData(cv_record);
 
   return UUID();
 }
 
 llvm::ArrayRef<minidump::Thread> MinidumpParser::GetThreads() {
   auto ExpectedThreads = GetMinidumpFile().getThreadList();
   if (ExpectedThreads)

lldb/source/Plugins/Process/minidump/MinidumpTypes.h

 
 LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
 
 enum class CvSignature : uint32_t {
   Pdb70 = 0x53445352, // RSDS
   ElfBuildId = 0x4270454c, // BpEL (Breakpad/Crashpad minidumps)
 };
 
-// Reference:
-// https://crashpad.chromium.org/doxygen/structcrashpad_1_1CodeViewRecordPDB70.html
-struct CvRecordPdb70 {
-  struct {
-    llvm::support::ulittle32_t Data1;
-    llvm::support::ulittle16_t Data2;
-    llvm::support::ulittle16_t Data3;
-    uint8_t Data4[8];
-  } Uuid;
-  llvm::support::ulittle32_t Age;
-  // char PDBFileName[];
-};
-static_assert(sizeof(CvRecordPdb70) == 20,
-              "sizeof CvRecordPdb70 is not correct!");
-
 enum class MinidumpMiscInfoFlags : uint32_t {
   ProcessID = (1 << 0),
   ProcessTimes = (1 << 1),
   LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ ProcessTimes)
 };
 
 template <typename T>
 Status consumeObject(llvm::ArrayRef<uint8_t> &Buffer, const T *&Object) {

lldb/source/Utility/UUID.cpp

static inline bool separate(size_t count) {
   case 6:
   case 8:
     return true;
   default:
     return false;
   }
 }
 
+UUID UUID::fromCvRecord(UUID::CvRecordPdb70 debug_info) {
+  llvm::sys::swapByteOrder(debug_info.Uuid.Data1);
+  llvm::sys::swapByteOrder(debug_info.Uuid.Data2);
+  llvm::sys::swapByteOrder(debug_info.Uuid.Data3);
+  llvm::sys::swapByteOrder(debug_info.Age);
+  if (debug_info.Age)
+    return UUID::fromOptionalData(&debug_info, sizeof(debug_info));
+  return UUID::fromOptionalData(&debug_info.Uuid, sizeof(debug_info.Uuid));
+}
+
 std::string UUID::GetAsString(llvm::StringRef separator) const {
   std::string result;
   llvm::raw_string_ostream os(result);
 
   for (auto B : llvm::enumerate(GetBytes())) {
     if (separate(B.index()))
       os << separator;