Re: [PATCH] gdb/python: allow Value.format_string to return styled output

[Andrew Burgess <aburgess@redhat.com>]

Hi Eli,

Didn't know if you wanted to give this patch a docs review or not.

Thanks,
Andrew


* Andrew Burgess <aburgess@redhat.com> [2022-01-25 17:27:33 +0000]:

> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2022-01-25 11:27:09 -0500]:
> 
> > 
> > 
> > On 2022-01-25 11:07, Andrew Burgess wrote:
> > > * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2022-01-25 07:56:52 -0500]:
> > > 
> > >> LGTM, just two small comments:
> > >>
> > >>> @@ -725,6 +728,7 @@ valpy_format_string (PyObject *self, PyObject *args, PyObject *kw)
> > >>>      return NULL;
> > >>>    if (!copy_py_bool_obj (&opts.static_field_print, static_members_obj))
> > >>>      return NULL;
> > >>> +  bool styling_p = (styling_obj != nullptr) && PyObject_IsTrue (styling_obj);
> > >>
> > >> Can styling_obj actually be nullptr?
> > > 
> > > Sure, the new 'styling' argument is optional, after the "|" in the
> > > gdb_PyArg_ParseTupleAndKeywords call, so if 'styling' isn't given
> > > styling_obj will be left as it's default value, which is nullptr.
> > > 
> > > I guess I could initialise styling_obj to Py_False, maybe that would
> > > be nicer?
> > 
> > Either way, what you have is fine.  It's just that I read the doc for
> > the "O" specifier, and it says "The pointer stored is not NULL.".  But
> > that's only if the value is present.  I never remember that part.
> 
> Cool.  Thanks for the review.  I'll wait for a doc review from Eli
> before pushing.
> 
> The updated patch is below.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 7e8ebb30bdf3217b380bc5130d34bae9ba6c1a35
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Mon Jan 24 15:29:49 2022 +0000
> 
>     gdb/python: allow Value.format_string to return styled output
>     
>     Add a new argument to the gdb.Value.format_string method, 'styling'.
>     This argument is False by default.
>     
>     When this argument is True, then the returned string can contain output
>     styling escape sequences.
>     
>     When this argument is False, then the returned string will not contain
>     any styling escape sequences.
>     
>     If the returned string is going to be printed to the user, then it is
>     often nice to retain the GDB styling.
>     
>     For the testing, we need to adjust the TERM environment variable, as
>     we do for all the styling tests.  I'm now running all of the C tests
>     in gdb.python/py-format-string.exp in an environment where styling
>     could be generated, but only my new test should actually produce
>     styled output, hopefully this will catch the case where a bug might
>     cause format_string to always produce styled output.
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 8c13cefb22f..5b71d6a1c08 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -146,6 +146,13 @@ show debug lin-lwp
>    ** New function gdb.host_charset(), returns a string, which is the
>       name of the current host charset.
>  
> +  ** The gdb.Value.format_string method now takes a 'styling'
> +     argument, which is a boolean.  When true, the returned string can
> +     include escape sequences to apply styling.  The styling will only
> +     be present if styling is otherwise turned on in GDB (see 'help
> +     set styling').  When false, which is the default if the argument
> +     is not given, then no styling is applied to the returned string.
> +
>  * New features in the GDB remote stub, GDBserver
>  
>    ** GDBserver is now supported on OpenRISC GNU/Linux.
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index f02380a27f4..0b3235018bb 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -1048,6 +1048,16 @@
>  the returned string.  For instance, @code{'x'} is equivalent to using the
>  @value{GDBN} command @code{print} with the @code{/x} option and formats
>  the value as a hexadecimal number.
> +
> +@item styling
> +@code{True} if @value{GDBN} should apply styling to the returned
> +string.  When styling is applied, the returned string might contain
> +ANSI terminal escape sequences.  Escape sequences will only be
> +included if styling is turned on, @xref{Output Styling}.
> +Additionally, @value{GDBN} only styles some value contents, so not
> +every output string will contain escape sequences.
> +
> +When @code{False}, which is the default, no output styling is applied.
>  @end table
>  @end defun
>  
> diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
> index 70b33d5a27b..12034657c31 100644
> --- a/gdb/python/py-value.c
> +++ b/gdb/python/py-value.c
> @@ -636,6 +636,7 @@ valpy_format_string (PyObject *self, PyObject *args, PyObject *kw)
>        "symbols",		/* See set print symbol on|off.  */
>        "unions",			/* See set print union on|off.  */
>        "address",		/* See set print address on|off.  */
> +      "styling",		/* Should we apply styling.  */
>        /* C++ options.  */
>        "deref_refs",		/* No corresponding setting.  */
>        "actual_objects",		/* See set print object on|off.  */
> @@ -680,13 +681,14 @@ valpy_format_string (PyObject *self, PyObject *args, PyObject *kw)
>    PyObject *symbols_obj = NULL;
>    PyObject *unions_obj = NULL;
>    PyObject *address_obj = NULL;
> +  PyObject *styling_obj = Py_False;
>    PyObject *deref_refs_obj = NULL;
>    PyObject *actual_objects_obj = NULL;
>    PyObject *static_members_obj = NULL;
>    char *format = NULL;
>    if (!gdb_PyArg_ParseTupleAndKeywords (args,
>  					kw,
> -					"|O!O!O!O!O!O!O!O!O!O!IIIs",
> +					"|O!O!O!O!O!O!O!O!O!O!O!IIIs",
>  					keywords,
>  					&PyBool_Type, &raw_obj,
>  					&PyBool_Type, &pretty_arrays_obj,
> @@ -695,6 +697,7 @@ valpy_format_string (PyObject *self, PyObject *args, PyObject *kw)
>  					&PyBool_Type, &symbols_obj,
>  					&PyBool_Type, &unions_obj,
>  					&PyBool_Type, &address_obj,
> +					&PyBool_Type, &styling_obj,
>  					&PyBool_Type, &deref_refs_obj,
>  					&PyBool_Type, &actual_objects_obj,
>  					&PyBool_Type, &static_members_obj,
> @@ -749,7 +752,7 @@ valpy_format_string (PyObject *self, PyObject *args, PyObject *kw)
>  	}
>      }
>  
> -  string_file stb;
> +  string_file stb (PyObject_IsTrue (styling_obj));
>  
>    try
>      {
> diff --git a/gdb/testsuite/gdb.python/py-format-string.exp b/gdb/testsuite/gdb.python/py-format-string.exp
> index 4c78bed1203..ac1353ea2e3 100644
> --- a/gdb/testsuite/gdb.python/py-format-string.exp
> +++ b/gdb/testsuite/gdb.python/py-format-string.exp
> @@ -991,6 +991,13 @@ proc_with_prefix test_invalid_args {} {
>      "ValueError: a single character is required.*"
>  }
>  
> +# Check the styling argument to format_string.  This function needs to
> +# be called with TERM set such that styling can be applied.
> +proc test_styling {} {
> +    gdb_test "python print(gdb.parse_and_eval(\"a_point_t\").format_string(styling=True, raw=True))" \
> +	"{[style x variable] = 42, [style y variable] = 12}"
> +}
> +
>  # Run all the tests in common for both C and C++.
>  proc_with_prefix test_all_common {} {
>    # No options.
> @@ -1023,9 +1030,16 @@ with_test_prefix "format_string" {
>    # Perform C Tests.
>    if { [build_inferior "${binfile}" "c"] == 0 } {
>      with_test_prefix "lang_c" {
> -      set current_lang "c"
> -      prepare_gdb "${binfile}"
> -      test_all_common
> +      save_vars { env(TERM) } {
> +	# We run all of these tests in an environment where styling
> +	# could work, but we only expect the final call to
> +	# test_styling to actually produce any styled output.
> +	setenv TERM ansi
> +	set current_lang "c"
> +	prepare_gdb "${binfile}"
> +	test_all_common
> +	test_styling
> +      }
>      }
>    }
>  
>