fix: Exclude circular dependant types when some subtype is not supported

mbektchiev · mbektchiev · commit 45ebd3b71513 · 2019-08-29T11:16:31.000+03:00
* Reset cached metadata when storing it in the meta container in order to revalidate it
* Store the complete exception instead of only the message (`isError` was being lost)
* Throw cached exceptions polymorphically (the throw statement uses the static type of the exception)
* fix unresolved bridge interface exception's description to not use the null pointer

`Tcl_HashTable` from the Catalyst SDK had this issue:

```
typedef struct Tcl_HashTable Tcl_HashTable;
...

struct Tcl_HashEntry {
...
    Tcl_HashTable *tablePtr;	/* Pointer to table containing entry. */
...
}
...

struct Tcl_HashTable {
...
    Tcl_HashEntry **buckets;
...
}
```

While defining `Tcl_HashEntry` we need to define `Tcl_HashTable`.
While defining it we reach back to `Tcl_HashEntry` and use it's
partially defined cached entry (which is currently considered valid
 as we still haven't reached the unsupported `union` field). This
results in caching and emitting the `Tcl_HashTable`'s metadata.
However, this is wrong since `Tcl_HashEntry` ultimately turns out
to be invalid and is missing from the metadata.
diff --git a/src/Binary/binaryTypeEncodingSerializer.cpp b/src/Binary/binaryTypeEncodingSerializer.cpp
@@ -169,7 +169,7 @@ unique_ptr<binary::TypeEncoding> binary::BinaryTypeEncodingSerializer::visitBrid
         return this->visitId(::Meta::IdType());
     }
     if (type.bridgedInterface == nullptr) {
-        throw logic_error(std::string("Unresolved bridged interface for BridgedInterfaceType with name '") + type.bridgedInterface->name + "'.");
+        throw logic_error(std::string("Unresolved bridged interface for BridgedInterfaceType with name '") + type.name + "'.");
     }
     auto s = new binary::InterfaceDeclarationReferenceEncoding();
     s->_name = this->_heapWriter.push_string(type.bridgedInterface->jsName);
diff --git a/src/Meta/CreationException.h b/src/Meta/CreationException.h
@@ -7,6 +7,19 @@
 #include <clang/AST/Type.h>
 #include <string>
 
