Make target.process.stop-on-sharedlibrary-events setting work

[jingham]

Summary

This setting is supposed to make us stop when we hit a shared library event. The shared library event callbacks happen as synchronous breakpoint callbacks - below the level of user callbacks. The information that the synchronous callback wanted to stop was getting propagated to the StopInfoBreakpoint correctly, but then we lost that bit of information when we went to do PerformAction. This patch takes that into account.

In this patch, if the synchronous callback says to stop, but the user has another breakpoint that's auto-continue, or a callback that says to continue, at the same spot, I obey the other breakpoint's opinions rather than the callback one. That seems to me right since the sync callbacks we have around are implementation details and the user can't even really know where they might be easily. So having them collide with the user's intentions doesn't seem right.

[JDevlieghere]

A few nits but the change itself looks sound.

lldb/source/Breakpoint/BreakpointOptions.cpp

457

s/expression/expressing/

459

lldb/test/API/functionalities/stop-on-sharedlibrary-load/Makefile

1

Not that it matters, but I guess this could as well be a C test?

[jingham]

I added a couple more tests for having a breakpoint at the load site before committing.

lldb/source/Breakpoint/BreakpointOptions.cpp

 //===-- BreakpointOptions.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Breakpoint/BreakpointOptions.h"
 
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/ThreadSpec.h"
bool BreakpointOptions::InvokeCallback(StoppointCallbackContext *context,
                                        lldb::user_id_t break_id,
                                        lldb::user_id_t break_loc_id) {
   if (m_callback) {
     if (context->is_synchronous == IsCallbackSynchronous()) {
         return m_callback(m_callback_baton_sp ? m_callback_baton_sp->data()
                                           : nullptr,
                       context, break_id, break_loc_id);
     } else if (IsCallbackSynchronous()) {
-      // If a synchronous callback is called at async time, it should not say
-      // to stop.
-      return false;
+      // If a synchronous callback is called at async time, we will say we
+      // should stop, we're really expression no opinion about stopping, and
+      // the StopInfoBreakpoint::PerformAction will note whether an async
+      // callback had already made a claim to stop or not based on the incoming
+      // values of m_should_stop & m_should_stop_is_valid.
+      return true;
     }
   }
   return true;
 }
 
 bool BreakpointOptions::HasCallback() const {
   return m_callback != BreakpointOptions::NullCallback;
 }

lldb/source/Target/StopInfo.cpp

 //===-- StopInfo.cpp ------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <string>
 
 #include "lldb/Breakpoint/Breakpoint.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Breakpoint/Watchpoint.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StopInfo.h"
