Page MenuHomePhabricator

Fix CommandInterpreter for _regex-break with options
Needs ReviewPublic

Authored by mib on Jul 17 2019, 12:11 AM.



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.

Event Timeline

mib created this revision.Jul 17 2019, 12:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 12:11 AM
teemperor added a reviewer: Restricted Project.Jul 17 2019, 12:17 AM
mib retitled this revision from Fix CommandInterpreter for _regexp-break with options to Fix CommandInterpreter for _regex-break with options.Jul 17 2019, 12:17 AM
mib edited the summary of this revision. (Show Details)
mib added reviewers: jingham, friss, JDevlieghere.
teemperor removed a reviewer: Restricted Project.Jul 17 2019, 12:17 AM
mib edited the summary of this revision. (Show Details)Jul 17 2019, 12:19 AM
teemperor requested changes to this revision.Jul 17 2019, 1:21 AM
teemperor added a subscriber: 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/
    lldb-Suite :: functionalities/breakpoint/breakpoint_hit_count/
    lldb-Suite :: functionalities/command_script/
    lldb-Suite :: functionalities/format/
    lldb-Suite :: functionalities/show_location/
    lldb-Suite :: functionalities/target_command/
    lldb-Suite :: functionalities/watchpoint/watchpoint_on_vectors/

I uploaded the test output here:

This revision now requires changes to proceed.Jul 17 2019, 1:21 AM
TWeaver added inline comments.

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

davide added a subscriber: davide.Jul 17 2019, 8:10 AM
davide added inline comments.

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

davide added inline comments.

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)


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

davide added inline comments.Jul 17 2019, 8:15 AM

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 added a subscriber: labath.Jul 17 2019, 8:23 AM
labath added inline comments.

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 added inline comments.Jul 17 2019, 10:31 AM

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

mib updated this revision to Diff 210393.Jul 17 2019, 12:31 PM

Updating D64853: Fix CommandInterpreter for _regex-break with options

JDevlieghere added inline comments.Jul 17 2019, 2:07 PM

I don't think you need this.


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


This is no longer necessary


# Check breakpoint with relative file path.


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


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




Just remove the header

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.

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.


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.


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.


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.


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.


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.


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.


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.