From c4a339510359d57c9d143c350911a2c528201e0e Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Mon, 20 Feb 2017 11:35:33 +0000 Subject: [PATCH] Fix a couple of corner cases in NameMatches Summary: I originally set out to move the NameMatches closer to the relevant function and add some unit tests. However, in the process I've found a couple of bugs in the implementation: - the early exits where not always correct: - (test==pattern) does not mean the match will always suceed because of regular expressions - pattern.empty() does not mean the match will fail because the "" is a valid prefix of any string So I cleaned up those and added some tests. The only tricky part here was that regcomp() implementation on darwin did not recognise the empty string as a regular expression and returned an REG_EMPTY error instead. The simples fix here seemed to be to replace the empty expression with an equivalent non-empty one. Reviewers: clayborg, zturner Subscribers: mgorny, lldb-commits Differential Revision: https://reviews.llvm.org/D30094 llvm-svn: 295651 --- lldb/include/lldb/Target/Process.h | 11 ++-- lldb/include/lldb/Utility/NameMatches.h | 14 ++++- lldb/include/lldb/lldb-private-enumerations.h | 13 ----- .../source/Commands/CommandObjectPlatform.cpp | 24 ++++---- lldb/source/Commands/CommandObjectProcess.cpp | 2 +- lldb/source/Core/ArchSpec.cpp | 2 +- .../GDBRemoteCommunicationClient.cpp | 14 ++--- .../GDBRemoteCommunicationServerCommon.cpp | 17 +++--- lldb/source/Target/Process.cpp | 8 +-- lldb/source/Utility/NameMatches.cpp | 25 +++----- lldb/source/Utility/RegularExpression.cpp | 12 ++-- lldb/unittests/Utility/CMakeLists.txt | 1 + lldb/unittests/Utility/NameMatchesTest.cpp | 58 +++++++++++++++++++ 13 files changed, 121 insertions(+), 80 deletions(-) create mode 100644 lldb/unittests/Utility/NameMatchesTest.cpp diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index da8f0da01d2f..0c3fc0770de2 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -48,6 +48,7 @@ #include "lldb/Target/QueueList.h" #include "lldb/Target/ThreadList.h" #include "lldb/Utility/Error.h" +#include "lldb/Utility/NameMatches.h" #include "lldb/lldb-private.h" #include "llvm/ADT/ArrayRef.h" @@ -305,11 +306,11 @@ public: class ProcessInstanceInfoMatch { public: ProcessInstanceInfoMatch() - : m_match_info(), m_name_match_type(eNameMatchIgnore), + : m_match_info(), m_name_match_type(NameMatch::Ignore), m_match_all_users(false) {} ProcessInstanceInfoMatch(const char *process_name, - NameMatchType process_name_match_type) + NameMatch process_name_match_type) : m_match_info(), m_name_match_type(process_name_match_type), m_match_all_users(false) { m_match_info.GetExecutableFile().SetFile(process_name, false); @@ -323,9 +324,9 @@ public: void SetMatchAllUsers(bool b) { m_match_all_users = b; } - NameMatchType GetNameMatchType() const { return m_name_match_type; } + NameMatch GetNameMatchType() const { return m_name_match_type; } - void SetNameMatchType(NameMatchType name_match_type) { + void SetNameMatchType(NameMatch name_match_type) { m_name_match_type = name_match_type; } @@ -338,7 +339,7 @@ public: protected: ProcessInstanceInfo m_match_info; - NameMatchType m_name_match_type; + NameMatch m_name_match_type; bool m_match_all_users; }; diff --git a/lldb/include/lldb/Utility/NameMatches.h b/lldb/include/lldb/Utility/NameMatches.h index 50ea7ba7f887..bc9ec703770a 100644 --- a/lldb/include/lldb/Utility/NameMatches.h +++ b/lldb/include/lldb/Utility/NameMatches.h @@ -9,12 +9,20 @@ #ifndef LLDB_UTILITY_NAMEMATCHES_H #define LLDB_UTILITY_NAMEMATCHES_H -#include "lldb/lldb-private-enumerations.h" - #include "llvm/ADT/StringRef.h" namespace lldb_private { -bool NameMatches(llvm::StringRef name, NameMatchType match_type, + +enum class NameMatch { + Ignore, + Equals, + Contains, + StartsWith, + EndsWith, + RegularExpression +}; + +bool NameMatches(llvm::StringRef name, NameMatch match_type, llvm::StringRef match); } diff --git a/lldb/include/lldb/lldb-private-enumerations.h b/lldb/include/lldb/lldb-private-enumerations.h index 8067568c4ad8..9572bee81177 100644 --- a/lldb/include/lldb/lldb-private-enumerations.h +++ b/lldb/include/lldb/lldb-private-enumerations.h @@ -115,19 +115,6 @@ typedef enum LazyBool { eLazyBoolYes = 1 } LazyBool; -//------------------------------------------------------------------ -/// Name matching -//------------------------------------------------------------------ -typedef enum NameMatchType { - eNameMatchIgnore, - eNameMatchEquals, - eNameMatchContains, - eNameMatchStartsWith, - eNameMatchEndsWith, - eNameMatchRegularExpression - -} NameMatchType; - //------------------------------------------------------------------ /// Instruction types //------------------------------------------------------------------ diff --git a/lldb/source/Commands/CommandObjectPlatform.cpp b/lldb/source/Commands/CommandObjectPlatform.cpp index efce4a058bbe..a8f83287ac00 100644 --- a/lldb/source/Commands/CommandObjectPlatform.cpp +++ b/lldb/source/Commands/CommandObjectPlatform.cpp @@ -1182,21 +1182,21 @@ protected: m_options.match_info.GetProcessInfo().GetName(); if (match_name && match_name[0]) { switch (m_options.match_info.GetNameMatchType()) { - case eNameMatchIgnore: + case NameMatch::Ignore: break; - case eNameMatchEquals: + case NameMatch::Equals: match_desc = "matched"; break; - case eNameMatchContains: + case NameMatch::Contains: match_desc = "contained"; break; - case eNameMatchStartsWith: + case NameMatch::StartsWith: match_desc = "started with"; break; - case eNameMatchEndsWith: + case NameMatch::EndsWith: match_desc = "ended with"; break; - case eNameMatchRegularExpression: + case NameMatch::RegularExpression: match_desc = "matched the regular expression"; break; } @@ -1342,31 +1342,31 @@ protected: case 'n': match_info.GetProcessInfo().GetExecutableFile().SetFile(option_arg, false); - match_info.SetNameMatchType(eNameMatchEquals); + match_info.SetNameMatchType(NameMatch::Equals); break; case 'e': match_info.GetProcessInfo().GetExecutableFile().SetFile(option_arg, false); - match_info.SetNameMatchType(eNameMatchEndsWith); + match_info.SetNameMatchType(NameMatch::EndsWith); break; case 's': match_info.GetProcessInfo().GetExecutableFile().SetFile(option_arg, false); - match_info.SetNameMatchType(eNameMatchStartsWith); + match_info.SetNameMatchType(NameMatch::StartsWith); break; case 'c': match_info.GetProcessInfo().GetExecutableFile().SetFile(option_arg, false); - match_info.SetNameMatchType(eNameMatchContains); + match_info.SetNameMatchType(NameMatch::Contains); break; case 'r': match_info.GetProcessInfo().GetExecutableFile().SetFile(option_arg, false); - match_info.SetNameMatchType(eNameMatchRegularExpression); + match_info.SetNameMatchType(NameMatch::RegularExpression); break; case 'A': @@ -1585,7 +1585,7 @@ public: if (partial_name) { match_info.GetProcessInfo().GetExecutableFile().SetFile( partial_name, false); - match_info.SetNameMatchType(eNameMatchStartsWith); + match_info.SetNameMatchType(NameMatch::StartsWith); } platform_sp->FindProcesses(match_info, process_infos); const uint32_t num_matches = process_infos.GetSize(); diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp index 5b7f1342328b..c6b1ee6aedc0 100644 --- a/lldb/source/Commands/CommandObjectProcess.cpp +++ b/lldb/source/Commands/CommandObjectProcess.cpp @@ -416,7 +416,7 @@ public: if (partial_name) { match_info.GetProcessInfo().GetExecutableFile().SetFile( partial_name, false); - match_info.SetNameMatchType(eNameMatchStartsWith); + match_info.SetNameMatchType(NameMatch::StartsWith); } platform_sp->FindProcesses(match_info, process_infos); const size_t num_matches = process_infos.GetSize(); diff --git a/lldb/source/Core/ArchSpec.cpp b/lldb/source/Core/ArchSpec.cpp index 330e3852e6c2..0d10ab7be441 100644 --- a/lldb/source/Core/ArchSpec.cpp +++ b/lldb/source/Core/ArchSpec.cpp @@ -259,7 +259,7 @@ struct ArchDefinition { size_t ArchSpec::AutoComplete(llvm::StringRef name, StringList &matches) { if (!name.empty()) { for (uint32_t i = 0; i < llvm::array_lengthof(g_core_definitions); ++i) { - if (NameMatches(g_core_definitions[i].name, eNameMatchStartsWith, name)) + if (NameMatches(g_core_definitions[i].name, NameMatch::StartsWith, name)) matches.AppendString(g_core_definitions[i].name); } } else { diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp index de11a3625cab..a8805d620090 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp @@ -1962,29 +1962,29 @@ uint32_t GDBRemoteCommunicationClient::FindProcesses( bool has_name_match = false; if (name && name[0]) { has_name_match = true; - NameMatchType name_match_type = match_info.GetNameMatchType(); + NameMatch name_match_type = match_info.GetNameMatchType(); switch (name_match_type) { - case eNameMatchIgnore: + case NameMatch::Ignore: has_name_match = false; break; - case eNameMatchEquals: + case NameMatch::Equals: packet.PutCString("name_match:equals;"); break; - case eNameMatchContains: + case NameMatch::Contains: packet.PutCString("name_match:contains;"); break; - case eNameMatchStartsWith: + case NameMatch::StartsWith: packet.PutCString("name_match:starts_with;"); break; - case eNameMatchEndsWith: + case NameMatch::EndsWith: packet.PutCString("name_match:ends_with;"); break; - case eNameMatchRegularExpression: + case NameMatch::RegularExpression: packet.PutCString("name_match:regex;"); break; } diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp index 34625247184d..4e3c91b0473a 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp @@ -360,16 +360,15 @@ GDBRemoteCommunicationServerCommon::Handle_qfProcessInfo( extractor.GetHexByteString(file); match_info.GetProcessInfo().GetExecutableFile().SetFile(file, false); } else if (key.equals("name_match")) { - NameMatchType name_match = - llvm::StringSwitch(value) - .Case("equals", eNameMatchEquals) - .Case("starts_with", eNameMatchStartsWith) - .Case("ends_with", eNameMatchEndsWith) - .Case("contains", eNameMatchContains) - .Case("regex", eNameMatchRegularExpression) - .Default(eNameMatchIgnore); + NameMatch name_match = llvm::StringSwitch(value) + .Case("equals", NameMatch::Equals) + .Case("starts_with", NameMatch::StartsWith) + .Case("ends_with", NameMatch::EndsWith) + .Case("contains", NameMatch::Contains) + .Case("regex", NameMatch::RegularExpression) + .Default(NameMatch::Ignore); match_info.SetNameMatchType(name_match); - if (name_match == eNameMatchIgnore) + if (name_match == NameMatch::Ignore) return SendErrorResponse(2); } else if (key.equals("pid")) { lldb::pid_t pid = LLDB_INVALID_PROCESS_ID; diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index f68a67c706aa..9b5150738f95 100644 --- a/lldb/source/Target/Process.cpp +++ b/lldb/source/Target/Process.cpp @@ -581,7 +581,7 @@ llvm::ArrayRef ProcessLaunchCommandOptions::GetDefinitions() { } bool ProcessInstanceInfoMatch::NameMatches(const char *process_name) const { - if (m_name_match_type == eNameMatchIgnore || process_name == nullptr) + if (m_name_match_type == NameMatch::Ignore || process_name == nullptr) return true; const char *match_name = m_match_info.GetName(); if (!match_name) @@ -627,7 +627,7 @@ bool ProcessInstanceInfoMatch::Matches( } bool ProcessInstanceInfoMatch::MatchAllProcesses() const { - if (m_name_match_type != eNameMatchIgnore) + if (m_name_match_type != NameMatch::Ignore) return false; if (m_match_info.ProcessIDIsValid()) @@ -659,7 +659,7 @@ bool ProcessInstanceInfoMatch::MatchAllProcesses() const { void ProcessInstanceInfoMatch::Clear() { m_match_info.Clear(); - m_name_match_type = eNameMatchIgnore; + m_name_match_type = NameMatch::Ignore; m_match_all_users = false; } @@ -3024,7 +3024,7 @@ Error Process::Attach(ProcessAttachInfo &attach_info) { if (platform_sp) { ProcessInstanceInfoMatch match_info; match_info.GetProcessInfo() = attach_info; - match_info.SetNameMatchType(eNameMatchEquals); + match_info.SetNameMatchType(NameMatch::Equals); platform_sp->FindProcesses(match_info, process_infos); const uint32_t num_matches = process_infos.GetSize(); if (num_matches == 1) { diff --git a/lldb/source/Utility/NameMatches.cpp b/lldb/source/Utility/NameMatches.cpp index 241fc76232ba..a76df3f929e8 100644 --- a/lldb/source/Utility/NameMatches.cpp +++ b/lldb/source/Utility/NameMatches.cpp @@ -13,32 +13,23 @@ using namespace lldb_private; -bool lldb_private::NameMatches(llvm::StringRef name, NameMatchType match_type, +bool lldb_private::NameMatches(llvm::StringRef name, NameMatch match_type, llvm::StringRef match) { - if (match_type == eNameMatchIgnore) - return true; - - if (name == match) - return true; - - if (name.empty() || match.empty()) - return false; - switch (match_type) { - case eNameMatchIgnore: // This case cannot occur: tested before + case NameMatch::Ignore: return true; - case eNameMatchEquals: + case NameMatch::Equals: return name == match; - case eNameMatchContains: + case NameMatch::Contains: return name.contains(match); - case eNameMatchStartsWith: + case NameMatch::StartsWith: return name.startswith(match); - case eNameMatchEndsWith: + case NameMatch::EndsWith: return name.endswith(match); - case eNameMatchRegularExpression: { + case NameMatch::RegularExpression: { RegularExpression regex(match); return regex.Execute(name); - } break; + } } return false; } diff --git a/lldb/source/Utility/RegularExpression.cpp b/lldb/source/Utility/RegularExpression.cpp index f25a35db0c07..15b183fdd661 100644 --- a/lldb/source/Utility/RegularExpression.cpp +++ b/lldb/source/Utility/RegularExpression.cpp @@ -81,14 +81,10 @@ RegularExpression::~RegularExpression() { Free(); } bool RegularExpression::Compile(llvm::StringRef str) { Free(); - if (!str.empty()) { - m_re = str; - m_comp_err = ::regcomp(&m_preg, m_re.c_str(), DEFAULT_COMPILE_FLAGS); - } else { - // No valid regular expression - m_comp_err = 1; - } - + // regcomp() on darwin does not recognize "" as a valid regular expression, so + // we substitute it with an equivalent non-empty one. + m_re = str.empty() ? "()" : str; + m_comp_err = ::regcomp(&m_preg, m_re.c_str(), DEFAULT_COMPILE_FLAGS); return m_comp_err == 0; } diff --git a/lldb/unittests/Utility/CMakeLists.txt b/lldb/unittests/Utility/CMakeLists.txt index cd6c19273bbb..9596abb8cfda 100644 --- a/lldb/unittests/Utility/CMakeLists.txt +++ b/lldb/unittests/Utility/CMakeLists.txt @@ -1,6 +1,7 @@ add_lldb_unittest(UtilityTests ConstStringTest.cpp ErrorTest.cpp + NameMatchesTest.cpp StringExtractorTest.cpp TaskPoolTest.cpp TimeoutTest.cpp diff --git a/lldb/unittests/Utility/NameMatchesTest.cpp b/lldb/unittests/Utility/NameMatchesTest.cpp new file mode 100644 index 000000000000..28ae16c7ddd5 --- /dev/null +++ b/lldb/unittests/Utility/NameMatchesTest.cpp @@ -0,0 +1,58 @@ +//===-- NameMatchesTest.cpp -------------------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "lldb/Utility/NameMatches.h" +#include "gtest/gtest.h" + +using namespace lldb_private; + +TEST(NameMatchesTest, Ignore) { + EXPECT_TRUE(NameMatches("foo", NameMatch::Ignore, "bar")); +} + +TEST(NameMatchesTest, Equals) { + EXPECT_TRUE(NameMatches("foo", NameMatch::Equals, "foo")); + EXPECT_FALSE(NameMatches("foo", NameMatch::Equals, "bar")); +} + +TEST(NameMatchesTest, Contains) { + EXPECT_TRUE(NameMatches("foobar", NameMatch::Contains, "foo")); + EXPECT_TRUE(NameMatches("foobar", NameMatch::Contains, "oob")); + EXPECT_TRUE(NameMatches("foobar", NameMatch::Contains, "bar")); + EXPECT_TRUE(NameMatches("foobar", NameMatch::Contains, "foobar")); + EXPECT_TRUE(NameMatches("", NameMatch::Contains, "")); + EXPECT_FALSE(NameMatches("", NameMatch::Contains, "foo")); + EXPECT_FALSE(NameMatches("foobar", NameMatch::Contains, "baz")); +} + +TEST(NameMatchesTest, StartsWith) { + EXPECT_TRUE(NameMatches("foo", NameMatch::StartsWith, "f")); + EXPECT_TRUE(NameMatches("foo", NameMatch::StartsWith, "")); + EXPECT_TRUE(NameMatches("", NameMatch::StartsWith, "")); + EXPECT_FALSE(NameMatches("foo", NameMatch::StartsWith, "b")); + EXPECT_FALSE(NameMatches("", NameMatch::StartsWith, "b")); +} + +TEST(NameMatchesTest, EndsWith) { + EXPECT_TRUE(NameMatches("foo", NameMatch::EndsWith, "o")); + EXPECT_TRUE(NameMatches("foo", NameMatch::EndsWith, "")); + EXPECT_TRUE(NameMatches("", NameMatch::EndsWith, "")); + EXPECT_FALSE(NameMatches("foo", NameMatch::EndsWith, "b")); + EXPECT_FALSE(NameMatches("", NameMatch::EndsWith, "b")); +} + +TEST(NameMatchesTest, RegularExpression) { + EXPECT_TRUE(NameMatches("foobar", NameMatch::RegularExpression, "foo")); + EXPECT_TRUE(NameMatches("foobar", NameMatch::RegularExpression, "f[oa]o")); + EXPECT_TRUE(NameMatches("foo", NameMatch::RegularExpression, "")); + EXPECT_TRUE(NameMatches("", NameMatch::RegularExpression, "")); + EXPECT_FALSE(NameMatches("foo", NameMatch::RegularExpression, "b")); + EXPECT_FALSE(NameMatches("", NameMatch::RegularExpression, "b")); + EXPECT_FALSE(NameMatches("^a", NameMatch::RegularExpression, "^a")); +}