[Support] Enable color diagnostics for mintty

[mstorsjo]

This doesn't look like it is matching the normal coding style - use clang-format(-diff) to get the proper style.

I can't comment much on the changes themselves though.

[SquallATF]

• run clang-format

[kristina]

Could you please explain the purpose of this patch, since mintty (I use it myself though as as the frontend for wslbridge) is not a native Windows console in any shape or form, unless I'm missing something? In either case this shouldn't really be necessary unless your setup involves running LLVM tools on Windows through mintty through Windows console.

As far as I understand, mintty is a xterm-256color compatible terminal emulator, so I'm not entirely sure why you're using Windows console APIs either?

If I may ask, could you write up a detailed explanation of what this patch is aiming to achieve and what specific problem does it solve?

[kristina]

Another major problem I see is the difficulty in adding a test for this specific patch, due to its complicated nature this would require a fairly complex unit test that mocks that specific aspect of mintty, which has not been provided either. Given that this will be linked in by all Windows tools, and the complexity of the patch, I think at the very least this should provide said test.

Let me add that the maintenance burden added by this is fairly significant as it depends on compatibility with a project unrelated to LLVM, very specific naming conventions and the use of native APIs to use the named pipe and should it change, the code will stop working as well as potentially cause regressions for Cygwin/MSYS based toolchains (as they use mintty as well)? So at the very least, this very least this should require a test.

Personally I don't think this should make it into the support library at all, I can see a lot of things going wrong here requiring reverting this. I'm adding @rnk as a reviewer as well, as he is more qualified in this area than me, and I think his opinion would be of high value here (regarding the cost-benefit aspect of this patch, even if other comments are addressed).

[SquallATF]

• rewrite detect mintty, port from vim.

[SquallATF]

• limit this change for mingw only

[rnk]

In D53567#1273729, @kristina wrote:

Another major problem I see is the difficulty in adding a test for this specific patch, due to its complicated nature this would require a fairly complex unit test that mocks that specific aspect of mintty, which has not been provided either. Given that this will be linked in by all Windows tools, and the complexity of the patch, I think at the very least this should provide said test.

Let me add that the maintenance burden added by this is fairly significant as it depends on compatibility with a project unrelated to LLVM, very specific naming conventions and the use of native APIs to use the named pipe and should it change, the code will stop working as well as potentially cause regressions for Cygwin/MSYS based toolchains (as they use mintty as well)? So at the very least, this very least this should require a test.

Personally I don't think this should make it into the support library at all, I can see a lot of things going wrong here requiring reverting this. I'm adding @rnk as a reviewer as well, as he is more qualified in this area than me, and I think his opinion would be of high value here (regarding the cost-benefit aspect of this patch, even if other comments are addressed).

Thanks!

