Represent invalid UUIDs as UUIDs with length zero

[labath]

Summary

During the previous attempt to generalize the UUID class, it was
suggested that we represent invalid UUIDs as length zero (previously, we
used an all-zero UUID for that). This meant that some valid build-ids
could not be represented (it's possible however unlikely that a checksum of
some file would be zero) and complicated adding support for variable
length build-ids (should a 16-byte empty UUID compare equal to a 20-byte
empty UUID?).

This patch resolves these issues by introducing a canonical
representation for an invalid UUID. The slight complication here is that
some clients (MachO) actually use the all-zero notation to mean "no UUID
has been set". To keep this use case working, I have introduced an
additional argument to the UUID constructor, which specifies whether an
all-zero vector should be considered a valid UUID. For the usages where
the UUID data comes from a MachO file, I set this argument to false.

I did not introduce a similar argument to the string parsing function
with the rationalle that if somebody went through the trouble of
spelling it out as a bunch of zeroes, he probably really means that.

[lemo]

The slight complication here is that
some clients (MachO) actually use the all-zero notation to mean "no UUID
has been set". To keep this use case working, I have introduced an
additional argument to the UUID constructor, which specifies whether an
all-zero vector should be considered a valid UUID. For the usages where
the UUID data comes from a MachO file, I set this argument to false.

What is the distinction between "no UUID has been set" and "invalid UUID"? I find this subtlety confusing, can we simplify it to just valid/not-valid UUIDs?

include/lldb/Utility/UUID.h

31

switch to llvm::SmallVector<uint8_t, 20> as suggested by Greg?

36–40

If we define this we should define at least the copy constructor as well (rule of 3/5). In this case the cleanest solution may be to allow the implicit definitions (which would also allow the implicit move operations - defining assignment operator as defaulted would inhibit the implicit move SMFs)

50–51

is this really needed? I'd prefer the truly explicit (pun intended) IsValid()

93

these can be simplified if we use llvm::SmallVector (or std::vector)

[labath]

In D48479#1140927, @lemo wrote:

The slight complication here is that
some clients (MachO) actually use the all-zero notation to mean "no UUID
has been set". To keep this use case working, I have introduced an
additional argument to the UUID constructor, which specifies whether an
all-zero vector should be considered a valid UUID. For the usages where
the UUID data comes from a MachO file, I set this argument to false.

What is the distinction between "no UUID has been set" and "invalid UUID"? I find this subtlety confusing, can we simplify it to just valid/not-valid UUIDs?

That is what I am trying to do (although not completely successfully, it seems ;) ). Once you have a UUID object around there should be no distinction. You either have a valid uuid (for now, consisting of 16 or 20 bytes), or you don't.

However, during parsing you need to know the meaning of a "0000...0" UUID. In a MachO file (at least based on the comments in the code) this value is used to denote the fact that the object file has no UUID. For elf, a "000..0" build-id is a perfectly valid identifier (and the lack of a build-id is denoted by the absence of the build-id section). The extra constructor argument is my way of trying to support both scenarios. The other possibility I see is to have a some kind of a factory function for one of the options (or both). I don't really have a preference between the two.

include/lldb/Utility/UUID.h

31

I'm deliberately keeping that for a separate patch. Here, I just want to prepare the ground by defining the "invalid" UUID more clearly. The part with the arbitrary UUID length will come after that.

36–40

Good point. I've deleted the copy constructor altogether.

50–51

