Author: labath Date: Mon Feb 20 05:35:33 2017 New Revision: 295651 URL: http://llvm.org/viewvc/llvm-project?rev=295651&view=rev Log: 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 Added: lldb/trunk/unittests/Utility/NameMatchesTest.cpp Modified: lldb/trunk/include/lldb/Target/Process.h lldb/trunk/include/lldb/Utility/NameMatches.h lldb/trunk/include/lldb/lldb-private-enumerations.h lldb/trunk/source/Commands/CommandObjectPlatform.cpp lldb/trunk/source/Commands/CommandObjectProcess.cpp lldb/trunk/source/Core/ArchSpec.cpp lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp lldb/trunk/source/Target/Process.cpp lldb/trunk/source/Utility/NameMatches.cpp lldb/trunk/source/Utility/RegularExpression.cpp lldb/trunk/unittests/Utility/CMakeLists.txt Modified: lldb/trunk/include/lldb/Target/Process.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/Process.h?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/include/lldb/Target/Process.h (original) +++ lldb/trunk/include/lldb/Target/Process.h Mon Feb 20 05:35:33 2017 @@ -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; }; Modified: lldb/trunk/include/lldb/Utility/NameMatches.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/NameMatches.h?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/include/lldb/Utility/NameMatches.h (original) +++ lldb/trunk/include/lldb/Utility/NameMatches.h Mon Feb 20 05:35:33 2017 @@ -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); } Modified: lldb/trunk/include/lldb/lldb-private-enumerations.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/lldb-private-enumerations.h?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/include/lldb/lldb-private-enumerations.h (original) +++ lldb/trunk/include/lldb/lldb-private-enumerations.h Mon Feb 20 05:35:33 2017 @@ -116,19 +116,6 @@ typedef enum LazyBool { } LazyBool; //------------------------------------------------------------------ -/// Name matching -//------------------------------------------------------------------ -typedef enum NameMatchType { - eNameMatchIgnore, - eNameMatchEquals, - eNameMatchContains, - eNameMatchStartsWith, - eNameMatchEndsWith, - eNameMatchRegularExpression - -} NameMatchType; - -//------------------------------------------------------------------ /// Instruction types //------------------------------------------------------------------ typedef enum InstructionType { Modified: lldb/trunk/source/Commands/CommandObjectPlatform.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Commands/CommandObjectPlatform.cpp?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/source/Commands/CommandObjectPlatform.cpp (original) +++ lldb/trunk/source/Commands/CommandObjectPlatform.cpp Mon Feb 20 05:35:33 2017 @@ -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(); Modified: lldb/trunk/source/Commands/CommandObjectProcess.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Commands/CommandObjectProcess.cpp?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/source/Commands/CommandObjectProcess.cpp (original) +++ lldb/trunk/source/Commands/CommandObjectProcess.cpp Mon Feb 20 05:35:33 2017 @@ -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(); Modified: lldb/trunk/source/Core/ArchSpec.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/ArchSpec.cpp?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/source/Core/ArchSpec.cpp (original) +++ lldb/trunk/source/Core/ArchSpec.cpp Mon Feb 20 05:35:33 2017 @@ -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 { Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp Mon Feb 20 05:35:33 2017 @@ -1962,29 +1962,29 @@ uint32_t GDBRemoteCommunicationClient::F 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; } Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp Mon Feb 20 05:35:33 2017 @@ -360,16 +360,15 @@ GDBRemoteCommunicationServerCommon::Hand extractor.GetHexByteString(file); match_info.GetProcessInfo().GetExecutableFile().SetFile(file, false); } else if (key.equals("name_match")) { - NameMatchType name_match = - llvm::StringSwitch<NameMatchType>(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<NameMatch>(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; Modified: lldb/trunk/source/Target/Process.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/Process.cpp?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/source/Target/Process.cpp (original) +++ lldb/trunk/source/Target/Process.cpp Mon Feb 20 05:35:33 2017 @@ -581,7 +581,7 @@ llvm::ArrayRef<OptionDefinition> Process } 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::MatchAllP 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 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) { Modified: lldb/trunk/source/Utility/NameMatches.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/NameMatches.cpp?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/source/Utility/NameMatches.cpp (original) +++ lldb/trunk/source/Utility/NameMatches.cpp Mon Feb 20 05:35:33 2017 @@ -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; } Modified: lldb/trunk/source/Utility/RegularExpression.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/RegularExpression.cpp?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/source/Utility/RegularExpression.cpp (original) +++ lldb/trunk/source/Utility/RegularExpression.cpp Mon Feb 20 05:35:33 2017 @@ -81,14 +81,10 @@ RegularExpression::~RegularExpression() 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; } Modified: lldb/trunk/unittests/Utility/CMakeLists.txt URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/CMakeLists.txt?rev=295651&r1=295650&r2=295651&view=diff ============================================================================== --- lldb/trunk/unittests/Utility/CMakeLists.txt (original) +++ lldb/trunk/unittests/Utility/CMakeLists.txt Mon Feb 20 05:35:33 2017 @@ -1,6 +1,7 @@ add_lldb_unittest(UtilityTests ConstStringTest.cpp ErrorTest.cpp + NameMatchesTest.cpp StringExtractorTest.cpp TaskPoolTest.cpp TimeoutTest.cpp Added: lldb/trunk/unittests/Utility/NameMatchesTest.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/NameMatchesTest.cpp?rev=295651&view=auto ============================================================================== --- lldb/trunk/unittests/Utility/NameMatchesTest.cpp (added) +++ lldb/trunk/unittests/Utility/NameMatchesTest.cpp Mon Feb 20 05:35:33 2017 @@ -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")); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits