Add a verification mechanism to CompilerType.

[aprantl]

Summary

Badly-written code can combine an unrelated TypeSystem and opaque type
pointer into a CompilerType. This is particularly an issue in
swift-lldb. This patch adds an assertion mechanism that catches these
kinds of mistakes early. Because this is an assertion-only code path
there is not cost for release builds.

[jingham]

Seems fine to me.

[aprantl]

ea960371861acad11b7018a5e280ae7a41ab9c02

[teemperor]

I think this change is slightly orthogonal to the way we are supposed to construct types in LLDB. After the quirky bugs we had with Decl * and DeclContext * pointers in the CompilerDeclContext, I moved all the Compiler*(TypeSystem *, opaque_ptr*) constructors calls into wrapper functions in TypeSystemClang where we could use the real types. This avoids all the pitfalls from having to pass void pointers to the constructor which breaks when multiple inheritance and implicit type conversion is involved. I also added a remark to the documentation of all these constructors to not use them directly but use the wrapper functions in the respective TypeSystem.

Because of this, this change is mostly a no-op upstream as TypeSystemClang already does this check for CompilerTypes (and also CompilerDeclContexts) in these wrapper functions. Now we essentially check every CompilerType twice (once in the wrapper function which only exists for Clang now and which is typesafe. And now also in the non-typesafe CompilerType constructor which is called by the wrapper function). My plan was to actually completely hide this constructor and only make it accessible to TypeSystem subclasses via some wrapper function, so in theory this check could safely happen in a less intrusive way in the TypeSystem* wrapper function. See for example TypeSystemClang::CreateDeclContext.

On another note, the verify function should in my opinion return a void and just assert directly. The main reason being that currently we only get an assertion that Verify() failed but this doesn't tell me what specific check has failed in the backend. I don't see any code that could check the bool result of Verify and do anything useful with it (beside asserting).

Also I'm very confused why a nullptr type with a valid type system is not triggering asserts? Do we actually have these types anywhere (and if yes, that would be kinda fishy IMHO).

Beside those points I think this change is an improvement over the current situation so I'm fine with keeping it. But in general I would much rather see Swift use a type-safe wrapper function instead and only keep this patch around until this has happened everywhere.

(Sorry for the late review, still working through my email backlog)

[aprantl]

Thanks for sharing your thoughts! Moving to a type-safe subclass does make sense, I don't think that calling the verifier there is the best choice. For example, if I'm calling CompilerType::GetPointerType(CompilerType pointee) I'd want the resulting type verified, too. Then again, if the implementation of that function calls the specialized constructor, that might be good enough.

As for null types; In swift-lldb they are constructed quite a lot, mostly as default and "error" return values from type system operations. There is probably no need for more than one kind of null type though.

lldb/include/lldb/Symbol/CompilerType.h

 public:
   /// Creates a CompilerType with the given TypeSystem and opaque compiler type.
   ///
   /// This constructor should only be called from the respective TypeSystem
   /// implementation.
   ///
   /// \see lldb_private::TypeSystemClang::GetType(clang::QualType)
   CompilerType(TypeSystem *type_system, lldb::opaque_compiler_type_t type)
-      : m_type(type), m_type_system(type_system) {}
+      : m_type(type), m_type_system(type_system) {
+    assert(Verify() && "verification failed");
+  }
 
   CompilerType(const CompilerType &rhs)
       : m_type(rhs.m_type), m_type_system(rhs.m_type_system) {}
 
   CompilerType() = default;
 
   /// Operators.
   /// \{
#endif
   bool GetValueAsScalar(const DataExtractor &data, lldb::offset_t data_offset,
                         size_t data_byte_size, Scalar &value) const;
   void Clear() {
     m_type = nullptr;
     m_type_system = nullptr;
   }
 
 private:
