Fix a couple of corner cases in NameMatches

[labath]

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.

lldb/trunk/include/lldb/Target/Process.h

 #include "lldb/Target/ExecutionContextScope.h"
 #include "lldb/Target/InstrumentationRuntime.h"
 #include "lldb/Target/Memory.h"
 #include "lldb/Target/ProcessInfo.h"
 #include "lldb/Target/ProcessLaunchInfo.h"
 #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"
 
 namespace lldb_private {
 
 template <typename B, typename S> struct Range;
 

 // ProcessInstanceInfoMatch
 //
 // A class to help matching one ProcessInstanceInfo to another.
 //----------------------------------------------------------------------
 
 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);
   }
 
   ProcessInstanceInfo &GetProcessInfo() { return m_match_info; }
 
   const ProcessInstanceInfo &GetProcessInfo() const { return m_match_info; }
 
   bool GetMatchAllUsers() const { return m_match_all_users; }
 
   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;
   }
 
   bool NameMatches(const char *process_name) const;
 
   bool Matches(const ProcessInstanceInfo &proc_info) const;
 
   bool MatchAllProcesses() const;
   void Clear();
 
 protected:
   ProcessInstanceInfo m_match_info;
-  NameMatchType m_name_match_type;
+  NameMatch m_name_match_type;
   bool m_match_all_users;
 };
 
 class ProcessInstanceInfoList {
 public:
   ProcessInstanceInfoList() = default;
 
   void Clear() { m_infos.clear(); }

lldb/trunk/include/lldb/Utility/NameMatches.h

 //===-- NameMatches.h -------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #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);
 }
 
 #endif

lldb/trunk/include/lldb/lldb-private-enumerations.h

 //----------------------------------------------------------------------
 typedef enum LazyBool {
   eLazyBoolCalculate = -1,
   eLazyBoolNo = 0,
   eLazyBoolYes = 1
 } LazyBool;
 
 //------------------------------------------------------------------
