[lldb] Make shared_from_this-related code safer

[tatyana-krasnukha]

Summary

The idea is: the fewer classes make an assumption that a target object is already managed by a shared_ptr - the fewer ways to make a mistake.

Pass TargetSP to filters' CreateFromStructuredData, don't let them guess whether the target object is managed by a shared_ptr.

Make Breakpoint sure that m_target.shared_from_this() is safe by passing TargetSP to all its static Create*** member-functions.
This should be enough since Breakpoint's constructors are private/protected and never called directly (except by Target itself).

The patch is a consequence of D74556.

[jingham]

LGTM

Targets and Breakpoints should only ever be managed by SP in lldb, so enforcing that seems like a good cleanup.

[tatyana-krasnukha]

Closed by commit b624b7dfd087

lldb/include/lldb/Breakpoint/Breakpoint.h

private:
     lldb::BreakpointSP m_new_breakpoint_sp;
     BreakpointLocationCollection m_locations;
 
     DISALLOW_COPY_AND_ASSIGN(BreakpointEventData);
   };
 
   // Saving & restoring breakpoints:
   static lldb::BreakpointSP CreateFromStructuredData(
-      Target &target, StructuredData::ObjectSP &data_object_sp, Status &error);
+      lldb::TargetSP target_sp, StructuredData::ObjectSP &data_object_sp,
+      Status &error);
 
   static bool
   SerializedBreakpointMatchesNames(StructuredData::ObjectSP &bkpt_object_sp,
                                    std::vector<std::string> &names);
 
   virtual StructuredData::ObjectSP SerializeToStructuredData();
 
   static const char *GetSerializationKey() { return "Breakpoint"; }
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,
+  static lldb::BreakpointSP CopyFromBreakpoint(lldb::TargetSP 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

lldb/include/lldb/Core/SearchFilter.h

public:
   virtual void GetDescription(Stream *s);
 
   /// Standard "Dump" method.  At present it does nothing.
   virtual void Dump(Stream *s) const;
 
   lldb::SearchFilterSP CopyForBreakpoint(Breakpoint &breakpoint);
 
   static lldb::SearchFilterSP
-  CreateFromStructuredData(Target &target,
+  CreateFromStructuredData(const lldb::TargetSP& target_sp,
                            const StructuredData::Dictionary &data_dict,
                            Status &error);
 
   virtual StructuredData::ObjectSP SerializeToStructuredData() {
     return StructuredData::ObjectSP();
   }
 
   static const char *GetSerializationKey() { return "SearchFilter"; }
public:
 
   ~SearchFilterForUnconstrainedSearches() override = default;
 
   bool ModulePasses(const FileSpec &module_spec) override;
 
   bool ModulePasses(const lldb::ModuleSP &module_sp) override;
 
   static lldb::SearchFilterSP
-  CreateFromStructuredData(Target &target,
+  CreateFromStructuredData(const lldb::TargetSP& target_sp,
                            const StructuredData::Dictionary &data_dict,
                            Status &error);
 
   StructuredData::ObjectSP SerializeToStructuredData() override;
 
 protected:
   lldb::SearchFilterSP DoCopyForBreakpoint(Breakpoint &breakpoint) override;
 };
public:
 
   uint32_t GetFilterRequiredItems() override;
 
   void Dump(Stream *s) const override;
 
   void Search(Searcher &searcher) override;
 
   static lldb::SearchFilterSP
-  CreateFromStructuredData(Target &target,
+  CreateFromStructuredData(const lldb::TargetSP& target_sp,
                            const StructuredData::Dictionary &data_dict,
                            Status &error);
 
   StructuredData::ObjectSP SerializeToStructuredData() override;
 
 protected:
   lldb::SearchFilterSP DoCopyForBreakpoint(Breakpoint &breakpoint) override;
 
public:
 
   uint32_t GetFilterRequiredItems() override;
 
   void Dump(Stream *s) const override;
 
   void Search(Searcher &searcher) override;
 
   static lldb::SearchFilterSP
-  CreateFromStructuredData(Target &target,
+  CreateFromStructuredData(const lldb::TargetSP& target_sp,
                            const StructuredData::Dictionary &data_dict,
                            Status &error);
 
   StructuredData::ObjectSP SerializeToStructuredData() override;
 
   void SerializeUnwrapped(StructuredData::DictionarySP &options_dict_sp);
 
 protected:
public:
 
   uint32_t GetFilterRequiredItems() override;
 
   void Dump(Stream *s) const override;
 
   void Search(Searcher &searcher) override;
 
   static lldb::SearchFilterSP
-  CreateFromStructuredData(Target &target,
+  CreateFromStructuredData(const lldb::TargetSP& target_sp,
                            const StructuredData::Dictionary &data_dict,
                            Status &error);
 
   StructuredData::ObjectSP SerializeToStructuredData() override;
 
 protected:
   lldb::SearchFilterSP DoCopyForBreakpoint(Breakpoint &breakpoint) override;
 
 private:
   FileSpecList m_cu_spec_list;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_SearchFilter_h_

lldb/source/Breakpoint/Breakpoint.cpp

: m_being_created(true), m_hardware(source_bp.m_hardware),
       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) {}
 
 // Destructor
 Breakpoint::~Breakpoint() = default;
 
-BreakpointSP Breakpoint::CopyFromBreakpoint(Target& new_target,
+BreakpointSP Breakpoint::CopyFromBreakpoint(TargetSP new_target,
     const Breakpoint& bp_to_copy_from) {
-  BreakpointSP bp(new Breakpoint(new_target, bp_to_copy_from));
+  if (!new_target)
+    return BreakpointSP();
+
+  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() {
StructuredData::ObjectSP Breakpoint::SerializeToStructuredData() {
   breakpoint_contents_sp->AddItem(BreakpointOptions::GetSerializationKey(),
                                   options_dict_sp);
 
   breakpoint_dict_sp->AddItem(GetSerializationKey(), breakpoint_contents_sp);
   return breakpoint_dict_sp;
 }
 
 lldb::BreakpointSP Breakpoint::CreateFromStructuredData(
-    Target &target, StructuredData::ObjectSP &object_data, Status &error) {
+    TargetSP target_sp, StructuredData::ObjectSP &object_data, Status &error) {
   BreakpointSP result_sp;
+  if (!target_sp)
+    return result_sp;
 
   StructuredData::Dictionary *breakpoint_dict = object_data->GetAsDictionary();
 
   if (!breakpoint_dict || !breakpoint_dict->IsValid()) {
     error.SetErrorString("Can't deserialize from an invalid data object.");
     return result_sp;
   }
 
if (create_error.Fail()) {
     return result_sp;
   }
 
   StructuredData::Dictionary *filter_dict;
   success = breakpoint_dict->GetValueForKeyAsDictionary(
       SearchFilter::GetSerializationKey(), filter_dict);
   SearchFilterSP filter_sp;
   if (!success)
-    filter_sp = std::make_shared<SearchFilterForUnconstrainedSearches>(
-        target.shared_from_this());
+    filter_sp =
+        std::make_shared<SearchFilterForUnconstrainedSearches>(target_sp);
   else {
-    filter_sp = SearchFilter::CreateFromStructuredData(target, *filter_dict,
-                                                       create_error);
+    filter_sp = SearchFilter::CreateFromStructuredData(target_sp, *filter_dict,
     if (create_error.Fail()) {
       error.SetErrorStringWithFormat(
           "Error creating breakpoint filter from data: %s.",
           create_error.AsCString());
       return result_sp;
     }
   }
 
   std::unique_ptr<BreakpointOptions> options_up;
   StructuredData::Dictionary *options_dict;
+  Target& target = *target_sp;
   success = breakpoint_dict->GetValueForKeyAsDictionary(
       BreakpointOptions::GetSerializationKey(), options_dict);
   if (success) {
     options_up = BreakpointOptions::CreateFromStructuredData(
         target, *options_dict, create_error);
     if (create_error.Fail()) {
       error.SetErrorStringWithFormat(
           "Error creating breakpoint options from data: %s.",
           create_error.AsCString());
       return result_sp;
     }
   }
 
   bool hardware = false;
   success = breakpoint_dict->GetValueForKeyAsBoolean(
       Breakpoint::GetKey(OptionNames::Hardware), hardware);
 
-  result_sp =
-      target.CreateBreakpoint(filter_sp, resolver_sp, false, hardware, true);
+  result_sp = target.CreateBreakpoint(filter_sp, resolver_sp, false,
+                                      hardware, true);
 
   if (result_sp && options_up) {
     result_sp->m_options_up = std::move(options_up);
   }
 
   StructuredData::Array *names_array;
   success = breakpoint_dict->GetValueForKeyAsArray(
       Breakpoint::GetKey(OptionNames::Names), names_array);

lldb/source/Core/SearchFilter.cpp

 void Searcher::GetDescription(Stream *s) {}
 
 SearchFilter::SearchFilter(const TargetSP &target_sp, unsigned char filterType)
     : m_target_sp(target_sp), SubclassID(filterType) {}
 
 SearchFilter::~SearchFilter() = default;
 
 SearchFilterSP SearchFilter::CreateFromStructuredData(
-    Target &target, const StructuredData::Dictionary &filter_dict,
+    const lldb::TargetSP& target_sp,
+    const StructuredData::Dictionary &filter_dict,
     Status &error) {
   SearchFilterSP result_sp;
   if (!filter_dict.IsValid()) {
     error.SetErrorString("Can't deserialize from an invalid data object.");
     return result_sp;
   }
 
   llvm::StringRef subclass_name;
SearchFilterSP SearchFilter::CreateFromStructuredData(
   if (!success || !subclass_options || !subclass_options->IsValid()) {
     error.SetErrorString("Filter data missing subclass options key.");
     return result_sp;
   }
 
   switch (filter_type) {
   case Unconstrained:
     result_sp = SearchFilterForUnconstrainedSearches::CreateFromStructuredData(
-        target, *subclass_options, error);
+        target_sp, *subclass_options, error);
     break;
   case ByModule:
     result_sp = SearchFilterByModule::CreateFromStructuredData(
-        target, *subclass_options, error);
+        target_sp, *subclass_options, error);
     break;
   case ByModules:
     result_sp = SearchFilterByModuleList::CreateFromStructuredData(
-        target, *subclass_options, error);
+        target_sp, *subclass_options, error);
     break;
   case ByModulesAndCU:
     result_sp = SearchFilterByModuleListAndCU::CreateFromStructuredData(
-        target, *subclass_options, error);
+        target_sp, *subclass_options, error);
     break;
   case Exception:
     error.SetErrorString("Can't serialize exception breakpoints yet.");
     break;
   default:
     llvm_unreachable("Should never get an uresolvable filter type.");
   }
 
void SearchFilter::Search(Searcher &searcher) {
   if (!m_target_sp)
     return;
   empty_sc.target_sp = m_target_sp;
 
   if (searcher.GetDepth() == lldb::eSearchDepthTarget) {
     searcher.SearchCallback(*this, empty_sc, nullptr);
     return;
   }
-  
   DoModuleIteration(empty_sc, searcher);
 }
 
 void SearchFilter::SearchInModuleList(Searcher &searcher, ModuleList &modules) {
   SymbolContext empty_sc;
 
   if (!m_target_sp)
     return;
Searcher::CallbackReturn SearchFilter::DoFunctionIteration(
   // FIXME: Implement...
   return Searcher::eCallbackReturnContinue;
 }
 
 //  SearchFilterForUnconstrainedSearches:
 //  Selects a shared library matching a given file spec, consulting the targets
 //  "black list".
 SearchFilterSP SearchFilterForUnconstrainedSearches::CreateFromStructuredData(
-    Target &target, const StructuredData::Dictionary &data_dict,
+    const lldb::TargetSP& target_sp,
+    const StructuredData::Dictionary &data_dict,
     Status &error) {
   // No options for an unconstrained search.
-  return std::make_shared<SearchFilterForUnconstrainedSearches>(
-      target.shared_from_this());
+  return std::make_shared<SearchFilterForUnconstrainedSearches>(target_sp);
 }
 
 StructuredData::ObjectSP
 SearchFilterForUnconstrainedSearches::SerializeToStructuredData() {
   // The options dictionary is an empty dictionary:
   auto result_sp = std::make_shared<StructuredData::Dictionary>();
   return WrapOptionsDict(result_sp);
 }

 void SearchFilterByModule::Dump(Stream *s) const {}
 
 lldb::SearchFilterSP
 SearchFilterByModule::DoCopyForBreakpoint(Breakpoint &breakpoint) {
   return std::make_shared<SearchFilterByModule>(*this);
 }
 
 SearchFilterSP SearchFilterByModule::CreateFromStructuredData(
-    Target &target, const StructuredData::Dictionary &data_dict,
+    const lldb::TargetSP& target_sp,
+    const StructuredData::Dictionary &data_dict,
     Status &error) {
   StructuredData::Array *modules_array;
   bool success = data_dict.GetValueForKeyAsArray(GetKey(OptionNames::ModList),
                                                  modules_array);
   if (!success) {
     error.SetErrorString("SFBM::CFSD: Could not find the module list key.");
     return nullptr;
   }
 
   size_t num_modules = modules_array->GetSize();
   if (num_modules > 1) {
     error.SetErrorString(
         "SFBM::CFSD: Only one modules allowed for SearchFilterByModule.");
     return nullptr;
   }
 
   llvm::StringRef module;
   success = modules_array->GetItemAtIndexAsString(0, module);
   if (!success) {
     error.SetErrorString("SFBM::CFSD: filter module item not a string.");
     return nullptr;
   }
   FileSpec module_spec(module);
 
-  return std::make_shared<SearchFilterByModule>(target.shared_from_this(),
-                                                module_spec);
+  return std::make_shared<SearchFilterByModule>(target_sp, module_spec);
 }
 
 StructuredData::ObjectSP SearchFilterByModule::SerializeToStructuredData() {
   auto options_dict_sp = std::make_shared<StructuredData::Dictionary>();
   auto module_array_sp = std::make_shared<StructuredData::Array>();
   module_array_sp->AddItem(
       std::make_shared<StructuredData::String>(m_module_spec.GetPath()));
   options_dict_sp->AddItem(GetKey(OptionNames::ModList), module_array_sp);

 void SearchFilterByModuleList::Dump(Stream *s) const {}
 
 lldb::SearchFilterSP
 SearchFilterByModuleList::DoCopyForBreakpoint(Breakpoint &breakpoint) {
   return std::make_shared<SearchFilterByModuleList>(*this);
 }
 
 SearchFilterSP SearchFilterByModuleList::CreateFromStructuredData(
-    Target &target, const StructuredData::Dictionary &data_dict,
+    const lldb::TargetSP& target_sp,
+    const StructuredData::Dictionary &data_dict,
     Status &error) {
   StructuredData::Array *modules_array;
   bool success = data_dict.GetValueForKeyAsArray(GetKey(OptionNames::ModList),
                                                  modules_array);
 
   if (!success)
-    return std::make_shared<SearchFilterByModuleList>(target.shared_from_this(),
+    return std::make_shared<SearchFilterByModuleList>(target_sp,
                                                       FileSpecList{});
   FileSpecList modules;
   size_t num_modules = modules_array->GetSize();
   for (size_t i = 0; i < num_modules; i++) {
     llvm::StringRef module;
     success = modules_array->GetItemAtIndexAsString(i, module);
     if (!success) {
       error.SetErrorStringWithFormat(
           "SFBM::CFSD: filter module item %zu not a string.", i);
       return nullptr;
     }
     modules.EmplaceBack(module);
   }
-  return std::make_shared<SearchFilterByModuleList>(target.shared_from_this(),
-                                                    modules);
+  return std::make_shared<SearchFilterByModuleList>(target_sp, modules);
 }
 
 void SearchFilterByModuleList::SerializeUnwrapped(
     StructuredData::DictionarySP &options_dict_sp) {
   SerializeFileSpecList(options_dict_sp, OptionNames::ModList,
                         m_module_spec_list);
 }
 
SearchFilterByModuleListAndCU::SearchFilterByModuleListAndCU(
     const FileSpecList &cu_list)
     : SearchFilterByModuleList(target_sp, module_list,
                                FilterTy::ByModulesAndCU),
       m_cu_spec_list(cu_list) {}
 
 SearchFilterByModuleListAndCU::~SearchFilterByModuleListAndCU() = default;
 
 lldb::SearchFilterSP SearchFilterByModuleListAndCU::CreateFromStructuredData(
-    Target &target, const StructuredData::Dictionary &data_dict,
+    const lldb::TargetSP& target_sp,
+    const StructuredData::Dictionary &data_dict,
     Status &error) {
   StructuredData::Array *modules_array = nullptr;
   SearchFilterSP result_sp;
   bool success = data_dict.GetValueForKeyAsArray(GetKey(OptionNames::ModList),
                                                  modules_array);
   FileSpecList modules;
   if (success) {
     size_t num_modules = modules_array->GetSize();
if (!success) {
       error.SetErrorStringWithFormat(
           "SFBM::CFSD: filter CU item %zu not a string.", i);
       return nullptr;
     }
     cus.EmplaceBack(cu);
   }
 
   return std::make_shared<SearchFilterByModuleListAndCU>(
-      target.shared_from_this(), modules, cus);
+      target_sp, modules, cus);
 }
 
 StructuredData::ObjectSP
 SearchFilterByModuleListAndCU::SerializeToStructuredData() {
   auto options_dict_sp = std::make_shared<StructuredData::Dictionary>();
   SearchFilterByModuleList::SerializeUnwrapped(options_dict_sp);
   SerializeFileSpecList(options_dict_sp, OptionNames::CUList, m_cu_spec_list);
   return WrapOptionsDict(options_dict_sp);

lldb/source/Target/Target.cpp

if (!target)
     return;
 
   m_stop_hooks = target->m_stop_hooks;
 
   for (const auto &breakpoint_sp : target->m_breakpoint_list.Breakpoints()) {
     if (breakpoint_sp->IsInternal())
       continue;
 
-    BreakpointSP new_bp(Breakpoint::CopyFromBreakpoint(*this, *breakpoint_sp));
+    BreakpointSP new_bp(
+        Breakpoint::CopyFromBreakpoint(shared_from_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);
   }
if (!bkpt_dict) {
       return error;
     }
     StructuredData::ObjectSP bkpt_data_sp =
         bkpt_dict->GetValueForKey(Breakpoint::GetSerializationKey());
     if (num_names &&
         !Breakpoint::SerializedBreakpointMatchesNames(bkpt_data_sp, names))
       continue;
 
-    BreakpointSP bkpt_sp =
-        Breakpoint::CreateFromStructuredData(*this, bkpt_data_sp, error);
+    BreakpointSP bkpt_sp = Breakpoint::CreateFromStructuredData(
+        shared_from_this(), bkpt_data_sp, error);
     if (!error.Success()) {
       error.SetErrorStringWithFormat(
           "Error restoring breakpoint %zu from %s: %s.", i,
           file.GetPath().c_str(), error.AsCString());
       return error;
     }
     new_bps.AddBreakpointID(BreakpointID(bkpt_sp->GetID()));
   }