[LLDB][NFC] Fix memory leak in IntstumentationRuntimeTSan.cpp

[fixathon]

Summary

ConvertToStructuredArray() relies on its caller to deallocate the heap-allocated object pointer it returns. One of its call-sites, in GetRenumberedThreadIds(), fails to deallocate causing a memory/resource leak. This fix aims to fix this issue via RAII-style clean up.

[fixathon]

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

226

Function creating heap-based allocation

232

Allocation of the leaked entity

264

Delete the heap-allocated resource via RAII on function exit.

287

Result is set here. Note, at this point, there's no continued dependency on the StructuredData::Array object returned by the call to ConvertToStructuredArray(), and that object does not need to persist.

[JDevlieghere]

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

226

Could this return a unique_ptr instead?

[clayborg]

So any functions that are "StructureData::<class> *" raw pointers in this source code should switch over to returning on of the StruturedData definitions from StructuredData.h:

class StructuredData {
  typedef std::shared_ptr<Object> ObjectSP;
  typedef std::shared_ptr<Array> ArraySP;
  typedef std::shared_ptr<Integer> IntegerSP;
  typedef std::shared_ptr<Float> FloatSP;
  typedef std::shared_ptr<Boolean> BooleanSP;
  typedef std::shared_ptr<String> StringSP;
  typedef std::shared_ptr<Dictionary> DictionarySP;
  typedef std::shared_ptr<Generic> GenericSP;

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

226

This should return StructureData::ArraySP. Look at the other usages of this function where they will eventually put the result into a dictionary

351

If we make ConvertToStructuredArray return a ArraySP, then this code would change

[fixathon]

I am going to clean up the code in the entire file where raw pointers should be shared_ptr (because they get cast to shared_ptr anyways). Also planning to convert the naked new and the 2-step shared_ptr creation to make_shared<> ; the former is less-safe for potential leaks, and the latter is strictly better.

@JDevlieghere Yes, some of these planned raw pointer changes could be unique_ptr, but these types are already typedef-ed in StructuredData.h as shared_ptr and I'd like to preserve the style in the absence of specific know performance issues in this code.

[fixathon]

Convert raw pointers to shared_ptr

[JDevlieghere]

In D131900#3724366, @fixathon wrote:

I am going to clean up the code in the entire file where raw pointers should be shared_ptr (because they get cast to shared_ptr anyways). Also planning to convert the naked new and the 2-step shared_ptr creation to make_shared<> ; the former is less-safe for potential leaks, and the latter is strictly better.

@JDevlieghere Yes, some of these planned raw pointer changes could be unique_ptr, but these types are already typedef-ed in StructuredData.h as shared_ptr and I'd like to preserve the style in the absence of specific know performance issues in this code.

Yup, any smart pointer works for me 👍

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

38

We don't do this anywhere in LLVM, and the coding guidelines explicitly state that "we prefer to explicitly prefix all identifiers from the standard namespace with an “std::” prefix". [1] This isn't exactly the same as using namespace std but I would argue the sentiment still holds.

[1] https://llvm.org/docs/CodingStandards.html#do-not-use-using-namespace-std

[fixathon]

Address the style issue by removing "using", and specifying std:: namespace explicitly.

Note, lldb style guide does not appear to prohibit the import of individual members from another namespace in a non-header file. Nevertheless, fixing the style for consistency.

[fixathon]

All pending suggestions have been resolved

[clayborg]

Looks great, maybe fix the pass by value in the callback and this is good?

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

233

Pass by reference to avoid an increment and decrement in the shared pointer due to a copy

348–350

combine this line

[fixathon]

Address comments about passing-by-ref for perf

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 
 #include <memory>
 
 using namespace lldb;
 using namespace lldb_private;
 
 LLDB_PLUGIN_DEFINE(InstrumentationRuntimeTSan)
 