My main reason for an operator bool is that it allows the if-declaration syntax (if (UUID u = getUUID()) doSomethingUsefulWith(u); The most llvm-y solution would be not have neither of these methods and use Optional<UUID> when you don't know if you have one, but as far as operator bool vs. isValid goes, both styles are used in llvm (and lldb).

[labath]

Delete copy constructor.

[lemo]

However, during parsing you need to know the meaning of a "0000...0" UUID.
In a MachO file (at least based on the comments in the code) this value is
used to denote the fact that the object file has no UUID. For elf, a
"000..0" build-id is a perfectly valid identifier (and the lack of a
build-id is denoted by the absence of the build-id section). The extra
constructor argument is my way of trying to support both scenarios. The
other possibility I see is to have a some kind of a factory function for
one of the options (or both). I don't really have a preference between the
two.

One solution might be to encapsulate the MachO convention in the MachO
code: check in there (maybe through a helper function) if the UUID is
"000...0" and map it to the empty UUID in that case. The UUID interface
would not have to know/care about this convention. Would this work?

[labath]

In D48479#1141067, @lemo wrote:

One solution might be to encapsulate the MachO convention in the MachO
code: check in there (maybe through a helper function) if the UUID is
"000...0" and map it to the empty UUID in that case. The UUID interface
would not have to know/care about this convention. Would this work?

I'm not sure if there is a suitable place for that function. This is needed in "ObjectFileMachO" and two dynamic loader plugins.

[lemo]

I'm not sure if there is a suitable place for that function. This is
needed in "ObjectFileMachO" and two dynamic loader plugins.

Then your factory idea may be the next best thing. While we're at it, maybe
we can remove the UUID::SetBytes() from the public interface, and make the
UUID an immutable value type:

Ex. instead of:

UUID uuid;
...
uuid.SetBytes(...)

We'd have:

UUID uuid;

uuid = UUID(...);
or
uuid = { ... };
or
uuid = UUID::factory(...);

What do you think?

[clayborg]

Would love to remove the "accept_zeroes" argument everywhere. Too much matching happens in LLDB and we can't have multiple shared libraries claiming zeros as their UUID

include/lldb/Utility/UUID.h

54

I am not sure accept_zeroes is a good idea. LLDB does a lot of matching based on UUID and we can't have multiple shared libraries claiming to have zeroes as their UUID. It will cause chaos. Zeroes as a build ID is not very useful

[labath]

In D48479#1141274, @clayborg wrote:

Would love to remove the "accept_zeroes" argument everywhere. Too much matching happens in LLDB and we can't have multiple shared libraries claiming zeros as their UUID

A zero UUID is a problem only if it someone put it there to mean "I don't know". "Real" UUIDs seem to acknowledge the concept of a nil value https://en.wikipedia.org/wiki/Universally_unique_identifier#Nil_UUID, and I guess that's what Mac is following. But we're using this class to represent other things besides the "real" UUIDs, and for those a zero value is as valid as any other number (it's extremely unlikely that a 20-byte hash will come out all-zero, but that isn't so much the case for a 32-bit CRC). I think the best we can do here is make the choice between the two treatments as explicit as possible. I hope the explicitly named constructor functions will be enough to achieve that.

[labath]

• Use static factory functions instead of the extra argument (the best names I could come up with is fromData and fromOptionalData).

[clayborg]

We should allow 4 and 8 byte UUIDs as pointed out by inlined comments. This means we should probably modify the UUID dumper to handle those cases as well.

include/lldb/Utility/UUID.h

92

Do we need this comment here? We currently take a 4 byte debug info CRC and call it a 16 byte UUID for no reason. Can we remove the need for this comment and allow any length?

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

735–736

Should we just save the UUId is 4 bytes long here?

741–742

Should we just save the UUId is 8 bytes long here?

929

Should we just save the UUId is 8 bytes long here?

938

Should we just save the UUId is 4 bytes long here?

source/Utility/DataExtractor.cpp

1101

This should take a UUID byte size as a second parameter and callers should be required to specify it explicitly

[clayborg]

Factory methods make things much clearer.

[labath]

include/lldb/Utility/UUID.h

92

I will do that in a follow-up patch. I wanted to keep this one for dealing with the whole zero issue.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

735–736

I will fix this and the other issues in the next patch.

source/Utility/DataExtractor.cpp

1101

Hmm.. this is actually unused. Can I just remove it?

[clayborg]

Patch is good. Feel free to remove the DataExtractor::DumpUUID() in a separate NFC commit or just remove it in this patch

source/Utility/DataExtractor.cpp

1101

Yes, please remove.

include/lldb/Utility/UUID.h

 
 namespace lldb_private {
 
   class Stream;
 
 class UUID {
 public:
   // Most UUIDs are 16 bytes, but some Linux build-ids (SHA1) are 20.
   typedef uint8_t ValueType[20];
 
   //------------------------------------------------------------------
   // Constructors and Destructors
   //------------------------------------------------------------------
-  UUID();
-  UUID(const UUID &rhs);
-  UUID(const void *uuid_bytes, uint32_t num_uuid_bytes);
-
-  ~UUID();
-
-  const UUID &operator=(const UUID &rhs);
+  UUID() = default;
+  UUID(const void *uuid_bytes, uint32_t num_uuid_bytes, bool accept_zeroes);
+  UUID(llvm::ArrayRef<uint8_t> bytes, bool accept_zeroes) {
+    SetBytes(bytes, accept_zeroes);
+  }
 
-  void Clear();
+  void Clear() { m_num_uuid_bytes = 0; }
 
   void Dump(Stream *s) const;
 
   llvm::ArrayRef<uint8_t> GetBytes() const {
     return {m_uuid, m_num_uuid_bytes};
   }
 
-  bool IsValid() const;
+  explicit operator bool() const { return IsValid(); }
+  bool IsValid() const { return m_num_uuid_bytes > 0; }
 
-  bool SetBytes(const void *uuid_bytes, uint32_t num_uuid_bytes = 16);
+  void SetBytes(const void *uuid_bytes, uint32_t num_uuid_bytes,
+                bool accept_zeroes);
+  void SetBytes(llvm::ArrayRef<uint8_t> bytes, bool accept_zeroes);
 
   std::string GetAsString(const char *separator = nullptr) const;
 
   size_t SetFromStringRef(llvm::StringRef str, uint32_t num_uuid_bytes = 16);
 
   // Decode as many UUID bytes (up to 16) as possible from the C string "cstr"
   // This is used for auto completion where a partial UUID might have been
   // typed in. It
public:
   DecodeUUIDBytesFromString(llvm::StringRef str, ValueType &uuid_bytes,
                             uint32_t &bytes_decoded,
                             uint32_t num_uuid_bytes = 16);
 
 protected:
   //------------------------------------------------------------------
   // Classes that inherit from UUID can see and modify these
   //------------------------------------------------------------------
-  uint32_t m_num_uuid_bytes; // Should be 16 or 20
+  uint32_t m_num_uuid_bytes = 0; // Should be 0, 16 or 20
   ValueType m_uuid;
 };
 
 bool operator==(const UUID &lhs, const UUID &rhs);
 bool operator!=(const UUID &lhs, const UUID &rhs);
 bool operator<(const UUID &lhs, const UUID &rhs);
 bool operator<=(const UUID &lhs, const UUID &rhs);
 bool operator>(const UUID &lhs, const UUID &rhs);
 bool operator>=(const UUID &lhs, const UUID &rhs);
 
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_UUID_H

source/API/SBModuleSpec.cpp

const uint8_t *SBModuleSpec::GetUUIDBytes() {
   return m_opaque_ap->GetUUID().GetBytes().data();
 }
 
 size_t SBModuleSpec::GetUUIDLength() {
   return m_opaque_ap->GetUUID().GetBytes().size();
 }
 
 bool SBModuleSpec::SetUUIDBytes(const uint8_t *uuid, size_t uuid_len) {
-  return m_opaque_ap->GetUUID().SetBytes(uuid, uuid_len);
+  m_opaque_ap->GetUUID().SetBytes(uuid, uuid_len, true);
+  return m_opaque_ap->GetUUID().IsValid();
 }
 
 bool SBModuleSpec::GetDescription(lldb::SBStream &description) {
   m_opaque_ap->Dump(description.ref());
   return true;
 }
 
 SBModuleSpecList::SBModuleSpecList() : m_opaque_ap(new ModuleSpecList()) {}

source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp

for (uint32_t kext_summary_offset = 0;
                                             m_kext_summary_header.entry_size);
          ++i, kext_summary_offset += m_kext_summary_header.entry_size) {
       lldb::offset_t offset = kext_summary_offset;
       const void *name_data =
           extractor.GetData(&offset, KERNEL_MODULE_MAX_NAME);
       if (name_data == NULL)
         break;
       image_infos[i].SetName((const char *)name_data);
-      UUID uuid(extractor.GetData(&offset, 16), 16);
+      UUID uuid(extractor.GetData(&offset, 16), 16, false);
       image_infos[i].SetUUID(uuid);
       image_infos[i].SetLoadAddress(extractor.GetU64(&offset));
       image_infos[i].SetSize(extractor.GetU64(&offset));
     }
     if (i < image_infos.size())
       image_infos.resize(i);
   } else {
     image_infos.clear();

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp

if (data.ValidOffsetForDataOfSize(offset,
           const lldb::offset_t name_offset =
               load_cmd_offset + data.GetU32(&offset);
           const char *path = data.PeekCStr(name_offset);
           lc_id_dylinker->SetFile(path, true, FileSpec::Style::native);
         }
         break;
 
       case llvm::MachO::LC_UUID:
-        dylib_info.uuid.SetBytes(data.GetData(&offset, 16));
+        dylib_info.uuid.SetBytes(data.GetData(&offset, 16), 16, false);
         break;
 
       default:
         break;
       }
       // Set offset to be the beginning of the next load command.
       offset = load_cmd_offset + load_cmd.cmdsize;
     }
