BreakpointResolverFileLine: Correct treatment of move-to-nearest-code for multiple modules

[labath]

Summary

move-to-nearest-code needs special treatment to avoid creating
superfluous breakpoints in case multiple compilation units. It already
had code to handle the case when the compilation units are in the same
module, but it still did not properly handle the case when the
compilation units are in different modules.

This fixes the issue by manually iterating over the modules (instead of
just CUs) to make sure we aggregate the matches properly.

[jingham]

Can you say more about the problem you are trying to solve. As far as breakpoints are concerned, if you find a match in each of several modules it seems to me you would always want to set a locations in each because modules can come and go independently.

This also introduces an N^2 dependency on number of modules since the searcher is already looping over the modules.

[labath]

In D30817#697960, @jingham wrote:

Can you say more about the problem you are trying to solve.

Yes.

Consider foo.h in the test case. Since the functions foo1 and foo2 are inline, they will only show up in the module if they are used. This means that the line table for module 1 will only contain entry for line 1 and module 2 will only contain line 2. If we treat each module separately, and we have move-to-nearest=true, we will set breakpoints on both lines, which are in two different functions and certainly not what the user had intended.

This is basically an extension of the existing code, which treated the same case, but only when the two compilation units were in the same module.

As far as breakpoints are concerned, if you find a match in each of several modules it seems to me you would always want to set a locations in each because modules can come and go independently.

Hm.. I had not considered that. I guess it's true that this would mean that adding or removing a module can affect how a breakpoint is resolved elsewhere, which does not seem ideal. I would still argue that this increases correctness, because then we will be misplacing the breakpoint only in some cases, where as now we do it always. I am open to suggestions though...

This also introduces an N^2 dependency on number of modules since the searcher is already looping over the modules.

I was under the impression that setting depth to target disables the iteration in the searcher.

[jingham]

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 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.

[labath]

