[lldb] Switch RegularExpression from llvm::Regex to std::regex

[kastiglione]

Summary

Use std::regex as the backing implementation of RegularExpression.

This provides common and expected regex features, from basic features like
\w and \d character classes, to more advanced features such as lookahead.

This change used to be gated on the minimum GCC version, because some versions
of GCC had incomplete support for std::regex, but the GCC required by llvm
for the past couple years should be sufficient, according to docs. At this time
GCC >=7.1 is required.

[kastiglione]

lldb/source/Utility/CMakeLists.txt

29

the std::regex constructor throws std::regex_error if the pattern is invalid. For this reason, exceptions are enabled for this one file.

lldb/source/Utility/RegularExpression.cpp

51

llvm::Regex considers the empty string to be an invalid pattern, while std::regex does not.

58

llvm::Regex considers the empty string to be an invalid pattern, while std::regex does not.

lldb/test/API/commands/breakpoint/set/func-regex/TestBreakpointRegexError.py

11–22

the error messages have changed.

lldb/test/API/functionalities/breakpoint/source_regexp/TestSourceRegexBreakpoints.py

37

exercise the features that aren't supported by llvm::Regex

69

exercise the features that aren't supported by llvm::Regex

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multiset/TestDataFormatterGenericMultiSet.py

132

previously, there was a ., which llvm::Regex matched to a newline, while std::regex does not, resulting in no match.

[JDevlieghere]

lldb/include/lldb/Utility/RegularExpression.h

93

There's an ongoing discussion on the forums [1] to replace llvm::Optional with std::optional. Until that's resolved we should stick with llvm's variant for consistency.

[1] https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716/10

lldb/source/Utility/CMakeLists.txt

29

What happens when exceptions are disabled? What does it mean to have this enabled for a single file? I don't know if it's part of the LLVM developer guide, but LLVM is supposed to build without RTTI and without exceptions.

[kastiglione]

lldb/include/lldb/Utility/RegularExpression.h

93

thanks for the details, I was actually planning to do this change, so that it would be easy to cherry pick into apple/llvm-project.

lldb/source/Utility/CMakeLists.txt

29

What happens when exceptions are disabled?

This cmake config enables exceptions for this one file, independent of LLVM_ENABLE_EH. No other source files will be allowed to catch or throw exceptions.

What does it mean to have this enabled for a single file?

It means this file can compile with a try/catch, and that inside this file, exceptions can be caught.

I don't know if it's part of the LLVM developer guide, but LLVM is supposed to build without RTTI and without exceptions.

llvm has LLVM_ENABLE_EH which allows llvm to be built with exceptions support enabled. Similarly, LLVM_ENABLE_RTTI allows RTTI to be enabled. It seems that both are disabled as a default, but not as a hard requirement.

I wondered if enabling RTTI would needed for exceptions, but at least for this code, the answer is no. The catch (const std::regex_error &e) block is exercised by TestBreakpointRegexError.py, so we know this code can and does catch an exception of that type, and accesses the error's member functions.

[kastiglione]

Switch to llvm::Optional

[kastiglione]

lldb/source/Utility/CMakeLists.txt

29

What makes me believe this use of exceptions is ok is that the API boundary continues to be exception free, callers continue to be exception disabled. Only the internal implementation, knows about exceptions.

[labath]

lldb/source/Utility/CMakeLists.txt

29

That is definitely not ok. ODR madness awaits therein. The standard library is full of functions that effectively do

#ifdef EXCEPTIONS
  something();
#else
  something_else();
#endif

Compiling just one file with exceptions enabled can cause two different versions of those functions to appear. It is sufficient that this file instantiates one function whose behavior is dependent on exceptions being available. When linking, the linker has to choose one of the two versions, and there's no telling which one will it use. This means that exceptions can creep in into the supposedly exception-free code, or vice-versa.

The only way this would be safe is if this code was in a shared library, and we took care to restrict the visibility of all symbols except the exported api of that library. And I don't think that's worth it.

[kastiglione]

lldb/source/Utility/CMakeLists.txt

29

@labath thanks for pointing that out. I had incomplete understanding and thought it could still work, but I see now that it could be an issue.

The only way this would be safe is if this code was in a shared library, and we took care to restrict the visibility of all symbols except the exported api of that library. And I don't think that's worth it.

what are your concerns with this approach?

