diff options
| -rw-r--r-- | source/slang-core-module/slang-embedded-core-module-source.cpp | 24 | ||||
| -rw-r--r-- | source/slang/slang-ast-base.h | 11 | ||||
| -rw-r--r-- | source/slang/slang-ast-builder.cpp | 247 | ||||
| -rw-r--r-- | source/slang/slang-ast-builder.h | 129 | ||||
| -rw-r--r-- | source/slang/slang-ast-decl.h | 7 | ||||
| -rw-r--r-- | source/slang/slang-check-decl.cpp | 10 | ||||
| -rw-r--r-- | source/slang/slang-end-to-end-request.cpp | 2 | ||||
| -rw-r--r-- | source/slang/slang-global-session.cpp | 52 | ||||
| -rw-r--r-- | source/slang/slang-global-session.h | 23 | ||||
| -rw-r--r-- | source/slang/slang-options.cpp | 5 |
10 files changed, 383 insertions, 127 deletions
diff --git a/source/slang-core-module/slang-embedded-core-module-source.cpp b/source/slang-core-module/slang-embedded-core-module-source.cpp index 65bfed844..0e03716ca 100644 --- a/source/slang-core-module/slang-embedded-core-module-source.cpp +++ b/source/slang-core-module/slang-embedded-core-module-source.cpp @@ -151,30 +151,6 @@ static const BaseTypeConversionInfo kBaseTypes[] = { kBaseTypeConversionRank_IntPtr}, }; -void Session::finalizeSharedASTBuilder() -{ - // Force creation of all builtin types so we can make sure - // they are created by the builtin AST builder instead of - // some user linkage's ast builder. This avoid the problem - // of storing a reference to these global types that are - // owned by a user linkage that gets deleted with the linkage. - // - globalAstBuilder->getNoneType(); - globalAstBuilder->getNullPtrType(); - globalAstBuilder->getBottomType(); - globalAstBuilder->getErrorType(); - globalAstBuilder->getInitializerListType(); - globalAstBuilder->getOverloadedType(); - globalAstBuilder->getStringType(); - globalAstBuilder->getEnumTypeType(); - globalAstBuilder->getDiffInterfaceType(); - globalAstBuilder->getSharedASTBuilder()->getDynamicType(); - globalAstBuilder->getSharedASTBuilder()->getDiffInterfaceType(); - globalAstBuilder->getSharedASTBuilder()->getNativeStringType(); - for (auto& baseType : kBaseTypes) - globalAstBuilder->getBuiltinType(baseType.tag); -} - // Given two base types, we need to be able to compute the cost of converting between them. ConversionCost getBaseTypeConversionCost( BaseTypeConversionInfo const& toInfo, diff --git a/source/slang/slang-ast-base.h b/source/slang/slang-ast-base.h index d0bdad2b1..dbbfe6b92 100644 --- a/source/slang/slang-ast-base.h +++ b/source/slang/slang-ast-base.h @@ -29,8 +29,8 @@ class NodeBase // Note that the astBuilder is not stored in the NodeBase derived types by default. SLANG_FORCE_INLINE void init(ASTNodeType inAstNodeType, ASTBuilder* inAstBuilder) { - SLANG_UNUSED(inAstBuilder); astNodeType = inAstNodeType; + _astBuilder = inAstBuilder; #ifdef _DEBUG _initDebug(inAstNodeType, inAstBuilder); #endif @@ -48,6 +48,15 @@ class NodeBase #ifdef _DEBUG int32_t _debugUID = 0; #endif + + /// Get the AST builder that was used to allocate this node. + ASTBuilder* getASTBuilder() { return _astBuilder; } + +private: + friend class ASTBuilder; + + /// The AST builder that was used to allocate this node. + ASTBuilder* _astBuilder = nullptr; }; // Casting of NodeBase diff --git a/source/slang/slang-ast-builder.cpp b/source/slang/slang-ast-builder.cpp index d3e99ee4b..d576ed0fb 100644 --- a/source/slang/slang-ast-builder.cpp +++ b/source/slang/slang-ast-builder.cpp @@ -10,21 +10,16 @@ namespace Slang // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! SharedASTBuilder !!!!!!!!!!!!!!!!!!!!!!!!!!!!!! -SharedASTBuilder::SharedASTBuilder() {} - -void SharedASTBuilder::init(Session* session) +SharedASTBuilder::SharedASTBuilder(Session* session, RootASTBuilder* rootASTBuilder) { m_namePool = session->getNamePool(); // Save the associated session m_session = session; - // We just want as a place to store allocations of shared types - { - RefPtr<ASTBuilder> astBuilder(new ASTBuilder); - astBuilder->m_sharedASTBuilder = this; - m_astBuilder = astBuilder.detach(); - } + // The root AST builder is the one that owns this `SharedASTBuilder`. + // + m_astBuilder = rootASTBuilder; // Clear the built in types memset(m_builtinTypes, 0, sizeof(m_builtinTypes)); @@ -169,20 +164,6 @@ Type* SharedASTBuilder::getOverloadedType() return m_overloadedType; } -SharedASTBuilder::~SharedASTBuilder() -{ - // Release built in types.. - for (Index i = 0; i < SLANG_COUNT_OF(m_builtinTypes); ++i) - { - m_builtinTypes[i] = nullptr; - } - - if (m_astBuilder) - { - m_astBuilder->releaseReference(); - } -} - void SharedASTBuilder::registerBuiltinDecl(Decl* decl, BuiltinTypeModifier* modifier) { auto type = DeclRefType::create(m_astBuilder, makeDeclRef<Decl>(decl)); @@ -222,22 +203,32 @@ Decl* SharedASTBuilder::tryFindMagicDecl(const String& name) // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! ASTBuilder !!!!!!!!!!!!!!!!!!!!!!!!!!!!!! -ASTBuilder::ASTBuilder(SharedASTBuilder* sharedASTBuilder, const String& name) - : m_sharedASTBuilder(sharedASTBuilder) - , m_name(name) - , m_id(sharedASTBuilder->m_id++) - , m_arena(2097152) +/// Default block size of 2MB. +static const size_t kASTBuilderMemoryArenaBlockSize = 2 * 1024 * 1024; + +ASTBuilder::ASTBuilder(ASTBuilder* parent, String const& debugName) + : m_parent(parent), m_name(debugName), m_arena(kASTBuilderMemoryArenaBlockSize) { + SLANG_ASSERT(parent); + auto sharedASTBuilder = parent->getSharedASTBuilder(); SLANG_ASSERT(sharedASTBuilder); - // Copy Val deduplication map over so we don't create duplicate Vals that are already - // existent in the core module. - m_cachedNodes = sharedASTBuilder->getInnerASTBuilder()->m_cachedNodes; + + m_depth = parent->m_depth + 1; + + m_sharedASTBuilder = sharedASTBuilder; + m_id = sharedASTBuilder->m_id++; } ASTBuilder::ASTBuilder() - : m_sharedASTBuilder(nullptr), m_id(-1), m_arena(2097152) + : m_arena(kASTBuilderMemoryArenaBlockSize) { - m_name = "SharedASTBuilder::m_astBuilder"; +} + +RootASTBuilder::RootASTBuilder(Session* globalSession) + : m_sharedASTBuilderStorage(globalSession, this) +{ + m_sharedASTBuilder = &m_sharedASTBuilderStorage; + m_name = "RootASTBuilder"; } ASTBuilder::~ASTBuilder() @@ -250,6 +241,196 @@ ASTBuilder::~ASTBuilder() incrementEpoch(); } +Val* ASTBuilder::_getOrCreateImplSlowPath(ValNodeDesc&& desc) +{ + // The most important thing we need to determine here + // is whether the node described by `desc` would be + // created using the arena of this `ASTBuilder`, or + // one of its ancestors (and if so, which one...). + // + ASTBuilder* astBuilderToUse = _findAppropriateASTBuilderForVal(desc); + + // Once we've identified the right level of the hierarchy, + // we can check the cache at that level and create + // the node if it doesn't already exist. + // + Val* valNode = astBuilderToUse->_getOrCreateValDirectly(std::move(desc)); + + // If the chosen `astBuilderToUse` was `this`, then the + // call to `_getOrCreateValDirectly` will have updated + // `m_cachedNodes` already. + // + // If the node was created using a different builder, + // which is an ancestor than this one (which would mean + // its depth is lower), then we can also update our + // own cache to match. + // + if (astBuilderToUse->m_depth < this->m_depth) + { + // Our approach to caching assumes that we cannot + // mix-and-match AST nodes from builders that aren't + // in some kind of ancestor/descendent relationship. + // Thus, if the builder that was chosen is less deep + // than `this`, we expect that to be because it is + // an ancestor. + // + SLANG_ASSERT(this->isDescendentOf(astBuilderToUse)); + + m_cachedNodes.add(ValKey(valNode), valNode); + } + // + // Note that we do *not* want to update our cache in + // the case where the chosen builder has higher depth + // then `this`, because `this` could outlive the chosen + // builder, and we don't want to be left with + // garbage pointers sitting in the cache. + // + // We also don't consider that case to be an error, + // because it is reasonable for code to do things like + // construct a specialized decl-ref for `Foo<Bar>` using + // the builder associated with declaration `Foo`, even + // when specializing to a type `Bar` that comes from a + // deeper/child builder. + + return valNode; +} + +ASTBuilder* ASTBuilder::_findAppropriateASTBuilderForVal(ValNodeDesc const& desc) +{ + // AST builders are arranged in a hierarchy, where a child builder + // can see nodes cached in its ancestors, but not vice versa. + // + // We basically want to allocate a given `Val` as far down + // the hierarchy as we can (away from the root), so that the + // lifetime of those allocations can be narrowly scoped. However, + // we also need to ensure that `Val`s are cached far enough + // *up* the hierarchy that deduplication is possible, and that + // we can be sure a `Val` lives at least as long as each of + // its operands. + // + // Our approach to the caching problem relies on a key + // constraint, that the `ASTBuilder` used for a `_getOrCreateImpl()` + // operation and all of the `ASTBuilder`s used to create the + // nodes referenced as operands in the `desc` must be part + // of a single path of parent links in the hierarchy. + // Put another way: for any two `ASTBuilder`s involved in the + // creation of the node or its operands, they must be in some + // kind of ancestor/descendent relationship. + // + // Given this constraint, we can determine that the `Val` should + // be allocated and cached on the *deepest* AST builder + // from among the operands (or on the root AST builder in the + // case where there are no operands). + // + // We thus initialize our variable to the *shallowest* builder, + // which is the one we'll use if there are no operands. + // + ASTBuilder* deepestBuilder = getSharedASTBuilder()->getInnerASTBuilder(); + for (auto const& operand : desc.operands) + { + // We are only interested in operands that reference + // an AST node, so we will skip over all others. + // + switch (operand.kind) + { + default: + continue; + + case ValNodeOperandKind::ASTNode: + case ValNodeOperandKind::ValNode: + break; + } + + // We now know that the operand is represented + // as an AST node, but we need to skip over + // null operands because they aren't relevant + // to picking the right AST builder to use. + // + NodeBase* node = operand.values.nodeOperand; + if (!node) + continue; + + // Once we have an AST node worth looking at, + // we find the AST builder responsible for + // allocating that node. + // + ASTBuilder* nodeBuilder = node->getASTBuilder(); + SLANG_ASSERT(nodeBuilder); + + // The approach we are taking here relies on all + // the AST builders involved being part of a single + // path in the hierarchy, so we will do a minimal + // amount of validation in debug builds to ensure + // that each of the node builders for the operands + // is in some kind of ancestor/descendent relationship + // with the builder being used to make the request. + // + SLANG_ASSERT(nodeBuilder->isDescendentOf(this) || this->isDescendentOf(nodeBuilder)); + + // If the builder we are looking at is deeper than the + // deepest builder we've seen previously, then we update + // our candiate for the deepest builder. + // + if (nodeBuilder->m_depth > deepestBuilder->m_depth) + deepestBuilder = nodeBuilder; + } + + // + // At the end of that loop, we have a maximally-deep builder, + // and because we require all the builders to come from + // a single path in the hierarchy, that builder is also + // uniquely determined (a maximum rather than just maximal). + // + + return deepestBuilder; +} + +bool ASTBuilder::isDescendentOf(ASTBuilder* ancestor) +{ + SLANG_ASSERT(ancestor); + + auto builder = this; + while (builder) + { + if (builder == ancestor) + return true; + builder = builder->m_parent; + } + return false; +} + + +Val* ASTBuilder::_getOrCreateValDirectly(ValNodeDesc&& desc) +{ + // This operation should only be called if `this` + // was determined to be the appropriate AST builder + // to use when allocating/caching a `Val` based on `desc`. + // + SLANG_ASSERT(this == _findAppropriateASTBuilderForVal(desc)); + + // We start by checking the cache. This might have + // already been done as part of `_getOrCreateImpl()`, + // but it is also possible that the `_getOrCreateImpl()` + // call was made on a descendent `ASTBuilder` and its + // cache might not (yet) contain the given node. + // + if (auto found = m_cachedNodes.tryGetValue(desc)) + return *found; + + // If we don't have a cache hit at this level, + // then we just need to create the node and + // update our cache. + // + auto node = as<Val>(desc.type.createInstance(this)); + SLANG_ASSERT(node); + for (auto& operand : desc.operands) + node->m_operands.add(operand); + + m_cachedNodes.add(ValKey(node), _Move(node)); + + return node; +} + Index ASTBuilder::getEpoch() { return getSharedASTBuilder()->m_session->m_epochId; diff --git a/source/slang/slang-ast-builder.h b/source/slang/slang-ast-builder.h index 5944b5e31..72f875f91 100644 --- a/source/slang/slang-ast-builder.h +++ b/source/slang/slang-ast-builder.h @@ -12,8 +12,11 @@ namespace Slang { +class RootASTBuilder; -class SharedASTBuilder : public RefObject +/// Data shared by multiple `ASTBuilder`s that belong to the same hierarchy. +/// +class SharedASTBuilder { friend class ASTBuilder; @@ -64,14 +67,7 @@ public: /// Session. NamePool* getNamePool() { return m_namePool; } - /// Must be called before used - void init(Session* session); - - SharedASTBuilder(); - - ~SharedASTBuilder(); - - ASTBuilder* getInnerASTBuilder() { return m_astBuilder; } + RootASTBuilder* getInnerASTBuilder() { return m_astBuilder; } Name* getThisTypeName() { @@ -82,6 +78,12 @@ public: return m_thisTypeName; } +private: + friend class RootASTBuilder; + + /// Initialize a shared AST builder owned by the given `rootASTBuilder`. + SharedASTBuilder(Session* globalSession, RootASTBuilder* rootASTBuilder); + protected: // State shared between ASTBuilders @@ -118,8 +120,15 @@ protected: Name* m_thisTypeName = nullptr; - // This is a private builder used for these shared types - ASTBuilder* m_astBuilder = nullptr; + /// The root AST builder that owns this shared storage. + /// + /// This field exists to support older code that passes around + /// the `SharedASTBuilder` state directly, rather than just + /// pass around the root AST builder. Once all the relevant + /// code is cleaned up, this field should be removed. + /// + RootASTBuilder* m_astBuilder = nullptr; + Session* m_session = nullptr; Index m_id = 1; @@ -192,26 +201,24 @@ class ASTBuilder : public RefObject friend class SharedASTBuilder; public: + /// Get a `Val` that has the AST node class and operands described by `desc`. + /// + /// If such a `Val` has already been created, it will be re-used. + /// Otherwise, an appropriate one will be created. + /// Val* _getOrCreateImpl(ValNodeDesc&& desc) { if (auto found = m_cachedNodes.tryGetValue(desc)) return *found; - auto node = as<Val>(desc.type.createInstance(this)); - SLANG_ASSERT(node); - for (auto& operand : desc.operands) - node->m_operands.add(operand); - auto result = node; - m_cachedNodes.add(ValKey(node), _Move(node)); - return result; + return _getOrCreateImplSlowPath(_Move(desc)); } /// A cache for AST nodes that are entirely defined by their node type, with /// no need for additional state. + /// Dictionary<ValKey, Val*, Hash<ValKey>, ValKeyEqual> m_cachedNodes; - Dictionary<GenericDecl*, List<Val*>> m_cachedGenericDefaultArgs; - /// Create AST types template<typename T> T* createImpl() @@ -685,17 +692,35 @@ public: BreakableStmt::UniqueID generateUniqueIDForStmt() { return create<UniqueStmtIDNode>(); } - /// Ctor - ASTBuilder(SharedASTBuilder* sharedASTBuilder, const String& name); + /// Construct an `ASTBuilder` as a child of the given `parent` builder + /// + /// The `debugName` is helpful to distinguish between different AST + /// builders when there are multiple active builders. + /// + ASTBuilder(ASTBuilder* parent, String const& debugName); - /// Dtor + /// Destructor. ~ASTBuilder(); + /// Is this AST builder a direct or indirect descendent of `ancestor`? + /// + /// Note that this returns `true` if `this == ancestor`. + /// + bool isDescendentOf(ASTBuilder* ancestor); + +private: + Val* _getOrCreateImplSlowPath(ValNodeDesc&& desc); + ASTBuilder* _findAppropriateASTBuilderForVal(ValNodeDesc const& desc); + Val* _getOrCreateValDirectly(ValNodeDesc&& desc); + protected: - // Special default Ctor that can only be used by SharedASTBuilder + /// Default constructor. + /// + /// Should not be used outside of the implementation of + /// `ASTBuilder` and `RootASTBuilder`. + /// ASTBuilder(); - template<typename T> SLANG_FORCE_INLINE T* _initAndAdd(T* node) { @@ -720,17 +745,65 @@ protected: return node; } + /// The parent of this AST builder. + /// + /// AST nodes allocated using the parent builder are + /// guaranteed to outlive nodes allocated using this builder. + /// Because of the lifetime guarantees, it is safe for + /// nodes created on a child/descendent builder to contain + /// pointers to nodes created on a parent/ancestor builder, + /// but not vice versa. + /// + RefPtr<ASTBuilder> m_parent; + + /// Nesting depth of this AST builder. + /// + /// If this builder has no parent, then the depth is zero. + /// Otherwise, the depth is one more than the depth of + /// the parent. + /// + Int m_depth = 0; + + /// Shared state for all AST builders in the same hierarchy. + /// + /// All `ASTBuilder`s that share the same root via their + /// `m_parent` fields should be associated with the same + /// `SharedASTBuilder`. The shared builder caches information + /// that logically belongs at the root of the hierarchy, + /// such as builtin types. + /// + SharedASTBuilder* m_sharedASTBuilder = nullptr; + String m_name; - Index m_id; + Index m_id = -1; /// List of all nodes that require being dtored when ASTBuilder is dtored List<NodeBase*> m_dtorNodes; - SharedASTBuilder* m_sharedASTBuilder; - MemoryArena m_arena; }; +/// An `ASTBuilder` that is at the root of its own hierarchy. +/// +/// Every AST builder that is not a `RootASTBuilder` must have +/// a parent AST builder. +/// +/// This class owns the storage for the `SharedASTBuilder` that +/// is used for caching information about things like builtin +/// declarations/types, which would be redundant to cache at +/// every level of the hierarchy. +/// +class RootASTBuilder : public ASTBuilder +{ +public: + /// Construct a root AST builder for the given `globalSession`. + /// + RootASTBuilder(Session* globalSession); + +private: + SharedASTBuilder m_sharedASTBuilderStorage; +}; + // Retrieves the ASTBuilder for the current compilation session. ASTBuilder* getCurrentASTBuilder(); diff --git a/source/slang/slang-ast-decl.h b/source/slang/slang-ast-decl.h index 3fa929d6f..40c55ec44 100644 --- a/source/slang/slang-ast-decl.h +++ b/source/slang/slang-ast-decl.h @@ -844,6 +844,13 @@ class GenericDecl : public ContainerDecl FIDDLE(...) // The decl that is genericized... FIDDLE() Decl* inner = nullptr; + + /// A cached list of arguments that can be used when forming + /// a reference to the inner declaration with "default + /// substitutions" (for each generic parameter, the coresponding + /// argument will be a reference to the parameter itself). + /// + List<Val*> _cachedArgsForDefaultSubstitution; }; FIDDLE() diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp index 98cdb9e61..dd785f0a8 100644 --- a/source/slang/slang-check-decl.cpp +++ b/source/slang/slang-check-decl.cpp @@ -3736,7 +3736,7 @@ void registerBuiltinDecl(ASTBuilder* astBuilder, Decl* decl) /// static void _registerBuiltinDeclsRec(Session* session, Decl* decl) { - SharedASTBuilder* sharedASTBuilder = session->m_sharedASTBuilder; + SharedASTBuilder* sharedASTBuilder = session->getSharedASTBuilder(); registerBuiltinDecl(sharedASTBuilder, decl); @@ -9376,10 +9376,10 @@ List<Val*> getDefaultSubstitutionArgs( SemanticsVisitor* semantics, GenericDecl* genericDecl) { - List<Val*> args; - if (astBuilder->m_cachedGenericDefaultArgs.tryGetValue(genericDecl, args)) - return args; + if (genericDecl->_cachedArgsForDefaultSubstitution.getCount() != 0) + return genericDecl->_cachedArgsForDefaultSubstitution; + List<Val*> args; for (auto mm : genericDecl->getDirectMemberDecls()) { if (auto genericTypeParamDecl = as<GenericTypeParamDecl>(mm)) @@ -9439,7 +9439,7 @@ List<Val*> getDefaultSubstitutionArgs( } if (shouldCache) - astBuilder->m_cachedGenericDefaultArgs[genericDecl] = args; + genericDecl->_cachedArgsForDefaultSubstitution = args; return args; } diff --git a/source/slang/slang-end-to-end-request.cpp b/source/slang/slang-end-to-end-request.cpp index d0f0a6f53..725fc1854 100644 --- a/source/slang/slang-end-to-end-request.cpp +++ b/source/slang/slang-end-to-end-request.cpp @@ -28,7 +28,7 @@ EndToEndCompileRequest::EndToEndCompileRequest(Session* session) : m_session(session), m_sink(nullptr, Lexer::sourceLocationLexer) { RefPtr<ASTBuilder> astBuilder( - new ASTBuilder(session->m_sharedASTBuilder, "EndToEnd::Linkage::astBuilder")); + new ASTBuilder(session->getASTBuilder(), "EndToEnd::Linkage::astBuilder")); m_linkage = new Linkage(session, astBuilder, session->getBuiltinLinkage()); init(); } diff --git a/source/slang/slang-global-session.cpp b/source/slang/slang-global-session.cpp index 7b922dcf0..20b993def 100644 --- a/source/slang/slang-global-session.cpp +++ b/source/slang/slang-global-session.cpp @@ -55,19 +55,19 @@ void Session::init() // Set up the command line options initCommandOptions(m_commandOptions); - // Set up shared AST builder - m_sharedASTBuilder = new SharedASTBuilder; - m_sharedASTBuilder->init(this); - - // And the global ASTBuilder - auto builtinAstBuilder = m_sharedASTBuilder->getInnerASTBuilder(); - globalAstBuilder = builtinAstBuilder; + // Create the root AST builder that will be used when + // loading the builtin modules, and which will serve as + // the parent for the the AST builder of any linkages + // created from this global session. + // + auto rootASTBuilder = new RootASTBuilder(this); + m_rootASTBuilder = rootASTBuilder; // Make sure our source manager is initialized builtinSourceManager.initialize(nullptr, nullptr); // Built in linkage uses the built in builder - m_builtinLinkage = new Linkage(this, builtinAstBuilder, nullptr); + m_builtinLinkage = new Linkage(this, rootASTBuilder, nullptr); m_builtinLinkage->m_optionSet.set(CompilerOptionName::DebugInformation, DebugInfoLevel::None); // Because the `Session` retains the builtin `Linkage`, @@ -85,22 +85,22 @@ void Session::init() // TODO: load these on-demand to avoid parsing // the core module code for languages the user won't use. - baseLanguageScope = builtinAstBuilder->create<Scope>(); + baseLanguageScope = rootASTBuilder->create<Scope>(); // Will stay in scope as long as ASTBuilder baseModuleDecl = populateBaseLanguageModule(m_builtinLinkage->getASTBuilder(), baseLanguageScope); - coreLanguageScope = builtinAstBuilder->create<Scope>(); + coreLanguageScope = rootASTBuilder->create<Scope>(); coreLanguageScope->nextSibling = baseLanguageScope; - hlslLanguageScope = builtinAstBuilder->create<Scope>(); + hlslLanguageScope = rootASTBuilder->create<Scope>(); hlslLanguageScope->nextSibling = coreLanguageScope; - slangLanguageScope = builtinAstBuilder->create<Scope>(); + slangLanguageScope = rootASTBuilder->create<Scope>(); slangLanguageScope->nextSibling = hlslLanguageScope; - glslLanguageScope = builtinAstBuilder->create<Scope>(); + glslLanguageScope = rootASTBuilder->create<Scope>(); glslLanguageScope->nextSibling = slangLanguageScope; glslModuleName = getNameObj("glsl"); @@ -126,17 +126,24 @@ void Session::init() Session::~Session() { - // This is necessary because this ASTBuilder uses the SharedASTBuilder also owned by the - // session. If the SharedASTBuilder gets dtored before the globalASTBuilder it has a dangling - // pointer, which is referenced in the ASTBuilder dtor (likely) causing a crash. + // Destroy the array of core (automatically-included) modules. + // + // TODO(tfoley): This code didn't have a comment clearly explaining + // why this step is necessary, but the other line that used to be + // here had a comment that expressed concern about the `SharedASTBuilder` + // gettng destruted before things that refer to it. It is possible + // that the underlying problem here is that the modules in the + // `coreModules` array are owned by the builtin linkage, and Bad Things + // would happen if the linkage gets destroyed while these modules + // are still alive. // - // By destroying first we know it is destroyed, before the SharedASTBuilder. - globalAstBuilder.setNull(); - - // destroy modules next coreModules = decltype(coreModules)(); } +SharedASTBuilder* Session::getSharedASTBuilder() +{ + return getASTBuilder()->getSharedASTBuilder(); +} Module* Session::getBuiltinModule(slang::BuiltinModuleName name) { @@ -403,8 +410,6 @@ SlangResult Session::compileBuiltinModule( } } - finalizeSharedASTBuilder(); - #ifdef _DEBUG if (moduleName == slang::BuiltinModuleName::Core) { @@ -458,7 +463,6 @@ SlangResult Session::loadBuiltinModule( coreModules.add(module); } - finalizeSharedASTBuilder(); return SLANG_OK; } @@ -757,7 +761,7 @@ static T makeFromSizeVersioned(const uint8_t* src) SLANG_NO_THROW SlangResult SLANG_MCALL Session::createSession(slang::SessionDesc const& inDesc, slang::ISession** outSession) { - RefPtr<ASTBuilder> astBuilder(new ASTBuilder(m_sharedASTBuilder, "Session::astBuilder")); + auto astBuilder = RefPtr(new ASTBuilder(m_rootASTBuilder, "Session::astBuilder")); slang::SessionDesc desc = makeFromSizeVersioned<slang::SessionDesc>((uint8_t*)&inDesc); RefPtr<Linkage> linkage = new Linkage(this, astBuilder, getBuiltinLinkage()); diff --git a/source/slang/slang-global-session.h b/source/slang/slang-global-session.h index 56984bdbd..e5615d676 100644 --- a/source/slang/slang-global-session.h +++ b/source/slang/slang-global-session.h @@ -232,12 +232,21 @@ public: Name* tryGetNameObj(String name) { return namePool.tryGetName(name); } // + /// Get the AST builder associated with this global session. + /// + /// This is the AST builder used for builtin modules. + /// + RefPtr<ASTBuilder> getASTBuilder() { return m_rootASTBuilder; } + + /// Get the shared AST builder state associated with this global session. + /// + /// Equivalent to `this->getASTBuilder()->getSharedASTBuilder()`. + /// + SharedASTBuilder* getSharedASTBuilder(); + /// This AST Builder should only be used for creating AST nodes that are global across requests /// not doing so could lead to memory being consumed but not used. - ASTBuilder* getGlobalASTBuilder() { return globalAstBuilder; } - void finalizeSharedASTBuilder(); - - RefPtr<ASTBuilder> globalAstBuilder; + ASTBuilder* getGlobalASTBuilder() { return m_rootASTBuilder; } // Generated code for core module, etc. String coreModulePath; @@ -257,8 +266,6 @@ public: void getBuiltinModuleSource(StringBuilder& sb, slang::BuiltinModuleName moduleName); - RefPtr<SharedASTBuilder> m_sharedASTBuilder; - SPIRVCoreGrammarInfo& getSPIRVCoreGrammarInfo() { if (!spirvCoreGrammarInfo) @@ -356,6 +363,10 @@ private: double m_downstreamCompileTime = 0.0; double m_totalCompileTime = 0.0; + + /// The AST builder that will be used for builtin modules. + /// + RefPtr<ASTBuilder> m_rootASTBuilder; }; /* Returns SLANG_OK if pass through support is available */ diff --git a/source/slang/slang-options.cpp b/source/slang/slang-options.cpp index b7f7268d1..e777d27bc 100644 --- a/source/slang/slang-options.cpp +++ b/source/slang/slang-options.cpp @@ -2294,11 +2294,6 @@ SlangResult OptionsParser::_parse(int argc, char const* const* argv) SLANG_RETURN_ON_FAIL(File::readAllBytes(fileName.value, contents)); SLANG_RETURN_ON_FAIL( m_session->loadCoreModule(contents.getData(), contents.getSizeInBytes())); - - // Ensure that the linkage's AST builder is up-to-date. - linkage->getASTBuilder()->m_cachedNodes = - asInternal(m_session)->getGlobalASTBuilder()->m_cachedNodes; - break; } case OptionKind::CompileCoreModule: |