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

1–4 ↗

(On Diff #91693)

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

164 ↗

(On Diff #91693)

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

1–4 ↗

(On Diff #91693)

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.

lldb/trunk/include/lldb/Breakpoint/BreakpointResolverFileLine.h

public:
   }
   static inline bool classof(const BreakpointResolver *V) {
     return V->getResolverID() == BreakpointResolver::FileLineResolver;
   }
 
   lldb::BreakpointResolverSP CopyForBreakpoint(Breakpoint &breakpoint) override;
 
 protected:
+  void FilterContexts(SymbolContextList &sc_list);
+
   friend class Breakpoint;
   FileSpec m_file_spec;   // This is the file spec we are looking for.
   uint32_t m_line_number; // This is the line number that we are looking for.
   bool m_inlines; // This determines whether the resolver looks for inlined
                   // functions or not.
   bool m_skip_prologue;
   bool m_exact_match;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(BreakpointResolverFileLine);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_BreakpointResolverFileLine_h_

lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_options/TestBreakpointOptions.py

def breakpoint_options_test(self):
             num_expected_locations=1)
         lldbutil.run_break_set_by_file_and_line(
             self,
             "main.cpp",
             self.line,
             extra_options="-K 0",
             num_expected_locations=1)
 
-        # This should create a breakpoint 0 locations.
-        lldbutil.run_break_set_by_file_and_line(
-            self,
-            "main.cpp",
-            self.line,
-            extra_options="-m 0",
-            num_expected_locations=0)
-
         # Run the program.
         self.runCmd("run", RUN_SUCCEEDED)
 
         # Stopped once.
         self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
                     substrs=["stop reason = breakpoint 2."])
 
         # Check the list of breakpoint.
         self.expect(
             "breakpoint list -f",
             "Breakpoint locations shown correctly",
             substrs=[
                 "1: file = 'main.cpp', line = %d, exact_match = 0, locations = 1" %
                 self.line,
                 "2: file = 'main.cpp', line = %d, exact_match = 0, locations = 1" %
-                self.line,
-                "3: file = 'main.cpp', line = %d, exact_match = 1, locations = 0" %
                 self.line])
 
         # Continue the program, there should be another stop.
         self.runCmd("process continue")
 
         # Stopped again.
         self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
                     substrs=["stop reason = breakpoint 1."])

lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_options/main.cpp

-// Set break point at this line.
-
 extern "C" int foo(void);
-int
-main (int argc, char **argv)
-{ 
+int main (int argc, char **argv) { // Set break point at this line.
   return foo();
 }

lldb/trunk/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

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

+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.line1 = line_number('foo.h', '// !BR1')
+        self.line2 = line_number('foo.h', '// !BR2')
+        self.line_main = line_number('main.cpp', '// !BR_main')
+
+    def test(self):
+        """Test target.move-to-nearest logic"""
+
+        self.build()
+        target = self.dbg.CreateTarget("a.out")
+        self.assertTrue(target, VALID_TARGET)
+
+        # Regardless of the -m value the breakpoint should have exactly one
+        # location on the foo functions
+        self.runCmd("settings set target.move-to-nearest-code true")
+        lldbutil.run_break_set_by_file_and_line(self, 'foo.h', self.line1,
+                loc_exact=True, extra_options="-m 1")
+        lldbutil.run_break_set_by_file_and_line(self, 'foo.h', self.line2,
+                loc_exact=True, extra_options="-m 1")
+
+        self.runCmd("settings set target.move-to-nearest-code false")
+        lldbutil.run_break_set_by_file_and_line(self, 'foo.h', self.line1,
+                loc_exact=True, extra_options="-m 0")
+        lldbutil.run_break_set_by_file_and_line(self, 'foo.h', self.line2,
+                loc_exact=True, extra_options="-m 0")
+
+
+        # Make sure we set a breakpoint in main with -m 1 for various lines in
+        # the function declaration
+        # "int"
+        lldbutil.run_break_set_by_file_and_line(self, 'main.cpp',
+                self.line_main-1, extra_options="-m 1")
+        # "main()"
+        lldbutil.run_break_set_by_file_and_line(self, 'main.cpp',
+                self.line_main, extra_options="-m 1")
+        # "{"
+        lldbutil.run_break_set_by_file_and_line(self, 'main.cpp',
+                self.line_main+1, extra_options="-m 1")
+        # "return .."
+        lldbutil.run_break_set_by_file_and_line(self, 'main.cpp',
+                self.line_main+2, extra_options="-m 1")

lldb/trunk/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();

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

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

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

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

lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-mi/breakpoint/TestMiBreak.py

def test_lldbmi_break_insert_settings(self):
         self.runCmd("-file-exec-and-symbols %s" % self.myexe)
         self.expect("\^done")
 
         # Set target.move-to-nearest-code=off and try to set BP #1 that
         # shouldn't be hit
         self.runCmd(
             "-interpreter-exec console \"settings set target.move-to-nearest-code off\"")
         self.expect("\^done")
-        line = line_number('main.cpp', '// BP_before_main')
-        self.runCmd("-break-insert -f main.cpp:%d" % line)
+        line_decl = line_number('main.cpp', '// BP_main_decl')
+        line_in = line_number('main.cpp', '// BP_in_main')
+        self.runCmd("-break-insert -f main.cpp:%d" % line_in)
         self.expect("\^done,bkpt={number=\"1\"")
 
         # Test that non-pending BP will not be set on non-existing line if target.move-to-nearest-code=off
         # Note: this increases the BP number by 1 even though BP #2 is invalid.
-        self.runCmd("-break-insert main.cpp:%d" % line)
+        self.runCmd("-break-insert main.cpp:%d" % line_in)
         self.expect(
             "\^error,msg=\"Command 'break-insert'. Breakpoint location 'main.cpp:%d' not found\"" %
-            line)
+            line_in)
 
         # Set target.move-to-nearest-code=on and target.skip-prologue=on and
