Downstream option handling (#1850)

* #include an absolute path didn't work - because paths were taken to always be relative.

* Added SourceLoc handling for command line parsing.

* Fix typo in debug.

* Fix issue around the DiagnosticSink used in options parsing not having a writer available - by having DiagnosticSink parenting.

* Small rename for clarity.

* WIP extracting command line args for downstream tools.

* Unit tests/bug fixes around extracting args.

* Use DownstreamArgs in the EndToEndCompileRequest

* Passing downstream compiler options downstream.

* Fix issue with endToEndReq being nullptr.

* Fix issue with diagnostics number change.

* Small improvements to how the source line is displayed if it's too long.
Default to 120, as suggested in previous review.

Co-authored-by: T. Foley <tfoleyNV@users.noreply.github.com>

7 files changed, 311 insertions, 36 deletions

diff --git a/source/compiler-core/slang-command-line-args.cpp b/source/compiler-core/slang-command-line-args.cpp
index da262e9bb..1d452bb10 100644
--- a/source/compiler-core/slang-command-line-args.cpp
+++ b/source/compiler-core/slang-command-line-args.cpp
@@ -11,7 +11,9 @@ void CommandLineArgs::setArgs(const char*const* args, size_t argCount)
 {
     m_args.clear();
 
-    const SourceLoc startLoc = m_sourceManager->getNextRangeStart();
+    SourceManager* sourceManager = m_context->getSourceManager();
+
+    const SourceLoc startLoc = sourceManager->getNextRangeStart();
 
     StringBuilder buf;
    
@@ -36,12 +38,32 @@ void CommandLineArgs::setArgs(const char*const* args, size_t argCount)
         buf << " ";
     }
 
-    SourceFile* sourceFile = m_sourceManager->createSourceFileWithString(PathInfo::makeUnknown(), buf.ProduceString());
-    m_sourceView = m_sourceManager->createSourceView(sourceFile, nullptr, SourceLoc::fromRaw(0));
+    SourceFile* sourceFile = sourceManager->createSourceFileWithString(PathInfo::makeUnknown(), buf.ProduceString());
+    SourceView* sourceView = sourceManager->createSourceView(sourceFile, nullptr, SourceLoc::fromRaw(0));
 
-    SLANG_ASSERT(m_sourceView->getRange().begin == startLoc);
+    SLANG_UNUSED(sourceView);
+    SLANG_ASSERT(sourceView->getRange().begin == startLoc);
 }
 
+bool CommandLineArgs::hasArgs(const char*const* args, Index count) const
+{
+    if (m_args.getCount() != count)
+    {
+        return false;
+    }
+
+    for (Index i = 0; i < count; ++i)
+    {
+        if (m_args[i].value != args[i])
+        {
+            return false;
+        }
+    }
+
+    return true;
+}
+
+
 /* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 
                          CommandLineReader
@@ -91,5 +113,162 @@ SlangResult CommandLineReader::expectArg(CommandLineArg& outArg)
     }
 }
 
+/* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+
+                         DownstreamArgs
+
+!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! */
+
+Index DownstreamArgs::addName(const String& name)
+{
+    Index index = m_names.indexOf(name);
+    if (index < 0)
+    {
+        index = m_names.getCount();
+        m_names.add(name);
+
+        CommandLineArgs args(m_context);
+        m_args.add(args);
+    }
+    return index;
+}
+
+Index DownstreamArgs::_findOrAddName(SourceLoc loc, const UnownedStringSlice& name, Flags flags, DiagnosticSink* sink)
+{
+    if (name.getLength() <= 0)
+    {
+        sink->diagnose(loc, MiscDiagnostics::downstreamToolNameNotDefined);
+        return -1;
+    }
+
+    if (flags & Flag::AllowNewNames)
+    {
+        return addName(name);
+    }
+
+    Index index = findName(name);
+    if (index >= 0)
+    {
+        return index;
+    }
+
+    sink->diagnose(loc, MiscDiagnostics::downstreamNameNotKnown);
+    return -1;
+}
+
+CommandLineArgs& DownstreamArgs::getArgsByName(char* name)
+{
+    Index index = findName(name);
+    SLANG_ASSERT(index >= 0);
+    return m_args[index];
+}
+
+SlangResult DownstreamArgs::stripDownstreamArgs(CommandLineArgs& ioArgs, Flags flags, DiagnosticSink* sink)
+{
+    CommandLineReader reader(&ioArgs, sink);
+
+    while (reader.hasArg())
+    {
+        const CommandLineArg& arg = reader.peekArg();
+
+        if (arg.value.startsWith("-X"))
+        {
+            if (arg.value.endsWith("..."))
+            {
+                const UnownedStringSlice name = arg.value.getUnownedSlice().subString(2, arg.value.getLength() - 5);
+                const Index nameIndex = _findOrAddName(arg.loc, name, flags, sink);
+                if (nameIndex < 0)
+                {
+                    return SLANG_FAIL;
+                }
+
+                Index depth = 1;
+                const Index startIndex = reader.getIndex();
+
+                Int index = startIndex + 1;
+                const Int count = ioArgs.m_args.getCount();
+
+                for (; index < count; ++index)
+                {
+                    const auto& curArg = ioArgs.m_args[index];
+
+                    if (curArg.value == "-X.")
+                    {
+                        depth--;
+                        // If we are at end of scope we are done
+                        if (depth <= 0)
+                        {
+                            break;
+                        }
+                    }
+                    else if (curArg.value.startsWith("-X") && curArg.value.endsWith("..."))
+                    {
+                        depth++;
+                    }
+                }
+
+                // We don't care if its 1, as we allow the main scope to be left open
+                if (depth > 1)
+                {
+                    sink->diagnose(arg.loc, MiscDiagnostics::unbalancedDownstreamArguments);
+                    return SLANG_FAIL;
+                }
+
+                // We are either at end of scope or at end of list
+                SLANG_ASSERT(depth <= 0 || index >= count);
+
+                // Add all of these args
+                CommandLineArgs& args = getArgsAt(nameIndex);
+
+                // Copy the values in the range
+                args.m_args.addRange(ioArgs.m_args.getBuffer() + startIndex + 1, index - (startIndex + 1));
+
+                // If we aren't at the end, we must be pointing to -X., so skip that
+                index += Index(index < count);
+                // Remove the range. The readers position, needs to be fixed though
+                ioArgs.m_args.removeRange(startIndex, index - startIndex);
+
+                // The reader should be at startIndex, and so doesn't need fixing
+                SLANG_ASSERT(reader.getIndex() == startIndex);
+            }
+            else if (arg.value == "-X.")
+            {
+                sink->diagnose(arg.loc, MiscDiagnostics::closeOfUnopenDownstreamArgs);
+                return SLANG_FAIL;
+            }
+            else
+            {
+                const Index startIndex = reader.getIndex();
+
+                // Extract the name
+                UnownedStringSlice name = arg.value.getUnownedSlice().tail(2);
+                const Index nameIndex = _findOrAddName(arg.loc, name, flags, sink);
+                if (nameIndex < 0)
+                {
+                    return SLANG_FAIL;
+                }
+
+                reader.advance();
+
+                CommandLineArg nextArg;
+                SLANG_RETURN_ON_FAIL(reader.expectArg(nextArg));
+
+                getArgsAt(nameIndex).add(nextArg);
+
+                // Rewind to the start index
+                reader.setIndex(startIndex);
+                // Remove the args
+                ioArgs.m_args.removeRange(startIndex, 2);
+            }
+        }
+        else
+        {
+            // Advance and leave
+            reader.advance();
+        }
+    }
+
+    return SLANG_OK;
+}
 
 } // namespace Slang
diff --git a/source/compiler-core/slang-command-line-args.h b/source/compiler-core/slang-command-line-args.h
index 6dea7408c..7500c1a91 100644
--- a/source/compiler-core/slang-command-line-args.h
+++ b/source/compiler-core/slang-command-line-args.h
@@ -17,6 +17,23 @@ struct CommandLineArg
     SourceLoc loc;          ///< The location of the arg
 };
 
