Improve the error reporting in the compiler server client.

There are two scenarios which could be better. First, if the client hits an unrecoverable error we need as much information as possible to debug it. The solution to this is stop catching our fatal exceptions and instead let them bubble out of the process, much like our exception fail fast.

Second, if the problem is that the compiler itself has crashed, we should report that error to the user and tell them where they can get more information about the crash (i.e., the event log). (changeset 1390397)

Src/Compilers/Core/NativeClient/NativeClient.vcxproj

@@ -185,7 +185,7 @@
     <ClCompile Include="native_client.cpp" />
     <ClCompile Include="pipe_utils.cpp" />
     <ClCompile Include="protocol.cpp" />
-    <ClCompile Include="run_csc.cpp" />
+    <ClCompile Include="run_inproc_compiler.cpp" />
     <ClCompile Include="satellite.cpp" />
     <ClCompile Include="smart_resources.cpp" />
     <ClCompile Include="stdafx.cpp">

Src/Compilers/Core/NativeClient/UIStrings.h

 //{{NO_DEPENDENCIES}}

Src/Compilers/Core/NativeClient/UIStrings.rc

 // Microsoft Visual C++ generated resource script.

Src/Compilers/Core/NativeClient/logging.cpp

@@ -189,6 +189,11 @@ void FailWithGetLastError(_In_z_ LPCWSTR optionalPrefix)
     throw FatalError(move(buffer));
 }
 
