[lldb] Warn the user about starting the --func-regex parameter with an asterisk

[teemperor]

Summary

Sometimes users think that setting a function regex for all function that contain the word 'needle' in their
name looks like this: *needle*. However, LLDB only searches the function name and doesn't fully match
it against the regex, so the leading and trailing '*' operators don't do anything and actually just cause the
regex engine to reject the regular expression with "repetition-operator operand invalid".

This patch makes this a bit more obvious to the user by printing a warning that a leading '*' before this
regular expression here doesn't have any purpose (and will cause an error). This doesn't attempt to detect
a case where there is only a trailing '*' as that would involve parsing the regex and it seems the most
common way to end up in this situation is by doing rbreak *needle*.

[JDevlieghere]

I think you're conflating two problems here:

1. If you pass breakpoint set --func-regex foo it basically matches .*foo.*. So using a wildcards before or after the function name is redundant.
2. The regex command takes a regular expression and not a shell glob. Passing .*foo.* instead of *foo* works fine, though it's redundant per (1).

TL;DR I don't think this warning is warranted nor correct.

If we do want to print a warning, we should print something along the lines of "the regex command takes a regular expression and not a shell glob" for a leading *. In addition, as well as for a leading .* we should print another warning saying that lldb, for your convenience, will automatically add wildcards before and after the input as to match everything that contains <input> and so that these wildcards are redundant. For completeness we should do the same for ?.

[teemperor]

• Now warning for * and ?.
• Clarified warning a bit.

[teemperor]

Yeah, maybe D78808 is already good enough. I'll close this for now unless we hear again that the related error message is confusing

[teemperor]

@JDevlieghere I actually wanted to abandon this but somehow that didn't go through. You think we should land this or should we wait until we actually get someone that is confused by the new error message?

[JDevlieghere]

I think this would improve the user experience so I'm fine with landing this.

lldb/source/Commands/CommandObjectBreakpoint.cpp

bool DoExecute(Args &command, CommandReturnObject &result) override {
     case eSetTypeFunctionRegexp: // Breakpoint by regular expression function
                                  // name
     {
       RegularExpression regexp(m_options.m_func_regexp);
       if (llvm::Error err = regexp.GetError()) {
         result.AppendErrorWithFormat(
             "Function name regular expression could not be compiled: %s",
             llvm::toString(std::move(err)).c_str());
+        // Check if the incorrect regex looks like a globbing expression and
+        // warn the user about it.
+        if (!m_options.m_func_regexp.empty()) {
+          if (m_options.m_func_regexp[0] == '*' ||
+              m_options.m_func_regexp[0] == '?')
+            result.AppendWarning(
+                "Function name regex does not accept glob patterns.");
+        }
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
 
       bp_sp = target.CreateFuncRegexBreakpoint(
           &(m_options.m_modules), &(m_options.m_filenames), std::move(regexp),
           m_options.m_language, m_options.m_skip_prologue, internal,
           m_options.m_hardware);

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):
 
     mydir = TestBase.compute_mydir(__file__)
 
     @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"])
+
+        # 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",
+                             "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",
+                             "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."])