gdb/styling: only check TERM environment once, during initialisation

We currently check the TERM environment variable any time we call one
of the ui_file::can_emit_style_escape() functions.  This seems
excessive.

During GDB's startup we also check for the NO_COLOR environment
variable and disable styling if this is set.

I propose that we combine these two checks, and perform them just once
during startup (as the current NO_COLOR check is currently done).  As
with the NO_COLOR check, if the TERM variable is set to "dumb"
indicating that styling is not supported then we should just set
cli_styling to false.

This does mean that the user can then 'set style enabled on', even for
a dumb terminal, which was not possible previously.  Before this
commit the "dumb" terminal check was separate and would prevent
styling even if 'set style enabled on' was in effect.

Of course, trying to turn on styling in a dumb terminal might not give
the results that a user hope for.  And so, I have implemented a check
in `set_style_enabled`, so in a dumb terminal a user will see this:

  (gdb) set style enabled on
  warning: The current terminal doesn't support styling.  Styled output might not appear as expected.

After which GDB will try to emit styling.  We could, potentially,
prevent styling being enabled instead of emitting a warning, but I'm
inclined to let the user turn on styling if they really want to.

Approved-By: Kevin Buettner <kevinb@redhat.com>
Acked-By: Tom Tromey <tom@tromey.com>

diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c
index 2e6a43f890b186e7734d3e10f1bd6267b04bd3e5..47f7d2ea693bc37c02deb5eb19cc2ce01d8aa48a 100644 (file)
--- a/gdb/cli/cli-style.c
+++ b/gdb/cli/cli-style.c
@@ -51,6 +51,46 @@ static const char * const cli_intensities[] = {
   nullptr
 };
 
+/* Return true if GDB's output terminal should support styling, otherwise,
+   return false.  This function really checks for things that indicate
+   styling might not be supported, so a return value of false indicates
+   we've seen something to indicate we should not perform styling.  A
+   return value of true is the default.  */
+
+static bool
+terminal_supports_styling ()
+{
+  const char *term = getenv ("TERM");
+
+  /* Windows doesn't by default define $TERM, but can support styles
+     regardless.  */
+#ifndef _WIN32
+  if (term == nullptr || strcmp (term, "dumb") == 0)
+    return false;
+#else
+  /* But if they do define $TERM, let us behave the same as on Posix
+     platforms, for the benefit of programs which invoke GDB as their
+     back-end.  */
+  if (term != nullptr && strcmp (term, "dumb") == 0)
+    return false;
+#endif
+
+  return true;
+}
+
+/* See cli/cli-style.h.  */
+
+void
+disable_styling_from_environment ()
+{
+  const char *no_color = getenv ("NO_COLOR");
+  if (no_color != nullptr && *no_color != '\0')
+    cli_styling = false;
+
+  if (!terminal_supports_styling ())
+    cli_styling = false;
+}
+
 /* See cli-style.h.  */
 
 cli_style_option file_name_style ("filename", ui_file_style::GREEN);
@@ -286,6 +326,17 @@ static cmd_list_element *style_disasm_show_list;
 static void
 set_style_enabled  (const char *args, int from_tty, struct cmd_list_element *c)
 {
+  /* This finds the 'set style enabled' command.  */
+  struct cmd_list_element *set_style_enabled_cmd
+    = lookup_cmd_exact ("enabled", style_set_list);
+
+  /* If the user does 'set style enabled on', but the terminal doesn't
+     appear to support styling, then warn the user.  */
+  if (c == set_style_enabled_cmd && cli_styling
+      && !terminal_supports_styling ())
+    warning ("The current terminal doesn't support styling.  Styled output "
+            "might not appear as expected.");
+
   g_source_cache.clear ();
   gdb::observers::styling_changed.notify ();
 }

diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h
index a1e538b04b3832408ae85c99d1beaf8242a4ee4f..1fb89a37455aadbfe7f98723e8111b8c265a5b15 100644 (file)
--- a/gdb/cli/cli-style.h
+++ b/gdb/cli/cli-style.h
@@ -160,4 +160,10 @@ extern bool disassembler_styling;
 /* True if styling is enabled.  */
 extern bool cli_styling;
 
