From 7b447bdad29c828b49ba63cefacc677bd7c58b28 Mon Sep 17 00:00:00 2001 From: jsmall-nvidia Date: Fri, 9 Jul 2021 16:29:23 -0400 Subject: Make Scope non ref counted (#1904) * Add debug symbols for release build. * Hack to try and capture failing compilation. * Typo fix for capture hack. * Specify return type on lambdas. * Added const. * Try breakpoint. * Up count * Let's capture everything so we can valgrind. * Disable always writing repros. * Make Scope non RefCounted. * Fix issue with not serializing Scope. * More comments around changes to Scope. Remove Scope* from serialization. * Remove code used for testing original issue. --- source/slang/slang-ast-base.h | 17 +++++++++++++++ source/slang/slang-ast-decl.h | 14 +++++++------ source/slang/slang-ast-expr.h | 18 ++++++++++------ source/slang/slang-ast-modifier.h | 5 +++-- source/slang/slang-ast-support-types.h | 22 ++----------------- source/slang/slang-check-decl.cpp | 8 +++---- source/slang/slang-check-shader.cpp | 15 ++++++++----- source/slang/slang-compiler.h | 14 ++++++------- source/slang/slang-lookup.cpp | 3 ++- source/slang/slang-lookup.h | 2 +- source/slang/slang-parser.cpp | 34 ++++++++++++++++++------------ source/slang/slang-parser.h | 6 +++--- source/slang/slang-serialize-factory.cpp | 11 ++++++++-- source/slang/slang.cpp | 36 ++++++++++++++++++++------------ 14 files changed, 121 insertions(+), 84 deletions(-) (limited to 'source') diff --git a/source/slang/slang-ast-base.h b/source/slang/slang-ast-base.h index bb92c4738..c74edb938 100644 --- a/source/slang/slang-ast-base.h +++ b/source/slang/slang-ast-base.h @@ -61,7 +61,24 @@ SLANG_FORCE_INLINE const T* as(const NodeBase* node) return (node && ReflectClassInfo::isSubClassOf(uint32_t(node->astNodeType), T::kReflectClassInfo)) ? static_cast(node) : nullptr; } +struct Scope : public NodeBase +{ + SLANG_AST_CLASS(Scope) + + // The container to use for lookup + // + // Note(tfoley): This is kept as an unowned pointer + // so that a scope can't keep parts of the AST alive, + // but the opposite it allowed. + ContainerDecl* containerDecl = nullptr; + SLANG_UNREFLECTED + // The parent of this scope (where lookup should go if nothing is found locally) + Scope* parent = nullptr; + + // The next sibling of this scope (a peer for lookup) + Scope* nextSibling = nullptr; +}; // Base class for all nodes representing actual syntax // (thus having a location in the source code) diff --git a/source/slang/slang-ast-decl.h b/source/slang/slang-ast-decl.h index 5b8c6a2f1..5b596ded0 100644 --- a/source/slang/slang-ast-decl.h +++ b/source/slang/slang-ast-decl.h @@ -381,10 +381,11 @@ class UsingDecl : public Decl SLANG_AST_CLASS(UsingDecl) /// An expression that identifies the entity (e.g., a namespace) to be brought into `scope` - Expr* arg; + Expr* arg = nullptr; + SLANG_UNREFLECTED /// The scope that the entity named by `arg` will be brought into - RefPtr scope; + Scope* scope = nullptr; }; class ImportDecl : public Decl @@ -393,12 +394,13 @@ class ImportDecl : public Decl // The name of the module we are trying to import NameLoc moduleNameAndLoc; - - // The scope that we want to import into - RefPtr scope; - + // The module that actually got imported ModuleDecl* importedModuleDecl = nullptr; + + SLANG_UNREFLECTED + // The scope that we want to import into + Scope* scope = nullptr; }; // A generic declaration, parameterized on types/values diff --git a/source/slang/slang-ast-expr.h b/source/slang/slang-ast-expr.h index d79982b5d..81bed9264 100644 --- a/source/slang/slang-ast-expr.h +++ b/source/slang/slang-ast-expr.h @@ -13,14 +13,16 @@ class DeclRefExpr: public Expr { SLANG_ABSTRACT_AST_CLASS(DeclRefExpr) - // The scope in which to perform lookup - RefPtr scope; - + // The declaration of the symbol being referenced DeclRef declRef; // The name of the symbol being referenced - Name* name; + Name* name = nullptr; + + SLANG_UNREFLECTED + // The scope in which to perform lookup + Scope* scope = nullptr; }; class VarExpr : public DeclRefExpr @@ -287,7 +289,9 @@ class ParenExpr: public Expr class ThisExpr: public Expr { SLANG_AST_CLASS(ThisExpr) - RefPtr scope; + + SLANG_UNREFLECTED + Scope* scope = nullptr; }; // An expression that binds a temporary variable in a local expression context @@ -322,7 +326,9 @@ class TaggedUnionTypeExpr: public Expr class ThisTypeExpr: public Expr { SLANG_AST_CLASS(ThisTypeExpr) - RefPtr scope; + + SLANG_UNREFLECTED + Scope* scope = nullptr; }; /// A type expression of the form `Left & Right`. diff --git a/source/slang/slang-ast-modifier.h b/source/slang/slang-ast-modifier.h index 467a9e7e7..2929a53c2 100644 --- a/source/slang/slang-ast-modifier.h +++ b/source/slang/slang-ast-modifier.h @@ -555,8 +555,9 @@ class AttributeBase : public Modifier class UncheckedAttribute : public AttributeBase { SLANG_AST_CLASS(UncheckedAttribute) - - RefPtr scope; + + SLANG_UNREFLECTED + Scope* scope = nullptr; }; // A `[name(arg0, ...)]` style attribute that has been validated. diff --git a/source/slang/slang-ast-support-types.h b/source/slang/slang-ast-support-types.h index 884668353..e7c2287ff 100644 --- a/source/slang/slang-ast-support-types.h +++ b/source/slang/slang-ast-support-types.h @@ -1097,24 +1097,6 @@ namespace Slang static const TypeExp empty; }; - - - struct Scope : public RefObject - { - // The parent of this scope (where lookup should go if nothing is found locally) - RefPtr parent; - - // The next sibling of this scope (a peer for lookup) - RefPtr nextSibling; - - // The container to use for lookup - // - // Note(tfoley): This is kept as an unowned pointer - // so that a scope can't keep parts of the AST alive, - // but the opposite it allowed. - ContainerDecl* containerDecl = nullptr; - }; - // Masks to be applied when lookup up declarations enum class LookupMask : uint8_t { @@ -1365,8 +1347,8 @@ namespace Slang struct LookupRequest { SemanticsVisitor* semantics = nullptr; - RefPtr scope = nullptr; - RefPtr endScope = nullptr; + Scope* scope = nullptr; + Scope* endScope = nullptr; LookupMask mask = LookupMask::Default; LookupOptions options = LookupOptions::None; diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp index 0f6075903..dbbf7e973 100644 --- a/source/slang/slang-check-decl.cpp +++ b/source/slang/slang-check-decl.cpp @@ -4999,11 +4999,9 @@ namespace Slang importedModulesList.add(moduleDecl); importedModulesSet.Add(moduleDecl); - - // Create a new sub-scope to wire the module // into our lookup chain. - auto subScope = new Scope(); + auto subScope = getASTBuilder()->create(); subScope->containerDecl = moduleDecl; subScope->nextSibling = scope->nextSibling; @@ -5049,7 +5047,7 @@ namespace Slang // Add the declarations from the imported module into the scope // that the `import` declaration is set to extend. // - importModuleIntoScope(scope.Ptr(), importedModuleDecl); + importModuleIntoScope(scope, importedModuleDecl); // Record the `import`ed module (and everything it depends on) // as a dependency of the module we are compiling. @@ -5105,7 +5103,7 @@ namespace Slang // of entities than just namespaces. // auto scope = decl->scope; - auto subScope = new Scope(); + auto subScope = getASTBuilder()->create(); subScope->containerDecl = namespaceDecl; subScope->nextSibling = scope->nextSibling; scope->nextSibling = subScope; diff --git a/source/slang/slang-check-shader.cpp b/source/slang/slang-check-shader.cpp index b6ed0224a..025a47c1b 100644 --- a/source/slang/slang-check-shader.cpp +++ b/source/slang/slang-check-shader.cpp @@ -1297,7 +1297,7 @@ namespace Slang sink); } - RefPtr ComponentType::_createScopeForLegacyLookup() + Scope* ComponentType::_createScopeForLegacyLookup(ASTBuilder* astBuilder) { // The shape of this logic is dictated by the legacy // behavior for name-based lookup/parsing of types @@ -1308,7 +1308,8 @@ namespace Slang // definitions (that scope is necessary because // it defines keywords like `true` and `false`). // - RefPtr scope = new Scope(); + + Scope* scope = astBuilder->create(); scope->parent = getLinkage()->getSessionImpl()->slangLanguageScope; // // Next, the scope needs to include all of the @@ -1317,7 +1318,7 @@ namespace Slang // for( auto module : getModuleDependencies() ) { - RefPtr moduleScope = new Scope(); + Scope* moduleScope = astBuilder->create(); moduleScope->containerDecl = module->getModuleDecl(); moduleScope->nextSibling = scope->nextSibling; @@ -1339,8 +1340,12 @@ namespace Slang { auto unspecialiedProgram = endToEndReq->getUnspecializedGlobalComponentType(); - - RefPtr scope = unspecialiedProgram->_createScopeForLegacyLookup(); + // TODO(JS): + // + // We create the scopes on the linkages ASTBuilder. We might want to create a temporary ASTBuilder, + // and let that memory get freed, but is like this because it's not clear if the scopes in ASTNode members + // will dangle if we do. + Scope* scope = unspecialiedProgram->_createScopeForLegacyLookup(endToEndReq->getLinkage()->getASTBuilder()); // We are going to do some semantic checking, so we need to // set up a `SemanticsVistitor` that we can use. diff --git a/source/slang/slang-compiler.h b/source/slang/slang-compiler.h index a933716a8..52a9f935d 100755 --- a/source/slang/slang-compiler.h +++ b/source/slang/slang-compiler.h @@ -478,7 +478,7 @@ namespace Slang /// This facility is only needed to support legacy APIs for string-based lookup /// and parsing via Slang reflection, and is not recommended for future APIs to use. /// - RefPtr _createScopeForLegacyLookup(); + Scope* _createScopeForLegacyLookup(ASTBuilder* astBuilder); protected: ComponentType(Linkage* linkage); @@ -1425,7 +1425,7 @@ namespace Slang /// SlangResult loadFile(String const& path, PathInfo& outPathInfo, ISlangBlob** outBlob); - Expr* parseTermString(String str, RefPtr scope); + Expr* parseTermString(String str, Scope* scope); Type* specializeType( Type* unspecializedType, @@ -2259,10 +2259,10 @@ namespace Slang /// Get the default compiler for a language DownstreamCompiler* getDefaultDownstreamCompiler(SourceLanguage sourceLanguage); - RefPtr baseLanguageScope; - RefPtr coreLanguageScope; - RefPtr hlslLanguageScope; - RefPtr slangLanguageScope; + Scope* baseLanguageScope = nullptr; + Scope* coreLanguageScope = nullptr; + Scope* hlslLanguageScope = nullptr; + Scope* slangLanguageScope = nullptr; ModuleDecl* baseModuleDecl = nullptr; List> stdlibModules; @@ -2321,7 +2321,7 @@ namespace Slang void init(); void addBuiltinSource( - RefPtr const& scope, + Scope* scope, String const& path, String const& source); ~Session(); diff --git a/source/slang/slang-lookup.cpp b/source/slang/slang-lookup.cpp index f41d8ff3b..20670e7b3 100644 --- a/source/slang/slang-lookup.cpp +++ b/source/slang/slang-lookup.cpp @@ -755,6 +755,7 @@ static void _lookUpInScopes( auto thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Default; auto scope = request.scope; + auto endScope = request.endScope; for (;scope != endScope; scope = scope->parent) { @@ -918,7 +919,7 @@ LookupResult lookUp( ASTBuilder* astBuilder, SemanticsVisitor* semantics, Name* name, - RefPtr scope, + Scope* scope, LookupMask mask) { LookupRequest request; diff --git a/source/slang/slang-lookup.h b/source/slang/slang-lookup.h index f983bfefc..0f034d100 100644 --- a/source/slang/slang-lookup.h +++ b/source/slang/slang-lookup.h @@ -21,7 +21,7 @@ LookupResult lookUp( ASTBuilder* astBuilder, SemanticsVisitor* semantics, Name* name, - RefPtr scope, + Scope* scope, LookupMask mask = LookupMask::Default); // Perform member lookup in the context of a type diff --git a/source/slang/slang-parser.cpp b/source/slang/slang-parser.cpp index 6d2abef49..7bb933b39 100644 --- a/source/slang/slang-parser.cpp +++ b/source/slang/slang-parser.cpp @@ -91,8 +91,8 @@ namespace Slang int anonymousCounter = 0; - RefPtr outerScope; - RefPtr currentScope; + Scope* outerScope = nullptr; + Scope* currentScope = nullptr; TokenReader tokenReader; DiagnosticSink* sink; @@ -114,7 +114,14 @@ namespace Slang } void PushScope(ContainerDecl* containerDecl) { - RefPtr newScope = new Scope(); + // TODO(JS): + // + // Previously Scope was ref counted. This meant that if a scope was pushed, but not used when popped + // it's memory would be freed. + // + // Here the memory is consumed and will not be freed until the astBuilder goes out of scope. + + Scope* newScope = astBuilder->create(); newScope->containerDecl = containerDecl; newScope->parent = currentScope; @@ -135,7 +142,7 @@ namespace Slang ASTBuilder* inAstBuilder, TokenSpan const& _tokens, DiagnosticSink * sink, - RefPtr const& outerScope) + Scope* outerScope) : tokenReader(_tokens) , astBuilder(inAstBuilder) , sink(sink) @@ -1236,7 +1243,7 @@ namespace Slang } } - static void AddMember(RefPtr scope, Decl* member) + static void AddMember(Scope* scope, Decl* member) { if (scope) { @@ -1399,7 +1406,8 @@ namespace Slang class ReplaceScopeVisitor : public ExprVisitor { public: - RefPtr scope; + Scope* scope = nullptr; + void visitDeclRefExpr(DeclRefExpr* expr) { expr->scope = scope; @@ -1830,7 +1838,7 @@ namespace Slang // TODO: do this better, e.g. by filling in the `declRef` field directly auto expr = parser->astBuilder->create(); - expr->scope = parser->currentScope.Ptr(); + expr->scope = parser->currentScope; expr->loc = decl->getNameLoc(); expr->name = decl->getName(); return expr; @@ -2071,7 +2079,7 @@ namespace Slang Token typeName = parser->ReadToken(TokenType::Identifier); auto basicType = parser->astBuilder->create(); - basicType->scope = parser->currentScope.Ptr(); + basicType->scope = parser->currentScope; basicType->loc = typeName.loc; basicType->name = typeName.getNameOrNull(); @@ -2529,7 +2537,7 @@ namespace Slang auto bufferDataTypeExpr = parser->astBuilder->create(); bufferDataTypeExpr->loc = bufferDataTypeDecl->loc; bufferDataTypeExpr->name = bufferDataTypeDecl->nameAndLoc.name; - bufferDataTypeExpr->scope = parser->currentScope.Ptr(); + bufferDataTypeExpr->scope = parser->currentScope; // Construct a type expression to reference the type constructor auto bufferWrapperTypeExpr = parser->astBuilder->create(); @@ -5388,7 +5396,7 @@ namespace Slang MemberExpr* memberExpr = parser->astBuilder->create(); // TODO(tfoley): why would a member expression need this? - memberExpr->scope = parser->currentScope.Ptr(); + memberExpr->scope = parser->currentScope; parser->FillPosition(memberExpr); memberExpr->baseExpression = expr; @@ -5504,7 +5512,7 @@ namespace Slang ASTBuilder* astBuilder, TokenSpan const& tokens, DiagnosticSink* sink, - RefPtr const& outerScope, + Scope* outerScope, NamePool* namePool, SourceLanguage sourceLanguage) { @@ -5521,7 +5529,7 @@ namespace Slang TranslationUnitRequest* translationUnit, TokenSpan const& tokens, DiagnosticSink* sink, - RefPtr const& outerScope) + Scope* outerScope) { Parser parser(astBuilder, tokens, sink, outerScope); parser.namePool = translationUnit->getNamePool(); @@ -6077,7 +6085,7 @@ namespace Slang ModuleDecl* populateBaseLanguageModule( ASTBuilder* astBuilder, - RefPtr scope) + Scope* scope) { Session* session = astBuilder->getGlobalSession(); diff --git a/source/slang/slang-parser.h b/source/slang/slang-parser.h index 0ec2dcb8a..4f888f87c 100644 --- a/source/slang/slang-parser.h +++ b/source/slang/slang-parser.h @@ -14,19 +14,19 @@ namespace Slang TranslationUnitRequest* translationUnit, TokenSpan const& tokens, DiagnosticSink* sink, - RefPtr const& outerScope); + Scope* outerScope); Expr* parseTermFromSourceFile( ASTBuilder* astBuilder, TokenSpan const& tokens, DiagnosticSink* sink, - RefPtr const& outerScope, + Scope* outerScope, NamePool* namePool, SourceLanguage sourceLanguage); ModuleDecl* populateBaseLanguageModule( ASTBuilder* astBuilder, - RefPtr scope); + Scope* scope); /// Information used to set up SyntaxDecl. Such decls /// when correctly setup define a callback. For some of the callbacks it's necessary diff --git a/source/slang/slang-serialize-factory.cpp b/source/slang/slang-serialize-factory.cpp index 9b3ba9994..a6b7e0ba8 100644 --- a/source/slang/slang-serialize-factory.cpp +++ b/source/slang/slang-serialize-factory.cpp @@ -49,8 +49,8 @@ void* DefaultSerialObjectFactory::create(SerialTypeKind typeKind, SerialSubType SerialIndex ModuleSerialFilter::writePointer(SerialWriter* writer, const RefObject* inPtr) { - // We don't serialize Module or Scope - if (as(inPtr) || as(inPtr)) + // We don't serialize Module + if (as(inPtr)) { writer->setPointerIndex(inPtr, SerialIndex(0)); return SerialIndex(0); @@ -65,6 +65,13 @@ SerialIndex ModuleSerialFilter::writePointer(SerialWriter* writer, const NodeBas NodeBase* ptr = const_cast(inPtr); SLANG_ASSERT(ptr); + // We don't serialize Scope + if (as(ptr)) + { + writer->setPointerIndex(inPtr, SerialIndex(0)); + return SerialIndex(0); + } + if (Decl* decl = as(ptr)) { ModuleDecl* moduleDecl = findModuleForDecl(decl); diff --git a/source/slang/slang.cpp b/source/slang/slang.cpp index acc4b6f77..ef92558cc 100644 --- a/source/slang/slang.cpp +++ b/source/slang/slang.cpp @@ -159,20 +159,20 @@ void Session::init() // TODO: load these on-demand to avoid parsing // stdlib code for languages the user won't use. - baseLanguageScope = new Scope(); + baseLanguageScope = builtinAstBuilder->create(); // Will stay in scope as long as ASTBuilder baseModuleDecl = populateBaseLanguageModule( m_builtinLinkage->getASTBuilder(), baseLanguageScope); - coreLanguageScope = new Scope(); + coreLanguageScope = builtinAstBuilder->create(); coreLanguageScope->nextSibling = baseLanguageScope; - hlslLanguageScope = new Scope(); + hlslLanguageScope = builtinAstBuilder->create(); hlslLanguageScope->nextSibling = coreLanguageScope; - slangLanguageScope = new Scope(); + slangLanguageScope = builtinAstBuilder->create(); slangLanguageScope->nextSibling = hlslLanguageScope; { @@ -417,7 +417,7 @@ SlangResult Session::_readBuiltinModule(ISlangFileSystem* fileSystem, Scope* sco else { // We need to create a new scope to link into the whole thing - auto subScope = new Scope(); + auto subScope = linkage->getASTBuilder()->create(); subScope->containerDecl = moduleDecl; subScope->nextSibling = scope->nextSibling; scope->nextSibling = subScope; @@ -1291,7 +1291,7 @@ SlangResult Linkage::loadFile(String const& path, PathInfo& outPathInfo, ISlangB return SLANG_OK; } -Expr* Linkage::parseTermString(String typeStr, RefPtr scope) +Expr* Linkage::parseTermString(String typeStr, Scope* scope) { // Create a SourceManager on the stack, so any allocations for 'SourceFile'/'SourceView' etc will be cleaned up SourceManager localSourceManager; @@ -1354,11 +1354,21 @@ Type* ComponentType::getTypeFromString( if(m_types.TryGetValue(typeStr, type)) return type; + + // TODO(JS): For now just used the linkages ASTBuilder to keep on scope + // + // The parseTermString uses the linkage ASTBuilder for it's parsing. + // + // It might be possible to just create a temporary ASTBuilder - the worry though is + // that the parsing sets a member variable in AST node to one of these scopes, and then + // it become a dangling pointer. So for now we go with the linkages. + auto astBuilder = getLinkage()->getASTBuilder(); + // Otherwise, we need to start looking in // the modules that were directly or // indirectly referenced. // - RefPtr scope = _createScopeForLegacyLookup(); + Scope* scope = _createScopeForLegacyLookup(astBuilder); auto linkage = getLinkage(); Expr* typeExpr = linkage->parseTermString( @@ -1537,7 +1547,7 @@ static void _calcViewInitiatingHierarchy(SourceManager* sourceManager, ViewIniti // This assumes they increase in SourceLoc implies an later within a source file - this is true currently. for (auto& pair : outHierarchy) { - pair.Value.sort([](SourceView* a, SourceView* b) { return a->getInitiatingSourceLoc().getRaw() < b->getInitiatingSourceLoc().getRaw(); }); + pair.Value.sort([](SourceView* a, SourceView* b) -> bool { return a->getInitiatingSourceLoc().getRaw() < b->getInitiatingSourceLoc().getRaw(); }); } } @@ -1675,7 +1685,7 @@ void FrontEndCompileRequest::parseTranslationUnit( // would be checked too (after those on the FrontEndCompileRequest). IncludeSystem includeSystem(&linkage->searchDirectories, linkage->getFileSystemExt(), linkage->getSourceManager()); - RefPtr languageScope; + Scope* languageScope = nullptr; switch (translationUnit->sourceLanguage) { case SourceLanguage::HLSL: @@ -2207,6 +2217,7 @@ SlangResult EndToEndCompileRequest::executeActionsInner() SlangResult EndToEndCompileRequest::executeActions() { SlangResult res = executeActionsInner(); + m_diagnosticOutput = getSink()->outputBuffer.ProduceString(); return res; } @@ -3762,7 +3773,7 @@ RefPtr findOrImportModule( } void Session::addBuiltinSource( - RefPtr const& scope, + Scope* scope, String const& path, String const& source) { @@ -3784,7 +3795,6 @@ void Session::addBuiltinSource( Name* moduleName = getNamePool()->getName(path); auto translationUnitIndex = compileRequest->addTranslationUnit(SourceLanguage::Slang, moduleName); - compileRequest->addTranslationUnitSourceString( translationUnitIndex, path, @@ -3819,7 +3829,7 @@ void Session::addBuiltinSource( else { // We need to create a new scope to link into the whole thing - auto subScope = new Scope(); + auto subScope = module->getASTBuilder()->create(); subScope->containerDecl = moduleDecl; subScope->nextSibling = scope->nextSibling; scope->nextSibling = subScope; @@ -4247,7 +4257,7 @@ SlangResult EndToEndCompileRequest::EndToEndCompileRequest::compile() return saveRes; } } - else if (m_dumpReproOnError && SLANG_FAILED(res)) + else if (m_dumpReproOnError && SLANG_FAILED(res)) { String reproFileName; SlangResult saveRes = SLANG_FAIL; -- cgit v1.2.3