[Lldb-commits] [PATCH] D64853: Fix CommandInterpreter for _regex-break with options

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

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.



================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/regex_break/TestRegexpBreakCommand.py:81
+    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")
----------------
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.


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/regex_break/TestRegexpBreakCommand.py:95-101
+        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'])
+
----------------
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.


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/regex_break/TestRegexpBreakCommand.py:107
+
+        break_results = lldbutil.run_break_set_command(
+            self, "b %s:%d" % (self.source, self.line))
----------------
JDevlieghere wrote:
> `# Check breakpoint with relative file path.`
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.


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/regex_break/TestRegexpBreakCommand.py:130
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stopped',
----------------
JDevlieghere wrote:
> Since this is repeated in every function, I'd factor this out into a method. 
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.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:519
   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"},
----------------
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.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:524
+      {"^(0x[[:xdigit:]]+)(?:$|\\s+)(-.*)*$", "breakpoint set --address %1 %2"},
+      {"^[\"']?([-+]?\\[[^[:space:]\"']+\\s+[^[:space:]\"']+\\])[\"']?(?:$|\\s+)(-.*)*$",
+       "breakpoint set --name '%1' %2"}, // Objective-C Identifier
----------------
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.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:540
+          "Set a breakpoint using one of several shorthand formats. "
+          "Accepts breakpoint options.",
           "\n"
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64853/new/

https://reviews.llvm.org/D64853