[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 added inline comments.


================
Comment at: lldb/include/lldb/Symbol/CompilerType.h:58
+    template <class TypeSystemType> TypeSystemType *dyn_cast_or_null() {
+      return llvm::dyn_cast_or_null<TypeSystemType>(m_typesystem_sp.get());
+    }
----------------
Maybe do something like this instead:
```
template <class TypeSystemType> std::shared_ptr<TypeSystemType> dyn_cast_or_null() {
  if (llvm::isa<TypeSystemType>(m_typesystem_sp.get()))
    return std::shared_ptr<TypeSystemType>(m_typesystem_sp, llvm::cast<TypeSystemType>(m_typesystem_sp.get()));
  return nullptr;
}
```

That said, `llvm::dyn_cast` supports custom casts for smart pointers (it already has them for std::unique_ptr), so it should be possible to completely avoid this wrapper class by implementing this logic inside llvm.
Although, then we'd have to answer awkward questions like "why are you using a shared_ptr in the first place".


================
Comment at: lldb/include/lldb/Symbol/CompilerType.h:60
+    }
+    operator bool() const { return static_cast<bool>(m_typesystem_sp); }
+    bool operator==(const TypeSystemSPWrapper &other) const;
----------------
add `explicit`


================
Comment at: lldb/include/lldb/Symbol/TypeSystem.h:517-520
+  /// Only to be used by TypeSystemMap and various unit tests.
+  void SetCanonicalWeakPtr(lldb::TypeSystemWP wp) { m_canonical_wp = wp; }
 protected:
+  lldb::TypeSystemWP m_canonical_wp;
----------------
Can this be replaced by inheriting from `std::enable_shared_from_this`?


================
Comment at: lldb/source/Core/DumpDataExtractor.cpp:322
                                                    size_t byte_size) {
-  if (target_sp) {
-    if (auto type_system_or_err =
-            target_sp->GetScratchTypeSystemForLanguage(eLanguageTypeC))
-      return type_system_or_err->GetFloatTypeSemantics(byte_size);
-    else
+  while (target_sp) {
+    auto type_system_or_err =
----------------
why?


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

https://reviews.llvm.org/D136650