[PATCH] Save and restore current term setting in terminalHasColors(). PR25007

[mitchnull]

Summary

Proposed fix for the reported terminal issue in PR25007.

• Save the current term settings before doing the check for color support.
• Restore the term settings after the tests (or if the test fails).

This fixes the reported issue for me (with vim-clang-complete), and keeps the original terminal color detection working (-f[no-]color-diagnostics).

I chose to duplicate the cleanup logic (set_curterm(OrigTerm)) twice (once on the error path and once on the normal path) instead of restructuring the function to keep the patch simpler.

Another option would be to use the already set up term in case set_curterm returns non-NULL term, but I felt this approach is safer (and closer to the original logic).

[joerg]

I see two issues with the current code and I think they should be considered as a whole for any chance here.

(1) We should only ever run this dance once. Even with clang we currently run it per source input.
(2) Since the dance is inherently not thread-safe, code using it as library, directly or indirectly, should have a way to either provide a fixed option OR to initialize the state at a safe point.

For (2), preserving the current state like in this patch is still a good idea, but it becomes less crucial.

[mitchnull]

I've tested the changes on freebsd (10.2-RELEASE-p7 i386). Works fine. It doesn't improve anything there, as the freebsd version of ncurses doesn't trigger the issue as the new linux version of ncurses, but at least it doesn't break anything either ;)

[mitchnull]

In D15077#300234, @joerg wrote:

I see two issues with the current code and I think they should be considered as a whole for any chance here.

(1) We should only ever run this dance once. Even with clang we currently run it per source input.

I don't think it's such a big issue, most build systems compile a single file at a time, not multiple source files on the same command line. I think it's better to keep the most frequent use-case simple (and fast).

(2) Since the dance is inherently not thread-safe, code using it as library, directly or indirectly, should have a way to either provide a fixed option OR to initialize the state at a safe point.

I glanced at the gnu ncurses code, and apparently it does have some thread-safety (at least cur_term is protected in set_curterm). Not sure about other curses implementations though. Still, this patch doesn't make anything worse, imo.

For (2), preserving the current state like in this patch is still a good idea, but it becomes less crucial.

[joerg]

In D15077#301348, @mitchnull wrote:

In D15077#300234, @joerg wrote:

I see two issues with the current code and I think they should be considered
as a whole for any chance here.

(1) We should only ever run this dance once. Even with clang we currently run it per source input.

I don't think it's such a big issue, most build systems compile a single file at a time,
not multiple source files on the same command line. I think it's better to keep the
most frequent use-case simple (and fast).

There have been long discussions about things like clangd and other uses where
it is done much more than once. That isn't very relevant, the point remains that we do
completely unnecessary recomputations, which are not that cheap either. Implementing
it doesn't complicate code by more than a few lines either.

(2) Since the dance is inherently not thread-safe, code using it as library,
directly or indirectly, should have a way to either provide a fixed option
OR to initialize the state at a safe point.

I glanced at the gnu ncurses code, and apparently it does have some
thread-safety (at least cur_term is protected in set_curterm). Not sure
about other curses implementations though. Still, this patch doesn't
make anything worse, imo.

It might protect its internal data structures by atomic updates. That doesn't
mean that a console editor using clang libraries in one thread wants its output
messed up from another thread because the terminal settings just changed.
They both depend on the global state of terminfo after all.

[chandlerc]

This looks like an entirely correct change to me. LGTM, please submit after fixing the below comment issue.

Joerg, I'm not really concerned about the issues you raise for a few reasons.

First and foremost, this is already *always* done under a single global lock for the entire address space. So we can't get correctness issues of tearing here from other callers of these routines

Second, this is file descriptor specific and so it doesn't seem unreasonable to call it multiple times, and *this* level isn't the level which should do the caching. We might want to do that in raw_ostream or elsewhere, but that's really orthogonal to the issue this patch fixes.

The remaining concern is the fact that there could be some other thread which is doing its own concurrent calls to setupterm and because those won't use the same global variables, they will race with these calls. True, but unavoidable. Callers of the *HasColors routines provided by Process.h need to be aware that they are calling a routine which is not safe to call concurrently with arbitrary other systems code. I don't think this is the layer to protect the callers from such failure modes.

The correct way for all of this to work is for the Driver library in Clang to accept already set up (and likely cached color state) raw_ostream objects, and for the same chunk of code as has main() in it (tools/driver/...) to set up that ostream for the Driver library. Then the user of the library can decide whether or not to access these routines. But still, this patch seems like a strict improvement. It should go in.

-Chandler

lib/Support/Unix/Process.inc

357

Please don't cite PRs in comments. The comment line above is sufficient.

[joerg]

In D15077#306894, @chandlerc wrote:

First and foremost, this is already *always* done under a single global lock for the entire
address space. So we can't get correctness issues of tearing here from other callers of these routines.

I never said that. I was always talking about other applications using the LLVM library.

Second, this is file descriptor specific and so it doesn't seem unreasonable to call it
multiple times, and *this* level isn't the level which should do the caching. We might
want to do that in raw_ostream or elsewhere, but that's really orthogonal to the issue
this patch fixes.

Actually, it isn't. setupterm needs a file descriptor, but only to remember it for following
operations and maybe change baud rate. The data we care about with the color codes is completely independent of that.

The remaining concern is the fact that there could be some other thread which is doing its
own concurrent calls to setupterm and because those won't use the same global variables,
they will race with these calls. True, but unavoidable. Callers of the *HasColors routines
provided by Process.h need to be aware that they are calling a routine which is not safe
to call concurrently with arbitrary other systems code. I don't think this is the layer to protect
the callers from such failure modes.

The patch changes the failure mode from "corruption always happens" to "corruption happens depending on the use order". The very need for the patch illustrates that the problem is well hidden and that library users are not generally aware of this dependency. That seems to be a pretty valid concern
about the implications of the patch.

The correct way for all of this to work is for the Driver library in Clang to accept already set up
(and likely cached color state) raw_ostream objects, and for the same chunk of code as has
main() in it (tools/driver/...) to set up that ostream for the Driver library. Then the user of the
library can decide whether or not to access these routines. But still, this patch seems like a strict
improvement. It should go in.

Well, do we have such interfaces or not? Without them, I don't see the patch as a strict improvement...

[mitchnull]

removed the ref to the PR from the comment

lib/Support/Unix/Process.inc

   // First, acquire a global lock because these C routines are thread hostile.
   MutexGuard G(*TermColorMutex);
 
+  // Save the current term (if set) to be restored after the test.
+  struct term *OrigTerm = set_curterm((struct term *)nullptr);
   int errret = 0;
-  if (setupterm((char *)nullptr, fd, &errret) != 0)
+  if (setupterm((char *)nullptr, fd, &errret) != 0) {
     // Regardless of why, if we can't get terminfo, we shouldn't try to print
     // colors.
+    (void)set_curterm(OrigTerm);
     return false;
+  }
 
   // Test whether the terminal as set up supports color output. How to do this
   // isn't entirely obvious. We can use the curses routine 'has_colors' but it

   bool HasColors = tigetnum(const_cast<char *>("colors")) > 0;
 
   // Now extract the structure allocated by setupterm and free its memory
-  // through a really silly dance.
-  struct term *termp = set_curterm((struct term *)nullptr);
+  // through a really silly dance. Also restore the previously saved term
+  // here.
+  struct term *termp = set_curterm(OrigTerm);
   (void)del_curterm(termp); // Drop any errors here.
 
   // Return true if we found a color capabilities for the current terminal.