I like the idea of using the function declaration line, as it will solve a couple of other corner cases also (we've had one user try to set a breakpoint on a macro definition and expect that to work). I'll try to implement that instead.

[labath]

I've updated the code as suggested. For the "fudge factor" I chose one, so that
this at least works in the fairly common case where you put an empty line
between two tiny functions. The "breakpoint on return type" case should still
work as long as the retyrn type does not span multiple lines, which I think is a
good compromise.

This required some tweaks to existing tests, as two of them were actually
relying on the move-the-breakpoint-into-a-function behavior. I have removed the
corresponding check from TestBreakpointOptions, as the new test supersedes that,
and I have tweaked TestMiBreak to test the move-nearest functionality
differently.

Let me know what you think.

[jingham]

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.

packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/foo.h

2–5

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.

source/Breakpoint/BreakpointResolverFileLine.cpp

165

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".

[labath]

In D30817#700825, @jingham wrote:

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.

I tried to to this as conservatively as possible. E.g. if I compile the binary with -gline-tables-only then sc.function will be null and we will not do any filtering. I guess we could arrive in this situation if the compiler chooses to not emit DW_AT_decl_line attributes, even though the rest of the DWARF is present, but I think at that point the user will have bigger problems than his breakpoints not being flexible enough.

packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/foo.h

2–5

I don't need or want to force inlining for this test. What I need is make sure the compiler does not emit code for the function if it is not used in the compilation unit. I suppose inline doesn't guarantee that, but neither does always_inline. I guess if some days compilers start emitting code for all functions, we'll need to update the test, but I doubt that's going to happen, as that would make intermediate files huge.

packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/Makefile

+LEVEL = ../../../make
+
+DYLIB_NAME := foo
+DYLIB_CXX_SOURCES := foo.cpp
+CXX_SOURCES := main.cpp
+CFLAGS_EXTRAS += -fPIC
+
+include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/TestMoveNearest.py

+"""Test that types defined in shared libraries work correctly."""
+
+from __future__ import print_function
+
+
+import unittest2
+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+
+class TestMoveNearest(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+        # Find the line number to break inside main().
+        self.source = 'foo.h'
+        self.line1 = line_number(self.source, '// !BR1')
+        self.line2 = line_number(self.source, '// !BR2')
+
+    def tearDown(self):
+        self.runCmd("settings clear target.move-to-nearest-code")
+        TestBase.tearDown(self)
+
+    def test(self):
+        """Test that target.move-to-nearest code setting behaves properly in the
+        presence of multiple modules"""
+
+        self.build()
+        target = self.dbg.CreateTarget("a.out")
+        self.assertTrue(target, VALID_TARGET)
+
+        # Regardless of the setting value the breakpoint should have exactly one
+        # location
+        self.runCmd("settings set target.move-to-nearest-code true")
+        lldbutil.run_break_set_by_file_and_line(self, self.source, self.line1,
+                loc_exact=True)
+        lldbutil.run_break_set_by_file_and_line(self, self.source, self.line2,
+                loc_exact=True)
+
+        self.runCmd("settings set target.move-to-nearest-code false")
+        lldbutil.run_break_set_by_file_and_line(self, self.source, self.line1,
+                loc_exact=True)
+        lldbutil.run_break_set_by_file_and_line(self, self.source, self.line2,
+                loc_exact=True)

packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/foo.h

+LLDB_TEST_API inline int foo1() { return 1; } // !BR1
+LLDB_TEST_API inline int foo2() { return 2; } // !BR2
+
+LLDB_TEST_API extern int call_foo1();
+LLDB_TEST_API extern int call_foo2();

packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/foo.cpp

+#include "foo.h"
+
+int call_foo1() { return foo1(); }

packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/main.cpp

+#include "foo.h"
+
+int call_foo2() { return foo2(); }
+
+int main() { return call_foo1() + call_foo2(); }

source/Breakpoint/BreakpointResolverFileLine.cpp

 // C Includes
 // C++ Includes
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/Function.h"
+#include "lldb/Target/Target.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 //----------------------------------------------------------------------
 // BreakpointResolverFileLine:
BreakpointResolverFileLine::SerializeToStructuredData() {
 
   return WrapOptionsDict(options_dict_sp);
 }
 
 Searcher::CallbackReturn
 BreakpointResolverFileLine::SearchCallback(SearchFilter &filter,
                                            SymbolContext &context,
                                            Address *addr, bool containing) {
+
   SymbolContextList sc_list;
 
   assert(m_breakpoint != NULL);
 
   // There is a tricky bit here.  You can have two compilation units that
-  // #include the same file, and
-  // in one of them the function at m_line_number is used (and so code and a
-  // line entry for it is generated) but in the
-  // other it isn't.  If we considered the CU's independently, then in the
-  // second inclusion, we'd move the breakpoint
-  // to the next function that actually generated code in the header file.  That
-  // would end up being confusing.
-  // So instead, we do the CU iterations by hand here, then scan through the
-  // complete list of matches, and figure out
-  // the closest line number match, and only set breakpoints on that match.
+  // #include the same file, and in one of them the function at m_line_number is
+  // used (and so code and a line entry for it is generated) but in the other it
+  // isn't.  If we considered the CU's independently, then in the second
+  // inclusion, we'd move the breakpoint to the next function that actually
+  // generated code in the header file.  That would end up being confusing.  So
+  // instead, we do the CU iterations by hand here, then scan through the
+  // complete list of matches, and figure out the closest line number match, and
+  // only set breakpoints on that match.
 
   // Note also that if file_spec only had a file name and not a directory, there
-  // may be many different file spec's in
-  // the resultant list.  The closest line match for one will not be right for
-  // some totally different file.
-  // So we go through the match list and pull out the sets that have the same
-  // file spec in their line_entry
-  // and treat each set separately.
+  // may be many different file spec's in the resultant list.  The closest line
+  // match for one will not be right for some totally different file.  So we go
+  // through the match list and pull out the sets that have the same file spec
+  // in their line_entry and treat each set separately.
+  const ModuleList &target_images = context.target_sp->GetImages();
+  std::lock_guard<std::recursive_mutex> guard(target_images.GetMutex());
+
+  size_t n_modules = target_images.GetSize();
+  for (size_t i = 0; i < n_modules; i++) {
+    // If this is the last level supplied, then call the callback directly,
+    // otherwise descend.
+    ModuleSP module_sp(target_images.GetModuleAtIndexUnlocked(i));
+    if (!filter.ModulePasses(module_sp))
+      continue;
 
-  const size_t num_comp_units = context.module_sp->GetNumCompileUnits();
+    const size_t num_comp_units = module_sp->GetNumCompileUnits();
     for (size_t i = 0; i < num_comp_units; i++) {
-    CompUnitSP cu_sp(context.module_sp->GetCompileUnitAtIndex(i));
-    if (cu_sp) {
-      if (filter.CompUnitPasses(*cu_sp))
-        cu_sp->ResolveSymbolContext(m_file_spec, m_line_number, m_inlines,
-                                    m_exact_match, eSymbolContextEverything,
-                                    sc_list);
+      CompUnitSP cu_sp(module_sp->GetCompileUnitAtIndex(i));
+      if (! cu_sp || !filter.CompUnitPasses(*cu_sp))
+        continue;
+
     }
   }
+
   StreamString s;
   s.Printf("for %s:%d ", m_file_spec.GetFilename().AsCString("<Unknown>"),
            m_line_number);
 
   SetSCMatchesByLine(filter, sc_list, m_skip_prologue, s.GetString());
 
   return Searcher::eCallbackReturnContinue;
 }
 
 Searcher::Depth BreakpointResolverFileLine::GetDepth() {
-  return Searcher::eDepthModule;
+  return Searcher::eDepthTarget;
 }
 
 void BreakpointResolverFileLine::GetDescription(Stream *s) {
   s->Printf("file = '%s', line = %u, exact_match = %d",
             m_file_spec.GetPath().c_str(), m_line_number, m_exact_match);
 }
 
 void BreakpointResolverFileLine::Dump(Stream *s) const {}