[Lldb-commits] [lldb] 1beffc1 - Support build-ids of other sizes than 16 in UUID::SetFromStringRef
Jaroslav Sevcik via lldb-commits
lldb-commits at lists.llvm.org
Sun Jun 7 03:04:45 PDT 2020
Author: Jaroslav Sevcik
Date: 2020-06-07T10:03:41Z
New Revision: 1beffc18886ae4cd72dfe1f9b6b79204cac51e5e
URL: https://github.com/llvm/llvm-project/commit/1beffc18886ae4cd72dfe1f9b6b79204cac51e5e
DIFF: https://github.com/llvm/llvm-project/commit/1beffc18886ae4cd72dfe1f9b6b79204cac51e5e.diff
LOG: Support build-ids of other sizes than 16 in UUID::SetFromStringRef
SBTarget::AddModule currently handles the UUID parameter in a very
weird way: UUIDs with more than 16 bytes are trimmed to 16 bytes. On
the other hand, shorter-than-16-bytes UUIDs are completely ignored. In
this patch, we change the parsing code to handle UUIDs of arbitrary
size.
To support arbitrary size UUIDs in SBTarget::AddModule, this patch
changes UUID::SetFromStringRef to parse UUIDs of arbitrary length. We
subtly change the semantics of SetFromStringRef - SetFromStringRef now
only succeeds if the entire input is consumed to prevent some
prefix-parsing confusion. This is up for discussion, but I believe
this is more consistent - we always return false for invalid UUIDs
rather than sometimes truncating to a valid prefix. Also, all the
call-sites except the API and interpreter seem to expect to consume
the entire input.
This also adds tests for adding existing modules 4-, 16-, and 20-byte
build-ids. Finally, we took the liberty of testing the minidump
scenario we care about - removing placeholder module from minidump and
replacing it with the real module.
Reviewed By: labath, friss
Differential Revision: https://reviews.llvm.org/D80755
Added:
lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-elf-build-id-4.yaml
lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml
Modified:
lldb/include/lldb/Utility/UUID.h
lldb/source/Interpreter/OptionValueUUID.cpp
lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/source/Utility/UUID.cpp
lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
lldb/unittests/Target/ModuleCacheTest.cpp
lldb/unittests/Utility/UUIDTest.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Utility/UUID.h b/lldb/include/lldb/Utility/UUID.h
index 0284357be44a..5327719094c0 100644
--- a/lldb/include/lldb/Utility/UUID.h
+++ b/lldb/include/lldb/Utility/UUID.h
@@ -63,18 +63,13 @@ class UUID {
std::string GetAsString(llvm::StringRef separator = "-") const;
- size_t SetFromStringRef(llvm::StringRef str, uint32_t num_uuid_bytes = 16);
+ bool SetFromStringRef(llvm::StringRef str);
// Same as SetFromStringRef, but if the resultant UUID is all 0 bytes, set the
// UUID to invalid.
- size_t SetFromOptionalStringRef(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
- /// Decode as many UUID bytes (up to 16) as possible from the C
- /// string \a cstr.
+ bool SetFromOptionalStringRef(llvm::StringRef str);
+
+ /// Decode as many UUID bytes as possible from the C string \a cstr.
///
/// \param[in] str
/// An llvm::StringRef that points at a UUID string value (no leading
@@ -88,8 +83,7 @@ class UUID {
/// The original string, with all decoded bytes removed.
static llvm::StringRef
DecodeUUIDBytesFromString(llvm::StringRef str,
- llvm::SmallVectorImpl<uint8_t> &uuid_bytes,
- uint32_t num_uuid_bytes = 16);
+ llvm::SmallVectorImpl<uint8_t> &uuid_bytes);
private:
UUID(llvm::ArrayRef<uint8_t> bytes) : m_bytes(bytes.begin(), bytes.end()) {}
diff --git a/lldb/source/Interpreter/OptionValueUUID.cpp b/lldb/source/Interpreter/OptionValueUUID.cpp
index 4da839233472..2bd749773556 100644
--- a/lldb/source/Interpreter/OptionValueUUID.cpp
+++ b/lldb/source/Interpreter/OptionValueUUID.cpp
@@ -38,7 +38,7 @@ Status OptionValueUUID::SetValueFromString(llvm::StringRef value,
case eVarSetOperationReplace:
case eVarSetOperationAssign: {
- if (m_uuid.SetFromStringRef(value) == 0)
+ if (!m_uuid.SetFromStringRef(value))
error.SetErrorStringWithFormat("invalid uuid string value '%s'",
value.str().c_str());
else {
diff --git a/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp b/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
index 2b14ece5cfa3..bd8eeedce57d 100644
--- a/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
+++ b/lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
@@ -205,7 +205,7 @@ llvm::Optional<InfoRecord> InfoRecord::parse(llvm::StringRef Line) {
// use this as the UUID. Otherwise, we should revert back to the module ID.
UUID ID;
if (Line.trim().empty()) {
- if (Str.empty() || ID.SetFromStringRef(Str, Str.size() / 2) != Str.size())
+ if (Str.empty() || !ID.SetFromStringRef(Str))
return llvm::None;
}
return InfoRecord(std::move(ID));
diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp b/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp
index ae2627488d6a..06190d0c036d 100644
--- a/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp
+++ b/lldb/source/Plugins/Process/MacOSX-Kernel/CommunicationKDP.cpp
@@ -445,7 +445,7 @@ lldb_private::UUID CommunicationKDP::GetUUID() {
if (uuid_str.size() < 32)
return uuid;
- if (uuid.SetFromStringRef(uuid_str) == 0) {
+ if (!uuid.SetFromStringRef(uuid_str)) {
UUID invalid_uuid;
return invalid_uuid;
}
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index fdf1397d7a69..47aa150152ac 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -3623,7 +3623,7 @@ bool GDBRemoteCommunicationClient::GetModuleInfo(
StringExtractor extractor(value);
std::string uuid;
extractor.GetHexByteString(uuid);
- module_spec.GetUUID().SetFromStringRef(uuid, uuid.size() / 2);
+ module_spec.GetUUID().SetFromStringRef(uuid);
} else if (name == "triple") {
StringExtractor extractor(value);
std::string triple;
@@ -3659,8 +3659,7 @@ ParseModuleSpec(StructuredData::Dictionary *dict) {
if (!dict->GetValueForKeyAsString("uuid", string))
return llvm::None;
- if (result.GetUUID().SetFromStringRef(string, string.size() / 2) !=
- string.size())
+ if (!result.GetUUID().SetFromStringRef(string))
return llvm::None;
if (!dict->GetValueForKeyAsInteger("file_offset", integer))
diff --git a/lldb/source/Utility/UUID.cpp b/lldb/source/Utility/UUID.cpp
index 9d7a3d557731..4177b43de818 100644
--- a/lldb/source/Utility/UUID.cpp
+++ b/lldb/source/Utility/UUID.cpp
@@ -61,10 +61,9 @@ static inline int xdigit_to_int(char ch) {
llvm::StringRef
UUID::DecodeUUIDBytesFromString(llvm::StringRef p,
- llvm::SmallVectorImpl<uint8_t> &uuid_bytes,
- uint32_t num_uuid_bytes) {
+ llvm::SmallVectorImpl<uint8_t> &uuid_bytes) {
uuid_bytes.clear();
- while (!p.empty()) {
+ while (p.size() >= 2) {
if (isxdigit(p[0]) && isxdigit(p[1])) {
int hi_nibble = xdigit_to_int(p[0]);
int lo_nibble = xdigit_to_int(p[1]);
@@ -73,11 +72,6 @@ UUID::DecodeUUIDBytesFromString(llvm::StringRef p,
// Skip both hex digits
p = p.drop_front(2);
-
- // Increment the byte that we are decoding within the UUID value and
- // break out if we are done
- if (uuid_bytes.size() == num_uuid_bytes)
- break;
} else if (p.front() == '-') {
// Skip dashes
p = p.drop_front();
@@ -89,35 +83,30 @@ UUID::DecodeUUIDBytesFromString(llvm::StringRef p,
return p;
}
-size_t UUID::SetFromStringRef(llvm::StringRef str, uint32_t num_uuid_bytes) {
+bool UUID::SetFromStringRef(llvm::StringRef str) {
llvm::StringRef p = str;
// Skip leading whitespace characters
p = p.ltrim();
llvm::SmallVector<uint8_t, 20> bytes;
- llvm::StringRef rest =
- UUID::DecodeUUIDBytesFromString(p, bytes, num_uuid_bytes);
-
- // If we successfully decoded a UUID, return the amount of characters that
- // were consumed
- if (bytes.size() == num_uuid_bytes) {
- *this = fromData(bytes);
- return str.size() - rest.size();
- }
+ llvm::StringRef rest = UUID::DecodeUUIDBytesFromString(p, bytes);
+
+ // Return false if we could not consume the entire string or if the parsed
+ // UUID is empty.
+ if (!rest.empty() || bytes.empty())
+ return false;
- // Else return zero to indicate we were not able to parse a UUID value
- return 0;
+ *this = fromData(bytes);
+ return true;
}
-size_t UUID::SetFromOptionalStringRef(llvm::StringRef str,
- uint32_t num_uuid_bytes) {
- size_t num_chars_consumed = SetFromStringRef(str, num_uuid_bytes);
- if (num_chars_consumed) {
+bool UUID::SetFromOptionalStringRef(llvm::StringRef str) {
+ bool result = SetFromStringRef(str);
+ if (result) {
if (llvm::all_of(m_bytes, [](uint8_t b) { return b == 0; }))
Clear();
}
-
- return num_chars_consumed;
-}
+ return result;
+}
diff --git a/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py b/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
index c57aa98d934c..2229ba05149d 100644
--- a/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
+++ b/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
@@ -185,3 +185,85 @@ def test_relative_module_name(self):
self.getSourcePath("relative_module_name.yaml"))
self.assertEqual(1, len(modules))
self.verify_module(modules[0], name, None)
+
+ def test_add_module_build_id_16(self):
+ """
+ Test that adding module with 16 byte UUID returns the existing
+ module or fails.
+ """
+ modules = self.get_minidump_modules("linux-arm-uuids-elf-build-id-16.yaml")
+ self.assertEqual(2, len(modules))
+
+ # Add the existing modules.
+ self.assertEqual(modules[0], self.target.AddModule(
+ "/some/local/a", "", "01020304-0506-0708-090A-0B0C0D0E0F10"))
+ self.assertEqual(modules[1], self.target.AddModule(
+ "/some/local/b", "", "0A141E28-323C-4650-5A64-6E78828C96A0"))
+
+ # Adding modules with non-existing UUID should fail.
+ self.assertFalse(
+ self.target.AddModule(
+ "a", "", "12345678-1234-1234-1234-123456789ABC").IsValid())
+ self.assertFalse(
+ self.target.AddModule(
+ "a", "", "01020304-0506-0708-090A-0B0C0D0E0F10-12345678").IsValid())
+
+ def test_add_module_build_id_20(self):
+ """
+ Test that adding module with 20 byte UUID returns the existing
+ module or fails.
+ """
+ modules = self.get_minidump_modules("linux-arm-uuids-elf-build-id-20.yaml")
+
+ # Add the existing modules.
+ self.assertEqual(modules[0], self.target.AddModule(
+ "/some/local/a", "", "01020304-0506-0708-090A-0B0C0D0E0F10-11121314"))
+ self.assertEqual(modules[1], self.target.AddModule(
+ "/some/local/b", "", "0A141E28-323C-4650-5A64-6E78828C96A0-AAB4BEC8"))
+
+ # Adding modules with non-existing UUID should fail.
+ self.assertFalse(
+ self.target.AddModule(
+ "a", "", "01020304-0506-0708-090A-0B0C0D0E0F10").IsValid())
+ self.assertFalse(
+ self.target.AddModule(
+ "a", "", "01020304-0506-0708-090A-0B0C0D0E0F10-12345678").IsValid())
+
+ def test_add_module_build_id_4(self):
+ """
+ Test that adding module with 4 byte UUID returns the existing
+ module or fails.
+ """
+ modules = self.get_minidump_modules("linux-arm-uuids-elf-build-id-4.yaml")
+
+ # Add the existing modules.
+ self.assertEqual(modules[0], self.target.AddModule(
+ "/some/local/a.so", "", "01020304"))
+ self.assertEqual(modules[1], self.target.AddModule(
+ "/some/local/b.so", "", "0A141E28"))
+
+ # Adding modules with non-existing UUID should fail.
+ self.assertFalse(
+ self.target.AddModule(
+ "a", "", "01020304-0506-0708-090A-0B0C0D0E0F10").IsValid())
+ self.assertFalse(self.target.AddModule("a", "", "01020305").IsValid())
+
+ def test_remove_placeholder_add_real_module(self):
+ """
+ Test that removing a placeholder module and adding back the real
+ module succeeds.
+ """
+ so_path = self.getBuildArtifact("libuuidmatch.so")
+ self.yaml2obj("libuuidmatch.yaml", so_path)
+ modules = self.get_minidump_modules("linux-arm-uuids-match.yaml")
+
+ uuid = "7295E17C-6668-9E05-CBB5-DEE5003865D5-5267C116";
+ self.assertEqual(1, len(modules))
+ self.verify_module(modules[0], "/target/path/libuuidmatch.so",uuid)
+
+ self.target.RemoveModule(modules[0])
+ new_module = self.target.AddModule(so_path, "", uuid)
+
+ self.verify_module(new_module, so_path, uuid)
+ self.assertEqual(new_module, self.target.modules[0])
+ self.assertEqual(1, len(self.target.modules))
diff --git a/lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-elf-build-id-4.yaml b/lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-elf-build-id-4.yaml
new file mode 100644
index 000000000000..8af5651aea67
--- /dev/null
+++ b/lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-elf-build-id-4.yaml
@@ -0,0 +1,19 @@
+--- !minidump
+Streams:
+ - Type: SystemInfo
+ Processor Arch: ARM
+ Platform ID: Linux
+ CSD Version: '15E216'
+ CPU:
+ CPUID: 0x00000000
+ - Type: ModuleList
+ Modules:
+ - Base of Image: 0x0000000000001000
+ Size of Image: 0x00001000
+ Module Name: '/tmp/a.so'
+ CodeView Record: 4C45704201020304
+ - Base of Image: 0x0000000000001000
+ Size of Image: 0x00001000
+ Module Name: '/tmp/b.so'
+ CodeView Record: 4C4570420A141E28
+...
diff --git a/lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml b/lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml
new file mode 100644
index 000000000000..6be31848d2b6
--- /dev/null
+++ b/lldb/test/API/functionalities/postmortem/minidump-new/linux-arm-uuids-match.yaml
@@ -0,0 +1,15 @@
+--- !minidump
+Streams:
+ - Type: SystemInfo
+ Processor Arch: ARM
+ Platform ID: Linux
+ CSD Version: '15E216'
+ CPU:
+ CPUID: 0x00000000
+ - Type: ModuleList
+ Modules:
+ - Base of Image: 0x0000000000001000
+ Size of Image: 0x00001000
+ Module Name: '/target/path/libuuidmatch.so'
+ CodeView Record: 525344537295E17C66689E05CBB5DEE5003865D55267C116
+...
diff --git a/lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp b/lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
index b7df2e35f8e3..b9a650d5fafa 100644
--- a/lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
+++ b/lldb/unittests/ObjectFile/ELF/TestObjectFileELF.cpp
@@ -156,7 +156,7 @@ TEST_F(ObjectFileELFTest, GetModuleSpecifications_EarlySectionHeaders) {
ModuleSpec Spec;
ASSERT_TRUE(Specs.GetModuleSpecAtIndex(0, Spec)) ;
UUID Uuid;
- Uuid.SetFromStringRef("1b8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9", 20);
+ Uuid.SetFromStringRef("1b8a73ac238390e32a7ff4ac8ebe4d6a41ecf5c9");
EXPECT_EQ(Spec.GetUUID(), Uuid);
}
@@ -284,4 +284,4 @@ TEST_F(ObjectFileELFTest, GetSymtab_NoSymEntryPointArmAddressClass) {
auto entry_point_addr = module_sp->GetObjectFile()->GetEntryPointAddress();
ASSERT_EQ(entry_point_addr.GetAddressClass(), AddressClass::eCode);
-}
\ No newline at end of file
+}
diff --git a/lldb/unittests/Target/ModuleCacheTest.cpp b/lldb/unittests/Target/ModuleCacheTest.cpp
index 60e05379deec..273338c83695 100644
--- a/lldb/unittests/Target/ModuleCacheTest.cpp
+++ b/lldb/unittests/Target/ModuleCacheTest.cpp
@@ -41,7 +41,6 @@ static const char dummy_remote_dir[] = "bin";
static const char module_name[] = "TestModule.so";
static const char module_uuid[] =
"F4E7E991-9B61-6AD4-0073-561AC3D9FA10-C043A476";
-static const uint32_t uuid_bytes = 20;
static const size_t module_size = 5602;
static FileSpec GetDummyRemotePath() {
@@ -87,7 +86,7 @@ void ModuleCacheTest::TryGetAndPut(const FileSpec &cache_dir,
ModuleCache mc;
ModuleSpec module_spec;
module_spec.GetFileSpec() = GetDummyRemotePath();
- module_spec.GetUUID().SetFromStringRef(module_uuid, uuid_bytes);
+ module_spec.GetUUID().SetFromStringRef(module_uuid);
module_spec.SetObjectSize(module_size);
ModuleSP module_sp;
bool did_create;
diff --git a/lldb/unittests/Utility/UUIDTest.cpp b/lldb/unittests/Utility/UUIDTest.cpp
index 2a7a93c537f0..1c84315d402a 100644
--- a/lldb/unittests/Utility/UUIDTest.cpp
+++ b/lldb/unittests/Utility/UUIDTest.cpp
@@ -45,7 +45,7 @@ TEST(UUIDTest, Validity) {
from_str.SetFromStringRef("00000000-0000-0000-0000-000000000000");
UUID opt_from_str;
opt_from_str.SetFromOptionalStringRef("00000000-0000-0000-0000-000000000000");
-
+
EXPECT_FALSE(empty);
EXPECT_TRUE(a16);
EXPECT_TRUE(a20);
@@ -57,25 +57,30 @@ TEST(UUIDTest, Validity) {
TEST(UUIDTest, SetFromStringRef) {
UUID u;
- EXPECT_EQ(32u, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
+ EXPECT_TRUE(u.SetFromStringRef("404142434445464748494a4b4c4d4e4f"));
EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
- EXPECT_EQ(36u, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+ EXPECT_TRUE(u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
- EXPECT_EQ(45u, u.SetFromStringRef(
- "40-41-42-43-4445464748494a4b4c4d4e4f-50515253", 20));
+ EXPECT_TRUE(
+ u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f-50515253"));
EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u);
- EXPECT_EQ(0u, u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f", 20));
- EXPECT_EQ(0u, u.SetFromStringRef("40xxxxx"));
- EXPECT_EQ(0u, u.SetFromStringRef(""));
- EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u)
+ EXPECT_TRUE(u.SetFromStringRef("40-41-42-43-4445464748494a4b4c4d4e4f"));
+
+ EXPECT_FALSE(u.SetFromStringRef("40xxxxx"));
+ EXPECT_FALSE(u.SetFromStringRef(""));
+ EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u)
<< "uuid was changed by failed parse calls";
- EXPECT_EQ(
- 32u, u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253", 16));
- EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNO", 16), u);
+ EXPECT_TRUE(u.SetFromStringRef("404142434445464748494a4b4c4d4e4f-50515253"));
+ EXPECT_EQ(UUID::fromData("@ABCDEFGHIJKLMNOPQRS", 20), u);
+
+ EXPECT_TRUE(u.SetFromStringRef("40414243"));
+ EXPECT_EQ(UUID::fromData("@ABCD", 4), u);
+
+ EXPECT_FALSE(u.SetFromStringRef("4"));
}
TEST(UUIDTest, StringConverion) {
More information about the lldb-commits
mailing list