[Lldb-commits] [PATCH] D30817: BreakpointResolverFileLine: Correct treatment of move-to-nearest-code for multiple modules

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I missed that you had set this to target.  I'd rather avoid doing that unless there's good reason since it means we duplicate all the module iteration logic.

To my mind, within reason it's always better to be conservative and set too many locations rather than too few.  There's nothing the user can do to construct a missing breakpoint - especially after they've missed it..., but we have "move-to-nearest-code false" as the way for users to tell us to be more radical in rejecting matches, and it is also simple to disable a location that was errantly set.

And linking breakpoint location logic cross-module seems like a bad idea to me.  If I decide that Module A's version of the breakpoint wins over module B's in the move, when A goes away there's nothing that will tell me to go revise that decision for a module that hasn't changed (B).  So this seems the wrong way to solve this problem.

BTW,  the formally correct way to solve this problem when "move-to-nearest-code" is true is to reject moves that  cross function boundaries.  IRL, you can't tell when a breakpoint leaves a function that didn't get emitted since you have no way of knowing on what line it would have ended without doing syntax analysis on the sources, and we can't assume sources are available.  But you should be able to tell when a moving a breakpoint by line crosses INTO a new function because you have the decl_file, decl_line for the function you would be moving it into.  Last time I looked, the debug info from clang wasn't quite right, however.  If you have:

int
foo()
{

}

Then clang put the decl file & line on the "foo" line.  So by those lights moving from a breakpoint set on the "int" line would be disallowed as moving across a function boundary.  This is actually something I've seen people do quite often - particularly when using a GUI.  But I think it would actually be good enough to establish a window, so that moving from "decl line - fudge-factor" was still allowed, where fudge factor is 2 or 3 lines.  That would make most reasonable cases work as expected.  It won't help for people who write:

int short_func() {}
int other_short_func () {}

with no spaces.  But for that is the sort of case the "move-to-nearest-code false" is for.

This is actually something that's been on my plate to do for a while, but it is kind of low on the list at present.  If you really want to solve this problem, the above suggestion would I think be a much better approach.


https://reviews.llvm.org/D30817