Re: [PATCH 4/4] Fix raw-frame-arguments in combination with frame-filters

[Joel Brobecker <brobecker@adacore.com>]

Hi Hannes,

On Tue, Dec 29, 2020 at 06:02:27PM +0100, Hannes Domani via Gdb-patches wrote:
> Currently, if frame-filters are active, raw-values is used instead of
> raw-frame-arguments to decide if a pretty-printer should be invoked for
> frame arguments in a backtrace.
> 
> This adds the PRINT_RAW_FRAME_ARGUMENTS flag to frame_filter_flag which is
> then used in the frame-filter to override the raw flag in enumerate_args.
> 
> gdb/ChangeLog:
> 
> 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* extension.h (enum frame_filter_flag): Add
> 	PRINT_RAW_FRAME_ARGUMENTS.
> 	* python/py-framefilter.c (enumerate_args): Override raw flag.
> 	raw_frame_args argument.
> 	(py_mi_print_variables): Forward raw flag.
> 	(py_print_args): Forward raw_frame_args flag.
> 	(py_print_frame): Handle PRINT_RAW_FRAME_ARGUMENTS.
> 	* stack.c (backtrace_command_1): Set PRINT_RAW_FRAME_ARGUMENTS.
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-12-29  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* gdb.python/py-frame-args.exp: Add bt raw-frame-arguments tests.
> 	* gdb.python/py-frame-args.py: Add basic frame-filter.

I'm sure someone more versed in the Python extension API would
immediately understand what you mean. Unfortunately, we seeem to be
in short supply of this at the moment, as highlighted by the lack
of meaninful review. Simon seems to have tried to look at the patch,
and so did I, and one barrier to the review is the fact that we need
to understand what problem exactly you are trying to resolve. I think
an example, with behavior behavior, why this is wrong, and behavior
after, would go a long way towards lowering that barrier...

In fact, speaking more generally, I think that commit messages that
do not make any assumption as to the level of expertise of the
person looking at the patch are always desirable, as they are more
inclusive (being accessible to a larger population).

If you can provide that, I will try to review your patch next weekend
if someone else hasn't done so already.