if (version_or_magic != static_cast<uint32_t>(-1) &&
       if (wordsize == 4) {
         sharedCacheUUID_address =
             all_image_infos + 84; // sharedCacheUUID <mach-o/dyld_images.h>
       }
       if (sharedCacheUUID_address != LLDB_INVALID_ADDRESS) {
         uuid_t shared_cache_uuid;
         if (m_process->ReadMemory(sharedCacheUUID_address, shared_cache_uuid,
                                   sizeof(uuid_t), err) == sizeof(uuid_t)) {
-          uuid.SetBytes(shared_cache_uuid);
+          uuid.SetBytes(shared_cache_uuid, 16, false);
           if (uuid.IsValid()) {
             using_shared_cache = eLazyBoolYes;
           }
         }
 
         if (version_or_magic >= 15) {
           // The sharedCacheBaseAddress field is the next one in the
           // dyld_all_image_infos struct.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

if (header.Parse(data, &header_offset)) {
                     CalculateELFNotesSegmentsCRC32(program_headers, data);
               } else {
                 gnu_debuglink_crc = calc_gnu_debuglink_crc32(
                     data.GetDataStart(), data.GetByteSize());
               }
             }
             if (gnu_debuglink_crc) {
               // Use 4 bytes of crc from the .gnu_debuglink section.
               uint32_t uuidt[4] = {gnu_debuglink_crc, 0, 0, 0};
-              uuid.SetBytes(uuidt, sizeof(uuidt));
+              uuid.SetBytes(uuidt, sizeof(uuidt), true);
             } else if (core_notes_crc) {
               // Use 8 bytes - first 4 bytes for *magic* prefix, mainly to make
               // it look different form .gnu_debuglink crc followed by 4 bytes
               // of note segments crc.
               uint32_t uuidt[4] = {g_core_uuid_magic, core_notes_crc, 0, 0};
-              uuid.SetBytes(uuidt, sizeof(uuidt));
+              uuid.SetBytes(uuidt, sizeof(uuidt), true);
             }
           }
 
           specs.Append(spec);
         }
       }
     }
   }