if (thread_sp) {
           // give all the breakpoints whose conditions say we are going to stop
           // a chance to run their callbacks. Of course if any callback
           // restarts the target by putting "continue" in the callback, then
           // we're going to restart, without running the rest of the callbacks.
           // And in this case we will end up not stopping even if another
           // location said we should stop. But that's better than not running
           // all the callbacks.
 
+          // There's one other complication here.  We may have run an async
+          // breakpoint callback that said we should stop.  We only want to
+          // override that if another breakpoint action says we shouldn't 
+          // stop.  If nobody else has an opinion, then we should stop if the
+          // async callback says we should.  An example of this is the async
+          // shared library load notification breakpoint and the setting
+          // stop-on-sharedlibrary-events.
+          // We'll keep the async value in async_should_stop, and track whether
+          // anyone said we should NOT stop in actually_said_continue.
+          bool async_should_stop = false;
+          if (m_should_stop_is_valid)
+            async_should_stop = m_should_stop;
+          bool actually_said_continue = false;
+
           m_should_stop = false;
 
           // We don't select threads as we go through them testing breakpoint
           // conditions and running commands. So we need to set the thread for
           // expression evaluation here:
           ThreadList::ExpressionExecutionThreadPusher thread_pusher(thread_sp);
 
           ExecutionContext exe_ctx(thread_sp->GetStackFrameAtIndex(0));
if (thread_sp) {
             std::pair<std::unordered_set<break_id_t>::iterator, bool> result =
                 precondition_breakpoints.insert(
                     bp_loc_sp->GetBreakpoint().GetID());
             if (!result.second)
               continue;
 
             bool precondition_result =
                 bp_loc_sp->GetBreakpoint().EvaluatePrecondition(context);
-            if (!precondition_result)
+            if (!precondition_result) {
+              actually_said_continue = true;
               continue;
-
+            }
             // Next run the condition for the breakpoint.  If that says we
             // should stop, then we'll run the callback for the breakpoint.  If
             // the callback says we shouldn't stop that will win.
 
             if (bp_loc_sp->GetConditionText() != nullptr) {
               Status condition_error;
               bool condition_says_stop =
                   bp_loc_sp->ConditionSaysStop(exe_ctx, condition_error);
if (thread_sp) {
                           loc_desc.GetData(),
                           static_cast<unsigned long long>(thread_sp->GetID()),
                           condition_says_stop);
                 if (!condition_says_stop) {
                   // We don't want to increment the hit count of breakpoints if
                   // the condition fails. We've already bumped it by the time
                   // we get here, so undo the bump:
                   bp_loc_sp->UndoBumpHitCount();
+                  actually_said_continue = true;
                   continue;
                 }
               }
             }
 
             // Check the auto-continue bit on the location, do this before the
             // callback since it may change this, but that would be for the
             // NEXT hit.  Note, you might think you could check auto-continue
if (thread_sp) {
             debugger.SetAsyncExecution(true);
 
             callback_says_stop = bp_loc_sp->InvokeCallback(&context);
 
             debugger.SetAsyncExecution(old_async);
 
             if (callback_says_stop && auto_continue_says_stop)
               m_should_stop = true;
+            else
+              actually_said_continue = true;
+
                   
             // If we are going to stop for this breakpoint, then remove the
             // breakpoint.
             if (callback_says_stop && bp_loc_sp &&
                 bp_loc_sp->GetBreakpoint().IsOneShot()) {
               thread_sp->GetProcess()->GetTarget().RemoveBreakpointByID(
                   bp_loc_sp->GetBreakpoint().GetID());
             }
             // Also make sure that the callback hasn't continued the target. If
             // it did, when we'll set m_should_start to false and get out of
             // here.
             if (HasTargetRunSinceMe()) {
               m_should_stop = false;
+              actually_said_continue = true;
               break;
             }
           }
+          // At this point if nobody actually told us to continue, we should
+          // give the async breakpoint callback a chance to weigh in:
+          if (!actually_said_continue && !m_should_stop) {
+            m_should_stop = async_should_stop;
+          }
         }
         // We've figured out what this stop wants to do, so mark it as valid so
         // we don't compute it again.
         m_should_stop_is_valid = true;
       } else {
         m_should_stop = true;
         m_should_stop_is_valid = true;
         Log *log_process(

lldb/test/API/functionalities/stop-on-sharedlibrary-load/Makefile

+CXX_SOURCES := main.cpp
+USE_LIBDL := 1
+
+a.out: lib_a
+
+include Makefile.rules
+
+lib_a:
+  $(MAKE) -f $(MAKEFILE_RULES) \
+    DYLIB_ONLY=YES DYLIB_CXX_SOURCES=a.cpp DYLIB_NAME=load_a
+
+

lldb/test/API/functionalities/stop-on-sharedlibrary-load/TestStopOnSharedlibraryEvents.py

+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestStopOnSharedlibraryEvents(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipIfRemote
+    @skipIfWindows
+    @no_debug_info_test
+    def test(self):
+        self.build()
+        main_spec = lldb.SBFileSpec("main.cpp")
+        # Launch and stop before the dlopen call.
+        target, process, _, _ = lldbutil.run_to_source_breakpoint(self,
+                                                                  "// Set a breakpoint here", main_spec)
+
+        # Now turn on shared library events, continue and make sure we stop for the event.
+        self.runCmd("settings set target.process.stop-on-sharedlibrary-events 1")
+        self.addTearDownHook(lambda: self.runCmd(
+            "settings set target.process.stop-on-sharedlibrary-events 0"))
+
+        # Since I don't know how to check that we are at the "right place" to stop for
+        # shared library events, make an breakpoint after the load is done and
+        # make sure we don't stop there:
+        bkpt = target.BreakpointCreateBySourceRegex("Set another here - we should not hit this one", main_spec)
+        self.assertGreater(bkpt.GetNumLocations(), 0, "Set our second breakpoint")
+        
+        process.Continue() 
+        self.assertEqual(process.GetState(), lldb.eStateStopped, "We didn't stop for the load")
+        self.assertEqual(bkpt.GetHitCount(), 0, "Hit our backstop breakpoint")
+        
+        # Finally, we should be stopped after the library is loaded:
+        found_it = False
+        for module in target.modules:
+            if module.file.basename.find("load_a") > -1:
+                found_it = True
+                break
+        self.assertTrue(found_it, "Found the loaded module.")
+        
+        

lldb/test/API/functionalities/stop-on-sharedlibrary-load/a.cpp

+extern int i_have_a_function();
+
+int
+i_have_a_function() {
+  return 10;
+}

lldb/test/API/functionalities/stop-on-sharedlibrary-load/main.cpp

+#include "dylib.h"
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+int main(int argc, char const *argv[]) {
+  const char *a_name = "load_a";
+  void *a_dylib_handle = NULL;
+
+  a_dylib_handle = dylib_open(a_name); // Set a breakpoint here.
+  if (a_dylib_handle == NULL) { // Set another here - we should not hit this one
+    fprintf(stderr, "%s\n", dylib_last_error());
+    exit(1);
+  }
+  return 0;
+}