Fix CommandInterpreter for _regex-break with options

[mib]

Summary

This patch fixes the regular expressions used by the Command Interpreter
to match the GDB breakpoint command style to the LLDB style.

The previous version passed the breakpoint options to _regex-break as a name
identifier, instead of recognising the options.

In addition to that, several test cases have been added to check if all the
supported _regex-break commands match their b alias.

[teemperor]

This breaks several tests on linux:

Failing Tests (20):
    LLDB :: Breakpoint/debug_addrx.test
    LLDB :: Breakpoint/debug_rnglist_basic.test
    LLDB :: Breakpoint/debug_rnglist_offset_pair.test
    LLDB :: Breakpoint/debug_rnglist_rlestartend.test
    LLDB :: Breakpoint/debug_rnglistx_rlex.test
    LLDB :: Breakpoint/implicit_const_form_support.test
    LLDB :: Breakpoint/jitbp_elf.test
    LLDB :: Breakpoint/single-file-split-dwarf.test
    LLDB :: Breakpoint/split-dwarf-5-addrbase.test
    LLDB :: Breakpoint/split-dwarf5-debug-stroffsets.test
    LLDB :: Expr/TestIRMemoryMap.test
    LLDB :: Settings/TestFrameFormatColor.test
    LLDB :: Watchpoint/SetErrorCases.test
    lldb-Suite :: functionalities/breakpoint/breakpoint_command/TestRegexpBreakCommand.py
    lldb-Suite :: functionalities/breakpoint/breakpoint_hit_count/TestBreakpointHitCount.py
    lldb-Suite :: functionalities/command_script/TestCommandScript.py
    lldb-Suite :: functionalities/format/TestFormats.py
    lldb-Suite :: functionalities/show_location/TestShowLocationDwarf5.py
    lldb-Suite :: functionalities/target_command/TestTargetCommand.py
    lldb-Suite :: functionalities/watchpoint/watchpoint_on_vectors/TestValueOfVectorVariable.py

I uploaded the test output here: https://teemperor.de/pub/FailingRegexLog.txt

[TWeaver]

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c

9

Hi there,

this is a little nit picky, and I'm not entirely sure If im right in saying so, but, if this is a C++ file as denoted by the license header, then shouldn't this include by <cstdlib> rather than the c library stdlib.h?

thanks for your time
Tom

[davide]

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c

9

This is definitely a C file, the header is wrong.

[davide]

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestRegexpBreakCommand.py

212–215

Something like this will definitely break on Windows (@compnerd), so you probably need to generalize this test or skip it there. Also, what about other platforms, e.g. iOS? (@jasonmolenda)

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c

17

Not that it matters a lot, but I would free this memory before returning.

[davide]

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestRegexpBreakCommand.py

242–257

I was under the impression this could be expressed more concisely [Jim has an utility for doing that, so he might point you in the right direction]

[labath]

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestRegexpBreakCommand.py

212–215

The best thing to do here is to build a shared library as a part of the test. This way, you can control exactly which symbols are present inside it. There are examples of building shared libraries in the test suite (TestLoadUnload comes to mind..)

[JDevlieghere]

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c

9

Also, I believe the consensus is that we don't need the header in test files.

[mib]

Updating D64853: Fix CommandInterpreter for _regex-break with options

[JDevlieghere]

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/regex_break/TestRegexpBreakCommand.py

5 ↗

