Improvements around review of debug serialization info (shader-slang#769)

* * Make SourceView and SourceFile no longer derive from RefObject
* Both have life time now managed by SourceManager
* Tidied up a little around the serialization test code - just create the IRModule once

* Simplified code around deleting SourceView/File.

* Looked into generateIRForTranslationUnit - seems reasonable to just call it once, because it has side effects.

Author: jsmall-zzz

source/slang/compiler.h

@@ -183,7 +183,7 @@ namespace Slang
         // The source file(s) that will be compiled to form this translation unit
         //
         // Usually, for HLSL or GLSL there will be only one file.
-        List<RefPtr<SourceFile> > sourceFiles;
+        List<SourceFile*> sourceFiles;
 
         // The entry points associated with this translation unit
         List<RefPtr<EntryPointRequest> > entryPoints;

source/slang/ir-serialize.cpp

@@ -1800,7 +1800,7 @@ static int _calcFixSourceLoc(const IRSerialData::DebugSourceInfo& info, SourceVi
             pathInfo.type = PathInfo::Type::FoundPath;
             pathInfo.foundPath = debugStringSlices[UInt(srcSourceInfo.m_pathIndex)];
 
-            RefPtr<SourceFile> sourceFile = sourceManager->createSourceFileWithSize(pathInfo, srcSourceInfo.m_endSourceLoc - srcSourceInfo.m_startSourceLoc);
+            SourceFile* sourceFile = sourceManager->createSourceFileWithSize(pathInfo, srcSourceInfo.m_endSourceLoc - srcSourceInfo.m_startSourceLoc);
             SourceView* sourceView = sourceManager->createSourceView(sourceFile);
 
             // We need to accumulate all line numbers, for this source file, both adjusted and unadjusted

source/slang/ir-serialize.h

@@ -473,7 +473,7 @@ struct IRSerialWriter
 
         SourceLoc::RawValue m_baseSourceLoc;            ///< The base source location
 
-        RefPtr<SourceFile> m_sourceFile;                ///< The source file
+        SourceFile* m_sourceFile;                       ///< The source file
         List<uint8_t> m_lineIndexUsed;                  ///< Has 1 if the line is used
         List<uint32_t> m_usedLineIndices;               ///< Holds the lines that have been hit                 
 

source/slang/preprocessor.cpp

@@ -842,8 +842,7 @@ static Token AdvanceToken(Preprocessor* preprocessor)
         // We create a dummy file to represent the token-paste operation
         PathInfo pathInfo = PathInfo::makeTokenPaste();
 
-        RefPtr<SourceFile> sourceFile = sourceManager->createSourceFileWithString(pathInfo, sb.ProduceString());
-
+        SourceFile* sourceFile = sourceManager->createSourceFileWithString(pathInfo, sb.ProduceString());
         SourceView* sourceView = sourceManager->createSourceView(sourceFile);
 
         Lexer lexer;
@@ -1634,7 +1633,7 @@ static void HandleIncludeDirective(PreprocessorDirectiveContext* context)
     auto sourceManager = context->preprocessor->getCompileRequest()->getSourceManager();
 
     // See if this an already loaded source file
-    RefPtr<SourceFile> sourceFile = sourceManager->findSourceFileRecursively(filePathInfo.canonicalPath);
+    SourceFile* sourceFile = sourceManager->findSourceFileRecursively(filePathInfo.canonicalPath);
     // If not create a new one, and add to the list of known source files
     if (!sourceFile)
     {
@@ -2268,8 +2267,8 @@ static void DefineMacro(
 
     auto sourceManager = preprocessor->translationUnit->compileRequest->getSourceManager();
 
-    RefPtr<SourceFile> keyFile = sourceManager->createSourceFileWithString(pathInfo, key);
-    RefPtr<SourceFile> valueFile = sourceManager->createSourceFileWithString(pathInfo, value);
+    SourceFile* keyFile = sourceManager->createSourceFileWithString(pathInfo, key);
+    SourceFile* valueFile = sourceManager->createSourceFileWithString(pathInfo, value);
 
     SourceView* keyView = sourceManager->createSourceView(keyFile);
     SourceView* valueView = sourceManager->createSourceView(valueFile);

source/slang/slang.cpp

@@ -420,7 +420,7 @@ RefPtr<Expr> CompileRequest::parseTypeString(TranslationUnitRequest * translatio
     SourceManager localSourceManager;
     localSourceManager.initialize(sourceManager);
 
-    Slang::RefPtr<Slang::SourceFile> srcFile(localSourceManager.createSourceFileWithString(PathInfo::makeTypeParse(), typeStr));
+    Slang::SourceFile* srcFile = localSourceManager.createSourceFileWithString(PathInfo::makeTypeParse(), typeStr);
 
     // We'll use a temporary diagnostic sink  
     DiagnosticSink sink;
@@ -561,17 +561,15 @@ void CompileRequest::generateIR()
     // in isolation.
     for( auto& translationUnit : translationUnits )
     {
-        // TODO JS:
-        // This is a bit of HACK. Apparently if we call generateIRForTranslationUnit(translationUnit) twice
-        // we get a different result (!).
-        // So here, we only create once even if we run verification.
-        RefPtr<IRModule> irModule;
+        // We want to only run generateIRForTranslationUnit once here. This is for two side effects:
+        // * it can dump ir 
+        // * it can generate diagnostics
+
+        /// Generate IR for translation unit
+        RefPtr<IRModule> irModule(generateIRForTranslationUnit(translationUnit));
 
         if (verifyDebugSerialization)
         {
-            /// Generate IR for translation unit
-            irModule = generateIRForTranslationUnit(translationUnit);
-
             // Verify debug information
             if (SLANG_FAILED(IRSerialUtil::verifySerialize(irModule, mSession, sourceManager, IRSerialBinary::CompressionType::None, IRSerialWriter::OptionFlag::DebugInfo)))
             {
@@ -583,16 +581,11 @@ void CompileRequest::generateIR()
         {              
             IRSerialData serialData;
             {
-                /// Generate IR for translation unit
-                if (!irModule)
-                {
-                    irModule = generateIRForTranslationUnit(translationUnit);
-                }
-
                 // Write IR out to serialData - copying over SourceLoc information directly
                 IRSerialWriter writer;
                 writer.write(irModule, sourceManager, IRSerialWriter::OptionFlag::RawSourceLocation, &serialData);
 
+                // Destroy irModule such that memory can be used for newly constructed read irReadModule  
                 irModule = nullptr;
             }
             RefPtr<IRModule> irReadModule;
@@ -602,18 +595,12 @@ void CompileRequest::generateIR()
                 reader.read(serialData, mSession, nullptr, irReadModule);
             }
 
-            // Use the serialized irModule
-            translationUnit->irModule = irReadModule;
+            // Set irModule to the read module
+            irModule = irReadModule;
         }
-        else
-        {
-            if (!irModule)
-            {
-                irModule = generateIRForTranslationUnit(translationUnit);
-            }
 
-            translationUnit->irModule = irModule; 
-        }
+        // Set the module on the translation unit
+        translationUnit->irModule = irModule;
     }
 }
 
@@ -779,7 +766,7 @@ void CompileRequest::addTranslationUnitSourceBlob(
     ISlangBlob*     sourceBlob)
 {
     PathInfo pathInfo = PathInfo::makePath(path);
-    RefPtr<SourceFile> sourceFile = getSourceManager()->createSourceFileWithBlob(pathInfo, sourceBlob);
+    SourceFile* sourceFile = getSourceManager()->createSourceFileWithBlob(pathInfo, sourceBlob);
 
     addTranslationUnitSourceFile(translationUnitIndex, sourceFile);
 }
@@ -790,7 +777,7 @@ void CompileRequest::addTranslationUnitSourceString(
     String const&   source)
 {
     PathInfo pathInfo = PathInfo::makePath(path);
-    RefPtr<SourceFile> sourceFile = getSourceManager()->createSourceFileWithString(pathInfo, source);
+    SourceFile* sourceFile = getSourceManager()->createSourceFileWithString(pathInfo, source);
 
     addTranslationUnitSourceFile(translationUnitIndex, sourceFile);
 }
@@ -915,7 +902,7 @@ RefPtr<ModuleDecl> CompileRequest::loadModule(
     translationUnit->compileFlags = 0;
 
     // Create with the 'friendly' name
-    RefPtr<SourceFile> sourceFile = getSourceManager()->createSourceFileWithBlob(filePathInfo, sourceBlob);
+    SourceFile* sourceFile = getSourceManager()->createSourceFileWithBlob(filePathInfo, sourceBlob);
 
     translationUnit->sourceFiles.Add(sourceFile);
 

source/slang/source-loc.cpp

@@ -332,6 +332,19 @@ void SourceManager::initialize(
     m_nextLoc = m_startLoc;
 }
 
+SourceManager::~SourceManager()
+{
+    for (auto item : m_sourceViews)
+    {
+        delete item;
+    }
+
+    for (auto item : m_sourceFiles)
+    {
+        delete item;
+    }
+}
+
 UnownedStringSlice SourceManager::allocateStringSlice(const UnownedStringSlice& slice)
 {
     const UInt numChars = slice.size();
@@ -359,22 +372,25 @@ SourceRange SourceManager::allocateSourceRange(UInt size)
     return SourceRange(beginLoc, endLoc);
 }
 
-RefPtr<SourceFile> SourceManager::createSourceFileWithSize(const PathInfo& pathInfo, size_t contentSize)
+SourceFile* SourceManager::createSourceFileWithSize(const PathInfo& pathInfo, size_t contentSize)
 {
     SourceFile* sourceFile = new SourceFile(pathInfo, contentSize);
+    m_sourceFiles.Add(sourceFile);
     return sourceFile;
 }
 
-RefPtr<SourceFile> SourceManager::createSourceFileWithString(const PathInfo& pathInfo, const String& contents)
+SourceFile* SourceManager::createSourceFileWithString(const PathInfo& pathInfo, const String& contents)
 {
     SourceFile* sourceFile = new SourceFile(pathInfo, contents.Length());
+    m_sourceFiles.Add(sourceFile);
     sourceFile->setContents(contents);
     return sourceFile;
 }
 
-RefPtr<SourceFile> SourceManager::createSourceFileWithBlob(const PathInfo& pathInfo, ISlangBlob* blob)
+SourceFile* SourceManager::createSourceFileWithBlob(const PathInfo& pathInfo, ISlangBlob* blob)
 {
-    RefPtr<SourceFile> sourceFile(new SourceFile(pathInfo, blob->getBufferSize()));
+    SourceFile* sourceFile = new SourceFile(pathInfo, blob->getBufferSize());
+    m_sourceFiles.Add(sourceFile);
     sourceFile->setContents(blob);
     return sourceFile;
 }
@@ -465,8 +481,8 @@ SourceView* SourceManager::findSourceViewRecursively(SourceLoc loc) const
 
 SourceFile* SourceManager::findSourceFile(const String& canonicalPath) const
 {
-    RefPtr<SourceFile>* filePtr = m_sourceFiles.TryGetValue(canonicalPath);
-    return (filePtr) ? filePtr->Ptr() : nullptr;
+    SourceFile*const* filePtr = m_sourceFileMap.TryGetValue(canonicalPath);
+    return (filePtr) ? *filePtr : nullptr;
 }
 
 SourceFile* SourceManager::findSourceFileRecursively(const String& canonicalPath) const
@@ -487,7 +503,7 @@ SourceFile* SourceManager::findSourceFileRecursively(const String& canonicalPath
 void SourceManager::addSourceFile(const String& canonicalPath, SourceFile* sourceFile)
 {
     SLANG_ASSERT(!findSourceFileRecursively(canonicalPath));
-    m_sourceFiles.Add(canonicalPath, sourceFile);
+    m_sourceFileMap.Add(canonicalPath, sourceFile);
 }
 
 HumaneSourceLoc SourceManager::getHumaneLoc(SourceLoc loc, SourceLocType type)

source/slang/source-loc.h

@@ -141,7 +141,7 @@ struct SourceRange
 
 // A logical or physical storage object for a range of input code
 // that has logically contiguous source locations.
-class SourceFile : public RefObject
+class SourceFile 
 {
 public:
 
@@ -217,7 +217,7 @@ struct SourceManager;
 It is distinct from a SourceFile - because a SourceFile may be included multiple times, with different interpretations (depending 
 on #defines for example).
 */ 
-class SourceView: public RefObject
+class SourceView
 {
     public:
 
@@ -285,7 +285,7 @@ class SourceView: public RefObject
 
     SourceManager* m_sourceManager;     /// Get the manager this belongs to 
     SourceRange m_range;                ///< The range that this SourceView applies to
-    RefPtr<SourceFile> m_sourceFile;    ///< The source file can hold the line breaks
+    SourceFile* m_sourceFile;           ///< The source file. Can hold the line breaks
     List<Entry> m_entries;              ///< An array entries describing how we should interpret a range, starting from the start location. 
 };
 
@@ -298,9 +298,9 @@ struct SourceManager
     SourceRange allocateSourceRange(UInt size);
 
         /// Create a SourceFile defined with the specified path, and content held within a blob
-    RefPtr<SourceFile> createSourceFileWithSize(const PathInfo& pathInfo, size_t contentSize);
-    RefPtr<SourceFile> createSourceFileWithString(const PathInfo& pathInfo, const String& contents);
-    RefPtr<SourceFile> createSourceFileWithBlob(const PathInfo& pathInfo, ISlangBlob* blob);
+    SourceFile* createSourceFileWithSize(const PathInfo& pathInfo, size_t contentSize);
+    SourceFile* createSourceFileWithString(const PathInfo& pathInfo, const String& contents);
+    SourceFile* createSourceFileWithBlob(const PathInfo& pathInfo, ISlangBlob* blob);
 
         /// Get the humane source location
     HumaneSourceLoc getHumaneLoc(SourceLoc loc, SourceLocType type = SourceLocType::Nominal);
@@ -348,6 +348,7 @@ struct SourceManager
     SourceManager() :
         m_memoryArena(2048)
     {}
+    ~SourceManager();
 
     protected:
 
@@ -362,16 +363,19 @@ struct SourceManager
     // The location to be used by the next source file to be loaded
     SourceLoc m_nextLoc;
 
-    // All of the SourceViews. These are held in increasing order of range, so can find by doing a binary chop.
-    List<RefPtr<SourceView> > m_sourceViews;                
+    // All of the SourceViews constructed on this SourceManager. These are held in increasing order of range, so can find by doing a binary chop.
+    List<SourceView*> m_sourceViews;
+    // All of the SourceFiles constructed on this SourceManager. This owns the SourceFile.
+    List<SourceFile*> m_sourceFiles;
+
     StringSlicePool m_slicePool;
 
     // Memory arena that can be used for holding data to held in scope as long as the Source is
-    // Can be used for storing the decoded contents of Token.Content for exampel
+    // Can be used for storing the decoded contents of Token. Content for example.
     MemoryArena m_memoryArena;
 
     // Maps canonical paths to source files
-    Dictionary<String, RefPtr<SourceFile> > m_sourceFiles;  
+    Dictionary<String, SourceFile*> m_sourceFileMap;  
 };
 
 } // namespace Slang