-        # set BP #3
+        # set BP #3 & #4
         self.runCmd(
             "-interpreter-exec console \"settings set target.move-to-nearest-code on\"")
         self.runCmd(
             "-interpreter-exec console \"settings set target.skip-prologue on\"")
         self.expect("\^done")
-        self.runCmd("-break-insert main.cpp:%d" % line)
+        self.runCmd("-break-insert main.cpp:%d" % line_in)
         self.expect("\^done,bkpt={number=\"3\"")
+        self.runCmd("-break-insert main.cpp:%d" % line_decl)
+        self.expect("\^done,bkpt={number=\"4\"")
 
-        # Set target.skip-prologue=off and set BP #4
+        # Set target.skip-prologue=off and set BP #5
         self.runCmd(
             "-interpreter-exec console \"settings set target.skip-prologue off\"")
         self.expect("\^done")
-        self.runCmd("-break-insert main.cpp:%d" % line)
-        self.expect("\^done,bkpt={number=\"4\"")
+        self.runCmd("-break-insert main.cpp:%d" % line_decl)
+        self.expect("\^done,bkpt={number=\"5\"")
 
-        # Test that BP #4 is located before BP #3
+        # Test that BP #5 is located before BP #4
         self.runCmd("-exec-run")
         self.expect("\^running")
         self.expect(
+            "\*stopped,reason=\"breakpoint-hit\",disp=\"del\",bkptno=\"5\"")
+
+        # Test that BP #4 is hit
+        self.runCmd("-exec-continue")
+        self.expect("\^running")
+        self.expect(
             "\*stopped,reason=\"breakpoint-hit\",disp=\"del\",bkptno=\"4\"")
 
         # Test that BP #3 is hit
         self.runCmd("-exec-continue")
         self.expect("\^running")
         self.expect(
             "\*stopped,reason=\"breakpoint-hit\",disp=\"del\",bkptno=\"3\"")
 
-        # Test that the target.language=pascal setting works and that BP #5 is
+        # Test that the target.language=pascal setting works and that BP #6 is
         # NOT set
         self.runCmd(
             "-interpreter-exec console \"settings set target.language c\"")
         self.expect("\^done")
         self.runCmd("-break-insert ns.foo1")
         self.expect("\^error")
 
-        # Test that the target.language=c++ setting works and that BP #6 is hit
+        # Test that the target.language=c++ setting works and that BP #7 is hit
         self.runCmd(
             "-interpreter-exec console \"settings set target.language c++\"")
         self.expect("\^done")
         self.runCmd("-break-insert ns::foo1")
