Relax size constraints on UUID objects

[sas]

Summary

For some reason, the current implementation of UUID assumes all UUIDs
will always be either 16 or 20 bytes. I saw some 8-bytes long UUIDs on
some builds, and there doesn't seem to be any obvious reason why we
should enforce 16 or 20 byte-long UUIDs.

Ran check-lldb to ensure this doesn't break anything.

[zturner]

If we're going to support arbitrarily sized UUIDs, maybe just make the class member a std::vector?

[davide]

Stephane, can we please add unittests for UUID? [I don't see any of them in tree] It shouldn't be particularly hard.
If it is, I'd love to take this an example of why it's hard and discuss on how we can improve.
Thanks for your understanding :)

[clayborg]

You can abandon https://reviews.llvm.org/D40537 and just do the fixes I suggested in that change here: init m_num_uuid_bytes to zero in constructor and in Clear() and change UUID::IsValid() to just return "m_num_uuid_bytes > 0". I agree we should relax the UUID length.

source/Utility/UUID.cpp

48

m_num_uuid_bytes = 0;

59–78

If we are going to relax the UUID size to anything between 0 and 20, then this function needs to print the correct number of digits. We shouldn't show more. This would allow us to set the UUID from the debug info CRC as a 4 byte UUID. Currently we display the UUID as a 20 byte version with only the first 4 bytes filled in.

102–103

Change to:

return m_num_uuid_bytes > 0;

include/lldb/Utility/UUID.h

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;
   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/Utility/UUID.cpp

if (this != &rhs) {
     ::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));
 }
 
 const void *UUID::GetBytes() const { return m_uuid; }
 
 std::string UUID::GetAsString(const char *separator) const {
   std::string result;
   char buf[256];
   if (!separator)
     separator = "-";
-  const uint8_t *u = (const uint8_t *)GetBytes();
-  if (sizeof(buf) >
-      (size_t)snprintf(buf, sizeof(buf), "%2.2X%2.2X%2.2X%2.2X%s%2.2X%2.2X%s%2."
-                                         "2X%2.2X%s%2.2X%2.2X%s%2.2X%2.2X%2.2X%"
-                                         "2.2X%2.2X%2.2X",
-                       u[0], u[1], u[2], u[3], separator, u[4], u[5], separator,
-                       u[6], u[7], separator, u[8], u[9], separator, u[10],
-                       u[11], u[12], u[13], u[14], u[15])) {
-    result.append(buf);
-    if (m_num_uuid_bytes == 20) {
-      if (sizeof(buf) > (size_t)snprintf(buf, sizeof(buf),
-                                         "%s%2.2X%2.2X%2.2X%2.2X", separator,
-                                         u[16], u[17], u[18], u[19]))
-        result.append(buf);
-    }
-  }
+  auto u = reinterpret_cast<const uint8_t *>(GetBytes());
+  int required_bytes;
+
+  required_bytes =
+      snprintf(buf, sizeof(buf),
+               "%2.2X%2.2X%2.2X%2.2X%s%2.2X%2.2X%s%2.2X%2.2X%s"
+               "%2.2X%2.2X%s%2.2X%2.2X%2.2X%2.2X%2.2X%2.2X",
+               u[0], u[1], u[2], u[3], separator, u[4], u[5], separator, u[6],
+               u[7], separator, u[8], u[9], separator, u[10], u[11], u[12],
+               u[13], u[14], u[15]);
+  assert(required_bytes < sizeof(buf));
+
+  if (m_num_uuid_bytes > 16) {
+    required_bytes = snprintf(buf, sizeof(buf), "%s%2.2X%2.2X%2.2X%2.2X",
+                              separator, u[16], u[17], u[18], u[19]);
+    assert(required_bytes < sizeof(buf));
+
   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;
-    }
+  if (uuid_bytes == nullptr || num_uuid_bytes == 0) {
+    return false;
 
-    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;
-}
+  m_num_uuid_bytes = num_uuid_bytes;
+  ::memset(m_uuid + num_uuid_bytes, 0, sizeof(m_uuid) - m_num_uuid_bytes);
+
 
 size_t UUID::GetByteSize() const { return m_num_uuid_bytes; }
 
 bool UUID::IsValid() const {
   return std::any_of(std::begin(m_uuid), std::begin(m_uuid) + m_num_uuid_bytes,
                      [](uint32_t val) { return val != 0; });
 }
 
 static inline int xdigit_to_int(char ch) {
   ch = tolower(ch);
   if (ch >= 'a' && ch <= 'f')
     return 10 + ch - 'a';
   return ch - '0';
 }