+/* Check for environment variables that indicate styling should start as
+   disabled.  If any are found then disable styling.  Styling is never
+   enabled by this call.  If styling was already disabled then it remains
+   disabled after this call.  */
+extern void disable_styling_from_environment ();
+
 #endif /* GDB_CLI_CLI_STYLE_H */

diff --git a/gdb/main.c b/gdb/main.c
index b12e5e107161432b0603377bde4c470fd0f50380..2545a1579245831f4df576db0d556694c306e4f7 100644 (file)
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -644,9 +644,9 @@ captured_main_1 (struct captured_main_args *context)
   int save_auto_load;
   int ret = 1;
 
-  const char *no_color = getenv ("NO_COLOR");
-  if (no_color != nullptr && *no_color != '\0')
-    cli_styling = false;
+  /* Check for environment variables which might cause GDB to start with
+     styling disabled.  */
+  disable_styling_from_environment ();
 
 #ifdef HAVE_USEFUL_SBRK
   /* Set this before constructing scoped_command_stats.  */

diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index b027ab7174b82d6467b293d41f8779df58c97731..571cb0f1a1582025b9bc74b2e3ade7856ad570d1 100644 (file)
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -678,6 +678,55 @@ proc test_colorsupport_truecolor_only { } {
     }
 }
 
+# Check that styling is auto-disabled when using a "dumb" terminal,
+# and/or when NO_COLOR is set.
+#
+# Also check that the user gets a warning if they enable styling in a
+# "dumb" terminal, but don't get a warning if they are in an "ansi"
+# terminal and NO_COLOR is set.
+proc test_enable_styling_warning { } {
+    save_vars { env(TERM) env(NO_COLOR) } {
+       foreach_with_prefix no_color { set unset } {
+           if { $no_color eq "set" } {
+               setenv NO_COLOR "xxx"
+           } else {
+               unset -nocomplain ::env(NO_COLOR)
+           }
+
+           foreach_with_prefix term_type { dumb unset ansi } {
+               if { $term_type eq "dumb" } {
+                   setenv TERM dumb
+               } elseif { $term_type eq "unset" } {
+                   unset -nocomplain ::env(TERM)
+               } elseif { $term_type eq "ansi" } {
+                   setenv TERM ansi
+               }
+
+               set styling_on_re "CLI output styling is enabled\\."
+               set styling_off_re "CLI output styling is disabled\\."
+
+               if { $term_type eq "ansi" } {
+                   if { $no_color eq "unset" } {
+                       set init_re $styling_on_re
+                   } else {
+                       set init_re $styling_off_re
+                   }
+                   set enable_re ""
+               } else {
+                   set init_re $styling_off_re
+                   set enable_re \
+                       "warning: The current terminal doesn't support styling\\.  Styled output might not appear as expected\\."
+               }
+
+
+               clean_restart
+               gdb_test "show style enabled" $init_re
+               gdb_test "set style enabled on" $enable_re
+           }
+       }
+    }
+}
+
 # Check to see if the Python styling of disassembler output is
 # expected or not, this styling requires Python support in GDB, and
 # the Python pygments module to be available.
@@ -718,3 +767,5 @@ test_colorsupport_8color
 test_colorsupport_256color
 test_colorsupport_truecolor
 test_colorsupport_truecolor_only
+
+test_enable_styling_warning

diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index dd15ea427bcf339c013ca9d68bae4d0b518def4a..1a2f5635b7655121ac71000929d140a620677104 100644 (file)
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -180,28 +180,12 @@ null_file::write_async_safe (const char *buf, long sizeof_buf)
 
 \f
 
-/* true if the gdb terminal supports styling, and styling is enabled.  */
+/* Return true if styling is currently enabled.  */
 
 static bool
 term_cli_styling ()
 {
-  if (!cli_styling)
-    return false;
-
-  const char *term = getenv ("TERM");
-  /* Windows doesn't by default define $TERM, but can support styles
-     regardless.  */
-#ifndef _WIN32
-  if (term == nullptr || !strcmp (term, "dumb"))
-    return false;
-#else
-  /* But if they do define $TERM, let us behave the same as on Posix
-     platforms, for the benefit of programs which invoke GDB as their
-     back-end.  */
-  if (term && !strcmp (term, "dumb"))
-    return false;
-#endif
-  return true;
+  return cli_styling;
 }
 
 \f