+class CommandLineContext : public RefObject
+{
+public:        
+        /// Get the source manager
+    SourceManager* getSourceManager() { return &m_sourceManager; }
+
+    CommandLineContext(ISlangFileSystemExt* fileSystemExt = nullptr)
+    {
+        m_sourceManager.initialize(nullptr, fileSystemExt);
+        // Make range start from high value, so can be differentiated from other uses
+        m_sourceManager.allocateSourceRange(~(~SourceLoc::RawValue(0) >> 1));
+    }
+
+protected:
+    SourceManager m_sourceManager;
+};
+
 struct CommandLineArgs
 {
     typedef CommandLineArg Arg;
@@ -30,18 +47,23 @@ struct CommandLineArgs
         /// NOTE! Should NOT include the executable name
     void setArgs(const char*const* args, size_t argCount);
 
-        /// Ctor with a source manager
-    CommandLineArgs(SourceManager* manager):
-        m_sourceManager(manager),
-        m_sourceView(nullptr)
+        /// True if has args in same order
+    bool hasArgs(const char*const* args, Index count) const;
+
+        /// Add an arg
+    void add(const Arg& arg) { m_args.add(arg); }
+
+        /// Ctor with a context
+    CommandLineArgs(CommandLineContext* context):
+        m_context(context)
     {
     }
+        /// Default Ctor
+    CommandLineArgs() {}
 
-    String m_executablePath;                ///< Can be optionally be set
-
+    //String m_executablePath;                ///< Can be optionally be set
     List<Arg> m_args;                       ///< The args
-    SourceManager* m_sourceManager;         ///< The source manager and associated diagnostics sink
-    SourceView* m_sourceView;        ///< contains the command line as source
+    RefPtr<CommandLineContext> m_context;   ///< The context, which mainly has source manager
 };
 
 struct CommandLineReader
@@ -77,6 +99,11 @@ struct CommandLineReader
     SlangResult expectArg(String& outArg);
     SlangResult expectArg(CommandLineArg& outArg);
 
+        /// Get the current index
+    Index getIndex() const { return m_index; }
+        /// Set the current index
+    void setIndex(Index index) { SLANG_ASSERT(index >= 0 && index <= m_args->getArgCount()); m_index = index; }
+
         /// Set up reader with args
     CommandLineReader(CommandLineArgs* args, DiagnosticSink* sink):
         m_args(args),
@@ -90,6 +117,51 @@ struct CommandLineReader
     Index m_index;
 };
 
+struct DownstreamArgs
+{
+    typedef uint32_t Flags;
+    struct Flag
+    {
+        enum Enum : Flags
+        {
+            AllowNewNames = 0x01,
+        };
+    };
+
+        /// Add a name, returns the index
+    Index addName(const String& name);
+        /// Find the index of a name. Returns < 0 if not found.
+    Index findName(const String& name) const { return m_names.indexOf(name); }
+
+        /// Get the args at the nameIndex
+    CommandLineArgs& getArgsAt(Index nameIndex) { return m_args[nameIndex]; }
+        /// Get args by name - will assert if name isn't found
+    CommandLineArgs& getArgsByName(char* name);
+
+        /// Looks for '-X' expressions, removing them from ioArgs and putting in appropriate args 
+    SlangResult stripDownstreamArgs(CommandLineArgs& ioArgs, Flags flags, DiagnosticSink* sink);
+
+        /// Get the context used
+    CommandLineContext* getContext() const { return m_context; }
+
+        /// Ctor
+    DownstreamArgs(CommandLineContext* context):
+        m_context(context)
+    {
+    }
+        /// Default ctor - for convenience, should really use with context normally
+    DownstreamArgs() {}
+
+protected:
+    Index _findOrAddName(SourceLoc loc, const UnownedStringSlice& name, Flags flags, DiagnosticSink* sink);
+
+    List<String> m_names;
+    List<CommandLineArgs> m_args;
+
+    RefPtr<CommandLineContext> m_context;
+};
+
+
 } // namespace Slang
 
 #endif
diff --git a/source/compiler-core/slang-diagnostic-sink.cpp b/source/compiler-core/slang-diagnostic-sink.cpp
index 0ad16b2b4..a6502abc9 100644
--- a/source/compiler-core/slang-diagnostic-sink.cpp
+++ b/source/compiler-core/slang-diagnostic-sink.cpp
@@ -239,10 +239,10 @@ static UnownedStringSlice _extractLineContainingPosition(const UnownedStringSlic
     return UnownedStringSlice(start, end);
 }
 
-static void _reduceLength(Index startIndex, StringBuilder& ioBuf)
+static void _reduceLength(Index startIndex, const UnownedStringSlice& prefix, StringBuilder& ioBuf)
 {
     StringBuilder buf;
-    buf << "...";
+    buf << prefix;
     buf.append(ioBuf.getUnownedSlice().tail(startIndex));
     ioBuf = buf;
 }