(On Diff #210393)

I don't think you need this.

23 ↗

(On Diff #210393)

Why not inline the test? Now you have two doc-strings, which are slightly different.

58 ↗

(On Diff #210393)

This is no longer necessary

107 ↗

(On Diff #210393)

# Check breakpoint with relative file path.

130 ↗

(On Diff #210393)

Since this is repeated in every function, I'd factor this out into a method.

171 ↗

(On Diff #210393)

We discussed a few scenarios yesterday with single and double quotes. Any reason you didn't include those?

243 ↗

(On Diff #210393)

s/consie/concise/

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/regex_break/main.c

1 ↗

(On Diff #210393)

Just remove the header

[jasonmolenda]

I know there are a lot of specific notes floating around right now, but two quick suggestions.

I see the idiom

self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
target = self.dbg.CreateTarget(exe)

repeated in several places. They're doing the same thing, doing the second is preferred.

In all? of the test_ methods, you rebuild the test binary. This can be in the setup method, can't it?

It looks like you've changed to creating your own dylib/solib to test module setting, instead of trying to use a libc function. Good change, it will make this a lot more portable.

[jingham]

This looks good in general. I also have a few nits, and you need to fix (and probably test) the behavior for files with spaces in their names.

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/regex_break/TestRegexpBreakCommand.py

81 ↗

(On Diff #210393)

consie -> concise? and "an alias"

Also the grammar is a little awkward, it makes it sound like a line number is an alias for _regexp-break.

95–101 ↗

(On Diff #210393)

This doesn't actually check that you stopped at the breakpoint you set. There shouldn't be any other breakpoints, but you never know... Might be better to get the breakpoint from the breakpoint (key "bpno")in the break_results and use lldbutils.run_to_breakpoint_do_run which will do the run and assert that you stopped at the correct breakpoint in one go.

107 ↗

(On Diff #210393)

Might also check a file name with a - in it, the first character is the trickiest - to make sure that doesn't get counted as an option.

130 ↗

(On Diff #210393)

Yes, you can just use lldbutils.run_to_breakpoint_do_run. You could even add a version that takes a breakpoint number rather than an SBBreakpoint.

lldb/source/Interpreter/CommandInterpreter.cpp

519–525

I think this one will have broken filenames with spaces in the name. IIUC, that's what the initial ".*" was from ^(.*. It is basically saying

^(string that ends in a non-space character) some spaces ':' more spaces (number)

and you changed that to

^(bunch of non-space characters) some spaces ':' more spaces (number)

Probably should add a test for files with spaces in the name.

Also, when you are gathering up the options you have:

(-.*)*$

Seems like that should be:

(-.*)?$

since if this starts with a - the .* will eat up the rest of the line, and you only want to account for the fact that the options might not be there.

Also it is a little odd that you use \\s whereas the rest of the patterns use [[:space:]]. We should probably use the posix one everywhere.

524

again, stick with one of \s or [:space:]...

Greg's original version would support:

b -[NSObject foo: bar: baz:]

whereas yours will only support:

b -[NSObject foo:bar:baz:]

people tend to write selectors jammed together like this, so it isn't a big deal.

540

You should say:

Accepts breakpoint options AFTER the breakpoint specification.

After all:

b -c foo main.c:12

is not going to work with your regex's, but for the most part in lldb you can freely provide options anywhere on the command line.

[teemperor]

(setting this to 'request changes' that it leaves the review queue and goes back to mib)

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestRegexpBreakCommand.py

 from lldbsuite.test.lldbtest import *
 import lldbsuite.test.lldbutil as lldbutil
 
 
 class RegexpBreakCommandTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
-    def test(self):
-        """Test _regexp-break command."""
+    def test_line(self):
+        """Test _regexp-break command with a line number."""
         self.build()
-        self.regexp_break_command()
+        self.regexp_break_command_line()
+
+    def test_file_line(self):
+        """Test _regexp-break command with a file and a line number."""
+        self.build()
+        self.regexp_break_command_file_line()
+
+    def test_address(self):
+        """Test _regexp-break command with an address."""
+        self.build()
+        self.regexp_break_command_address()
+
+    def test_name(self):
+        """Test _regexp-break command with a name."""
+        self.build()
+        self.regexp_break_command_name()
+
+    def test_ampersand_name(self):
+        """Test _regexp-break command with an ampersand name."""
+        self.build()
+        self.regexp_break_command_ampersand_name()
+
+    def test_shlib_method(self):
+        """Test _regexp-break command with a shared library method."""
+        self.build()
+        self.regexp_break_command_shlib_method()
+
+    def test_regex(self):
+        """Test _regexp-break command with a regular expression."""
+        self.build()
+        self.regexp_break_command_regex()
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
+        self.runCmd("settings set use-color false")
         # Find the line number to break inside main().
         self.source = 'main.c'
+        self.regex = '// Set break point at this line.'
         self.line = line_number(
-            self.source, '// Set break point at this line.')
+            self.source, self.regex)
+
+    def function_address(self, target, function_name, skip_prologue=False):
+        # Create a target by the debugger.
+        self.assertTrue(target, VALID_TARGET)
+
+        sc_list = target.FindFunctions(str(function_name), lldb.eFunctionNameTypeAuto)
+        self.assertTrue(sc_list.GetSize() == 1)
+
+        symbol = sc_list[0].GetSymbol()
+        start_addr = symbol.GetStartAddress()
+        prologue_size = symbol.GetPrologueByteSize()
+        addr = int(start_addr)
+        if skip_prologue:
+         addr = addr + prologue_size
+        return addr
 
-    def regexp_break_command(self):
-        """Test the super consie "b" command, which is analias for _regexp-break."""
+    def regexp_break_command_line(self):
+        """Test the super consie "b" command with a line number, which is analias for _regexp-break."""
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         break_results = lldbutil.run_break_set_command(
             self, "b %d" %
             self.line)
         lldbutil.check_breakpoint_result(
             self,
             break_results,
             file_name='main.c',
             line_number=self.line,
             num_locations=1)
 
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stopped',
+                             'stop reason = breakpoint'])
+
+    def regexp_break_command_file_line(self):
+        """Test the super consie "b" command with a file and line number, which is analias for _regexp-break."""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
         break_results = lldbutil.run_break_set_command(
             self, "b %s:%d" % (self.source, self.line))
         lldbutil.check_breakpoint_result(
             self,
             break_results,
             file_name='main.c',
             line_number=self.line,
             num_locations=1)
def regexp_break_command_file_line(self):
             num_locations=1)
 
         self.runCmd("run", RUN_SUCCEEDED)
 
         # The stop reason of the thread should be breakpoint.
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
                     substrs=['stopped',
                              'stop reason = breakpoint'])
+
+    def regexp_break_command_address(self):
+        """Test the super consie "b" command with an address, which is analias for _regexp-break."""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+        target = self.dbg.CreateTarget(exe)
+
+        addr = self.function_address(target, "main")
+        self.runCmd("process launch -s", RUN_SUCCEEDED)
+
+        break_results = lldbutil.run_break_set_command(
+            self, "b 0x%x" %
+            addr)
+        lldbutil.check_breakpoint_result(
+            self,
+            break_results,
+            file_name=self.source,
+            line_number=-1,
+            num_locations=1)
+
+        self.runCmd("continue", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stopped',
+                             'stop reason = breakpoint'])
+
+    def regexp_break_command_name(self):
+        """Test the super consie "b" command with a function name, which is analias for _regexp-break."""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+        target = self.dbg.CreateTarget(exe)
+
+        function_name = "main"
+        addr = self.function_address(target, function_name, skip_prologue=True)
+        self.line = line_number(self.source, "int main")
+
+        break_results = lldbutil.run_break_set_command(
+            self, "b %s" %
+            function_name)
+
+        break_set_addr = break_results['address']
+        self.assertTrue(int(break_set_addr,16) == addr)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stopped',
+                             'stop reason = breakpoint'])
+
+    def regexp_break_command_ampersand_name(self):
+        """Test the super consie "b" command with a function reference, which is analias for _regexp-break."""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+        target = self.dbg.CreateTarget(exe)
+
+        function_name = "main"
+        break_expected_addr = self.function_address(target, function_name)
+
+        break_results = lldbutil.run_break_set_command(
+            self, "b &%s" %
+            function_name)
+
+        break_set_addr = break_results['address']
+        self.assertTrue(int(break_set_addr, 16) == break_expected_addr)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stopped',
+                             'stop reason = breakpoint'])
+
+    def regexp_break_command_shlib_method(self):
+        """Test the super consie "b" command with a shared library method, which is analias for _regexp-break."""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+        target = self.dbg.CreateTarget(exe)
+        os_description = target.GetPlatform().GetOSDescription().split()[0]
+
+        module_name = "libsystem_malloc.dylib"
+        if not os_description == "Darwin":
+            module_name = "libc.so"
+
+        method_name = "malloc"
+
+        break_results = lldbutil.run_break_set_command(
+            self, "b %s`%s" %
+            (module_name, method_name))
+
+        self.assertTrue(break_results['module'] == module_name)
+        self.assertTrue(break_results['symbol'] == method_name)
+
+        shlib_method = module_name + "`" + method_name
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stopped',
+                             'stop reason = breakpoint'])
+
+        self.expect("frame info", substrs=[shlib_method])
+
+    def regexp_break_command_regex(self):
+        """Test the super consie "b" command with a regular expression, which is analias for _regexp-break."""
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+        break_results = lldbutil.run_break_set_command(
+            self, "b /%s/" %
+            self.regex)
+        lldbutil.check_breakpoint_result(
+            self,
+            break_results,
+            file_name='main.c',
+            line_number=self.line,
+            num_locations=1)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stopped',
+                             'stop reason = breakpoint'])

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c

 //===-- main.c --------------------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