+#ifndef NDEBUG
+  /// If the type is valid, ask the TypeSystem to verify the integrity
+  /// of the type to catch CompilerTypes that mix and match invalid
+  /// TypeSystem/Opaque type pairs.
+  bool Verify() const;
+#endif
+
   lldb::opaque_compiler_type_t m_type = nullptr;
   TypeSystem *m_type_system = nullptr;
 };
 
 bool operator==(const CompilerType &lhs, const CompilerType &rhs);
 bool operator!=(const CompilerType &lhs, const CompilerType &rhs);
 
 struct CompilerType::IntegralTemplateArgument {
   llvm::APSInt value;
   CompilerType type;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SYMBOL_COMPILERTYPE_H

lldb/include/lldb/Symbol/TypeSystem.h

public:
   virtual bool DeclContextIsClassMethod(
       void *opaque_decl_ctx, lldb::LanguageType *language_ptr,
       bool *is_instance_method_ptr, ConstString *language_object_name_ptr) = 0;
 
   virtual bool DeclContextIsContainedInLookup(void *opaque_decl_ctx,
                                               void *other_opaque_decl_ctx) = 0;
 
   // Tests
+#ifndef NDEBUG
+  /// Verify the integrity of the type to catch CompilerTypes that mix
+  /// and match invalid TypeSystem/Opaque type pairs.
+  virtual bool Verify(lldb::opaque_compiler_type_t type) = 0;
+#endif
 
   virtual bool IsArrayType(lldb::opaque_compiler_type_t type,
                            CompilerType *element_type, uint64_t *size,
                            bool *is_incomplete) = 0;
 
   virtual bool IsAggregateType(lldb::opaque_compiler_type_t type) = 0;
 
   virtual bool IsAnonymousType(lldb::opaque_compiler_type_t type);

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

public:
   static ClangASTMetadata *DeclContextGetMetaData(const CompilerDeclContext &dc,
                                                   const clang::Decl *object);
 
   static clang::ASTContext *
   DeclContextGetTypeSystemClang(const CompilerDeclContext &dc);
 
   // Tests
 
+#ifndef NDEBUG
+  bool Verify(lldb::opaque_compiler_type_t type) override;
+#endif
+  
   bool IsArrayType(lldb::opaque_compiler_type_t type,
                    CompilerType *element_type, uint64_t *size,
                    bool *is_incomplete) override;
 
   bool IsVectorType(lldb::opaque_compiler_type_t type,
                     CompilerType *element_type, uint64_t *size) override;
 
   bool IsAggregateType(lldb::opaque_compiler_type_t type) override;

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

ConvertAccessTypeToObjCIvarAccessControl(AccessType access) {
   case eAccessPackage:
     return clang::ObjCIvarDecl::Package;
   }
   return clang::ObjCIvarDecl::None;
 }
 
 // Tests
 
+#ifndef NDEBUG
+bool TypeSystemClang::Verify(lldb::opaque_compiler_type_t type) {
+  return !type || llvm::isa<clang::Type>(GetQualType(type).getTypePtr());
+}
+#endif
+
 bool TypeSystemClang::IsAggregateType(lldb::opaque_compiler_type_t type) {
   clang::QualType qual_type(RemoveWrappingTypes(GetCanonicalQualType(type)));
 
   const clang::Type::TypeClass type_class = qual_type->getTypeClass();
   switch (type_class) {
   case clang::Type::IncompleteArray:
   case clang::Type::VariableArray:
   case clang::Type::ConstantArray:

lldb/source/Symbol/CompilerType.cpp

case lldb::eEncodingIEEE754:
         }
       }
       break;
     }
   }
   return false;
 }
 
+#ifndef NDEBUG
+bool CompilerType::Verify() const {
+  return !IsValid() || m_type_system->Verify(m_type);
+}
+#endif
+
 bool lldb_private::operator==(const lldb_private::CompilerType &lhs,
                               const lldb_private::CompilerType &rhs) {
   return lhs.GetTypeSystem() == rhs.GetTypeSystem() &&
          lhs.GetOpaqueQualType() == rhs.GetOpaqueQualType();
 }
 
 bool lldb_private::operator!=(const lldb_private::CompilerType &lhs,
                               const lldb_private::CompilerType &rhs) {
   return !(lhs == rhs);
 }

lldb/source/Symbol/TypeSystem.cpp

lldb::TypeSystemSP TypeSystem::CreateInstance(lldb::LanguageType language,
   return CreateInstanceHelper(language, module, nullptr);
 }
 
 lldb::TypeSystemSP TypeSystem::CreateInstance(lldb::LanguageType language,
                                               Target *target) {
   return CreateInstanceHelper(language, nullptr, target);
 }
 
+#ifndef NDEBUG
+bool TypeSystem::Verify(lldb::opaque_compiler_type_t type) { return true; }
+#endif
+
 bool TypeSystem::IsAnonymousType(lldb::opaque_compiler_type_t type) {
   return false;
 }
 
 CompilerType TypeSystem::GetArrayType(lldb::opaque_compiler_type_t type,
                                       uint64_t size) {
   return CompilerType();
 }