labath created this revision.
labath added reviewers: clayborg, sas, lemo, davide.
Herald added subscribers: arichardson, emaste.
Herald added a reviewer: espindola.
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.
https://reviews.llvm.org/D48479
Files:
include/lldb/Utility/UUID.h
source/API/SBModuleSpec.cpp
source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
source/Plugins/Process/minidump/MinidumpParser.cpp
source/Utility/DataExtractor.cpp
source/Utility/UUID.cpp
unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
unittests/Utility/UUIDTest.cpp
Index: unittests/Utility/UUIDTest.cpp
===================================================================
--- unittests/Utility/UUIDTest.cpp
+++ unittests/Utility/UUIDTest.cpp
@@ -15,10 +15,10 @@
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);
@@ -34,3 +34,40 @@
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);
+}
Index: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===================================================================
--- unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -194,7 +194,7 @@
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());
}
@@ -218,7 +218,8 @@
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());
}
Index: source/Utility/UUID.cpp
===================================================================
--- source/Utility/UUID.cpp
+++ source/Utility/UUID.cpp
@@ -21,29 +21,8 @@
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 {
@@ -74,36 +53,20 @@
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 (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(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);
}
-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];
+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 = {};
+
+ m_num_uuid_bytes = bytes.size();
+ std::memcpy(m_uuid, bytes.data(), bytes.size());
}
static inline int xdigit_to_int(char ch) {
@@ -155,14 +118,15 @@
// 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();
}
Index: source/Utility/DataExtractor.cpp
===================================================================
--- source/Utility/DataExtractor.cpp
+++ source/Utility/DataExtractor.cpp
@@ -1100,11 +1100,9 @@
//----------------------------------------------------------------------
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);
}
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===================================================================
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -114,18 +114,9 @@
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();
}
Index: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===================================================================
--- source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -2087,10 +2087,8 @@
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()) {
@@ -4861,7 +4859,7 @@
if (!memcmp(uuid_bytes, opencl_uuid, 16))
return false;
- uuid.SetBytes(uuid_bytes);
+ uuid.SetBytes(uuid_bytes, 16, false);
return true;
}
return false;
@@ -5392,12 +5390,11 @@
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);
+ return true;
}
}
}
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===================================================================
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -733,13 +733,13 @@
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);
}
}
@@ -926,16 +926,16 @@
// 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);
}
}
Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
===================================================================
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
@@ -898,7 +898,7 @@
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:
@@ -1110,7 +1110,7 @@
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;
}
Index: source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===================================================================
--- source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -1368,7 +1368,7 @@
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));
Index: source/API/SBModuleSpec.cpp
===================================================================
--- source/API/SBModuleSpec.cpp
+++ source/API/SBModuleSpec.cpp
@@ -90,7 +90,8 @@
}
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) {
Index: include/lldb/Utility/UUID.h
===================================================================
--- include/lldb/Utility/UUID.h
+++ include/lldb/Utility/UUID.h
@@ -33,25 +33,28 @@
//------------------------------------------------------------------
// Constructors and Destructors
//------------------------------------------------------------------
- UUID();
- UUID(const UUID &rhs);
- UUID(const void *uuid_bytes, uint32_t num_uuid_bytes);
-
- ~UUID();
+ 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);
+ }
- const UUID &operator=(const UUID &rhs);
+ UUID &operator=(const UUID &rhs) = default;
- 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;
@@ -85,7 +88,7 @@
//------------------------------------------------------------------
// 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;
};
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