@@ -313,21 +313,28 @@ static void _sourceLocationNoteDiagnostic(DiagnosticSink* sink, SourceView* sour
         const Index maxLength = sink->getSourceLineMaxLength();
         if (maxLength > 0)
         {
-            Index endIndex = lexer ? caretLine.getLength() : (caretIndex + (maxLength / 4));
+            const UnownedStringSlice ellipsis = UnownedStringSlice::fromLiteral("...");
+            const UnownedStringSlice spaces = UnownedStringSlice::fromLiteral("   ");
+            SLANG_ASSERT(ellipsis.getLength() == spaces.getLength());
+
+            // We use the caretLine length if we have a lexer, because it will have underscores such that it's end is the end of
+            // the item at issue.
+            // If we don't have the lexer, we guesstimate using 1/4 of the maximum length
+            const Index endIndex = lexer ? caretLine.getLength() : (caretIndex + (maxLength / 4));
 
             if (endIndex > maxLength)
             {
-                Index startIndex = endIndex - (maxLength - 3);
+                const Index startIndex = endIndex - (maxLength - ellipsis.getLength());
 
-                _reduceLength(startIndex, sourceLine);
-                _reduceLength(startIndex, caretLine);
+                _reduceLength(startIndex, ellipsis, sourceLine);
+                _reduceLength(startIndex, spaces, caretLine);
             }
 
             if (sourceLine.getLength() > maxLength)
             {
                 StringBuilder buf;
-                buf.append(sourceLine.getUnownedSlice().head(maxLength - 3));
-                buf << "...";
+                buf.append(sourceLine.getUnownedSlice().head(maxLength - ellipsis.getLength()));
+                buf << ellipsis;
                 sourceLine = buf;
             }
         }
diff --git a/source/compiler-core/slang-diagnostic-sink.h b/source/compiler-core/slang-diagnostic-sink.h
index 09502e48a..7f7ea7c4e 100644
--- a/source/compiler-core/slang-diagnostic-sink.h
+++ b/source/compiler-core/slang-diagnostic-sink.h
@@ -258,7 +258,8 @@ protected:
     int m_internalErrorLocsNoted = 0;
 
     /// If 0, then there is no limit, otherwise max amount of chars of the source line location
-    Index m_sourceLineMaxLength = 0;            
+    /// We don't know the size of a terminal in general, but for now we'll guess 120.
+    Index m_sourceLineMaxLength = 120;            
 
     Flags m_flags = 0;
 
diff --git a/source/compiler-core/slang-downstream-compiler.h b/source/compiler-core/slang-downstream-compiler.h
index 22ad5e20c..53b378eb0 100644
--- a/source/compiler-core/slang-downstream-compiler.h
+++ b/source/compiler-core/slang-downstream-compiler.h
@@ -310,6 +310,9 @@ public:
             /// The stage being compiled for 
         SlangStage stage = SLANG_STAGE_NONE;
 
+            /// Arguments that are specific to a particular compiler implementation.
+        List<String> compilerSpecificArguments;
+
             /// NOTE! Not all downstream compilers can use the fileSystemExt/sourceManager. This option will be ignored in those scenarios.
         ISlangFileSystemExt* fileSystemExt = nullptr;
         SourceManager* sourceManager = nullptr;
diff --git a/source/compiler-core/slang-dxc-compiler.cpp b/source/compiler-core/slang-dxc-compiler.cpp
index 7e7850780..0b46bd09e 100644
--- a/source/compiler-core/slang-dxc-compiler.cpp
+++ b/source/compiler-core/slang-dxc-compiler.cpp
@@ -212,14 +212,23 @@ SlangResult DXCDownstreamCompiler::compile(const CompileOptions& options, RefPtr
         0,
         dxcSourceBlob.writeRef()));
 
-    WCHAR const* args[16];
-    UINT32 argCount = 0;
+    List<const WCHAR*> args;
+
+    // Add all compiler specific options
+    List<OSString> compilerSpecific;
+    compilerSpecific.setCount(options.compilerSpecificArguments.getCount());
+
+    for (Index i = 0; i < options.compilerSpecificArguments.getCount(); ++i)
+    {
+        compilerSpecific[i] = options.compilerSpecificArguments[i].toWString();
+        args.add(compilerSpecific[i]);
+    }
 
     // TODO: deal with
     bool treatWarningsAsErrors = false;
     if (treatWarningsAsErrors)
     {
-        args[argCount++] = L"-WX";
+        args.add(L"-WX");
     }
 
     switch (options.matrixLayout)