btw I spoke to Louis Dionne of libc++ and he said that it could be possible to include whether -fno-exceptions is used in the libc++ ABI tag, which would allow mixing files built with and without exceptions.

[mib]

This would be pretty neat, if we're able to make it work without creating side-effects to other component.

[labath]

lldb/source/Utility/CMakeLists.txt

29

The only way this would be safe is if this code was in a shared library, and we took care to restrict the visibility of all symbols except the exported api of that library. And I don't think that's worth it.

what are your concerns with this approach?

Well.. I don't even know where to start.

For one, since a shared library is a shippable artifact, it requires adjustment to all distribution methods up- and down-stream. I know this would definitely be an issue for us, since google's internal build system has a strong distaste for shared libraries. This is more on the extreme end, but I expect that most of the other build/deployment systems would need to be adjusted as well.

Now you should shrug that off as a "downstream problem", but there's also something to be said about consistency. Even as things stand now, LLDB is pretty much the only llvm component, which _requires_ to be built with shared libraries. For this case, we at least have a fairly good reason for it -- python extension modules must be shared libraries (although one could still imagine building a extension-free lldb in static mode). Everything else in llvm works both in static and in dynamic modes (for better or worse, there are two dynamic modes).

For this feature, I think the reasoning is pretty weak, because we're essentially doing it to work around _another_ llvm policy. And unlike the shared library thing, this one is explicitly spelled out, in no uncertain terms. So we would immediately become outliers, not in one, but two ways. Actually, three, if you count the fact that we would be using a different regex implementation than the rest of llvm.

Why don't you try proposing an llvm-wide llvm::Regex->std::regex replacement? I expect the answer to be a resounding NO, but who knows, maybe I'll be surprised, and someone will even come up with a way to do this safely...

btw I spoke to Louis Dionne of libc++ and he said that it could be possible to include whether -fno-exceptions is used in the libc++ ABI tag, which would allow mixing files built with and without exceptions.

I think that would be a very cool feature of libc++, but given that libc++ is not the only host c++ library we use with lldb/llvm, I don't think it will help in this case.

However, I think it might be tractable to excise the regex component out of libc++ and pull it into llvm as a separate, independent library. There is already a precedent for that -- the demangler library. The reason we're doing that is because we don't want to maintain two demangler libraries (one for the runtime and one for the tools), but that is precisely the situation that we're in with llvm::Regex and libc++ std::regex (admittedly the demangler is a more central component of llvm than the regex engine). And since this internal library would not need to conform to any specific API, we could make it usable even without exceptions. I'm not saying this would be a walk in the park, but I think you could gather enough support to pull this off.

lldb/include/lldb/Utility/RegularExpression.h

 //===-- RegularExpression.h -------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_REGULAREXPRESSION_H
 #define LLDB_UTILITY_REGULAREXPRESSION_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Regex.h"
 