if (m_uuid.IsValid()) {
 
     core_notes_crc = CalculateELFNotesSegmentsCRC32(m_program_headers, m_data);
 
     if (core_notes_crc) {
       // Use 8 bytes - first 4 bytes for *magic* prefix, mainly to make it look
       // different form .gnu_debuglink crc - followed by 4 bytes of note
       // segments crc.
       uint32_t uuidt[4] = {g_core_uuid_magic, core_notes_crc, 0, 0};
-      m_uuid.SetBytes(uuidt, sizeof(uuidt));
+      m_uuid.SetBytes(uuidt, sizeof(uuidt), true);
     }
   } else {
     if (!m_gnu_debuglink_crc)
       m_gnu_debuglink_crc =
           calc_gnu_debuglink_crc32(m_data.GetDataStart(), m_data.GetByteSize());
     if (m_gnu_debuglink_crc) {
       // Use 4 bytes of crc from the .gnu_debuglink section.
       uint32_t uuidt[4] = {m_gnu_debuglink_crc, 0, 0, 0};
-      m_uuid.SetBytes(uuidt, sizeof(uuidt));
+      m_uuid.SetBytes(uuidt, sizeof(uuidt), true);
     }
   }
 
   if (m_uuid.IsValid()) {
     *uuid = m_uuid;
     return true;
   }
 

