[Lldb-commits] [PATCH] D136650: Make CompilerType safe [Was: Add a check for TypeSystem use-after-free problems]

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

Some more comments inline, but I think this is essentially as good as we can make it.



================
Comment at: lldb/include/lldb/Symbol/Type.h:300
 
-  TypeSystem *GetTypeSystem(bool prefer_dynamic);
+  CompilerType::TypeSystemSPWrapper GetTypeSystem(bool prefer_dynamic);
 
----------------
Maybe the wrapper type should just be its own top-level entity? Or `TypeSystem::SPWrapper`?


================
Comment at: lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp:193-194
+    if (!m_type_system_sp) {
+      m_type_system = new TypeSystemClang("siginfo", triple);
+      m_type_system_sp.reset(m_type_system);
+    }
----------------
Remove `m_type_system`, change `m_type_system_sp` to `std::shared_ptr<TypeSystemClang>`


================
Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:316-321
+    std::lock_guard<std::mutex> guard(m_mutex);
+    if (!m_type_system_sp) {
+      m_type_system = new TypeSystemClang("siginfo", triple);
+      m_type_system_sp.reset(m_type_system);
+    }
+  }
----------------
Same here (but also remove the `_wp` variable).


================
Comment at: lldb/source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp:211
+    std::lock_guard<std::mutex> guard(m_mutex);
+    if (!m_type_system_sp) {
+      m_type_system = new TypeSystemClang("siginfo", triple);
----------------
and here


================
Comment at: lldb/source/Symbol/TypeSystem.cpp:278-279
   m_map[language] = type_system_sp;
-  if (type_system_sp.get())
-    return *type_system_sp.get();
+  if (type_system_sp) {
+    assert(!type_system_sp->weak_from_this().expired());
+    return type_system_sp;
----------------
With the current implementation, I don't think these can ever fire.


================
Comment at: lldb/source/Target/Target.cpp:2461
+            Language::GetNameForLanguageType(language) +
+            llvm::StringRef("is no longer live"),
+        llvm::inconvertibleErrorCode());
----------------
Maybe this error needs some updating? (also, missing spaces -- the language will get glued to the error msg)


================
Comment at: lldb/unittests/TestingSupport/Symbol/ClangTestUtils.h:48
+/// Simulates a Clang type system owned by a TypeSystemMap.
+class TypeSystemClangHolder {
+  TypeSystemClang *m_ast;
----------------
Can this be replaced by a `shared_ptr<TypeSystemClang>` now?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136650/new/

https://reviews.llvm.org/D136650