+#include <regex>
+
 namespace lldb_private {
 
 class RegularExpression {
 public:
   /// The default constructor that initializes the object state such that it
   /// contains no compiled regular expression.
   RegularExpression() = default;
 
public:
   bool operator==(const RegularExpression &rhs) const {
     return GetText() == rhs.GetText();
   }
 
 private:
   /// A copy of the original regular expression text.
   std::string m_regex_text;
   /// The compiled regular expression.
-  mutable llvm::Regex m_regex;
+  std::regex m_regex;
+  llvm::Optional<std::regex_error> m_regex_error;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_REGULAREXPRESSION_H

lldb/source/Utility/CMakeLists.txt

 if (CMAKE_SYSTEM_NAME MATCHES "Windows")
   list(APPEND LLDB_SYSTEM_LIBS ws2_32 rpcrt4)
 endif ()
 
 if (NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB )
     list(APPEND LLDB_SYSTEM_LIBS atomic)
 endif()
 
+set_source_files_properties(
+  RegularExpression.cpp
+  PROPERTIES COMPILE_OPTIONS
+  "-fcxx-exceptions"
+)
+
 add_lldb_library(lldbUtility
   ArchSpec.cpp
   Args.cpp
   Baton.cpp
   Broadcaster.cpp
   CompletionRequest.cpp
   Connection.cpp
   ConstString.cpp

lldb/source/Utility/RegularExpression.cpp

 //===-- RegularExpression.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Utility/RegularExpression.h"
+#include "llvm/ADT/StringRef.h"
 
 #include <string>
 
 using namespace lldb_private;
 
 RegularExpression::RegularExpression(llvm::StringRef str)
-    : m_regex_text(std::string(str)),
-      // m_regex does not reference str anymore after it is constructed.
-      m_regex(llvm::Regex(str)) {}
+    : m_regex_text(std::string(str)) {
+  try {
+    m_regex = m_regex_text;
+  } catch (const std::regex_error &e) {
+    m_regex_error = e;
+  }
+}
 
 RegularExpression::RegularExpression(const RegularExpression &rhs)
     : RegularExpression(rhs.GetText()) {}
 
 bool RegularExpression::Execute(
     llvm::StringRef str,
     llvm::SmallVectorImpl<llvm::StringRef> *matches) const {
   if (!IsValid())
     return false;
-  return m_regex.match(str, matches);
+
+  if (!matches)
+    return std::regex_search(str.begin(), str.end(), m_regex);
+
+  std::cmatch result;
+  if (!std::regex_search(str.begin(), str.end(), result, m_regex))
+    return false;
+
+  matches->clear();
+  for (const auto &sub : result) {
+    llvm::StringRef match(sub.first, sub.length());
+    matches->push_back(match);
+  }
+  return true;
 }
 
-bool RegularExpression::IsValid() const { return m_regex.isValid(); }
+bool RegularExpression::IsValid() const {
+  return !m_regex_text.empty() && !m_regex_error;
+}
 
 llvm::StringRef RegularExpression::GetText() const { return m_regex_text; }
 
 llvm::Error RegularExpression::GetError() const {
-  std::string error;
-  if (!m_regex.isValid(error))
+  if (!IsValid()) {
+    std::string error = m_regex_error ? m_regex_error->what() : "empty regex";
     return llvm::make_error<llvm::StringError>(error,
                                                llvm::inconvertibleErrorCode());
+  }
   return llvm::Error::success();
 }

lldb/test/API/commands/breakpoint/set/func-regex/TestBreakpointRegexError.py

 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 class TestCase(TestBase):
 
     @no_debug_info_test
     def test_error(self):
         self.expect("breakpoint set --func-regex (", error=True,
-                    substrs=["error: Function name regular expression could " +
-                             "not be compiled: parentheses not balanced"])
+                    substrs=["error: Function name regular expression could "
+                             "not be compiled: The expression contained mismatched ( and )."])
 
         # Point out if looks like the user provided a globbing expression.
         self.expect("breakpoint set --func-regex *a", error=True,
-                    substrs=["error: Function name regular expression could " +
-                             "not be compiled: repetition-operator operand invalid",
+                    substrs=["error: Function name regular expression could "
+                             "not be compiled: One of *?+{ was not preceded by a valid regular expression.",
                              "warning: Function name regex does not accept glob patterns."])
         self.expect("breakpoint set --func-regex ?a", error=True,
-                    substrs=["error: Function name regular expression could " +
-                             "not be compiled: repetition-operator operand invalid",
+                    substrs=["error: Function name regular expression could "
+                             "not be compiled: One of *?+{ was not preceded by a valid regular expression.",
                              "warning: Function name regex does not accept glob patterns."])
         # Make sure that warning is only shown for invalid regular expressions
         # that look like a globbing expression (i.e., they have a leading * or ?).
         self.expect("breakpoint set --func-regex a*+", error=True, matching=False,
                     substrs=["warning: Function name regex does not accept glob patterns."])
         self.expect("breakpoint set --func-regex a?+", error=True, matching=False,
                     substrs=["warning: Function name regex does not accept glob patterns."])

lldb/test/API/functionalities/breakpoint/source_regexp/TestSourceRegexBreakpoints.py

def source_regex_locations(self):
 
         # First look just in main:
         target_files = lldb.SBFileSpecList()
         target_files.Append(lldb.SBFileSpec("a.c"))
 
         func_names = lldb.SBStringList()
         func_names.AppendString("a_func")
 
-        source_regex = "Set . breakpoint here"
+        source_regex = r"Set \w\d? breakpoint here"
         main_break = target.BreakpointCreateBySourceRegex(
             source_regex, lldb.SBFileSpecList(), target_files, func_names)
         num_locations = main_break.GetNumLocations()
         self.assertEqual(
             num_locations, 1,
             "a.c in a_func should give one breakpoint, got %d." %
             (num_locations))
 
         loc = main_break.GetLocationAtIndex(0)
         self.assertTrue(loc.IsValid(), "Got a valid location.")
         address = loc.GetAddress()
         self.assertTrue(
             address.IsValid(),
             "Got a valid address from the location.")
 
-        a_func_line = line_number("a.c", "Set A breakpoint here")
+        a_func_line = line_number("a.c", "Set A1 breakpoint here")
         line_entry = address.GetLineEntry()
         self.assertTrue(line_entry.IsValid(), "Got a valid line entry.")
         self.assertEquals(line_entry.line, a_func_line,
                         "Our line number matches the one lldbtest found.")
 
     def source_regex_restrictions(self):
         """ Test that restricting source expressions to files & to functions. """
         # Create a target by the debugger.
         exe = self.getBuildArtifact("a.out")
         target = self.dbg.CreateTarget(exe)
         self.assertTrue(target, VALID_TARGET)
 
         # First look just in main:
         target_files = lldb.SBFileSpecList()
         target_files.Append(lldb.SBFileSpec("main.c"))
-        source_regex = "Set . breakpoint here"
+        source_regex = r"Set \w\d? breakpoint here"
         main_break = target.BreakpointCreateBySourceRegex(
             source_regex, lldb.SBFileSpecList(), target_files, lldb.SBStringList())
 
         num_locations = main_break.GetNumLocations()
         self.assertEqual(
             num_locations, 2,
             "main.c should have 2 matches, got %d." %
             (num_locations))

lldb/test/API/functionalities/breakpoint/source_regexp/a.c

 #include <stdio.h>
 
 #include "a.h"
 
 static int
 main_func(int input)
 {
-  return printf("Set B breakpoint here: %d", input);
+  return printf("Set B2 breakpoint here: %d", input);
 }
 
 int
 a_func(int input)
 {
-  input += 1; // Set A breakpoint here;
+  input += 1; // Set A1 breakpoint here;
   return main_func(input);
 }

lldb/test/API/functionalities/breakpoint/source_regexp/main.c

 #include <stdio.h>
 #include "a.h"
 
 int
 main_func(int input)
 {
-  return printf("Set B breakpoint here: %d.\n", input);
+  return printf("Set B0 breakpoint here: %d.\n", input);
 }
 
 int
 main()
 {
   a_func(10);
   main_func(10);
   printf("Set a breakpoint here:\n");
   return 0;
 }

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/multiset/TestDataFormatterGenericMultiSet.py

def do_test_with_run_command(self, stdlib_type):
                 '[0] = "a"',
                 '[1] = "a very long string is right here"',
                 '[2] = "c"'])
 
     def do_test_ref_and_ptr(self, stdlib_type):
         """Test that the data formatters work on ref and ptr."""
         self.build(dictionary={stdlib_type: "1"})
         (self.target, process, _, bkpt) = lldbutil.run_to_source_breakpoint(
-            self, "Stop here to check by ref and ptr.",
+            self, "Stop here to check by ref and ptr",
             lldb.SBFileSpec("main.cpp", False))
         # The reference should print just like the value:
         self.check("ref", 7)
 
         self.expect("frame variable ptr",
                     substrs=["ptr =", "size=7"])
         self.expect("expr ptr",
                     substrs=["size=7"])

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/set/TestDataFormatterGenericSet.py

class GenericSetDataFormatterTestCase(TestBase):
     def test_with_run_command_libcpp(self):
         self.do_test_with_run_command(USE_LIBCPP)
 
 
     def do_test_ref_and_ptr(self,stdlib_type):
         """Test that the data formatters work on ref and ptr."""
         self.build()
         (self.target, process, _, bkpt) = lldbutil.run_to_source_breakpoint(
-            self, "Stop here to check by ref and ptr.",
+            self, "Stop here to check by ref and ptr",
             lldb.SBFileSpec("main.cpp", False))
         # The reference should print just like the value:
         self.check("ref", 7)
         self.check("ptr", 7)
 
         self.expect("frame variable ptr",
                     substrs=["ptr =", "size=7"])
         self.expect("expr ptr",
                     substrs=["size=7"])
 
     @add_test_categories(["libstdcxx"]) 
     def test_ref_and_ptr_libstdcpp(self):
         self.do_test_ref_and_ptr(USE_LIBSTDCPP)
     
     @add_test_categories(["libc++"])
     def test_ref_and_ptr_libcpp(self):
         self.do_test_ref_and_ptr(USE_LIBCPP)
 No newline at end of file