source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

UUID ObjectFileMachO::GetSharedCacheUUID(FileSpec dyld_shared_cache,
   DataExtractor dsc_header_data(DscData, byte_order, addr_byte_size);
 
   char version_str[7];
   lldb::offset_t offset = 0;
   memcpy(version_str, dsc_header_data.GetData(&offset, 6), 6);
   version_str[6] = '\0';
   if (strcmp(version_str, "dyld_v") == 0) {
     offset = offsetof(struct lldb_copy_dyld_cache_header_v1, uuid);
-    uint8_t uuid_bytes[sizeof(uuid_t)];
-    memcpy(uuid_bytes, dsc_header_data.GetData(&offset, sizeof(uuid_t)),
-           sizeof(uuid_t));
-    dsc_uuid.SetBytes(uuid_bytes);
+    dsc_uuid.SetBytes(dsc_header_data.GetData(&offset, sizeof(uuid_t)),
+                      sizeof(uuid_t), false);
   }
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_SYMBOLS));
   if (log && dsc_uuid.IsValid()) {
     log->Printf("Shared cache %s has UUID %s", dyld_shared_cache.GetPath().c_str(), 
                 dsc_uuid.GetAsString().c_str());
   }
   return dsc_uuid;
 }
if (load_cmd.cmd == LC_UUID) {
 
         const uint8_t opencl_uuid[] = {0x8c, 0x8e, 0xb3, 0x9b, 0x3b, 0xa8,
                                        0x4b, 0x16, 0xb6, 0xa4, 0x27, 0x63,
                                        0xbb, 0x14, 0xf0, 0x0d};
 
         if (!memcmp(uuid_bytes, opencl_uuid, 16))
           return false;
 
-        uuid.SetBytes(uuid_bytes);
+        uuid.SetBytes(uuid_bytes, 16, false);
         return true;
       }
       return false;
     }
     offset = cmd_offset + load_cmd.cmdsize;
   }
   return false;
 }
for (uint32_t i = 0; i < m_header.ncmds; ++i) {
               offset = fileoff;
               uint32_t version;
               if (m_data.GetU32 (&offset, &version, 1) != nullptr && version == 1)
               {
                   uint32_t type = 0;
                   uuid_t raw_uuid;
                   memset (raw_uuid, 0, sizeof (uuid_t));
 
-                  if (m_data.GetU32 (&offset, &type, 1)
-                      && m_data.GetU64 (&offset, &address, 1)
-                      && m_data.CopyData (offset, sizeof (uuid_t), raw_uuid) != 0
-                      && uuid.SetBytes (raw_uuid, sizeof (uuid_t)))
-                  {
-                      return true;
+                  if (m_data.GetU32(&offset, &type, 1) &&
+                      m_data.GetU64(&offset, &address, 1) &&
+                      m_data.CopyData(offset, sizeof(uuid_t), raw_uuid) != 0) {
+                    uuid.SetBytes(raw_uuid, sizeof(uuid_t), false);
                   }
               }
           }
       }
       offset = cmd_offset + lc.cmdsize;
     }
   }
   return false;

source/Plugins/Process/minidump/MinidumpParser.cpp

