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

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

This seems good to me.  Thanks for doing this.

I made a few inline comments, but none are serious.

Maybe I'm a little over-cautious about this sort of thing, but GetStartLineSourceInfo will return the first line in the line table if there is no decl_file & decl_line.  I prefer to only do this if we had decl_file and decl_line, since we know that the fudge factor will be too small for the case where the function beginning is really the first line table entry.

That's a judgement call, however.

The more we're making fancy choices filtering breakpoints for people, the more we need some way to tell them what we're doing.  It is great that this change will for example mean we no longer set breakpoints in some random function if you accidentally set one in an #ifdef'ed out function.  But the reason why you ended up with no locations is still left entirely mysterious.  Maybe we need to keep "rejected locations" with some explanation, and then "break list -v" would show them?

But that's definitely out of the scope of this change.



================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/foo.h:1-4
+LLDB_TEST_API inline int foo1() { return 1; } // !BR1
+
+LLDB_TEST_API inline int foo2() { return 2; } // !BR2
+
----------------
At other places in the testsuite we use something like:


```
#define INLINE inline __attribute__((always_inline))

```
to force inlining if we intend it.  IIRC inline is just advisory.




================
Comment at: source/Breakpoint/BreakpointResolverFileLine.cpp:164
+    // but that's the best we can do for now.
+    if (m_line_number < line - 1) {
+      LLDB_LOG(log, "removing symbol context at {0}:{1}", file, line);
----------------
Can you make this a 


```
const size_t decl_line_is_too_late_fudge = 1;

```

or something like that.  That will make it obvious what's going on with the "line - 1".


https://reviews.llvm.org/D30817