+
+#define DEFINE_POLYMORPHIC_THROW \
+virtual void polymorhicThrow() override { \
+    throw *this; \
+}
+
+
+#define POLYMORPHIC_THROW(ex) do \
+{ \
+    ex->polymorhicThrow(); \
+    throw std::logic_error("polymorphicThrow should never return"); \
+} while(false)
+
 namespace Meta {
 class CreationException {
 public:
@@ -20,7 +33,11 @@ class CreationException {
         , _isError(isError)
     {
     }
+    
+    virtual ~CreationException() { }
 
+    virtual void polymorhicThrow() = 0;
+    
     std::string getMessage() const
     {
         return _message;
@@ -49,7 +66,7 @@ class MetaCreationException : public CreationException {
     {
     }
 
-    std::string getDetailedMessage() const
+    std::string getDetailedMessage() const override
     {
         return _meta->identificationString() + " : " + this->getMessage();
     }
@@ -59,6 +76,8 @@ class MetaCreationException : public CreationException {
         return _meta;
     }
 
+    DEFINE_POLYMORPHIC_THROW;
+
 private:
     const Meta* _meta;
 };
@@ -71,7 +90,7 @@ class TypeCreationException : public CreationException {
     {
     }
 
-    std::string getDetailedMessage() const
+    std::string getDetailedMessage() const override
     {
         return std::string("[Type ") + (_type == nullptr ? "" : _type->getTypeClassName()) + "] : " + this->getMessage();
     }
@@ -81,7 +100,9 @@ class TypeCreationException : public CreationException {
         return _type;
     }
 
+    DEFINE_POLYMORPHIC_THROW;
+
 private:
     const clang::Type* _type;
 };
-}
+}
diff --git a/src/Meta/DeclarationConverterVisitor.h b/src/Meta/DeclarationConverterVisitor.h
@@ -52,12 +52,35 @@ class DeclarationConverterVisitor : public clang::RecursiveASTVisitor<Declaratio
     bool Visit(T* decl)
     {
         try {
-            Meta* meta = this->_metaFactory.create(*decl);
+            // Remove from cache if present to have a chance to process any errors
+            // in dependant types which have been pending when it was cached the 1st time.
+            // If we have the following (inspired from Tcl_HashTable):
+            // struct HashTable;
+            //
+            // struct HashEntry {
+            //  HashTable*table;
+            //  union {
+            //  }
+            // }
+            //
+            // struct HashTable {
+            //  HashEntry **entries;
+            // }
+            // We do not support unions, so HashEntry is not included in the metadata.
+            // But before the cache purge we were leaving HashTable and it caused crashes
+            // if accessed at runtime.
+
+            Meta* meta = this->_metaFactory.create(*decl, /*resetCached*/ true);
             _metaContainer.push_back(meta);
             log(std::stringstream() << "verbose: Included " << meta->jsName << " from " << meta->module->getFullModuleName());
         } catch (MetaCreationException& e) {
-            if(e.isError()) {
+            if (e.isError()) {
                 log(std::stringstream() << "verbose: Exception " << e.getDetailedMessage());
+            } else {
+                  // Uncomment for maximum verbosity when debugging metadata generation issues
+//                auto namedDecl = clang::dyn_cast<clang::NamedDecl>(decl);
+//                auto name = namedDecl ? namedDecl->getNameAsString() : "<unknown>";
+//                log(std::stringstream() << "verbose: Skipping " << name << ": " << e.getMessage());
             }
         }
         return true;
diff --git a/src/Meta/MetaEntities.h b/src/Meta/MetaEntities.h
@@ -99,8 +99,6 @@ enum MetaType {
 };
 
 class Meta {
-    MAKE_NONCOPYABLE(Meta);
-
 public:
     MetaType type = MetaType::Undefined;
     MetaFlags flags = MetaFlags::None;
diff --git a/src/Meta/MetaFactory.cpp b/src/Meta/MetaFactory.cpp
@@ -24,7 +24,6 @@ static bool metasComparerByJsName(Meta* meta1, Meta* meta2)
     return compareJsNames(meta1->jsName, meta2->jsName);
 }
 
-    
 void MetaFactory::validate(Type* type)
 {
     ValidateMetaTypeVisitor validator(*this);
@@ -38,12 +37,12 @@ void MetaFactory::validate(Meta* meta)
     if (declIt == this->_metaToDecl.end()) {
         throw MetaCreationException(meta, "Metadata not created", true);
     }
-    
+
     auto metaIt = this->_cache.find(declIt->second);
     assert(metaIt != this->_cache.end());
-    if (!metaIt->second.second.empty()) {
+    if (metaIt->second.second.get() != nullptr) {
 //        printf("**** Validation failed for %s: %s ***\n\n", meta->name.c_str(), metaIt->second.second.c_str());
-        throw MetaCreationException(meta, metaIt->second.second, false);
+        POLYMORPHIC_THROW(metaIt->second.second);
     }
 }
     
@@ -68,85 +67,89 @@ string MetaFactory::getTypedefOrOwnName(const clang::TagDecl* tagDecl)
     return tagDecl->getNameAsString();
 }
 
-Meta* MetaFactory::create(const clang::Decl& decl)
+template<class T>
+void resetMetaAndAddToMap(std::unique_ptr<Meta>& metaPtrRef, MetaToDeclMap& metaToDecl, const clang::Decl& decl) {
+    if (metaPtrRef.get()) {
+        // The pointer has been previously allocated. Reset it's value and assert that it's already present in the map
+        static_cast<T&>(*metaPtrRef) = T();
+        assert(metaToDecl[metaPtrRef.get()] == &decl);
+    } else {
+        // Allocate memory and add to map
+        metaPtrRef.reset(new T());
+        metaToDecl[metaPtrRef.get()] = &decl;
+    }
+}
+
+Meta* MetaFactory::create(const clang::Decl& decl, bool resetCached /* = false*/)
 {
-    // check for cached Meta
-    Cache::const_iterator cachedMetaIt = _cache.find(&decl);
-    if (cachedMetaIt != _cache.end()) {
+    // Check for cached Meta
+    Cache::iterator cachedMetaIt = _cache.find(&decl);
+    if (!resetCached && cachedMetaIt != _cache.end()) {
         Meta* meta = cachedMetaIt->second.first.get();
-        std::string errorMessage = cachedMetaIt->second.second;
-        if (errorMessage.empty()) {
-            /* TODO: The meta object is not guaranteed to be fully initialized. If the meta object is in the creation stack
-                 * it will appear in cache, but will not be fully initialized. This may cause some inconsistent results.
-                 * */
-            return meta;
+        if (auto creationException = cachedMetaIt->second.second.get()) {
+            POLYMORPHIC_THROW(creationException);
         }
-        throw MetaCreationException(meta, errorMessage, false);
-    }
 
-    std::pair<Cache::iterator, bool> insertionResult = _cache.insert(std::make_pair<Cache::key_type, Cache::mapped_type>(&decl, std::make_pair<std::unique_ptr<Meta>, std::string>(nullptr, std::string())));
+        /* TODO: The meta object is not guaranteed to be fully initialized. If the meta object is in the creation stack
+             * it will appear in cache, but will not be fully initialized. This may cause some inconsistent results.
+             * */
+        
+        return meta;
+    }
 
-    assert(insertionResult.second);
-    Cache::iterator insertionIt = insertionResult.first;
-    std::unique_ptr<Meta>& insertedMetaPtrRef = insertionIt->second.first;
-    std::string& insertedErrorMessageRef = insertionIt->second.second;
+    if (cachedMetaIt == _cache.end()) {
+        std::pair<Cache::iterator, bool> insertionResult = _cache.insert(std::make_pair(&decl, std::make_pair(nullptr, nullptr)));
+        assert(insertionResult.second);
+        cachedMetaIt = insertionResult.first;
+    }
+    std::unique_ptr<Meta>& insertedMetaPtrRef = cachedMetaIt->second.first;
+    std::unique_ptr<CreationException>& insertedException = cachedMetaIt->second.second;
 
     try {
         if (const clang::FunctionDecl* function = clang::dyn_cast<clang::FunctionDecl>(&decl)) {
-            insertedMetaPtrRef.reset(new FunctionMeta());
-            _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+            resetMetaAndAddToMap<FunctionMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
             populateIdentificationFields(*function, *insertedMetaPtrRef.get());
             createFromFunction(*function, insertedMetaPtrRef.get()->as<FunctionMeta>());
         } else if (const clang::RecordDecl* record = clang::dyn_cast<clang::RecordDecl>(&decl)) {
             if (record->isStruct()) {
-                insertedMetaPtrRef.reset(new StructMeta());
-                _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+                resetMetaAndAddToMap<StructMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
                 populateIdentificationFields(*record, *insertedMetaPtrRef.get());
                 createFromStruct(*record, insertedMetaPtrRef.get()->as<StructMeta>());
             } else {
-                insertedMetaPtrRef.reset(new UnionMeta());
-                _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+                resetMetaAndAddToMap<UnionMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
                 populateIdentificationFields(*record, *insertedMetaPtrRef.get());
                 throw MetaCreationException(insertedMetaPtrRef.get(), "The record is union.", false);
             }
         } else if (const clang::VarDecl* var = clang::dyn_cast<clang::VarDecl>(&decl)) {
-            insertedMetaPtrRef.reset(new VarMeta());
-            _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+            resetMetaAndAddToMap<VarMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
             populateIdentificationFields(*var, *insertedMetaPtrRef.get());
             createFromVar(*var, insertedMetaPtrRef.get()->as<VarMeta>());
         } else if (const clang::EnumDecl* enumDecl = clang::dyn_cast<clang::EnumDecl>(&decl)) {
-            insertedMetaPtrRef.reset(new EnumMeta());
-            _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+            resetMetaAndAddToMap<EnumMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
             populateIdentificationFields(*enumDecl, *insertedMetaPtrRef.get());
             createFromEnum(*enumDecl, insertedMetaPtrRef.get()->as<EnumMeta>());
         } else if (const clang::EnumConstantDecl* enumConstantDecl = clang::dyn_cast<clang::EnumConstantDecl>(&decl)) {
-            insertedMetaPtrRef.reset(new EnumConstantMeta());
-            _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+            resetMetaAndAddToMap<EnumConstantMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
             populateIdentificationFields(*enumConstantDecl, *insertedMetaPtrRef.get());
             createFromEnumConstant(*enumConstantDecl, insertedMetaPtrRef.get()->as<EnumConstantMeta>());
         } else if (const clang::ObjCInterfaceDecl* interface = clang::dyn_cast<clang::ObjCInterfaceDecl>(&decl)) {
-            insertedMetaPtrRef.reset(new InterfaceMeta());
-            _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+            resetMetaAndAddToMap<InterfaceMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
             populateIdentificationFields(*interface, *insertedMetaPtrRef.get());
             createFromInterface(*interface, insertedMetaPtrRef.get()->as<InterfaceMeta>());
         } else if (const clang::ObjCProtocolDecl* protocol = clang::dyn_cast<clang::ObjCProtocolDecl>(&decl)) {
-            insertedMetaPtrRef.reset(new ProtocolMeta());
-            _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+            resetMetaAndAddToMap<ProtocolMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
             populateIdentificationFields(*protocol, *insertedMetaPtrRef.get());
             createFromProtocol(*protocol, insertedMetaPtrRef.get()->as<ProtocolMeta>());
         } else if (const clang::ObjCCategoryDecl* category = clang::dyn_cast<clang::ObjCCategoryDecl>(&decl)) {
-            insertedMetaPtrRef.reset(new CategoryMeta());
-            _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+            resetMetaAndAddToMap<CategoryMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
             populateIdentificationFields(*category, *insertedMetaPtrRef.get());
             createFromCategory(*category, insertedMetaPtrRef.get()->as<CategoryMeta>());
         } else if (const clang::ObjCMethodDecl* method = clang::dyn_cast<clang::ObjCMethodDecl>(&decl)) {
-            insertedMetaPtrRef.reset(new MethodMeta());
-            _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+            resetMetaAndAddToMap<MethodMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
             populateIdentificationFields(*method, *insertedMetaPtrRef.get());
             createFromMethod(*method, insertedMetaPtrRef.get()->as<MethodMeta>());
         } else if (const clang::ObjCPropertyDecl* property = clang::dyn_cast<clang::ObjCPropertyDecl>(&decl)) {
-            insertedMetaPtrRef.reset(new PropertyMeta());
-            _metaToDecl[insertedMetaPtrRef.get()] = &decl;
+            resetMetaAndAddToMap<PropertyMeta>(insertedMetaPtrRef, this->_metaToDecl, decl);
             populateIdentificationFields(*property, *insertedMetaPtrRef.get());
             createFromProperty(*property, insertedMetaPtrRef.get()->as<PropertyMeta>());
         } else {
@@ -156,16 +159,16 @@ Meta* MetaFactory::create(const clang::Decl& decl)
         return insertedMetaPtrRef.get();
     } catch (MetaCreationException& e) {
         if (e.getMeta() == insertedMetaPtrRef.get()) {
-            insertedErrorMessageRef = e.getMessage();
+            insertedException = llvm::make_unique<MetaCreationException>(e);
             throw;
         }
         std::string message = CreationException::constructMessage("Can't create meta dependency.", e.getDetailedMessage());
-        insertedErrorMessageRef = message;
-        throw MetaCreationException(insertedMetaPtrRef.get(), message, e.isError());
+        insertedException = llvm::make_unique<MetaCreationException>(insertedMetaPtrRef.get(), message, e.isError());
+        POLYMORPHIC_THROW(insertedException);
     } catch (TypeCreationException& e) {
         std::string message = CreationException::constructMessage("Can't create type dependency.", e.getDetailedMessage());
-        insertedErrorMessageRef = message;
-        throw MetaCreationException(insertedMetaPtrRef.get(), message, e.isError());
+        insertedException = llvm::make_unique<MetaCreationException>(insertedMetaPtrRef.get(), message, e.isError());
+        POLYMORPHIC_THROW(insertedException);
     }
 }
 
diff --git a/src/Meta/MetaFactory.h b/src/Meta/MetaFactory.h
@@ -1,5 +1,6 @@
 #pragma once
 
+#include "CreationException.h"
 #include "MetaEntities.h"
 #include "TypeFactory.h"
 #include "Utils/Noncopyable.h"
@@ -10,7 +11,7 @@
 
 namespace Meta {
 
-typedef std::unordered_map<const clang::Decl*, std::pair<std::unique_ptr<Meta>, std::string> > Cache;
+typedef std::unordered_map<const clang::Decl*, std::pair<std::unique_ptr<Meta>, std::unique_ptr<CreationException>> > Cache;
 typedef std::unordered_map<const Meta*, const clang::Decl*> MetaToDeclMap;
 
 class MetaFactory {
@@ -22,7 +23,7 @@ class MetaFactory {
     {
     }
 
-    Meta* create(const clang::Decl& decl);
+    Meta* create(const clang::Decl& decl, bool resetCached = false);
 
     bool tryCreate(const clang::Decl& decl, Meta** meta);
 
diff --git a/src/Meta/TypeFactory.cpp b/src/Meta/TypeFactory.cpp
diff --git a/src/Meta/TypeFactory.h b/src/Meta/TypeFactory.h

Original file line number	Diff line number	Diff line change
`@@ -169,7 +169,7 @@ unique_ptr<binary::TypeEncoding> binary::BinaryTypeEncodingSerializer::visitBrid`
`169`	`169`	`return this->visitId(::Meta::IdType());`
`170`	`170`	`}`
`171`	`171`	`if (type.bridgedInterface == nullptr) {`
`172`		`- throw logic_error(std::string("Unresolved bridged interface for BridgedInterfaceType with name '") + type.bridgedInterface->name + "'.");`
	`172`	`+ throw logic_error(std::string("Unresolved bridged interface for BridgedInterfaceType with name '") + type.name + "'.");`
`173`	`173`	`}`
`174`	`174`	`auto s = new binary::InterfaceDeclarationReferenceEncoding();`
`175`	`175`	`s->_name = this->_heapWriter.push_string(type.bridgedInterface->jsName);`
Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,19 @@`
`7`	`7`	`#include <clang/AST/Type.h>`
`8`	`8`	`#include <string>`
`9`	`9`
	`10`	`+`
	`11`	`+#define DEFINE_POLYMORPHIC_THROW \`
	`12`	`+virtual void polymorhicThrow() override { \`
	`13`	`+ throw *this; \`
	`14`	`+}`
	`15`	`+`
	`16`	`+`
	`17`	`+#define POLYMORPHIC_THROW(ex) do \`
	`18`	`+{ \`
	`19`	`+ ex->polymorhicThrow(); \`
	`20`	`+ throw std::logic_error("polymorphicThrow should never return"); \`
	`21`	`+} while(false)`
	`22`	`+`
`10`	`23`	`namespace Meta {`
`11`	`24`	`class CreationException {`
`12`	`25`	`public:`
`@@ -20,7 +33,11 @@ class CreationException {`
`20`	`33`	`, _isError(isError)`
`21`	`34`	`{`
`22`	`35`	`}`
	`36`	`+`
	`37`	`+ virtual ~CreationException() { }`
`23`	`38`
	`39`	`+ virtual void polymorhicThrow() = 0;`
	`40`	`+`
`24`	`41`	`std::string getMessage() const`
`25`	`42`	`{`
`26`	`43`	`return _message;`
`@@ -49,7 +66,7 @@ class MetaCreationException : public CreationException {`
`49`	`66`	`{`
`50`	`67`	`}`
`51`	`68`
`52`		`- std::string getDetailedMessage() const`
	`69`	`+ std::string getDetailedMessage() const override`
`53`	`70`	`{`
`54`	`71`	`return _meta->identificationString() + " : " + this->getMessage();`
`55`	`72`	`}`
`@@ -59,6 +76,8 @@ class MetaCreationException : public CreationException {`
`59`	`76`	`return _meta;`
`60`	`77`	`}`
`61`	`78`
	`79`	`+ DEFINE_POLYMORPHIC_THROW;`
	`80`	`+`
`62`	`81`	`private:`
`63`	`82`	`const Meta* _meta;`
`64`	`83`	`};`
`@@ -71,7 +90,7 @@ class TypeCreationException : public CreationException {`
`71`	`90`	`{`
`72`	`91`	`}`
`73`	`92`
`74`		`- std::string getDetailedMessage() const`
	`93`	`+ std::string getDetailedMessage() const override`
`75`	`94`	`{`
`76`	`95`	`return std::string("[Type ") + (_type == nullptr ? "" : _type->getTypeClassName()) + "] : " + this->getMessage();`
`77`	`96`	`}`
`@@ -81,7 +100,9 @@ class TypeCreationException : public CreationException {`
`81`	`100`	`return _type;`
`82`	`101`	`}`
`83`	`102`
	`103`	`+ DEFINE_POLYMORPHIC_THROW;`
	`104`	`+`
`84`	`105`	`private:`
`85`	`106`	`const clang::Type* _type;`
`86`	`107`	`};`
`87`		`-}`
	`108`	`+}`