This is an archive of the discontinued LLVM Phabricator instance.

Add --move-to-nearest-code / target.move-to-nearest-code options
ClosedPublic

Authored by ki.stfu on Apr 25 2015, 2:11 PM.

Details

Summary

This option forces to only set a source line breakpoint when there is an exact-match

This patch includes the following commits:

  1. Add the -m/--exact-match option in "breakpoint set" command
    1. Add exact_match arg in BreakpointResolverFileLine ctor
    2. Add m_exact_match field in BreakpointResolverFileLine
    3. Add exact_match arg in BreakpointResolverFileRegex ctor
    4. Add m_exact_match field in BreakpointResolverFileRegex
    5. Add exact_match arg in Target::CreateSourceRegexBreakpoint
    6. Add exact_match arg in Target::CreateBreakpoint
    7. Add -m/--exact-match option in "breakpoint set" command
  2. Add target.exact-match option to skip BP if source line doesn't match
    1. Add target.exact-match global option
    2. Add Target::GetExactMatch
    3. Refactor Target::CreateSourceRegexBreakpoint to accept LazyBool exact_match (was bool)
    4. Refactor Target::CreateBreakpoint to accept LazyBool exact_match (was bool)
  3. Add target.exact-match test in SettingsCommandTestCase
  4. Add BreakpointOptionsTestCase tests to test --skip-prologue/--exact-match options
  5. Fix a few typos in lldbutil.check_breakpoint_result func
  6. Rename --exact-match/m_exact_match/exact_match/GetExactMatch to --move-to-nearest-code/m_move_to_nearest_code/move_to_nearest_code/GetMoveToNearestCode
  7. Add exact_match field in BreakpointResolverFileLine::GetDescription and BreakpointResolverFileRegex::GetDescription, for example:

was:

1: file = '/Users/IliaK/p/llvm/tools/lldb/test/functionalities/breakpoint/breakpoint_command/main.c', line = 12, locations = 1, resolved = 1, hit count = 2
  1.1: where = a.out`main + 20 at main.c:12, address = 0x0000000100000eb4, resolved, hit count = 2

now:

1: file = '/Users/IliaK/p/llvm/tools/lldb/test/functionalities/breakpoint/breakpoint_command/main.c', line = 12, exact_match = 0, locations = 1, resolved = 1, hit count = 2
  1.1: where = a.out`main + 20 at main.c:12, address = 0x0000000100000eb4, resolved, hit count = 2

Diff Detail

Repository
rL LLVM

Event Timeline

ki.stfu updated this revision to Diff 24431.Apr 25 2015, 2:11 PM
ki.stfu retitled this revision from to Add --exact-match / target.exact-match options.
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added reviewers: jingham, clayborg.
ki.stfu added subscribers: jingham, clayborg, Unknown Object (MLST).
ki.stfu updated this object.Apr 25 2015, 2:13 PM
ki.stfu updated this revision to Diff 24432.Apr 25 2015, 2:16 PM

Fix test/lldbutil.py

ki.stfu updated this object.Apr 25 2015, 2:17 PM

Friendly ping.

jingham edited edge metadata.Apr 28 2015, 9:43 AM

The implementation seems fine.

"exact-match" would be an okay name for the feature if it was only for file & line breakpoints, but for regex breakpoints it seems confusing - is it an exact match with the regex? Anyway, it doesn't express what the flag really does. What you are really doing is "not moving the breakpoint if the specified line contains no code." We should come up with an option name and help string that makes this clear. Maybe --move-to-nearest-code={true,false}?

For extra credit, "break set" should tell you that it WOULD have set the breakpoint except that --move-to-nearest-code was set to false. Otherwise, I don't know how you'd tell the difference.

"exact-match" would be an okay name for the feature if it was only for file & line breakpoints, but for regex breakpoints it seems confusing - is it an exact match with the regex?

Yes, but only for regex that means file:line breakpoints.

Anyway, it doesn't express what the flag really does. What you are really doing is "not moving the breakpoint if the specified line contains no code." We should come up with an option name and help string that makes this clear. Maybe --move-to-nearest-code={true,false}?

It's a long name for option, or not? What I should use as a short option?

For extra credit, "break set" should tell you that it WOULD have set the breakpoint except that --move-to-nearest-code was set to false. Otherwise, I don't know how you'd tell the difference.

How it should tell that? Can you suggest a format to do that?

"exact-match" would be an okay name for the feature if it was only for file & line breakpoints, but for regex breakpoints it seems confusing - is it an exact match with the regex?