-/// Name matching
-//------------------------------------------------------------------
-typedef enum NameMatchType {
-  eNameMatchIgnore,
-  eNameMatchEquals,
-  eNameMatchContains,
-  eNameMatchStartsWith,
-  eNameMatchEndsWith,
-  eNameMatchRegularExpression
-
-} NameMatchType;
-
-//------------------------------------------------------------------
 /// Instruction types
 //------------------------------------------------------------------
 typedef enum InstructionType {
   eInstructionTypeAny, // Support for any instructions at all (at least one)
   eInstructionTypePrologueEpilogue, // All prologue and epilogue instructions
                                     // that push and pop register values and
                                     // modify sp/fp
   eInstructionTypePCModifying,      // Any instruction that modifies the program

lldb/trunk/source/Commands/CommandObjectPlatform.cpp

if (platform_sp) {
             ProcessInstanceInfoList proc_infos;
             const uint32_t matches =
                 platform_sp->FindProcesses(m_options.match_info, proc_infos);
             const char *match_desc = nullptr;
             const char *match_name =
                 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;
               }
             }
 
             if (matches == 0) {
               if (match_desc)
                 result.AppendErrorWithFormat(
Error SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
                         : PlatformSP();
         match_info.GetProcessInfo().GetArchitecture().SetTriple(
             option_arg, platform_sp.get());
       } break;
 
       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':
         show_args = true;
         break;
 
       case 'v':
         verbose = true;
bool HandleOptionArgumentCompletion(
 
         PlatformSP platform_sp(interpreter.GetPlatform(true));
         if (platform_sp) {
           ProcessInstanceInfoList process_infos;
           ProcessInstanceInfoMatch match_info;
           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();
           if (num_matches > 0) {
             for (uint32_t i = 0; i < num_matches; ++i) {
               matches.AppendString(
                   process_infos.GetProcessNameAtIndex(i),
                   process_infos.GetProcessNameLengthAtIndex(i));

lldb/trunk/source/Commands/CommandObjectProcess.cpp

bool HandleOptionArgumentCompletion(
 
         PlatformSP platform_sp(interpreter.GetPlatform(true));
         if (platform_sp) {
           ProcessInstanceInfoList process_infos;
           ProcessInstanceInfoMatch match_info;
           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();
           if (num_matches > 0) {
             for (size_t i = 0; i < num_matches; ++i) {
               matches.AppendString(
                   process_infos.GetProcessNameAtIndex(i),
                   process_infos.GetProcessNameLengthAtIndex(i));

lldb/trunk/source/Core/ArchSpec.cpp

struct ArchDefinition {
   size_t num_entries;
   const ArchDefinitionEntry *entries;
   const char *name;
 };
 
 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 {
     for (uint32_t i = 0; i < llvm::array_lengthof(g_core_definitions); ++i)
       matches.AppendString(g_core_definitions[i].name);
   }
   return matches.GetSize();
 }

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

if (m_supports_qfProcessInfo) {
     StreamString packet;
     packet.PutCString("qfProcessInfo");
     if (!match_info.MatchAllProcesses()) {
       packet.PutChar(':');
       const char *name = match_info.GetProcessInfo().GetName();
       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;
         }
         if (has_name_match) {
           packet.PutCString("name:");
           packet.PutBytesAsRawHex8(name, ::strlen(name));
           packet.PutChar(';');
         }

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

if (packet.GetChar() == ':') {
     while (packet.GetNameColonValue(key, value)) {
       bool success = true;
       if (key.equals("name")) {
         StringExtractor extractor(value);
         std::string file;
         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;
         if (value.getAsInteger(0, pid))
           return SendErrorResponse(2);
         match_info.GetProcessInfo().SetProcessID(pid);
       } else if (key.equals("parent_pid")) {
         lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;

lldb/trunk/source/Target/Process.cpp

static OptionDefinition g_process_launch_options[] = {
      "Set whether to shell expand arguments to the process when launching."},
 };
 
 llvm::ArrayRef<OptionDefinition> ProcessLaunchCommandOptions::GetDefinitions() {
   return llvm::makeArrayRef(g_process_launch_options);
 }
 
 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)
     return true;
 
   return lldb_private::NameMatches(process_name, m_name_match_type, match_name);
 }
 
bool ProcessInstanceInfoMatch::Matches(
   if (m_match_info.GetArchitecture().IsValid() &&
       !m_match_info.GetArchitecture().IsCompatibleMatch(
           proc_info.GetArchitecture()))
     return false;
   return true;
 }
 
 bool ProcessInstanceInfoMatch::MatchAllProcesses() const {
-  if (m_name_match_type != eNameMatchIgnore)
+  if (m_name_match_type != NameMatch::Ignore)
     return false;
 
   if (m_match_info.ProcessIDIsValid())
     return false;
 
   if (m_match_info.ParentProcessIDIsValid())
     return false;
 
bool ProcessInstanceInfoMatch::MatchAllProcesses() const {
   if (m_match_all_users)
     return false;
 
   return true;
 }
 
 void ProcessInstanceInfoMatch::Clear() {
   m_match_info.Clear();
-  m_name_match_type = eNameMatchIgnore;
+  m_name_match_type = NameMatch::Ignore;
   m_match_all_users = false;
 }
 
 ProcessSP Process::FindPlugin(lldb::TargetSP target_sp,
                               llvm::StringRef plugin_name,
                               ListenerSP listener_sp,
                               const FileSpec *crash_file_path) {
   static uint32_t g_process_unique_id = 0;
if (attach_info.GetExecutableFile().GetPath(process_name,
         }
       } else {
         ProcessInstanceInfoList process_infos;
         PlatformSP platform_sp(GetTarget().GetPlatform());
 
         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) {
             attach_pid = process_infos.GetProcessIDAtIndex(0);
             // Fall through and attach using the above process ID
           } else {
             match_info.GetProcessInfo().GetExecutableFile().GetPath(
                 process_name, sizeof(process_name));

lldb/trunk/source/Utility/NameMatches.cpp

 //===-- NameMatches.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 "lldb/Utility/RegularExpression.h"
 
 #include "llvm/ADT/StringRef.h"
 
 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;
 }

lldb/trunk/source/Utility/RegularExpression.cpp

 //
 // RETURNS
 //  True if the regular expression compiles successfully, false
 //  otherwise.
 //----------------------------------------------------------------------
 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;
   return m_comp_err == 0;
 }
 
 //----------------------------------------------------------------------
 // Execute a regular expression match using the compiled regular
 // expression that is already in this object against the match
 // string "s". If any parens are used for regular expression
 // matches "match_count" should indicate the number of regmatch_t

lldb/trunk/unittests/Utility/CMakeLists.txt

 add_lldb_unittest(UtilityTests
   ConstStringTest.cpp
   ErrorTest.cpp
+  NameMatchesTest.cpp
   StringExtractorTest.cpp
   TaskPoolTest.cpp
   TimeoutTest.cpp
   UriParserTest.cpp
   VASprintfTest.cpp
 
   LINK_LIBS
       lldbUtility
   LINK_COMPONENTS
     Support
   )

lldb/trunk/unittests/Utility/NameMatchesTest.cpp

+//===-- 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"));
+}