> ---
>  gdb/extension.h                            |  4 ++++
>  gdb/python/py-framefilter.c                | 14 ++++++++++----
>  gdb/stack.c                                |  2 ++
>  gdb/testsuite/gdb.python/py-frame-args.exp | 22 ++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-frame-args.py  | 13 +++++++++++++
>  5 files changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/extension.h b/gdb/extension.h
> index c840dbc704..f3ec61b163 100644
> --- a/gdb/extension.h
> +++ b/gdb/extension.h
> @@ -103,6 +103,10 @@ enum frame_filter_flag
>  
>      /* Set this flag if elided frames should not be printed.  */
>      PRINT_HIDE = 1 << 5,
> +
> +    /* Set this flag if pretty printers for frame arguments should not
> +       be invoked.  */
> +    PRINT_RAW_FRAME_ARGUMENTS = 1 << 6,
>    };
>  
>  DEF_ENUM_FLAGS_TYPE (enum frame_filter_flag, frame_filter_flags);
> diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
> index 944a0eee50..c61bfa4c2e 100644
> --- a/gdb/python/py-framefilter.c
> +++ b/gdb/python/py-framefilter.c
> @@ -422,12 +422,14 @@ static enum ext_lang_bt_status
>  enumerate_args (PyObject *iter,
>  		struct ui_out *out,
>  		enum ext_lang_frame_args args_type,
> +		bool raw_frame_args,
>  		int print_args_field,
>  		struct frame_info *frame)
>  {
>    struct value_print_options opts;
>  
>    get_user_print_options (&opts);
> +  opts.raw = raw_frame_args;
>  
>    if (args_type == CLI_SCALAR_VALUES)
>      {
> @@ -655,8 +657,8 @@ py_mi_print_variables (PyObject *filter, struct ui_out *out,
>    ui_out_emit_list list_emitter (out, "variables");
>  
>    if (args_iter != Py_None
> -      && (enumerate_args (args_iter.get (), out, args_type, 1, frame)
> -	  == EXT_LANG_BT_ERROR))
> +      && (enumerate_args (args_iter.get (), out, args_type, opts->raw, 1,
> +			  frame) == EXT_LANG_BT_ERROR))
>      return EXT_LANG_BT_ERROR;
>  
>    if (locals_iter != Py_None
> @@ -701,6 +703,7 @@ static enum ext_lang_bt_status
>  py_print_args (PyObject *filter,
>  	       struct ui_out *out,
>  	       enum ext_lang_frame_args args_type,
> +	       bool raw_frame_args,
>  	       struct frame_info *frame)
>  {
>    gdbpy_ref<> args_iter (get_py_iter_from_func (filter, "frame_args"));
> @@ -726,7 +729,8 @@ py_print_args (PyObject *filter,
>  	}
>      }
>    else if (args_iter != Py_None
> -	   && (enumerate_args (args_iter.get (), out, args_type, 0, frame)
> +	   && (enumerate_args (args_iter.get (), out, args_type,
> +			       raw_frame_args, 0, frame)
>  	       == EXT_LANG_BT_ERROR))
>      return EXT_LANG_BT_ERROR;
>  
> @@ -957,7 +961,9 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
>       wrong.  */
>    if (print_args && (location_print || out->is_mi_like_p ()))
>      {
> -      if (py_print_args (filter, out, args_type, frame) == EXT_LANG_BT_ERROR)
> +      bool raw_frame_args = (flags & PRINT_RAW_FRAME_ARGUMENTS) != 0;
> +      if (py_print_args (filter, out, args_type, raw_frame_args, frame)
> +	  == EXT_LANG_BT_ERROR)
>  	return EXT_LANG_BT_ERROR;
>      }
>  
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 943b3db087..07abdea8c1 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -2028,6 +2028,8 @@ backtrace_command_1 (const frame_print_options &fp_opts,
>      flags |= PRINT_LOCALS;
>    if (bt_opts.hide)
>      flags |= PRINT_HIDE;
> +  if (fp_opts.print_raw_frame_arguments)
> +    flags |= PRINT_RAW_FRAME_ARGUMENTS;
>  
>    if (!bt_opts.no_filters)
>      {
> diff --git a/gdb/testsuite/gdb.python/py-frame-args.exp b/gdb/testsuite/gdb.python/py-frame-args.exp
> index fd9c1f4342..7c621e1302 100644
> --- a/gdb/testsuite/gdb.python/py-frame-args.exp
> +++ b/gdb/testsuite/gdb.python/py-frame-args.exp
> @@ -34,6 +34,28 @@ gdb_test_no_output "source ${remote_python_file}" "load python file"
>  gdb_breakpoint [gdb_get_line_number "break-here"]
>  gdb_continue_to_breakpoint "break-here" ".* break-here .*"
>  
> +# Test raw-frame-arguments on backtrace with and without frame-filter
> +foreach filtered [list "enable" "disable"] {
> +    gdb_test_no_output "$filtered frame-filter global BasicFrameFilter"
> +
> +    gdb_test "bt 1" \
> +	".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
> +	"bt frame-filter=$filtered,pretty"
> +
> +    gdb_test "bt -raw-frame-arguments on 1" \
> +	".*foo \\(x=42, ss=\[.\]{3}\\).*" \
> +	"bt frame-filter=$filtered,raw"
> +
> +    # "set print raw-values" should not affect frame arguments
> +    gdb_test_no_output "set print raw-values on" \
> +	"raw-values-on,frame-filter=$filtered"
> +    gdb_test "bt 1" \
> +	".*foo \\(x=42, ss=super struct = {\[.\]{3}}\\).*" \
> +	"bt frame-filter=$filtered,pretty,raw-values"
> +    gdb_test_no_output "set print raw-values off" \
> +	"raw-values-off,frame-filter=$filtered"
> +}
> +
>  # Test all combinations with raw off.
>  
>  gdb_test_no_output "set print raw-frame-arguments off"
> diff --git a/gdb/testsuite/gdb.python/py-frame-args.py b/gdb/testsuite/gdb.python/py-frame-args.py
> index c2908829c5..aed2070d94 100644
> --- a/gdb/testsuite/gdb.python/py-frame-args.py
> +++ b/gdb/testsuite/gdb.python/py-frame-args.py
> @@ -73,3 +73,16 @@ pretty_printers_dict = {}
>  
>  register_pretty_printers ()
>  gdb.pretty_printers.append (lookup_function)
> +
> +
> +class BasicFrameFilter (object):
> +    def __init__ (self):
> +        self.name = "BasicFrameFilter"
> +        self.priority = 100
> +        self.enabled = True
> +        gdb.frame_filters[self.name] = self
> +
> +    def filter (self, frame_iter):
> +        return frame_iter
> +
> +BasicFrameFilter ()
> -- 
> 2.29.2

-- 
Joel