[lldb] Don't call CopyForBreakpoint from a Breakpoint's constructor

[tatyana-krasnukha]

Summary

Some implementations (BreakpointResolverScripted) try calling the breakpoint's shared_from_this(),
that makes LLDB crash.

Added a test case that reproduced the issue.

[jingham]

LGTM

[JDevlieghere]

LGTM

lldb/include/lldb/Breakpoint/Breakpoint.h

573

nit: How about renaming this to Breakpoint::CopyToTarget instead? That seems a bit more explanatory than Breakpoint:: CopyFromBreakpoint.

[tatyana-krasnukha]

Closed by commit 185ef697ef5c

lldb/include/lldb/Breakpoint/Breakpoint.h

public:
   }
   bool AllowDisable() const {
     return GetPermissions().GetAllowDisable();
   }
   bool AllowDelete() const {
     return GetPermissions().GetAllowDelete();
   }
 
+  // This one should only be used by Target to copy breakpoints from target to
+  // target - primarily from the dummy target to prime new targets.
+  static lldb::BreakpointSP CopyFromBreakpoint(Target& new_target,
+      const Breakpoint &bp_to_copy_from);
+
 protected:
   friend class Target;
   // Protected Methods
 
   /// Constructors and Destructors
   /// Only the Target can make a breakpoint, and it owns the breakpoint
   /// lifespans. The constructor takes a filter and a resolver.  Up in Target
   /// there are convenience variants that make breakpoints for some common
protected:
   void IncrementHitCount() { m_hit_count++; }
 
   void DecrementHitCount() {
     assert(m_hit_count > 0);
     m_hit_count--;
   }
 
 private:
-  // This one should only be used by Target to copy breakpoints from target to
-  // target - primarily from the dummy target to prime new targets.
-  Breakpoint(Target &new_target, Breakpoint &bp_to_copy_from);
+  // To call from CopyFromBreakpoint.
+  Breakpoint(Target &new_target, const Breakpoint &bp_to_copy_from);
 
   // For Breakpoint only
   bool m_being_created;
   bool
       m_hardware; // If this breakpoint is required to use a hardware breakpoint
   Target &m_target; // The target that holds this breakpoint.
   std::unordered_set<std::string> m_name_list; // If not empty, this is the name
                                                // of this breakpoint (many

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py

def test_command_line(self):
         self.do_test_cli()
 
     def test_bad_command_lines(self):
         """Make sure we get appropriate errors when we give invalid key/value
            options"""
         self.build()
         self.do_test_bad_options()
 
+    @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24528")
+    def test_copy_from_dummy_target(self):
+        """Make sure we don't crash during scripted breakpoint copy from dummy target"""
+        self.build()
+        self.do_test_copy_from_dummy_target()
+
     def make_target_and_import(self):
-        target = lldbutil.run_to_breakpoint_make_target(self)
+        target = self.make_target()
+        self.import_resolver_script()
+        return target
+
+    def make_target(self):
+        return lldbutil.run_to_breakpoint_make_target(self)
+
+    def import_resolver_script(self):
         interp = self.dbg.GetCommandInterpreter()
         error = lldb.SBError()
 
         script_name = os.path.join(self.getSourceDir(), "resolver.py")
         source_name = os.path.join(self.getSourceDir(), "main.c")
 
         command = "command script import " + script_name
         result = lldb.SBCommandReturnObject()
         interp.HandleCommand(command, result)
         self.assertTrue(result.Succeeded(), "com scr imp failed: %s"%(result.GetError()))
-        return target
 
     def make_extra_args(self):
         json_string = '{"symbol":"break_on_me", "test1": "value1"}'
         json_stream = lldb.SBStream()
         json_stream.Print(json_string)
         extra_args = lldb.SBStructuredData()
         error = extra_args.SetFromJSON(json_stream)
         self.assertTrue(error.Success(), "Error making SBStructuredData: %s"%(error.GetCString()))
def do_test_bad_options(self):
         self.expect("break set -P resolver.Resolver -k a_key", error = True, msg="Missing value at end",
            substrs=['Key: "a_key" missing value'])
         self.expect("break set -P resolver.Resolver -v a_value", error = True, msg="Missing key at end",
            substrs=['Value: "a_value" missing matching key'])
         self.expect("break set -P resolver.Resolver -v a_value -k a_key -v another_value", error = True, msg="Missing key among args",
            substrs=['Value: "a_value" missing matching key'])
         self.expect("break set -P resolver.Resolver -k a_key -k a_key -v another_value", error = True, msg="Missing value among args",
            substrs=['Key: "a_key" missing value'])