 lldb::InstrumentationRuntimeSP
 InstrumentationRuntimeTSan::CreateInstance(const lldb::ProcessSP &process_sp) {
   return InstrumentationRuntimeSP(new InstrumentationRuntimeTSan(process_sp));
 }
 
 void InstrumentationRuntimeTSan::Initialize() {
   PluginManager::RegisterPlugin(

 for (int i = 0; i < t.unique_tid_count; i++) {
     t.unique_tids[i].idx = i;
     __tsan_get_report_unique_tid(t.report, i, &t.unique_tids[i].tid);
 }
 
 t;
 )";
 
-static StructuredData::Array *
+static StructuredData::ArraySP
 CreateStackTrace(ValueObjectSP o,
                  const std::string &trace_item_name = ".trace") {
-  StructuredData::Array *trace = new StructuredData::Array();
+  auto trace_sp = std::make_shared<StructuredData::Array>();
   ValueObjectSP trace_value_object =
       o->GetValueForExpressionPath(trace_item_name.c_str());
   size_t count = trace_value_object->GetNumChildren();
   for (size_t j = 0; j < count; j++) {
     addr_t trace_addr =
         trace_value_object->GetChildAtIndex(j, true)->GetValueAsUnsigned(0);
     if (trace_addr == 0)
       break;
-    trace->AddItem(
-        StructuredData::ObjectSP(new StructuredData::Integer(trace_addr)));
+    trace_sp->AddItem(std::make_shared<StructuredData::Integer>(trace_addr));
   }
-  return trace;
+  return trace_sp;
 }
 
-static StructuredData::Array *ConvertToStructuredArray(
+static StructuredData::ArraySP ConvertToStructuredArray(
     ValueObjectSP return_value_sp, const std::string &items_name,
     const std::string &count_name,
-    std::function<void(ValueObjectSP o, StructuredData::Dictionary *dict)> const
+    std::function<void(const ValueObjectSP &o,
+                       const StructuredData::DictionarySP &dict)> const
         &callback) {
-  StructuredData::Array *array = new StructuredData::Array();
+  auto array_sp = std::make_shared<StructuredData::Array>();
   unsigned int count =
       return_value_sp->GetValueForExpressionPath(count_name.c_str())
           ->GetValueAsUnsigned(0);
   ValueObjectSP objects =
       return_value_sp->GetValueForExpressionPath(items_name.c_str());
   for (unsigned int i = 0; i < count; i++) {
     ValueObjectSP o = objects->GetChildAtIndex(i, true);
-    StructuredData::Dictionary *dict = new StructuredData::Dictionary();
+    auto dict_sp = std::make_shared<StructuredData::Dictionary>();
 
-    callback(o, dict);
+    callback(o, dict_sp);
 
-    array->AddItem(StructuredData::ObjectSP(dict));
+    array_sp->AddItem(dict_sp);
   }
-  return array;
+  return array_sp;
 }
 
 static std::string RetrieveString(ValueObjectSP return_value_sp,
                                   ProcessSP process_sp,
                                   const std::string &expression_path) {
   addr_t ptr =
       return_value_sp->GetValueForExpressionPath(expression_path.c_str())
           ->GetValueAsUnsigned(0);
   std::string str;
   Status error;
   process_sp->ReadCStringFromMemory(ptr, str, error);
   return str;
 }
 
 static void
 GetRenumberedThreadIds(ProcessSP process_sp, ValueObjectSP data,
                        std::map<uint64_t, user_id_t> &thread_id_map) {
   ConvertToStructuredArray(
       data, ".threads", ".thread_count",
-      [process_sp, &thread_id_map](ValueObjectSP o,
-                                   StructuredData::Dictionary *dict) {
+      [process_sp, &thread_id_map](const ValueObjectSP &o,
+                                   const StructuredData::DictionarySP &dict) {
         uint64_t thread_id =
             o->GetValueForExpressionPath(".tid")->GetValueAsUnsigned(0);
         uint64_t thread_os_id =
             o->GetValueForExpressionPath(".os_id")->GetValueAsUnsigned(0);
         user_id_t lldb_user_id = 0;
 
         bool can_update = true;
         ThreadSP lldb_thread = process_sp->GetThreadList().FindThreadByID(
             thread_os_id, can_update);
         if (lldb_thread) {
           lldb_user_id = lldb_thread->GetIndexID();
         } else {
           // This isn't a live thread anymore.  Ask process to assign a new
           // Index ID (or return an old one if we've already seen this
           // thread_os_id). It will also make sure that no new threads are
           // assigned this Index ID.
           lldb_user_id = process_sp->AssignIndexIDToThread(thread_os_id);
         }
 
         thread_id_map[thread_id] = lldb_user_id;
       });
 }
 
 static user_id_t Renumber(uint64_t id,
                           std::map<uint64_t, user_id_t> &thread_id_map) {
   auto IT = thread_id_map.find(id);
   if (IT == thread_id_map.end())
     return 0;
if (result != eExpressionCompleted) {
     Debugger::ReportWarning(ss.GetString().str(),
                             process_sp->GetTarget().GetDebugger().GetID());
     return StructuredData::ObjectSP();
   }
 
   std::map<uint64_t, user_id_t> thread_id_map;
   GetRenumberedThreadIds(process_sp, main_value, thread_id_map);
 
-  StructuredData::Dictionary *dict = new StructuredData::Dictionary();
+  auto dict = std::make_shared<StructuredData::Dictionary>();
   dict->AddStringItem("instrumentation_class", "ThreadSanitizer");
   dict->AddStringItem("issue_type",
                       RetrieveString(main_value, process_sp, ".description"));
   dict->AddIntegerItem("report_count",
                        main_value->GetValueForExpressionPath(".report_count")
                            ->GetValueAsUnsigned(0));
-  dict->AddItem("sleep_trace", StructuredData::ObjectSP(CreateStackTrace(
-                                   main_value, ".sleep_trace")));
+  dict->AddItem("sleep_trace", CreateStackTrace(
+                                   main_value, ".sleep_trace"));
 
-  StructuredData::Array *stacks = ConvertToStructuredArray(
+  StructuredData::ArraySP stacks = ConvertToStructuredArray(
       main_value, ".stacks", ".stack_count",
-      [thread_sp](ValueObjectSP o, StructuredData::Dictionary *dict) {
+      [thread_sp](const ValueObjectSP &o,
+                  const StructuredData::DictionarySP &dict) {
         dict->AddIntegerItem(
             "index",
             o->GetValueForExpressionPath(".idx")->GetValueAsUnsigned(0));
-        dict->AddItem("trace", StructuredData::ObjectSP(CreateStackTrace(o)));
+        dict->AddItem("trace", CreateStackTrace(o));
         // "stacks" happen on the current thread
         dict->AddIntegerItem("thread_id", thread_sp->GetIndexID());
       });
-  dict->AddItem("stacks", StructuredData::ObjectSP(stacks));
+  dict->AddItem("stacks", stacks);
 
-  StructuredData::Array *mops = ConvertToStructuredArray(
+  StructuredData::ArraySP mops = ConvertToStructuredArray(
       main_value, ".mops", ".mop_count",
-      [&thread_id_map](ValueObjectSP o, StructuredData::Dictionary *dict) {
+      [&thread_id_map](const ValueObjectSP &o,
+                       const StructuredData::DictionarySP &dict) {
         dict->AddIntegerItem(
             "index",
             o->GetValueForExpressionPath(".idx")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "thread_id",
             Renumber(
                 o->GetValueForExpressionPath(".tid")->GetValueAsUnsigned(0),
                 thread_id_map));
         dict->AddIntegerItem(
             "size",
             o->GetValueForExpressionPath(".size")->GetValueAsUnsigned(0));
         dict->AddBooleanItem(
             "is_write",
             o->GetValueForExpressionPath(".write")->GetValueAsUnsigned(0));
         dict->AddBooleanItem(
             "is_atomic",
             o->GetValueForExpressionPath(".atomic")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "address",
             o->GetValueForExpressionPath(".addr")->GetValueAsUnsigned(0));
-        dict->AddItem("trace", StructuredData::ObjectSP(CreateStackTrace(o)));
+        dict->AddItem("trace", CreateStackTrace(o));
       });
-  dict->AddItem("mops", StructuredData::ObjectSP(mops));
+  dict->AddItem("mops", mops);
 
-  StructuredData::Array *locs = ConvertToStructuredArray(
+  StructuredData::ArraySP locs = ConvertToStructuredArray(
       main_value, ".locs", ".loc_count",
-      [process_sp, &thread_id_map](ValueObjectSP o,
-                                   StructuredData::Dictionary *dict) {
+      [process_sp, &thread_id_map](const ValueObjectSP &o,
+                                   const StructuredData::DictionarySP &dict) {
         dict->AddIntegerItem(
             "index",
             o->GetValueForExpressionPath(".idx")->GetValueAsUnsigned(0));
         dict->AddStringItem("type", RetrieveString(o, process_sp, ".type"));
         dict->AddIntegerItem(
             "address",
             o->GetValueForExpressionPath(".addr")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "start",
             o->GetValueForExpressionPath(".start")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "size",
             o->GetValueForExpressionPath(".size")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "thread_id",
             Renumber(
                 o->GetValueForExpressionPath(".tid")->GetValueAsUnsigned(0),
                 thread_id_map));
         dict->AddIntegerItem(
             "file_descriptor",
             o->GetValueForExpressionPath(".fd")->GetValueAsUnsigned(0));
         dict->AddIntegerItem("suppressable",
                              o->GetValueForExpressionPath(".suppressable")
                                  ->GetValueAsUnsigned(0));
-        dict->AddItem("trace", StructuredData::ObjectSP(CreateStackTrace(o)));
+        dict->AddItem("trace", CreateStackTrace(o));
         dict->AddStringItem("object_type",
                             RetrieveString(o, process_sp, ".object_type"));
       });
-  dict->AddItem("locs", StructuredData::ObjectSP(locs));
+  dict->AddItem("locs", locs);
 
-  StructuredData::Array *mutexes = ConvertToStructuredArray(
+  StructuredData::ArraySP mutexes = ConvertToStructuredArray(
       main_value, ".mutexes", ".mutex_count",
-      [](ValueObjectSP o, StructuredData::Dictionary *dict) {
+      [](const ValueObjectSP &o, const StructuredData::DictionarySP &dict) {
         dict->AddIntegerItem(
             "index",
             o->GetValueForExpressionPath(".idx")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "mutex_id",
             o->GetValueForExpressionPath(".mutex_id")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "address",
             o->GetValueForExpressionPath(".addr")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "destroyed",
             o->GetValueForExpressionPath(".destroyed")->GetValueAsUnsigned(0));
-        dict->AddItem("trace", StructuredData::ObjectSP(CreateStackTrace(o)));
+        dict->AddItem("trace", CreateStackTrace(o));
       });
-  dict->AddItem("mutexes", StructuredData::ObjectSP(mutexes));
+  dict->AddItem("mutexes", mutexes);
 
-  StructuredData::Array *threads = ConvertToStructuredArray(
+  StructuredData::ArraySP threads = ConvertToStructuredArray(
       main_value, ".threads", ".thread_count",
-      [process_sp, &thread_id_map](ValueObjectSP o,
-                                   StructuredData::Dictionary *dict) {
+      [process_sp, &thread_id_map](const ValueObjectSP &o,
+                                   const StructuredData::DictionarySP &dict) {
         dict->AddIntegerItem(
             "index",
             o->GetValueForExpressionPath(".idx")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "thread_id",
             Renumber(
                 o->GetValueForExpressionPath(".tid")->GetValueAsUnsigned(0),
                 thread_id_map));
         dict->AddIntegerItem(
             "thread_os_id",
             o->GetValueForExpressionPath(".os_id")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "running",
             o->GetValueForExpressionPath(".running")->GetValueAsUnsigned(0));
         dict->AddStringItem("name", RetrieveString(o, process_sp, ".name"));
         dict->AddIntegerItem(
             "parent_thread_id",
             Renumber(o->GetValueForExpressionPath(".parent_tid")
                          ->GetValueAsUnsigned(0),
                      thread_id_map));
-        dict->AddItem("trace", StructuredData::ObjectSP(CreateStackTrace(o)));
+        dict->AddItem("trace", CreateStackTrace(o));
       });
-  dict->AddItem("threads", StructuredData::ObjectSP(threads));
+  dict->AddItem("threads", threads);
 
-  StructuredData::Array *unique_tids = ConvertToStructuredArray(
+  StructuredData::ArraySP unique_tids = ConvertToStructuredArray(
       main_value, ".unique_tids", ".unique_tid_count",
-      [&thread_id_map](ValueObjectSP o, StructuredData::Dictionary *dict) {
+      [&thread_id_map](const ValueObjectSP &o,
+                       const StructuredData::DictionarySP &dict) {
         dict->AddIntegerItem(
             "index",
             o->GetValueForExpressionPath(".idx")->GetValueAsUnsigned(0));
         dict->AddIntegerItem(
             "tid",
             Renumber(
                 o->GetValueForExpressionPath(".tid")->GetValueAsUnsigned(0),
                 thread_id_map));
       });
-  dict->AddItem("unique_tids", StructuredData::ObjectSP(unique_tids));
+  dict->AddItem("unique_tids", unique_tids);
 
-  return StructuredData::ObjectSP(dict);
+  return dict;
 }
 
 std::string
 InstrumentationRuntimeTSan::FormatDescription(StructuredData::ObjectSP report) {
   std::string description = std::string(report->GetAsDictionary()
                                             ->GetValueForKey("issue_type")
                                             ->GetAsString()
                                             ->GetValue());
info->GetObjectForDotSeparatedPath(path)->GetAsArray()->ForEach(
 
         if (pcs.size() == 0)
           return true;
 
         StructuredData::ObjectSP thread_id_obj =
             o->GetObjectForDotSeparatedPath("thread_os_id");
         tid_t tid = thread_id_obj ? thread_id_obj->GetIntegerValue() : 0;
 
-        HistoryThread *history_thread =
-            new HistoryThread(*process_sp, tid, pcs);
-        ThreadSP new_thread_sp(history_thread);
+        ThreadSP new_thread_sp =
+            std::make_shared<HistoryThread>(*process_sp, tid, pcs);
         new_thread_sp->SetName(GenerateThreadName(path, o, info).c_str());
 
         // Save this in the Process' ExtendedThreadList so a strong pointer
         // retains the object
         process_sp->GetExtendedThreadList().AddThread(new_thread_sp);
         threads->AddThread(new_thread_sp);
 
         return true;
       });
 }
 
 lldb::ThreadCollectionSP
 InstrumentationRuntimeTSan::GetBacktracesFromExtendedStopInfo(
     StructuredData::ObjectSP info) {
-  ThreadCollectionSP threads;
-  threads = std::make_shared<ThreadCollection>();
+
+  ThreadCollectionSP threads = std::make_shared<ThreadCollection>();
 
   if (info->GetObjectForDotSeparatedPath("instrumentation_class")
           ->GetStringValue() != "ThreadSanitizer")
     return threads;
 
   ProcessSP process_sp = GetProcessSP();
 
   AddThreadsForPath("stacks", threads, process_sp, info);
   AddThreadsForPath("mops", threads, process_sp, info);
   AddThreadsForPath("locs", threads, process_sp, info);
   AddThreadsForPath("mutexes", threads, process_sp, info);
   AddThreadsForPath("threads", threads, process_sp, info);
 
   return threads;
 }