UUID MinidumpParser::GetModuleUUID(const MinidumpModule *module) {
   const CvSignature cv_signature =
       static_cast<CvSignature>(static_cast<const uint32_t>(*signature));
 
   if (cv_signature == CvSignature::Pdb70) {
     // PDB70 record
     const CvRecordPdb70 *pdb70_uuid = nullptr;
     Status error = consumeObject(cv_record, pdb70_uuid);
     if (!error.Fail())
-      return UUID(pdb70_uuid, sizeof(*pdb70_uuid));
-  } else if (cv_signature == CvSignature::ElfBuildId) {
-    // ELF BuildID (found in Breakpad/Crashpad generated minidumps)
-    //
-    // This is variable-length, but usually 20 bytes
-    // as the binutils ld default is a SHA-1 hash.
-    // (We'll handle only 16 and 20 bytes signatures,
-    // matching LLDB support for UUIDs)
-    //
-    if (cv_record.size() == 16 || cv_record.size() == 20)
-      return UUID(cv_record.data(), cv_record.size());
-  }
+      return UUID(pdb70_uuid, sizeof(*pdb70_uuid), true);
+  } else if (cv_signature == CvSignature::ElfBuildId)
+    return UUID(cv_record, true);
 
   return UUID();
 }
 
 llvm::ArrayRef<MinidumpThread> MinidumpParser::GetThreads() {
   llvm::ArrayRef<uint8_t> data = GetStream(MinidumpStreamType::ThreadList);
 
   if (data.size() == 0)

source/Utility/DataExtractor.cpp

lldb::offset_t DataExtractor::PutToLog(Log *log, offset_t start_offset,
   return offset; // Return the offset at which we ended up
 }
 
 //----------------------------------------------------------------------
 // DumpUUID
 //
 // Dump out a UUID starting at 'offset' bytes into the buffer
 //----------------------------------------------------------------------
 void DataExtractor::DumpUUID(Stream *s, offset_t offset) const {
   if (s) {
-    const uint8_t *uuid_data = PeekData(offset, 16);
-    if (uuid_data) {
-      lldb_private::UUID uuid(uuid_data, 16);
+    if (UUID uuid{PeekData(offset, 16), 16, true})
       uuid.Dump(s);
-    } else {
+    else {
       s->Printf("<not enough data for UUID at offset 0x%8.8" PRIx64 ">",
                 offset);
     }
   }
 }
 
 size_t DataExtractor::Copy(DataExtractor &dest_data) const {
   if (m_data_sp) {

source/Utility/UUID.cpp

 
 // C Includes
 #include <ctype.h>
 #include <stdio.h>
 #include <string.h>
 
 using namespace lldb_private;
 
-UUID::UUID() { Clear(); }
-
-UUID::UUID(const UUID &rhs) {
-  SetBytes(rhs.m_uuid, rhs.m_num_uuid_bytes);
-}
-
-UUID::UUID(const void *uuid_bytes, uint32_t num_uuid_bytes) {
-  SetBytes(uuid_bytes, num_uuid_bytes);
-}
-
-const UUID &UUID::operator=(const UUID &rhs) {
-  if (this != &rhs) {
-    m_num_uuid_bytes = rhs.m_num_uuid_bytes;
-    ::memcpy(m_uuid, rhs.m_uuid, sizeof(m_uuid));
-  }
-  return *this;
-}
-
-UUID::~UUID() {}
-
-void UUID::Clear() {
-  m_num_uuid_bytes = 16;
-  ::memset(m_uuid, 0, sizeof(m_uuid));
+UUID::UUID(const void *uuid_bytes, uint32_t num_uuid_bytes, bool accept_zeroes) {
+  SetBytes(uuid_bytes, num_uuid_bytes, accept_zeroes);
 }
 
 std::string UUID::GetAsString(const char *separator) const {
   std::string result;
   char buf[256];
   if (!separator)
     separator = "-";
   const uint8_t *u = GetBytes().data();
std::string UUID::GetAsString(const char *separator) const {
   }
   return result;
 }
 
 void UUID::Dump(Stream *s) const {
   s->PutCString(GetAsString().c_str());
 }
 
-bool UUID::SetBytes(const void *uuid_bytes, uint32_t num_uuid_bytes) {
-  if (uuid_bytes) {
-    switch (num_uuid_bytes) {
-    case 20:
-      m_num_uuid_bytes = 20;
-      break;
-    case 16:
-      m_num_uuid_bytes = 16;
-      m_uuid[16] = m_uuid[17] = m_uuid[18] = m_uuid[19] = 0;
-      break;
-    default:
-      // Unsupported UUID byte size
-      m_num_uuid_bytes = 0;
-      break;
-    }
+void UUID::SetBytes(const void *uuid_bytes, uint32_t num_uuid_bytes, bool accept_zeroes) {
+  if (uuid_bytes)
+    SetBytes({reinterpret_cast<const uint8_t *>(uuid_bytes), num_uuid_bytes},
+             accept_zeroes);
 
-    if (m_num_uuid_bytes > 0) {
-      ::memcpy(m_uuid, uuid_bytes, m_num_uuid_bytes);
-      return true;
-    }
-  }
-  ::memset(m_uuid, 0, sizeof(m_uuid));
-  return false;
-}
+void UUID::SetBytes(llvm::ArrayRef<uint8_t> bytes, bool accept_zeroes) {
+  if (bytes.size() != 20 && bytes.size() != 16)
+    bytes = {};
+  if (!accept_zeroes && llvm::all_of(bytes, [](uint8_t b) { return b == 0; }))
+    bytes = {};
 
-bool UUID::IsValid() const {
-  return m_uuid[0] || m_uuid[1] || m_uuid[2] || m_uuid[3] || m_uuid[4] ||
-         m_uuid[5] || m_uuid[6] || m_uuid[7] || m_uuid[8] || m_uuid[9] ||
-         m_uuid[10] || m_uuid[11] || m_uuid[12] || m_uuid[13] || m_uuid[14] ||
-         m_uuid[15] || m_uuid[16] || m_uuid[17] || m_uuid[18] || m_uuid[19];
+  m_num_uuid_bytes = bytes.size();
+  std::memcpy(m_uuid, bytes.data(), bytes.size());
 }
 
 static inline int xdigit_to_int(char ch) {
   ch = tolower(ch);
   if (ch >= 'a' && ch <= 'f')
     return 10 + ch - 'a';
   return ch - '0';
 }

 }
 
 size_t UUID::SetFromStringRef(llvm::StringRef str, uint32_t num_uuid_bytes) {
   llvm::StringRef p = str;
 
   // Skip leading whitespace characters
   p = p.ltrim();
 
+  ValueType bytes;
   uint32_t bytes_decoded = 0;
   llvm::StringRef rest =
-      UUID::DecodeUUIDBytesFromString(p, m_uuid, bytes_decoded, num_uuid_bytes);
+      UUID::DecodeUUIDBytesFromString(p, bytes, bytes_decoded, num_uuid_bytes);
 
   // If we successfully decoded a UUID, return the amount of characters that
   // were consumed
   if (bytes_decoded == num_uuid_bytes) {
-    m_num_uuid_bytes = num_uuid_bytes;
+    SetBytes(bytes, bytes_decoded, true);
     return str.size() - rest.size();
   }
 
   // Else return zero to indicate we were not able to parse a UUID value
   return 0;
 }
 
 bool lldb_private::operator==(const lldb_private::UUID &lhs,

unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

HandlePacket(
       R"([{"uuid":"404142434445464748494a4b4c4d4e4f","triple":"i386-pc-linux",)"
       R"("file_path":"/foo/bar.so","file_offset":0,"file_size":1234}]])");
 
   auto result = async_result.get();
   ASSERT_TRUE(result.hasValue());
   ASSERT_EQ(1u, result->size());
   EXPECT_EQ("/foo/bar.so", result.getValue()[0].GetFileSpec().GetPath());
   EXPECT_EQ(triple, result.getValue()[0].GetArchitecture().GetTriple());
-  EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16), result.getValue()[0].GetUUID());
+  EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), result.getValue()[0].GetUUID());
   EXPECT_EQ(0u, result.getValue()[0].GetObjectOffset());
   EXPECT_EQ(1234u, result.getValue()[0].GetObjectSize());
 }
 
 TEST_F(GDBRemoteCommunicationClientTest, GetModulesInfo_UUID20) {
   llvm::Triple triple("i386-pc-linux");
 
   FileSpec file_spec("/foo/bar.so", false, FileSpec::Style::posix);
   std::future<llvm::Optional<std::vector<ModuleSpec>>> async_result =
       std::async(std::launch::async,
                  [&] { return client.GetModulesInfo(file_spec, triple); });
   HandlePacket(
       server,
       "jModulesInfo:["
       R"({"file":"/foo/bar.so","triple":"i386-pc-linux"}])",
       R"([{"uuid":"404142434445464748494a4b4c4d4e4f50515253","triple":"i386-pc-linux",)"
       R"("file_path":"/foo/bar.so","file_offset":0,"file_size":1234}]])");
 
   auto result = async_result.get();
   ASSERT_TRUE(result.hasValue());
   ASSERT_EQ(1u, result->size());
   EXPECT_EQ("/foo/bar.so", result.getValue()[0].GetFileSpec().GetPath());
   EXPECT_EQ(triple, result.getValue()[0].GetArchitecture().GetTriple());
-  EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20), result.getValue()[0].GetUUID());
+  EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20, true),
+            result.getValue()[0].GetUUID());
   EXPECT_EQ(0u, result.getValue()[0].GetObjectOffset());
   EXPECT_EQ(1234u, result.getValue()[0].GetObjectSize());
 }
 
 TEST_F(GDBRemoteCommunicationClientTest, GetModulesInfoInvalidResponse) {
   llvm::Triple triple("i386-pc-linux");
   FileSpec file_spec("/foo/bar.so", false, FileSpec::Style::posix);
 

unittests/Utility/UUIDTest.cpp

 #include "gtest/gtest.h"
 
 #include "lldb/Utility/UUID.h"
 
 using namespace lldb_private;
 
 TEST(UUIDTest, RelationalOperators) {
   UUID empty;
-  UUID a16("1234567890123456", 16);
-  UUID b16("1234567890123457", 16);
-  UUID a20("12345678901234567890", 20);
-  UUID b20("12345678900987654321", 20);
+  UUID a16("1234567890123456", 16, true);
+  UUID b16("1234567890123457", 16, true);
+  UUID a20("12345678901234567890", 20, true);
+  UUID b20("12345678900987654321", 20, true);
 
   EXPECT_EQ(empty, empty);
   EXPECT_EQ(a16, a16);
   EXPECT_EQ(a20, a20);
 
   EXPECT_NE(a16, b16);
   EXPECT_NE(a20, b20);
   EXPECT_NE(a16, a20);
   EXPECT_NE(empty, a16);
 
   EXPECT_LT(empty, a16);
   EXPECT_LT(a16, a20);
   EXPECT_LT(a16, b16);
   EXPECT_GT(a20, b20);
 }
+
+TEST(UUIDTest, Validity) {
+  UUID empty;
+  std::vector<uint8_t> zeroes(20, 0);
+  UUID a16(zeroes.data(), 16, true);
+  UUID a20(zeroes.data(), 20, true);
+  UUID a16_0(zeroes.data(), 16, false);
+  UUID a20_0(zeroes.data(), 20, false);
+  EXPECT_FALSE(empty);
+  EXPECT_TRUE(a16);
+  EXPECT_TRUE(a20);
+  EXPECT_FALSE(a16_0);
+  EXPECT_FALSE(a20_0);
+}
+
+TEST(UUIDTest, SetFromStringRef) {
+  UUID u;
+  EXPECT_EQ(32, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
+  EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), u);
+
+  EXPECT_EQ(36, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+  EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), u);
+
+  EXPECT_EQ(45, u.SetFromStringRef(
+                    "40-41-42-43-4445464748494a4b4c4d4e4f-50515253", 20));
+  EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20, true), u);
+
+  EXPECT_EQ(0, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f", 20));
+  EXPECT_EQ(0, u.SetFromStringRef("40xxxxx"));
+  EXPECT_EQ(0, u.SetFromStringRef(""));
+  EXPECT_EQ(UUID("@ABCDEFGHIJKLMNOPQRS", 20, true), u)
+      << "uuid was changed by failed parse calls";
+
+  EXPECT_EQ(
+      32, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253", 16));
+  EXPECT_EQ(UUID("@ABCDEFGHIJKLMNO", 16, true), u);
+}