Yes, but only for regex that means file:line breakpoints.

Got that, but it is still confusing.

Anyway, it doesn't express what the flag really does. What you are really doing is "not moving the breakpoint if the specified line contains no code." We should come up with an option name and help string that makes this clear. Maybe --move-to-nearest-code={true,false}?

It's a long name for option, or not? What I should use as a short option?

The long option names can be verbose if that helps make them descriptive, after all lldb does tab completion and shortest unique match on option names, so you wouldn't ever have to type it all if you didn't want to.

I chose --move-to-nearest-code in part because you could still use "-m" as the short option...

For extra credit, "break set" should tell you that it WOULD have set the breakpoint except that --move-to-nearest-code was set to false. Otherwise, I don't know how you'd tell the difference.

How it should tell that? Can you suggest a format to do that?

The breakpoint's GetDescription calls down into the Resolvers description method. So you'd have to remember that you COULD have matched some lines were it not for the exact in the Resolver, and report this in its Description method. That part shouldn't be hard. You're also going to have to get ResolveSymbolContext to tell you how many matches the exact_match ruled out.

You probably should have the resolver's description mention that exact match is turned on, since otherwise there's no way to recover this fact. So the file & line description could say something like:

file = 'foo.c', line = 11, move_to_nearest=true: num_skipped=10

ki.stfu planned changes to this revision.Apr 28 2015, 1:27 PM
ki.stfu requested a review of this revision.Apr 30 2015, 5:23 AM

@jingham,

I took a look on this again and I'm confused what should I rename. My solution is based on using the exact=true option in CompileUnit::ResolveSymbolContext. The call stack for this arg looks like the following:

1. CompileUnit::ResolveSymbolContext (exact=true) <-- I suppose we are not going to rename it
2. BreakpointResolverFileLine::SearchCallback(exact_match=true) <-- It looks reasonable for me
3. Target::CreateBreakpoint(exact_match=true) <-- It's too
4. CommandObjectBreakpointSet::DoExecute(m_options.m_exact_match=true)  <-- and this
5. breakpoint set ... --exact-match <-- I agree that it's a bit confused for regex but just a bit

Which of them I should rename to MoveToNearestCode (is it a right name?)

The same question for target.exact-match option:

  1. Should it be target.move-to-nearest-code?
  2. And what about TargetProperties::GetExactMatch()? Should I rename it to TargetProperties::GetMoveToNearestCode()?

All these changes looks ugly for me.

ki.stfu updated this revision to Diff 24700.Apr 30 2015, 6:05 AM
ki.stfu edited edge metadata.

Add MiBreakTestCase.test_lldbmi_break_insert_settings test

@jingham,

I took a look on this again and I'm confused what should I rename. My solution is based on using the exact=true option in CompileUnit::ResolveSymbolContext. The call stack for this arg looks like the following:

1. CompileUnit::ResolveSymbolContext (exact=true) <-- I suppose we are not going to rename it
2. BreakpointResolverFileLine::SearchCallback(exact_match=true) <-- It looks reasonable for me

These two seem fine to me, there isn't anything else exact or exact_match could mean.

  1. Target::CreateBreakpoint(exact_match=true) <-- It's too
  2. CommandObjectBreakpointSet::DoExecute(m_options.m_exact_match=true) <-- and this
  3. breakpoint set ... --exact-match <-- I agree that it's a bit confused for regex but just a bit

These three seem confusing since there are a bunch of things exact match could mean.

Which of them I should rename to MoveToNearestCode (is it a right name?)

The same question for target.exact-match option:
# Should it be target.move-to-nearest-code?
# And what about TargetProperties::GetExactMatch()? Should I rename it to TargetProperties::GetMoveToNearestCode()?

Yes, I would change these two.

All these changes looks ugly for me.

Why?

ki.stfu planned changes to this revision.May 4 2015, 10:07 PM

Thanks! I'll rework this according to your comments.

All these changes looks ugly for me.

Why?

I thought it is too long name.

ki.stfu retitled this revision from Add --exact-match / target.exact-match options to Add --move-to-nearest-code / target.move-to-nearest-code options.May 7 2015, 1:10 AM
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu edited the test plan for this revision. (Show Details)May 7 2015, 1:12 AM
ki.stfu updated this revision to Diff 25142.May 7 2015, 1:28 AM

Rename --exact-match/m_exact_match/exact_match/GetExactMatch to --move-to-nearest-code/m_move_to_nearest_code/move_to_nearest_code/GetMoveToNearestCode