@@ -228,7 +237,7 @@ SlangResult DXCDownstreamCompiler::compile(const CompileOptions& options, RefPtr
             break;
 
         case SLANG_MATRIX_LAYOUT_ROW_MAJOR:
-            args[argCount++] = L"-Zpr";
+            args.add(L"-Zpr");
             break;
     }
 
@@ -238,7 +247,7 @@ SlangResult DXCDownstreamCompiler::compile(const CompileOptions& options, RefPtr
             break;
 
         case FloatingPointMode::Precise:
-            args[argCount++] = L"-Gis"; // "force IEEE strictness"
+            args.add(L"-Gis"); // "force IEEE strictness"
             break;
     }
 
@@ -247,10 +256,10 @@ SlangResult DXCDownstreamCompiler::compile(const CompileOptions& options, RefPtr
         default:
             break;
 
-        case OptimizationLevel::None:       args[argCount++] = L"-Od"; break;
-        case OptimizationLevel::Default:    args[argCount++] = L"-O1"; break;
-        case OptimizationLevel::High:       args[argCount++] = L"-O2"; break;
-        case OptimizationLevel::Maximal:    args[argCount++] = L"-O3"; break;
+        case OptimizationLevel::None:       args.add(L"-Od"); break;
+        case OptimizationLevel::Default:    args.add(L"-O1"); break;
+        case OptimizationLevel::High:       args.add(L"-O2"); break;
+        case OptimizationLevel::Maximal:    args.add(L"-O3"); break;
     }
 
     switch (options.debugInfoType)
@@ -259,7 +268,7 @@ SlangResult DXCDownstreamCompiler::compile(const CompileOptions& options, RefPtr
             break;
 
         default:
-            args[argCount++] = L"-Zi";
+            args.add(L"-Zi");
             break;
     }
 
@@ -278,14 +287,14 @@ SlangResult DXCDownstreamCompiler::compile(const CompileOptions& options, RefPtr
     // work on mainline Clang. Thus the only option we have available
     // is the big hammer of turning off *all* warnings coming from dxc.
     //
-    args[argCount++] = L"-no-warnings";
+    args.add(L"-no-warnings");
 
     OSString wideEntryPointName = options.entryPointName.toWString();
     OSString wideProfileName = options.profileName.toWString();
 
     if (options.flags & CompileOptions::Flag::EnableFloat16)
     {
-        args[argCount++] = L"-enable-16bit-types";
+        args.add(L"-enable-16bit-types");
     }
 
     SearchDirectoryList searchDirectories;
@@ -303,8 +312,8 @@ SlangResult DXCDownstreamCompiler::compile(const CompileOptions& options, RefPtr
         sourcePath.begin(),
         wideEntryPointName.begin(),
         wideProfileName.begin(),
-        args,
-        argCount,
+        args.getBuffer(),
+        UINT32(args.getCount()),
         nullptr,            // `#define`s
         0,                  // `#define` count
         &includeHandler,    // `#include` handler
diff --git a/source/compiler-core/slang-misc-diagnostic-defs.h b/source/compiler-core/slang-misc-diagnostic-defs.h
index df31b9f01..e5c7c335e 100644
--- a/source/compiler-core/slang-misc-diagnostic-defs.h
+++ b/source/compiler-core/slang-misc-diagnostic-defs.h
@@ -24,7 +24,11 @@
 
 DIAGNOSTIC(-1, Note, seeTokenPasteLocation, "see token pasted location")
 
-DIAGNOSTIC(21, Error, expectedArgumentForOption, "expected an argument for command-line option '$0'")
+DIAGNOSTIC(100000, Error, downstreamNameNotKnown, "downstream tool name not known")
+DIAGNOSTIC(100001, Error, expectedArgumentForOption, "expected an argument for command-line option '$0'")
+DIAGNOSTIC(100002, Error, unbalancedDownstreamArguments, "unbalanced downstream arguments")
+DIAGNOSTIC(100003, Error, closeOfUnopenDownstreamArgs, "close of an unopen downstream argument scope")
+DIAGNOSTIC(100004, Error, downstreamToolNameNotDefined, "downstream tool name not defined")
 
 DIAGNOSTIC(99999, Note, noteLocationOfInternalError, "an internal error threw an exception while working on code near this location")