Save and restore previous terminal after setting the terminal for checking if terminal supports colors.

[augusto2112]

Summary

The call to "set_curterm" inside the "terminalHasColors" function breaks the EditLine configuration on some Linux distributions, causing certain characters that have functions bound to them to not show up and backspace to stop deleting characters (only visually). This patch ensures that term struct is restored after the routine for cheking if terminal supports colors is done, which fixes the aforementioned issue.

[augusto2112]

This is the second (and better) solution that addresses this issue. Previous discussions can be found here.

[labath]

This change seems obviously correct. We'll still need to be careful about using this functionality on the lldb side though. This fix will only help if there is proper synchronization between using this function and libedit initialization. If we still run into issues, we'll need to either add more synchronization, or do something different altogether.

Regarding testing, I suppose it should be possible to write a unit test which checks that this function really preserves the terminal value, but it's not clear to me how useful would that be....

llvm/lib/Support/Unix/Process.inc

357–358

Now that its purpose is also to restore the terminal value, this code is no longer that "silly".
I'd update this to something like:

// Restore the current terminal to the original value. Be sure to also free the structure allocated by setupterm.

[augusto2112]

We'll still need to be careful about using this functionality on the lldb side though. This fix will only help if there is proper synchronization between using this function and libedit initialization

The only way I think this could have adverse effects is if some component is relying on this function to clear the current settings, which would stop happening after this change.

@labath can this be merged or do we need someone else to approve it? I tagged everyone I found that could review it, but maybe we should add more people?

[labath]

Ownership of various Support components is a little bit unclear, but I think this is sufficiently obvious that I'll just go ahead and commit it for you. Thanks for your patience.

llvm/lib/Support/Unix/Process.inc