Friendly ping.

clayborg accepted this revision.May 12 2015, 10:18 AM
clayborg edited edge metadata.

I will defer to Jim on this one.

This revision is now accepted and ready to land.May 12 2015, 10:18 AM

I think we talked about coming up with some way to inform users that we didn't get a breakpoint location for the specification but would have if "move-to-nearest-code" was true. I don't see that. It isn't necessary to gate checking this in on that, but it would be good to do as a follow up. One of the common uses for this is "I accidentally put a breakpoint is #if 0-ed. A warning saying the source location contained no code so we didn't set a breakpoint there would help this out.

Another way to do this is to make another flag like "disabled" for locations, something like "not moved" then the regular break list mechanism could be made to show these, but like with disabled breakpoints we would know not to set them.

ki.stfu updated this revision to Diff 25680.May 13 2015, 7:17 AM
ki.stfu edited edge metadata.

Add exact_match field in the breakpoint's description

ki.stfu requested a review of this revision.May 13 2015, 7:19 AM
ki.stfu edited edge metadata.

@jingham, take a look again please. Now format looks like:

1: file = '/Users/IliaK/p/llvm/tools/lldb/test/functionalities/breakpoint/breakpoint_command/main.c', line = 12, exact_match = 0, locations = 1, resolved = 1, hit count = 2
  1.1: where = a.out`main + 20 at main.c:12, address = 0x0000000100000eb4, resolved, hit count = 2

3: file = 'main.c', line = 12, exact_match = 0, locations = 1, resolved = 1, hit count = 2
    Breakpoint commands:
      return bktptcmd.function(frame, bp_loc, internal_dict)

  3.1: where = a.out`main + 20 at main.c:12, address = 0x0000000100000eb4, resolved, hit count = 2

That's great. I was thinking something more ambitious, like (this is just a sketch):

3: file = 'main.c', line = 12, exact_match = 1, locations = 1, resolved = 1, hit count = 0

  Breakpoint commands:
    return bktptcmd.function(frame, bp_loc, internal_dict)

3.1: where = a.out`main + 20 at main.c:12, not resolved because exact_match = 1, moved address = 0x0000000100000eb4

Note you have to do it on location, since there could be other locations that do have exact match addresses. But as I said, that's extra credit, it would be fine to check this in and then do this refinement when you have time.

3.1: where = a.out`main + 20 at main.c:12, not resolved because exact_match = 1, moved address = 0x0000000100000eb4

How I can determine why it wasn't resolved? (I mean, it wasn't resolved due to exact_match=1, or symbols just not loaded yet?)

You would have to do the exact match check slightly differently in the resolver. Instead of just calling into ResolveSymbolContext with one or the other value of exact match, call with exact_match = false. If you got a result, but the line number didn't match, make a new location anyway but set the not yet existing "not-enabled-because-of-exact-match" flag. Then you'll have to make IsEnabled on the locations check both the enabled flag and the "not-enabled-because-of-exact-match" flags.

Jim

ki.stfu planned changes to this revision.May 14 2015, 10:52 AM
ki.stfu requested a review of this revision.May 15 2015, 10:55 AM

Possible I'll do it later but at the moment I have no time for that. I gonna commit it right now, if you don't mind of course.

I don't mind. I said all along that was "extra-credit".

Jim

ki.stfu updated this object.May 15 2015, 11:04 AM
ki.stfu edited edge metadata.
ki.stfu updated this revision to Diff 25879.May 15 2015, 11:07 AM

Rebase against ToT

ki.stfu updated this revision to Diff 25881.May 15 2015, 11:14 AM

Fix MiBreakTestCase.test_lldbmi_break_insert_settings after r236832

This revision was automatically updated to reflect the committed changes.

@jingham, what you think about hiding of the exact_match field in BP description when it's false? It will fix these errors: http://lab.llvm.org:8011/builders/lldb-x86_64-darwin-13.4/builds/2941

That seems fine as a "reduce noise by not showing flags when set to their default values."

OTOH we should NOT be writing tests that rely on the exact format of break list output, except as mediated by the breakpoint setting commands in lldbutil.py. I didn't look at the tests that were failing, I'm a bit busy right now. But if people are using the command line to set breakpoints and check breakpoints and not using the run_break_set_by_* functions, they should stop doing that. We should be able to change the output from break list and ONLY have to change this set of functions, not go adjusting test suite code all over the testsuite... So whoever wrote these tests should clean them up.

Jim