+
+    def do_test_copy_from_dummy_target(self):
+        # Import breakpoint scripted resolver.
+        self.import_resolver_script()
+
+        # Create a scripted breakpoint.
+        self.runCmd("breakpoint set -P resolver.Resolver -k symbol -v break_on_me",
+                    BREAKPOINT_CREATED)
+
+        # This is the function to remove breakpoints from the dummy target
+        # to get a clean state for the next test case.
+        def cleanup():
+            self.runCmd('breakpoint delete -D -f', check=False)
+            self.runCmd('breakpoint list', check=False)
+
+        # Execute the cleanup function during test case tear down.
+        self.addTearDownHook(cleanup)
+
+        # Check that target creating doesn't crash.
+        target = self.make_target()

lldb/source/Breakpoint/Breakpoint.cpp

Breakpoint::Breakpoint(Target &target, SearchFilterSP &filter_sp,
                        bool resolve_indirect_symbols)
     : m_being_created(true), m_hardware(hardware), m_target(target),
       m_filter_sp(filter_sp), m_resolver_sp(resolver_sp),
       m_options_up(new BreakpointOptions(true)), m_locations(*this),
       m_resolve_indirect_symbols(resolve_indirect_symbols), m_hit_count(0) {
   m_being_created = false;
 }
 
-Breakpoint::Breakpoint(Target &new_target, Breakpoint &source_bp)
+Breakpoint::Breakpoint(Target &new_target, const Breakpoint &source_bp)
     : m_being_created(true), m_hardware(source_bp.m_hardware),
       m_target(new_target), m_name_list(source_bp.m_name_list),
       m_options_up(new BreakpointOptions(*source_bp.m_options_up)),
       m_locations(*this),
       m_resolve_indirect_symbols(source_bp.m_resolve_indirect_symbols),
-      m_hit_count(0) {
-  // Now go through and copy the filter & resolver:
-  m_resolver_sp = source_bp.m_resolver_sp->CopyForBreakpoint(*this);
-  m_filter_sp = source_bp.m_filter_sp->CopyForBreakpoint(*this);
-}
+      m_hit_count(0) {}
 
 // Destructor
 Breakpoint::~Breakpoint() = default;
 
+BreakpointSP Breakpoint::CopyFromBreakpoint(Target& new_target,
+    const Breakpoint& bp_to_copy_from) {
+  BreakpointSP bp(new Breakpoint(new_target, bp_to_copy_from));
+  // Now go through and copy the filter & resolver:
+  bp->m_resolver_sp = bp_to_copy_from.m_resolver_sp->CopyForBreakpoint(*bp);
+  bp->m_filter_sp = bp_to_copy_from.m_filter_sp->CopyForBreakpoint(*bp);
+  return bp;
+}
+
 // Serialization
 StructuredData::ObjectSP Breakpoint::SerializeToStructuredData() {
   // Serialize the resolver:
   StructuredData::DictionarySP breakpoint_dict_sp(
       new StructuredData::Dictionary());
   StructuredData::DictionarySP breakpoint_contents_sp(
       new StructuredData::Dictionary());
 

lldb/source/Target/Target.cpp

 }
 
 void Target::PrimeFromDummyTarget(Target *target) {
   if (!target)
     return;
 
   m_stop_hooks = target->m_stop_hooks;
 
-  for (BreakpointSP breakpoint_sp : target->m_breakpoint_list.Breakpoints()) {
+  for (const auto &breakpoint_sp : target->m_breakpoint_list.Breakpoints()) {
     if (breakpoint_sp->IsInternal())
       continue;
 
-    BreakpointSP new_bp(new Breakpoint(*this, *breakpoint_sp.get()));
-    AddBreakpoint(new_bp, false);
+    BreakpointSP new_bp(Breakpoint::CopyFromBreakpoint(*this, *breakpoint_sp));
+    AddBreakpoint(std::move(new_bp), false);
   }
 
   for (auto bp_name_entry : target->m_breakpoint_names) {
 
     BreakpointName *new_bp_name = new BreakpointName(*bp_name_entry.second);
     AddBreakpointName(new_bp_name);
   }
 }