So, funny story, I wanted to do this for ninja (https://github.com/ninja-build/ninja/pull/659) and @thakis didn't like the complexity it added.

If we do decide to do this, I don't think it's the kind of thing we can reasonably test in LLVM, for the reasons @kristina mentioned. If people agree it's a valuable feature that's worth the complexity, I think we can commit it, and rely on mintty users to tell us when the colors don't end up printing.

lib/Support/Windows/Process.inc

231

This is plain old win32 API code. Why make it mingw-specific? Then clang-cl will use colors in mintty. I previously used it as my primary development environment, even for targeting MSVC. Today I use cmd, since it's improved greatly, but it's still pretty janky.

[kristina]

In D53567#1274707, @rnk wrote:

In D53567#1273729, @kristina wrote:

Another major problem I see is the difficulty in adding a test for this specific patch, due to its complicated nature this would require a fairly complex unit test that mocks that specific aspect of mintty, which has not been provided either. Given that this will be linked in by all Windows tools, and the complexity of the patch, I think at the very least this should provide said test.

Let me add that the maintenance burden added by this is fairly significant as it depends on compatibility with a project unrelated to LLVM, very specific naming conventions and the use of native APIs to use the named pipe and should it change, the code will stop working as well as potentially cause regressions for Cygwin/MSYS based toolchains (as they use mintty as well)? So at the very least, this very least this should require a test.

Personally I don't think this should make it into the support library at all, I can see a lot of things going wrong here requiring reverting this. I'm adding @rnk as a reviewer as well, as he is more qualified in this area than me, and I think his opinion would be of high value here (regarding the cost-benefit aspect of this patch, even if other comments are addressed).

Thanks!

So, funny story, I wanted to do this for ninja (https://github.com/ninja-build/ninja/pull/659) and @thakis didn't like the complexity it added.

If we do decide to do this, I don't think it's the kind of thing we can reasonably test in LLVM, for the reasons @kristina mentioned. If people agree it's a valuable feature that's worth the complexity, I think we can commit it, and rely on mintty users to tell us when the colors don't end up printing.

I think if there's interest in this and mock-based tests are too difficult, it's possible to have one of the Windows builders run a very specific test with mintty just so we can watch out if something does go wrong. That said, I think for now it should need an opt-in (possibly through -mllvm) flag, and if we do get one or more Windows builders to run it (it should be relatively lightweight as it's literally just testing the support library) and it remains stable across mintty releases, and someone maintains it (I'm up for that, can even supply a builder just for this specific, hypothetical test), the opt-in requirement could eventually be dropped in favor of an "auto" style thing just as with other color diagnostics.

I may be overthinking it, but I think that's a semi-reasonable plan for rolling this out and I think a tool-style test with FileCheck would be the most appropriate here?

Anyway, would like to hear some opinions and seeing how many are interested in such a feature and what they think of the roll-out plan I proposed, despite it sounding slightly (over-)complicated.

[SquallATF]

lib/Support/Windows/Process.inc

231

1. GetFileInformationByHandleEx will failed build on windows xp without FileExtd.lib, does llvm need support windows xp?
2. I did not want to affect MSVC users, if not necessary.

[rnk]

In D53567#1275043, @kristina wrote:

I think if there's interest in this and mock-based tests are too difficult, it's possible to have one of the Windows builders run a very specific test with mintty just so we can watch out if something does go wrong. That said, I think for now it should need an opt-in (possibly through -mllvm) flag, and if we do get one or more Windows builders to run it (it should be relatively lightweight as it's literally just testing the support library) and it remains stable across mintty releases, and someone maintains it (I'm up for that, can even supply a builder just for this specific, hypothetical test), the opt-in requirement could eventually be dropped in favor of an "auto" style thing just as with other color diagnostics.

I may be overthinking it, but I think that's a semi-reasonable plan for rolling this out and I think a tool-style test with FileCheck would be the most appropriate here?

Anyway, would like to hear some opinions and seeing how many are interested in such a feature and what they think of the roll-out plan I proposed, despite it sounding slightly (over-)complicated.

I don't think we want to try to maintain some kind of mintty integration test harness infrastructure. It's hard to make these kinds of tests reliable, and when they fail, it's often a signal that something in the environment changed, not LLVM, like a new version of mintty that uses differently named pipes.

And, even if we had this test and it worked, it's not the kind of information an LLVM contributor wants to get from a buildbot somewhere, or a new contributor the first time they run the test suite on a new machine. We want the test suite to have a high likelihood of passing by default, with as few environmental requirements of dependencies as possible. Our coreutils dependence here gets in the way on Windows, of course, and causes all kinds of problems.

If we were to test this, I'd suggest using a unit test that creates appropriately named pipes and re-lauches itself as a subprocess and then emits some colored output to stdout. The parent process would read from the named pipe and look for some ANSI escape codes.

lib/Support/Windows/Process.inc

231

I think the XP ship has sailed. I think our STL requirements now require using an MSVC CRT version that doesn't support XP.

I would also like this feature in MSVC builds, and less ifdefs are good if possible. I'm willing to apply this patch locally and fix any compatibility issues in it.

lib/Support/Windows/Process.inc

static std::error_code GetExecutableName(SmallVectorImpl<char> &Filename) {
   if (EC)
     return EC;
 
   StringRef Base = sys::path::filename(Filename.data());
   Filename.assign(Base.begin(), Base.end());
   return std::error_code();
 }
 
+#if defined(__MINGW32__)
+// Hack to detect mintty, ported from vim
+// https://fossies.org/linux/vim/src/iscygpty.c
+static bool IsMintty(HANDLE Console) {
+  const int Size = sizeof(FILE_NAME_INFO) + sizeof(WCHAR) * MAX_PATH;
+  char Buf[Size];
+  size_t Index = 0;
+
+  if (Console == INVALID_HANDLE_VALUE)
+    return false;
+  // Cygwin/msys's pty is a pipe.
+  if (GetFileType(Console) != FILE_TYPE_PIPE)
+    return false;
+  FILE_NAME_INFO *NameInfo = reinterpret_cast<FILE_NAME_INFO *>(Buf);
+  if (NameInfo == NULL)
+    return false;
+  // Check the name of the pipe:
+  // '\{cygwin,msys}-XXXXXXXXXXXXXXXX-ptyN-{from,to}-master'
+  if (GetFileInformationByHandleEx(Console, FileNameInfo, NameInfo, Size)) {
+    SmallVector<char, MAX_PATH> Res;
+    std::error_code EC = windows::UTF16ToUTF8(
+        NameInfo->FileName, NameInfo->FileNameLength / sizeof(WCHAR), Res);
+    if (EC) {
+      return false;
+    }
+    std::string PipeName = std::string(Res.data());
+    if (PipeName.find("\\cygwin-") == Index) {
+      Index += 8;
+    } else if (PipeName.find("\\msys-") == Index) {
+      Index += 6;
+    }
+    if (Index > 0) {
+      while (PipeName.length() > Index && isxdigit(PipeName[Index])) {
+        Index++;
+      }
+      if (PipeName.find("-pty", Index)) {
+        Index += 4;
+      } else {
+        Index = 0;
+      }
+      if (Index > 0) {
+        while (PipeName.length() > Index && isdigit(PipeName[Index])) {
+          Index++;
+        }
+        if (PipeName.find("-from-master", Index) != Index &&
+            PipeName.find("-to-master", Index) != Index) {
+          Index = 0;
+        }
+      }
+    }
+  }
+  return Index > 0;
+}
+#endif
+
 std::error_code
 windows::GetCommandLineArguments(SmallVectorImpl<const char *> &Args,
                                  BumpPtrAllocator &Alloc) {
   const wchar_t *CmdW = GetCommandLineW();
   assert(CmdW);
   std::error_code EC;
   SmallString<MAX_PATH> Cmd;
   EC = windows::UTF16ToUTF8(CmdW, wcslen(CmdW), Cmd);
bool Process::StandardOutIsDisplayed() {
   return FileDescriptorIsDisplayed(1);
 }
 
 bool Process::StandardErrIsDisplayed() {
   return FileDescriptorIsDisplayed(2);
 }
 
 bool Process::FileDescriptorIsDisplayed(int fd) {
-  DWORD Mode;  // Unused
-  return (GetConsoleMode((HANDLE)_get_osfhandle(fd), &Mode) != 0);
+  DWORD Mode; // Unused
+  HANDLE Console;
+  Console = (HANDLE)_get_osfhandle(fd);
+  bool ret = (GetConsoleMode(Console, &Mode) != 0);
+#if defined(__MINGW32__)
+  if (!ret) {
+    ret = IsMintty(Console);
+  }
+#endif
+  return ret;
 }
 
 unsigned Process::StandardOutColumns() {
   unsigned Columns = 0;
   CONSOLE_SCREEN_BUFFER_INFO csbi;
   if (GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &csbi))
     Columns = csbi.dwSize.X;
   return Columns;

 }
 
 bool Process::StandardErrHasColors() {
   return FileDescriptorHasColors(2);
 }
 
 static bool UseANSI = false;
 void Process::UseANSIEscapeCodes(bool enable) {
+  DWORD Mode;
+  HANDLE Console;
 #if defined(ENABLE_VIRTUAL_TERMINAL_PROCESSING)
   if (enable) {
-    HANDLE Console = GetStdHandle(STD_OUTPUT_HANDLE);
-    DWORD Mode;
+    Console = GetStdHandle(STD_OUTPUT_HANDLE);
     GetConsoleMode(Console, &Mode);
     Mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
     SetConsoleMode(Console, Mode);
   }
 #endif
+#if defined(__MINGW32__)
+  Console = GetStdHandle(STD_ERROR_HANDLE);
+  bool ret = (GetConsoleMode(Console, &Mode) != 0);
+  if (!ret) {
+    enable = enable || IsMintty(Console);
+  }
+#endif
   UseANSI = enable;
 }
 
 namespace {
 class DefaultColors
 {
   private:
     WORD defaultColor;