#if defined(HAVE_GETRUSAGE)
   ::getrusage(RUSAGE_SELF, &RU);
   return { toDuration(RU.ru_utime), toDuration(RU.ru_stime) };
 #else
 #warning Cannot get usage times on this platform
   return { std::chrono::microseconds::zero(), std::chrono::microseconds::zero() };
 #endif
 }
 
 Process::Pid Process::getProcessId() {
   static_assert(sizeof(Pid) >= sizeof(pid_t),
                 "Process::Pid should be big enough to store pid_t");
   return Pid(::getpid());
 }
 
 // On Cygwin, getpagesize() returns 64k(AllocationGranularity) and
 // offset in mmap(3) should be aligned to the AllocationGranularity.
 Expected<unsigned> Process::getPageSize() {
 #if defined(HAVE_GETPAGESIZE)
   static const int page_size = ::getpagesize();
 #elif defined(HAVE_SYSCONF)
   static long page_size = ::sysconf(_SC_PAGE_SIZE);
 #else
 #error Cannot get the page size on this machine
 #endif
   if (page_size == -1)
     return errorCodeToError(std::error_code(errno, std::generic_category()));
 
   return static_cast<unsigned>(page_size);
 }
 
 size_t Process::GetMallocUsage() {
 #if defined(HAVE_MALLINFO)
   struct mallinfo mi;
   mi = ::mallinfo();
   return mi.uordblks;
 #elif defined(HAVE_MALLOC_ZONE_STATISTICS) && defined(HAVE_MALLOC_MALLOC_H)
   malloc_statistics_t Stats;
   malloc_zone_statistics(malloc_default_zone(), &Stats);
   return Stats.size_in_use;   // darwin
if (EndOfMemory != ((char*)-1) && StartOfMemory != ((char*)-1))
     return EndOfMemory - StartOfMemory;
   return 0;
 #else
 #warning Cannot get malloc info on this platform
   return 0;
 #endif
 }
 
 void Process::GetTimeUsage(TimePoint<> &elapsed, std::chrono::nanoseconds &user_time,
                            std::chrono::nanoseconds &sys_time) {
   elapsed = std::chrono::system_clock::now();
   std::tie(user_time, sys_time) = getRUsageTimes();
 }
 
 #if defined(HAVE_MACH_MACH_H) && !defined(__GNU__)
 #include <mach/mach.h>
 #endif
 
 // Some LLVM programs such as bugpoint produce core files as a normal part of
 // their operation. To prevent the disk from filling up, this function
 // does what's necessary to prevent their generation.
 void Process::PreventCoreFiles() {
 #if HAVE_SETRLIMIT
   struct rlimit rlim;
   rlim.rlim_cur = rlim.rlim_max = 0;
   setrlimit(RLIMIT_CORE, &rlim);
 #endif
 
 #if defined(HAVE_MACH_MACH_H) && !defined(__GNU__)
   // Disable crash reporting on Mac OS X 10.0-10.4
#if defined(HAVE_MACH_MACH_H) && !defined(__GNU__)
   signal(SIGFPE,  _exit);
   signal(SIGSEGV, _exit);
   signal(SIGBUS,  _exit);
 #endif
 
   coreFilesPrevented = true;
 }
 
 Optional<std::string> Process::GetEnv(StringRef Name) {
   std::string NameStr = Name.str();
   const char *Val = ::getenv(NameStr.c_str());
   if (!Val)
     return None;
   return std::string(Val);
 }
 
 namespace {
private:
   FDCloser(const FDCloser &) = delete;
   void operator=(const FDCloser &) = delete;
 
   int &FD;
   bool KeepOpen;
 };
 }
 
 std::error_code Process::FixupStandardFileDescriptors() {
   int NullFD = -1;
   FDCloser FDC(NullFD);
   const int StandardFDs[] = {STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO};
   for (int StandardFD : StandardFDs) {
     struct stat st;
     errno = 0;
     if (RetryAfterSignal(-1, ::fstat, StandardFD, &st) < 0) {
       assert(errno && "expected errno to be set if fstat failed!");
for (int StandardFD : StandardFDs) {
     if (NullFD == StandardFD)
       FDC.keepOpen();
     else if (dup2(NullFD, StandardFD) < 0)
       return std::error_code(errno, std::generic_category());
   }
   return std::error_code();
 }
 
 std::error_code Process::SafelyCloseFileDescriptor(int FD) {
   // Create a signal set filled with *all* signals.
   sigset_t FullSet;
   if (sigfillset(&FullSet) < 0)
     return std::error_code(errno, std::generic_category());
   // Atomically swap our current signal mask with a full mask.
   sigset_t SavedSet;
 #if LLVM_ENABLE_THREADS
   if (int EC = pthread_sigmask(SIG_SETMASK, &FullSet, &SavedSet))

 #endif
   // The error code from close takes precedence over the one from
   // pthread_sigmask.
   if (ErrnoFromClose)
     return std::error_code(ErrnoFromClose, std::generic_category());
   return std::error_code(EC, std::generic_category());
 }
 
 bool Process::StandardInIsUserInput() {
   return FileDescriptorIsDisplayed(STDIN_FILENO);
 }
 
 bool Process::StandardOutIsDisplayed() {
   return FileDescriptorIsDisplayed(STDOUT_FILENO);
 }
 
 bool Process::StandardErrIsDisplayed() {
   return FileDescriptorIsDisplayed(STDERR_FILENO);
 }
 
 bool Process::FileDescriptorIsDisplayed(int fd) {
 #if HAVE_ISATTY
   return isatty(fd);
 #else
   // If we don't have isatty, just return false.
   return false;
 #endif
 }
 
 static unsigned getColumns() {
   // If COLUMNS is defined in the environment, wrap to that many columns.
   if (const char *ColumnsStr = std::getenv("COLUMNS")) {
     int Columns = std::atoi(ColumnsStr);
     if (Columns > 0)
       return Columns;
   }
 
   // We used to call ioctl TIOCGWINSZ to determine the width. It is considered
   // unuseful.
   return 0;
 }
 
 unsigned Process::StandardOutColumns() {
   if (!StandardOutIsDisplayed())
     return 0;
 
   return getColumns();
 }
 
 unsigned Process::StandardErrColumns() {
   if (!StandardErrIsDisplayed())
     return 0;
 
   return getColumns();
 }
 
 #ifdef LLVM_ENABLE_TERMINFO
 // We manually declare these extern functions because finding the correct

 static ManagedStatic<std::mutex> TermColorMutex;
 #endif
 
 static bool terminalHasColors(int fd) {
 #ifdef LLVM_ENABLE_TERMINFO
   // First, acquire a global lock because these C routines are thread hostile.
   std::lock_guard<std::mutex> G(*TermColorMutex);
 
+  struct term *previous_term = set_curterm(nullptr);
   int errret = 0;
   if (setupterm(nullptr, fd, &errret) != 0)
     // Regardless of why, if we can't get terminfo, we shouldn't try to print
     // colors.
     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
   // would be nice to avoid a dependency on curses proper when we can make do
   // with a minimal terminfo parsing library. Also, we don't really care whether
   // the terminal supports the curses-specific color changing routines, merely
   // if it will interpret ANSI color escape codes in a reasonable way. Thus, the
   // strategy here is just to query the baseline colors capability and if it
   // supports colors at all to assume it will translate the escape codes into
   // whatever range of colors it does support. We can add more detailed tests
   // here if users report them as necessary.
   //
   // The 'tigetnum' routine returns -2 or -1 on errors, and might return 0 if
   // the terminfo says that no colors are supported.
   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(nullptr);
+  struct term *termp = set_curterm(previous_term);
   (void)del_curterm(termp); // Drop any errors here.
 
   // Return true if we found a color capabilities for the current terminal.
   if (HasColors)
     return true;
 #else
   // When the terminfo database is not available, check if the current terminal
   // is one of terminals that are known to support ANSI color escape codes.
return StringSwitch<bool>(TermStr)
       .Default(false);
   }
 #endif
 
   // Otherwise, be conservative.
   return false;
 }
 
 bool Process::FileDescriptorHasColors(int fd) {
   // A file descriptor has colors if it is displayed and the terminal has
   // colors.
   return FileDescriptorIsDisplayed(fd) && terminalHasColors(fd);
 }
 
 bool Process::StandardOutHasColors() {
   return FileDescriptorHasColors(STDOUT_FILENO);
 }
 
 bool Process::StandardErrHasColors() {
   return FileDescriptorHasColors(STDERR_FILENO);
 }
 
 void Process::UseANSIEscapeCodes(bool /*enable*/) {
   // No effect.
 }
 
 bool Process::ColorNeedsFlush() {
   // No, we use ANSI escape sequences.
   return false;
 }