+#include <stdlib.h>
+
 int main (int argc, char const *argv[])
 {
     // Add a body to the function, so we can set more than one
     // breakpoint in it.
     static volatile int var = 0;
     var++;
+    malloc(sizeof(int));
     return 0; // Set break point at this line.
 }

lldb/source/Interpreter/CommandInterpreter.cpp

void CommandInterpreter::LoadCommandDictionary() {
   m_command_dict["type"] = CommandObjectSP(new CommandObjectType(*this));
   m_command_dict["version"] = CommandObjectSP(new CommandObjectVersion(*this));
   m_command_dict["watchpoint"] =
       CommandObjectSP(new CommandObjectMultiwordWatchpoint(*this));
   m_command_dict["language"] =
       CommandObjectSP(new CommandObjectLanguage(*this));
 
   const char *break_regexes[][2] = {
-      {"^(.*[^[:space:]])[[:space:]]*:[[:space:]]*([[:digit:]]+)[[:space:]]*$",
-       "breakpoint set --file '%1' --line %2"},
-      {"^/([^/]+)/$", "breakpoint set --source-pattern-regexp '%1'"},
-      {"^([[:digit:]]+)[[:space:]]*$", "breakpoint set --line %1"},
-      {"^\\*?(0x[[:xdigit:]]+)[[:space:]]*$", "breakpoint set --address %1"},
-      {"^[\"']?([-+]?\\[.*\\])[\"']?[[:space:]]*$",
-       "breakpoint set --name '%1'"},
+      {"^([^[:space:]]+)[[:space:]]*:[[:space:]]*([[:digit:]]+)(?:$|\\s+)(-.*)*$",
+       "breakpoint set --file '%1' --line %2 %3"},
+      {"/([^/]+)/(?:$|\\s+)(-.*)*$", "breakpoint set --source-pattern-regexp '%1' %2"},
+      {"^([[:digit:]]+)(?:$|\\s+)(-.*)*$", "breakpoint set --line %1 %2"},
+      {"^(0x[[:xdigit:]]+)(?:$|\\s+)(-.*)*$", "breakpoint set --address %1 %2"},
+      {"^[\"']?([-+]?\\[[^[:space:]\"']+\\s+[^[:space:]\"']+\\])[\"']?(?:$|\\s+)(-.*)*$",
+       "breakpoint set --name '%1' %2"}, // Objective-C Identifier
       {"^(-.*)$", "breakpoint set %1"},
-      {"^(.*[^[:space:]])`(.*[^[:space:]])[[:space:]]*$",
-       "breakpoint set --name '%2' --shlib '%1'"},
-      {"^\\&(.*[^[:space:]])[[:space:]]*$",
+      {"^([^[:space:]]+)`([^[:space:]]+)(?:$|\\s+)(-.*)*$",
+       "breakpoint set --name '%2' --shlib '%1' %3"},
+      {"^\\&([^[:space:]]+)(?:$|\\s+)(-.*)*$",
        "breakpoint set --name '%1' --skip-prologue=0"},
-      {"^[\"']?(.*[^[:space:]\"'])[\"']?[[:space:]]*$",
-       "breakpoint set --name '%1'"}};
+      {"^[\"']?([^[:space:]\"']+)[\"']?(?:$|\\s+(-.*))*$",
+       "breakpoint set --name '%1' %2"}};
 
   size_t num_regexes = llvm::array_lengthof(break_regexes);
 
   std::unique_ptr<CommandObjectRegexCommand> break_regex_cmd_up(
       new CommandObjectRegexCommand(
           *this, "_regexp-break",
-          "Set a breakpoint using one of several shorthand formats.",
+          "Set a breakpoint using one of several shorthand formats. Accepts breakpoint options.",
           "\n"
           "_regexp-break <filename>:<linenum>\n"
           "              main.c:12             // Break at line 12 of "
           "main.c\n\n"
           "_regexp-break <linenum>\n"
           "              12                    // Break at line 12 of current "
           "file\n\n"
           "_regexp-break 0x<address>\n"
           "              0x1234000             // Break at address "
           "0x1234000\n\n"
           "_regexp-break <name>\n"
           "              main                  // Break in 'main' after the "
           "prologue\n\n"
           "_regexp-break &<name>\n"
           "              &main                 // Break at first instruction "
           "in 'main'\n\n"
           "_regexp-break <module>`<name>\n"
           "              libc.so`malloc        // Break in 'malloc' from "
           "'libc.so'\n\n"
           "_regexp-break /<source-regex>/\n"
           "              /break here/          // Break on source lines in "
           "current file\n"
           "                                    // containing text 'break "
           "here'.\n",
-          2,
+          3,
           CommandCompletions::eSymbolCompletion |
               CommandCompletions::eSourceFileCompletion,
           false));
 
   if (break_regex_cmd_up) {
     bool success = true;
     for (size_t i = 0; i < num_regexes; i++) {
       success = break_regex_cmd_up->AddRegexCommand(break_regexes[i][0],