-        self.expect("\^done,bkpt={number=\"6\"")
+        self.expect("\^done,bkpt={number=\"7\"")
         self.runCmd("-exec-continue")
         self.expect("\^running")
         self.expect(
-            "\*stopped,reason=\"breakpoint-hit\",disp=\"del\",bkptno=\"6\"")
+            "\*stopped,reason=\"breakpoint-hit\",disp=\"del\",bkptno=\"7\"")
 
         # Test that BP #1 and #2 weren't set by running to program exit
         self.runCmd("-exec-continue")
         self.expect("\^running")
         self.expect("\*stopped,reason=\"exited-normally\"")
 
     @skipIfWindows  # llvm.org/pr24452: Get lldb-mi tests working on Windows
     @skipIfFreeBSD  # llvm.org/pr22411: Failure presumably due to known thread races

lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-mi/breakpoint/main.cpp

 #include <cstdio>
 
 namespace ns
 {
     int foo1(void) { printf("In foo1\n"); return 1; }
     int foo2(void) { printf("In foo2\n"); return 2; }
 }
 
-// BP_before_main
-
 int x;
-int
-main(int argc, char const *argv[])
-{
+int main(int argc, char const *argv[]) { // BP_main_decl
     printf("Print a formatted string so that GCC does not optimize this printf call: %s\n", argv[0]);
+  // This is a long comment with no code inside
+  // This is a long comment with no code inside
+  // This is a long comment with no code inside
+  // BP_in_main
+  // This is a long comment with no code inside
+  // This is a long comment with no code inside
+  // This is a long comment with no code inside
     x = ns::foo1() + ns::foo2();
     return 0; // BP_return
 }

lldb/trunk/source/Breakpoint/BreakpointResolverFileLine.cpp

BreakpointResolverFileLine::SerializeToStructuredData() {
   options_dict_sp->AddBooleanItem(GetKey(OptionNames::SkipPrologue),
                                   m_skip_prologue);
   options_dict_sp->AddBooleanItem(GetKey(OptionNames::ExactMatch),
                                   m_exact_match);
 
   return WrapOptionsDict(options_dict_sp);
 }
 
+// Filter the symbol context list to remove contexts where the line number was
+// moved into a new function. We do this conservatively, so if e.g. we cannot
+// resolve the function in the context (which can happen in case of
+// line-table-only debug info), we leave the context as is. The trickiest part
+// here is handling inlined functions -- in this case we need to make sure we
+// look at the declaration line of the inlined function, NOT the function it was
+// inlined into.
+void BreakpointResolverFileLine::FilterContexts(SymbolContextList &sc_list) {
+  if (m_exact_match)
+    return; // Nothing to do. Contexts are precise.
+
+  Log * log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS);
+  for(uint32_t i = 0; i < sc_list.GetSize(); ++i) {
+    SymbolContext sc;
+    sc_list.GetContextAtIndex(i, sc);
+    if (! sc.block)
+      continue;
+
+    FileSpec file;
+    uint32_t line;
+    const Block *inline_block = sc.block->GetContainingInlinedBlock();
+    if (inline_block) {
+      const Declaration &inline_declaration = inline_block->GetInlinedFunctionInfo()->GetDeclaration();
+      if (!inline_declaration.IsValid())
+        continue;
+      file = inline_declaration.GetFile();
+      line = inline_declaration.GetLine();
+    } else if (sc.function)
+      sc.function->GetStartLineSourceInfo(file, line);
+    else
+      continue;
+
+    if (file != sc.line_entry.file) {
+      LLDB_LOG(log, "unexpected symbol context file {0}", sc.line_entry.file);
+      continue;
+    }
+
+    // Compare the requested line number with the line of the function
+    // declaration. In case of a function declared as:
+    //
+    // int
+    // foo()
+    // {
+    //   ...
+    //
+    // the compiler will set the declaration line to the "foo" line, which is
+    // the reason why we have -1 here. This can fail in case of two inline
+    // functions defined back-to-back:
+    //
+    // inline int foo1() { ... }
+    // inline int foo2() { ... }
+    //
+    // but that's the best we can do for now.
+    const int decl_line_is_too_late_fudge = 1;
+    if (m_line_number < line - decl_line_is_too_late_fudge) {
+      LLDB_LOG(log, "removing symbol context at {0}:{1}", file, line);
+      sc_list.RemoveContextAtIndex(i);
+      --i;
+    }
+  }
+}
+
 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 size_t num_comp_units = context.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);
     }
   }
+
+  FilterContexts(sc_list);
+
   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;
 }