+void FailWithGetLastError(_In_ const wstring& optionalPrefix)
+{
+    FailWithGetLastError(optionalPrefix.c_str());
+}
+
 void FailFormatted(UINT loadResource, ...)
 {
     va_list varargs;

Src/Compilers/Core/NativeClient/logging.h

@@ -29,5 +29,6 @@ void LogWin32Error(UINT loadResource);
 void LogWin32Error(_In_z_ LPCWSTR message);
 void FailWithGetLastError(UINT loadResource);
 void FailWithGetLastError(_In_z_ LPCWSTR optionalPrefix = nullptr);
+void FailWithGetLastError(_In_ const std::wstring& optionalPrefix);
 void FailFormatted(UINT loadResource, ...);
 void FailFormatted(_In_z_ LPCWSTR message, ...);

Src/Compilers/Core/NativeClient/native_client.cpp

@@ -10,6 +10,12 @@
 #include "satellite.h"
 #include "UIStrings.h"
 
+int RunInProcCompiler(
+    _In_ const std::wstring& processPath,
+    _In_ const std::list<std::wstring> args,
+    _Out_ std::vector<BYTE>& stdOut,
+    _Out_ std::vector<BYTE>& stdErr);
+
 // This is small, native code executable which opens a named pipe 
 // to the compiler server to do the actual compilation. It is a native code
 // executable because the entire point is to start fast, and then use the 
@@ -56,9 +62,11 @@ wstring GetCurrentDirectory()
 
 // Returns the arguments passed to the executable (including
 // the executable name)
-std::unique_ptr<LPCWSTR, decltype(&::LocalFree)> GetCommandLineArgs(_Out_ int& argsCount)
+std::unique_ptr<LPCWSTR, decltype(&::LocalFree)> GetCommandLineArgs(
+    _In_z_ LPCWSTR rawArgs,
+    _Out_ int& argsCount)
 {
-    auto args = const_cast<LPCWSTR*>(CommandLineToArgvW(GetCommandLine(), &argsCount));
+    auto args = const_cast<LPCWSTR*>(CommandLineToArgvW(rawArgs, &argsCount));
     if (args == nullptr)
     {
         FailWithGetLastError(IDS_CommandLineToArgvWFailed);
@@ -133,7 +141,7 @@ bool GetExpectedProcessPath(
     processPath.clear();
     processPath.resize(MAX_PATH);
     while (GetModuleFileNameW(NULL, &processPath[0],
-		static_cast<DWORD>(processPath.size())) == 0)
+        static_cast<DWORD>(processPath.size())) == 0)
     {
         if (GetLastError() == ERROR_INSUFFICIENT_BUFFER) 
         {
@@ -607,10 +615,6 @@ bool TryRunServerCompilation(
     return false;
 }
 
-int RunCsc(
-    _In_ const wstring& processPath,
-    _In_ const list<wstring> args);
-
 bool ProcessSlashes(_Inout_ WCHAR * & outBuffer, _Inout_ LPCWSTR * pszCur)
 {
     // All this weird slash stuff follows the standard argument processing routines
@@ -755,71 +759,101 @@ void SetPreferredUILangForMessages(LPCWSTR rawCommandLineArgs[], int argsCount,
     }
 }
 
-int Run(RequestLanguage language)
+// Shared helper for compilation.
+// If printOutput is true then the output will be directly printed to stdout
+// and stderr. Otherwise, it will be returned through the out parameters.
+// If print output is true the value in the output parameters is undefined.
+int Run(_In_ RequestLanguage language,
+        _In_ LPCWSTR cmdLineString,
+        _In_ LPCWSTR currentDirectory,
+        _In_ LPCWSTR libDirectory)
 {
-    try
+    int exitCode = 1;
+    LPCWSTR uiDllname = L"vbcsc2ui.dll";
+    LPCWSTR clientExeName = language == RequestLanguage::CSHARPCOMPILE
+        ? L"csc.exe" : L"vbc.exe";
+
+    int commandLineCount;
+    auto commandLine = GetCommandLineArgs(cmdLineString, commandLineCount);
+    auto args = commandLine.get();
+    auto argsCount = commandLineCount;
+
+    // Omit process name
+    args += 1;
+    argsCount -= 1;
+
+    g_hinstMessages = GetMessageDll(uiDllname);
+
+    if (!g_hinstMessages)
+    {
+        // Fall back to this module if none was found.
+        g_hinstMessages = GetModuleHandle(NULL);
+    }
+    wstring libEnvVariable;
+
+    // Change stderr, stdout to binary, because the output we get from the server already 
+    // has CR and LF in it. If we don't do this, we get CR CR LF at each newline.
+    (void)_setmode(_fileno(stdout), _O_BINARY);
+    (void)_setmode(_fileno(stderr), _O_BINARY);
+
+    // Process the /preferreduilang switch and refetch the resource dll
+    SetPreferredUILangForMessages(
+        args,
+        argsCount,
+        uiDllname);
+
+    // Get the args without the native client-specific arguments
+    list<wstring> argsList(args, args + argsCount);
+    wstring keepAlive;
+    // Throws FatalError if parsing fails
+    ParseAndValidateClientArguments(argsList, keepAlive);
+
+    // Try to use the compiler server
+    CompletedResponse response;
+    if (TryRunServerCompilation(
+        language,
+        currentDirectory,
+        argsList,
+        keepAlive,
+        libDirectory,
+        response))
+    {
+        exitCode = response.ExitCode;
+        OutputResponse(response);
+    }
+    else
     {
-        LPCWSTR uiDllname = L"vbcsc2ui.dll";
-        LPCWSTR clientExeName = language == RequestLanguage::CSHARPCOMPILE
-            ? L"csc.exe" : L"vbc.exe";
-        g_hinstMessages = GetMessageDll(uiDllname);
-
-        if (!g_hinstMessages)
+        // Fallback to csc.exe
+        wstring processPath;
+        if (!GetExpectedProcessPath(clientExeName, processPath))
         {
-            // Fall back to this module if none was found.
-            g_hinstMessages = GetModuleHandle(NULL);
+            FailWithGetLastError(GetResourceString(IDS_ConnectToInProcCompilerFailed));
         }
+        vector<BYTE> rawOutOutput, rawErrOutput;
+        exitCode = RunInProcCompiler(processPath, argsList, rawOutOutput, rawErrOutput);
 
-        auto currentDirectory = GetCurrentDirectory();
-        int commandLineCount;
-        auto commandLine = GetCommandLineArgs(commandLineCount);
-        // Omit process name
-        auto rawArgs = commandLine.get() + 1;
-        auto rawArgsCount = commandLineCount - 1;
-        wstring libEnvVariable;
-
-        // Change stderr, stdout to binary, because the output we get from the server already 
-        // has CR and LF in it. If we don't do this, we get CR CR LF at each newline.
-        (void)_setmode(_fileno(stdout), _O_BINARY);
-        (void)_setmode(_fileno(stderr), _O_BINARY);
-
-        // Process the /preferreduilang switch and refetch the resource dll
-        SetPreferredUILangForMessages(
-            rawArgs,
-            rawArgsCount,
-            uiDllname);
-
-        // Get the args without the native client-specific arguments
-        list<wstring> argsList(rawArgs, rawArgs + rawArgsCount);
-        wstring keepAlive;
-        // Throws FatalError if parsing fails
-        ParseAndValidateClientArguments(argsList, keepAlive);
-
-        // Try to use the compiler server
-        CompletedResponse response;
-        if (TryRunServerCompilation(
-                language,
-                currentDirectory.c_str(),
-                argsList,
-                keepAlive,
-                GetEnvVar(L"LIB", libEnvVariable) ? libEnvVariable.c_str() : nullptr,
-                response))
+        if (exitCode != 0 && rawOutOutput.empty() && rawErrOutput.empty())
         {
-            OutputResponse(response);
-            return response.ExitCode;
+            OutputWideString(stderr, GetResourceString(IDS_ExceptionFilterCrash), true);
+            exitCode = -1;
         }
-
-        // Fallback to csc.exe
-        wstring processPath;
-        if (!GetExpectedProcessPath(clientExeName, processPath))
+        else
         {
-            throw FatalError(GetResourceString(IDS_CreateClientProcessFailed));
+            fwrite(rawOutOutput.data(), sizeof(BYTE), rawOutOutput.size(), stdout);
+            fwrite(rawErrOutput.data(), sizeof(BYTE), rawErrOutput.size(), stderr);
         }
-        return RunCsc(processPath, argsList);
     }
-    catch (FatalError &e)
-    {
-        OutputWideString(stderr, e.message, true);
-    }
-    return 1;
+    return exitCode;
 }
+
+int Run(RequestLanguage language)
+{
+    auto currentDirectory = GetCurrentDirectory();
+    wstring libEnvVariable;
+
+    return Run(
+        language,
+        GetCommandLineW(),
+        currentDirectory.c_str(),
+        GetEnvVar(L"LIB", libEnvVariable) ? libEnvVariable.c_str() : nullptr);
+}
\ No newline at end of file

Src/Compilers/Core/NativeClient/native_client.h

@@ -9,4 +9,4 @@ int Run(RequestLanguage language);
 
 void ParseAndValidateClientArguments(
     _Inout_ list<wstring>& arguments,
-    _Out_ wstring& keepAliveValue);
+    _Out_ wstring& keepAliveValue);
\ No newline at end of file

Src/Compilers/Core/NativeClient/run_csc.cpp → Src/Compilers/Core/NativeClient/run_inproc_compiler.cpp

@@ -5,36 +5,30 @@
 #include "smart_resources.h"
 #include <sstream>
 
-int RunCsc(
+void ReadOutput(_In_ HANDLE outHandle, _Out_ vector<BYTE>& output);
+
+int RunInProcCompiler(
     _In_ const wstring& processPath,
-    _In_ const list<wstring> args)
+    _In_ const list<wstring> args,
+    _Out_ vector<BYTE>& stdOut,
+    _Out_ vector<BYTE>& stdErr)
 {
     SECURITY_ATTRIBUTES attr;
     attr.nLength = sizeof(SECURITY_ATTRIBUTES);
     attr.bInheritHandle = TRUE;
     attr.lpSecurityDescriptor = NULL;
 
-    // Get stdout and stderr to pass to csc.exe
-    auto stdOut = GetStdHandle(STD_OUTPUT_HANDLE);
+    // Get stdin to pass to csc.exe
     auto stdIn = GetStdHandle(STD_INPUT_HANDLE);
-    auto stdErr = GetStdHandle(STD_ERROR_HANDLE);
 
-    // Duplicate the handles for inheritance
-    HANDLE outDup;
+    // Create handles for the child to write to and the parent to read from
+    HANDLE stdOutRead;
+    HANDLE stdOutWrite;
+    HANDLE stdErrRead;
+    HANDLE stdErrWrite;
     HANDLE inDup;
-    HANDLE errDup;
 
     auto thisHandle = GetCurrentProcess();
-
-    DuplicateHandle(
-        thisHandle,
-        stdOut,
-        thisHandle,
-        &outDup,
-        0,
-        TRUE,
-        DUPLICATE_SAME_ACCESS);
-
     DuplicateHandle(
         thisHandle,
         stdIn,
@@ -44,23 +38,35 @@ int RunCsc(
         TRUE,
         DUPLICATE_SAME_ACCESS);
 
-    DuplicateHandle(
-        thisHandle,
-        stdErr,
-        thisHandle,
-        &errDup,
-        0,
-        TRUE,
-        DUPLICATE_SAME_ACCESS);
+    if (!CreatePipe(&stdOutRead, &stdOutWrite, &attr, 0))
+    {
+        FailWithGetLastError(GetResourceString(IDS_ConnectToInProcCompilerFailed));
+    }
+
+    if (!CreatePipe(&stdErrRead, &stdErrWrite, &attr, 0))
+    {
+        FailWithGetLastError(GetResourceString(IDS_ConnectToInProcCompilerFailed));
+    }
+
+    // Mark the read end of the pipes non-inheritable
+    if (!SetHandleInformation(stdOutRead, HANDLE_FLAG_INHERIT, 0))
+    {
+        FailWithGetLastError(GetResourceString(IDS_ConnectToInProcCompilerFailed));
+    }
+
+    if (!SetHandleInformation(stdErrRead, HANDLE_FLAG_INHERIT, 0))
+    {
+        FailWithGetLastError(GetResourceString(IDS_ConnectToInProcCompilerFailed));
+    }
 
     PROCESS_INFORMATION procInfo = {};
     STARTUPINFO startInfo = {};
     BOOL success = FALSE;
 
     startInfo.cb = sizeof(STARTUPINFO);
-    startInfo.hStdOutput = outDup;
+    startInfo.hStdOutput = stdOutWrite;
     startInfo.hStdInput = inDup;
-    startInfo.hStdError = errDup;
+    startInfo.hStdError = stdErrWrite;
     startInfo.dwFlags |= STARTF_USESTDHANDLES;
 
     // Assemble the command line
@@ -83,7 +89,6 @@ int RunCsc(
     wcscpy_s(argsCpy.get(), argsString.size() + 1, argsString.c_str());
 
     // Create the child process. 
-
     success = CreateProcess(NULL,
         argsCpy.get(),     // command line 
         NULL,          // process security attributes 
@@ -95,8 +100,16 @@ int RunCsc(
         &startInfo,  // STARTUPINFO pointer 
         &procInfo);  // receives PROCESS_INFORMATION 
 
+    CloseHandle(stdOutWrite);
+    CloseHandle(inDup);
+    CloseHandle(stdErrWrite);
+
     if (success)
     {
+        // Read stdout and stderr from the process
+        ReadOutput(stdOutRead, stdOut);
+        ReadOutput(stdErrRead, stdErr);
+
         // Wait for the process to exit and return the exit code
         LogFormatted(IDS_CreatedProcess, procInfo.dwProcessId);
         WaitForSingleObject(procInfo.hProcess, INFINITE);
@@ -108,10 +121,37 @@ int RunCsc(
         CloseHandle(procInfo.hProcess);
         CloseHandle(procInfo.hThread);
 
+        CloseHandle(stdOutRead);
+        CloseHandle(stdErrRead);
+
         return exitCode;
     }
     else
     {
-        throw FatalError(GetResourceString(IDS_CreateClientProcessFailed));
+        CloseHandle(stdOutRead);
+        CloseHandle(stdErrRead);
+
+        FailWithGetLastError(GetResourceString(IDS_CreatingProcess));
+        // Unreachable
+        return -1;
+    }
+}
+
+void ReadOutput(
+    _In_ HANDLE handle,
+    _Out_ vector<BYTE>& output)
+{
+    DWORD read;
+    const int bufSize = 4096;
+    BYTE buf[bufSize];
+    BOOL success = false;
+    output.clear();
+
+    for (;;)
+    {
+        success = ReadFile(handle, buf, bufSize, &read, nullptr);
+        if (!success || !read)
+            break;
+        output.insert(output.end(), buf, buf + read);
     }
 }

Src/Compilers/Core/VBCSCompilerTests/CompilerServerTests.cs

@@ -300,6 +300,82 @@ static void Main()
             VerifyResultAndOutput(result, tempDirectory, "Hello, world.\r\n");
         }
 
+        [Fact]
+        public void CscFallBackOutputNoUtf8()
+        {
+            var files = new Dictionary<string, string> { { "hello.cs", "♕" } };
+
+            // Delete VBCSCompiler.exe so csc2 is forced to fall back to csc.exe
+            File.Delete(CompilerServerExecutable);
+            var result = RunCommandLineCompiler(CSharpCompilerClientExecutable, "/nologo hello.cs", tempDirectory, files);
+            Assert.Equal(result.ExitCode, 1);
+            Assert.True(result.ContainsErrors);
+            Assert.Equal("hello.cs(1,1): error CS1056: Unexpected character '?'", result.Output.Trim());
+        }
+
+        [Fact]
+        public void CscFallBackOutputUtf8()
+        {
+            var srcFile = tempDirectory.CreateFile("test.cs").WriteAllText("♕").Path;
+            var tempOut = tempDirectory.CreateFile("output.txt");
+
+            // Delete VBCSCompiler.exe so csc2 is forced to fall back to csc.exe
+            File.Delete(CompilerServerExecutable);
+
+            var result = ProcessLauncher.Run("cmd",
+                string.Format("/C {0} /utf8output /nologo /t:library {1} > {2}",
+                CSharpCompilerClientExecutable,
+                srcFile, tempOut.Path));
+
+            Assert.Equal("", result.Output.Trim());
+            Assert.Equal("test.cs(1,1): error CS1056: Unexpected character '♕'".Trim(),
+                tempOut.ReadAllText().Trim().Replace(srcFile, "test.cs"));
+            Assert.Equal(1, result.ExitCode);
+        }
+
+        [Fact]
+        public void VbcFallbackNoUtf8()
+        {
+            var srcFile = tempDirectory.CreateFile("test.vb").WriteAllText("♕").Path;
+
+            // Delete VBCSCompiler.exe so csc2 is forced to fall back to csc.exe
+            File.Delete(CompilerServerExecutable);
+
+            var result = ProcessLauncher.Run(
+                BasicCompilerClientExecutable,
+                "/nologo test.vb",
+                tempDirectory.Path);
+
+            Assert.Equal(result.ExitCode, 1);
+            Assert.True(result.ContainsErrors);
+            Assert.Equal(@"test.vb(1) : error BC30037: Character is not valid.
+
+?
+~", result.Output.Trim().Replace(srcFile, "test.vb"));
+        }
+
+        [Fact]
+        public void VbcFallbackUtf8()
+        {
+            var srcFile = tempDirectory.CreateFile("test.vb").WriteAllText("♕").Path;
+            var tempOut = tempDirectory.CreateFile("output.txt");
+
+            // Delete VBCSCompiler.exe so csc2 is forced to fall back to csc.exe
+            File.Delete(CompilerServerExecutable);
+
+            var result = ProcessLauncher.Run("cmd",
+                string.Format("/C {0} /utf8output /nologo /t:library {1} > {2}",
+                BasicCompilerClientExecutable,
+                srcFile, tempOut.Path));
+
+            Assert.Equal("", result.Output.Trim());
+            Assert.Equal(@"test.vb(1) : error BC30037: Character is not valid.
+
+♕
+~", tempOut.ReadAllText().Trim().Replace(srcFile, "test.vb"));
+            Assert.Equal(1, result.ExitCode);
+        }
+
         [Fact]
         public void FallbackToVbc()
         {
@@ -1705,7 +1781,7 @@ public void Utf8OutputInRspFileVbc()
             Assert.Equal(1, result.ExitCode);
         }
 
-        [ConditionalFact(typeof(RunKeepAliveTests))]
+        [Fact]
         public async Task ServerRespectsAppConfig()
         {
             var exeConfigPath = Path.Combine(compilerDirectory, CompilerServerExeName + ".config");