public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Andrew Burgess <aburgess@redhat.com>
To: Gareth Rees <grees@undo.io>, gdb-patches@sourceware.org
Subject: Re: [PATCH] [gdb/mi] Don't treat references to compound values as "simple".
Date: Thu, 08 Sep 2022 11:27:49 +0100	[thread overview]
Message-ID: <877d2ema8q.fsf@redhat.com> (raw)
In-Reply-To: <20220908071831.10166-1-grees@undo.io>


Thanks for this patch.  I have a few notes, in line below.

Gareth Rees via Gdb-patches <gdb-patches@sourceware.org> writes:

> SUMMARY
>
> The '--simple-values' argument to '-stack-list-arguments' and similar
> GDB/MI commands does not take reference types into account, so that
> references to arbitrarily large structures are considered "simple" and
> printed. This means that the '--simple-values' argument cannot be used
> by IDEs when tracing the stack due to the time taken to print large
> structures passed by reference.
>
> DETAILS
>
> Various GDB/MI commands ('-stack-list-arguments', '-stack-list-locals',
> '-stack-list-variables' and so on) take a PRINT-VALUES argument which
> may be '--no-values' (0), '--all-values' (1) or '--simple-values' (2).
> In the '--simple-values' case, the command is supposed to print the
> name, type, and value of variables with simple types, and print only the
> name and type of variables with compound types.
>
> The '--simple-values' argument ought to be suitable for IDEs that need
> to update their user interface with the program's call stack every time
> the program stops. However, it does not take C++ reference types into
> account, and this makes the argument unsuitable for this purpose.
>
> For example, consider the following C++ program:
>
>     struct s {
>         int v[10];
>     };
>
>     int
>     sum(const struct s &s)
>     {
>         int total = 0;
>         for (int i = 0; i < 10; ++i) total += s.v[i];
>         return total;
>     }
>
>     int
>     main(void)
>     {
>         struct s s = { { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 } };
>         return sum(s);
>     }
>
> If we start GDB in MI mode and continue to 'sum', the behaviour of
> '-stack-list-arguments' is as follows:
>
>     (gdb)
>     -stack-list-arguments --simple-values
>     ^done,stack-args=[frame={level="0",args=[{name="s",type="const s &",value="@0x7fffffffe310: {v = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}}"}]},frame={level="1",args=[]}]
>
> Note that the value of the argument 's' was printed, even though 's' is
> a reference to a structure, which is not a simple value.
>
> See https://github.com/microsoft/MIEngine/pull/673 for a case where this
> behaviour caused Microsoft to avoid the use of '--simple-values' in
> their MIEngine debug adapter, because it caused Visual Studio Code to
> take too long to refresh the call stack in the user interface.
>
> SOLUTIONS
>
> There are two ways we could fix this problem, depending on whether we
> consider the current behaviour to be a bug.
>
> 1. If the current behaviour is a bug, then we can update the behaviour
>    of '--simple-values' so that it takes reference types into account:
>    that is, a value is simple if it is neither an array, struct, or
>    union, nor a reference to an array, struct or union.
>
>    In this case we must add a feature to the '-list-features' command so
>    that IDEs can detect that it is safe to use the '--simple-values'
>    argument when refreshing the call stack.
>
> 2. If the current behaviour is not a bug, then we can add a new option
>    for the PRINT-VALUES argument, for example, '--simplest-values' (3),
>    that would be suitable for use by IDEs.
>
>    In this case we must add a feature to the '-list-features' command so
>    that IDEs can detect that the '--simplest-values' argument is
>    available for use when refreshing the call stack.
>
> PATCH
>
> This patch implements solution (1) as I think the current behaviour of
> not printing structures, but printing references to structures, is
> surprising. However, if you prefer solution (2) I would be happy to
> implement that instead.

I'm agree that this feels like a bug, and that solution #1 seems like
the way to go.

> ---
>  gdb/NEWS                                     | 12 ++++++
>  gdb/doc/gdb.texinfo                          |  7 ++++
>  gdb/mi/mi-cmd-stack.c                        |  6 +--
>  gdb/mi/mi-cmd-var.c                          | 22 ++++++----
>  gdb/mi/mi-cmds.h                             |  5 +++
>  gdb/mi/mi-main.c                             |  7 +---
>  gdb/python/py-framefilter.c                  |  6 +--
>  gdb/testsuite/gdb.mi/print-simple-values.cc  | 44 ++++++++++++++++++++
>  gdb/testsuite/gdb.mi/print-simple-values.exp | 40 ++++++++++++++++++
>  9 files changed, 126 insertions(+), 23 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.mi/print-simple-values.cc
>  create mode 100644 gdb/testsuite/gdb.mi/print-simple-values.exp
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index dee0ac2ecd8..855f42a9549 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -114,6 +114,18 @@ maintenance info line-table
>    entry corresponds to an address where a breakpoint should be placed
>    to be at the first instruction past a function's prologue.
>  
> +* MI changes
> +
> +  ** The '--simple-values' argument to the '-stack-list-arguments',
> +     '-stack-list-locals', '-stack-list-variables', and
> +     '-var-list-children' commands takes reference types into account:
> +     that is, a value is now considered simple if it is neither an
> +     array, structure, or union, nor a reference to an array, structure,
> +     or union. (Previously all references were considered simple.)
> +     Support for this feature can be verified by using the
> +     '-list-features' command, which should contain
> +     "simple-values-ref-types".
> +
>  * New targets
>  
>  GNU/Linux/LoongArch (gdbserver)	loongarch*-*-linux*
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 238a49b027d..5ccf6609709 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -37295,6 +37295,13 @@ option (@pxref{GDB/MI Program Execution}).
>  @item data-disassemble-a-option
>  Indicates that the @code{-data-disassemble} command supports the @option{-a}
>  option (@pxref{GDB/MI Data Manipulation}).
> +@item simple-values-ref-types
> +Indicates that the @code{--simple-values} argument to the
> +@code{-stack-list-arguments}, @code{-stack-list-locals},
> +@code{-stack-list-variables}, and @code{-var-list-children} commands
> +takes reference types into account: that is, a value is considered
> +simple if it neither an array, structure, or union, nor a reference to
> +an array, structure, or union.
>  @end ftable
>  
>  @subheading The @code{-list-target-features} Command
> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index 0fe204dbc66..f8f4ad266d0 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -571,7 +571,6 @@ list_args_or_locals (const frame_print_options &fp_opts,
>    const struct block *block;
>    struct symbol *sym;
>    struct block_iterator iter;
> -  struct type *type;
>    const char *name_of_result;
>    struct ui_out *uiout = current_uiout;
>  
> @@ -650,10 +649,7 @@ list_args_or_locals (const frame_print_options &fp_opts,
>  	      switch (values)
>  		{
>  		case PRINT_SIMPLE_VALUES:
> -		  type = check_typedef (sym2->type ());
> -		  if (type->code () != TYPE_CODE_ARRAY
> -		      && type->code () != TYPE_CODE_STRUCT
> -		      && type->code () != TYPE_CODE_UNION)
> +                  if (mi_simple_values_type_p (sym2->type ()))
>  		    {
>  		case PRINT_ALL_VALUES:
>  		  if (sym->is_argument ())
> diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
> index 3db09cf7815..f9d332d13a4 100644
> --- a/gdb/mi/mi-cmd-var.c
> +++ b/gdb/mi/mi-cmd-var.c
> @@ -335,15 +335,21 @@ mi_print_value_p (struct varobj *var, enum print_values print_values)
>    if (type == NULL)
>      return 1;
>    else
> -    {
> -      type = check_typedef (type);
> +    return mi_simple_values_type_p (type);
> +}
>  
> -      /* For PRINT_SIMPLE_VALUES, only print the value if it has a type
> -	 and that type is not a compound type.  */
> -      return (type->code () != TYPE_CODE_ARRAY
> -	      && type->code () != TYPE_CODE_STRUCT
> -	      && type->code () != TYPE_CODE_UNION);
> -    }
> +int
> +mi_simple_values_type_p (struct type *type)

This new function should have a comment like:

  /* See mi-cmds.h.  */

And also, should be returning `bool`.

> +{
> +  type = check_typedef (type);
> +
> +  if (type->code () == TYPE_CODE_REF || type->code () == TYPE_CODE_RVALUE_REF)
> +    type = TYPE_TARGET_TYPE (type);
> +
> +  return (type

Checks for nullptr should be written as:

  type != nullptr

but, can you ever get nullptr here?  Certainly, if type is nullptr on
entry then an assert in check_typedef will trigger, I'd expect that
after calling check_typedef and TYPE_TARGET_TYPE you should still have a
valid type pointer ... but maybe there's some case you've seen where
this is not true?

Unless you know a case where type could be nullptr I would rather the
type check be moved into a separate:

  gdb_assert (type != nullptr);

before the return.

> +          && type->code () != TYPE_CODE_ARRAY
> +          && type->code () != TYPE_CODE_STRUCT
> +          && type->code () != TYPE_CODE_UNION);
>  }
>  
>  void
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index 9ffb11bf997..81a5578ca1c 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -226,4 +226,9 @@ using remove_mi_cmd_entries_ftype
>    = gdb::function_view<bool (mi_command *)>;
>  extern void remove_mi_cmd_entries (remove_mi_cmd_entries_ftype callback);
>  
> +/* Return 1 if type is suitable for printing with PRINT_SIMPLE_VALUES, that is,
> +   if type is neither a compound type nor a reference to a compound type. */
> +
> +extern int mi_simple_values_type_p (struct type *type);

Update the comment to reflect an update of the return type to bool.

> +
>  #endif /* MI_MI_CMDS_H */
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index b758f398e2a..dd1962fd0e2 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1663,6 +1663,7 @@ mi_cmd_list_features (const char *command, char **argv, int argc)
>        uiout->field_string (NULL, "undefined-command-error-code");
>        uiout->field_string (NULL, "exec-run-start-option");
>        uiout->field_string (NULL, "data-disassemble-a-option");
> +      uiout->field_string (NULL, "simple-values-ref-types");
>  
>        if (ext_lang_initialized_p (get_ext_lang_defn (EXT_LANG_PYTHON)))
>  	uiout->field_string (NULL, "python");
> @@ -2462,7 +2463,6 @@ static void
>  print_variable_or_computed (const char *expression, enum print_values values)
>  {
>    struct value *val;
> -  struct type *type;
>    struct ui_out *uiout = current_uiout;
>  
>    string_file stb;
> @@ -2482,12 +2482,9 @@ print_variable_or_computed (const char *expression, enum print_values values)
>    switch (values)
>      {
>      case PRINT_SIMPLE_VALUES:
> -      type = check_typedef (value_type (val));
>        type_print (value_type (val), "", &stb, -1);
>        uiout->field_stream ("type", stb);
> -      if (type->code () != TYPE_CODE_ARRAY
> -	  && type->code () != TYPE_CODE_STRUCT
> -	  && type->code () != TYPE_CODE_UNION)
> +      if (mi_simple_values_type_p (value_type (val)))
>  	{
>  	  struct value_print_options opts;
>  
> diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
> index 366f3745b9d..227a9ecb4f1 100644
> --- a/gdb/python/py-framefilter.c
> +++ b/gdb/python/py-framefilter.c
> @@ -235,14 +235,10 @@ py_print_value (struct ui_out *out, struct value *val,
>    if (args_type == MI_PRINT_SIMPLE_VALUES
>        || args_type == MI_PRINT_ALL_VALUES)
>      {
> -      struct type *type = check_typedef (value_type (val));
> -
>        if (args_type == MI_PRINT_ALL_VALUES)
>  	should_print = 1;
>        else if (args_type == MI_PRINT_SIMPLE_VALUES
> -	       && type->code () != TYPE_CODE_ARRAY
> -	       && type->code () != TYPE_CODE_STRUCT
> -	       && type->code () != TYPE_CODE_UNION)
> +	       && mi_simple_values_type_p (value_type (val)))
>  	should_print = 1;
>      }
>    else if (args_type != NO_VALUES)
> diff --git a/gdb/testsuite/gdb.mi/print-simple-values.cc b/gdb/testsuite/gdb.mi/print-simple-values.cc
> new file mode 100644
> index 00000000000..15567c740a0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/print-simple-values.cc
> @@ -0,0 +1,44 @@
> +/* This test case is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* PRINT_SIMPLE_VALUES test program.
> +
> +   Test that PRINT_SIMPLE_VALUES handles C++ reference types correctly.
> +
> +   In the function f(), arguments a, b, and c are either ints or references to
> +   int, and their values should be printed by PRINT_SIMPLE_VALUES, but arguments
> +   s, t, and u are structures or references to structures, and their values
> +   should not be printed by PRINT_SIMPLE_VALUES. */

Two spaces before '*/' to close this comment.

Test programs should follow GNU style unless specifically required not
to for some test reason.

> +
> +struct s
> +{
> +    int v;

It seems like everything is over indented in the test, 4 spaces instead
of 2.

> +};
> +
> +int
> +f(int a, int &b, int &&c, struct s s, struct s &t, struct s &&u)

Space after 'f'.

> +{
> +    return a + b + c + s.v + t.v + u.v;

Indentation.

> +}
> +
> +int
> +main(void)

Space after 'main'.

> +{
> +    int a = 1, b = 2;
> +    struct s s = { 4 }, t = { 5 };
> +    return f(a, b, 3, s, t, (struct s){ 6 });

Space after 'f' and after '(struct s)' (I think).

> +}
> diff --git a/gdb/testsuite/gdb.mi/print-simple-values.exp b/gdb/testsuite/gdb.mi/print-simple-values.exp
> new file mode 100644
> index 00000000000..c1b5a54ad3b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/print-simple-values.exp
> @@ -0,0 +1,40 @@
> +# Copyright 2022 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +

Test scripts should have a brief summary here of what the test is
checking.  This will be similar to the comment you have in the source
file, so maybe you just want to move that here, but you can keep the
source comment and add an extra description here, whatever works.

> +if { [skip_cplus_tests] } { continue }
> +
> +load_lib mi-support.exp
> +standard_testfile .cc
> +
> +if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable {debug c++}] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +mi_clean_restart $binfile
> +
> +mi_runto_main
> +
> +mi_gdb_test "-break-insert f" "\\^done.*" "set breakpoint on f"
> +
> +mi_send_resuming_command "exec-continue" "exec-continue to breakpoint on f"
> +
> +mi_expect_stop "breakpoint-hit" "f" ".*" ".*" ".*" {.* disp="keep"} "run until breakpoint on f"

We still try to wrap lines to under 80 characters when possible, so
here, and maybe the previous line too should be wrapped.

> +
> +mi_gdb_test "-stack-list-arguments 2" \
> +    "\\^done,stack-args=\\\[frame=\{level=\"0\",args=\\\[\{name=\"a\",type=\"int\",value=\"1\"\},\{name=\"b\",type=\"int &\",value=\"@0x\[0-9a-f\]+: 2\"\},\{name=\"c\",type=\"int &&\",value=\"@0x\[0-9a-f\]+: 3\"\},\{name=\"s\",type=\"s\"\},\{name=\"t\",type=\"s &\"\},\{name=\"u\",type=\"s &&\"\}\\\]\},frame=\{level=\"1\",args=\\\[\\\]\}\\\]" \
> +    "stack list arguments, simple types: names, types and values, compound types: names and types"

But lines like these can't easily be wrapped, so its fine for these to exist.

Thanks,
Andrew

> +
> +mi_gdb_exit
> -- 
> 2.26.0


  reply	other threads:[~2022-09-08 10:27 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-08  7:18 Gareth Rees
2022-09-08 10:27 ` Andrew Burgess [this message]
2022-09-08 11:02   ` [PATCH v2] " Gareth Rees
2022-09-08 13:30     ` Eli Zaretskii
2022-09-08 13:58       ` Gareth Rees
2022-09-08 14:07         ` Eli Zaretskii
2022-09-09  8:01       ` [PATCH v3] [PR mi/29554] New PRINT-VALUES option '--scalar-values' Gareth Rees
2022-09-15  9:06         ` [PING] " Gareth Rees
2022-09-25  8:15         ` Gareth Rees
2022-09-25  8:25           ` Eli Zaretskii
2022-09-25  9:00             ` Gareth Rees
2022-09-25 10:16               ` Eli Zaretskii
2022-09-26 12:48                 ` Gareth Rees
2022-09-25 10:16           ` Eli Zaretskii
2022-09-26 12:46         ` [PATCH v4] " Gareth Rees
2022-10-04  9:08           ` [PING] " Gareth Rees
2022-10-18 11:59             ` Gareth Rees
2022-10-12 16:38           ` Andrew Burgess
2022-10-20 17:47             ` [PATCH v5] " Gareth Rees
2022-10-20 18:00               ` Eli Zaretskii
2022-11-03 16:20               ` [PING] " Gareth Rees
2022-11-14  9:25                 ` Gareth Rees
2022-12-01 13:41                 ` Gareth Rees
2022-12-14  8:50                 ` Gareth Rees
2023-02-01 10:00                 ` Gareth Rees
2023-02-16 10:08                 ` Gareth Rees
2023-03-06  9:52                 ` Gareth Rees
2023-03-08 12:35               ` Andrew Burgess
2023-03-10 11:04                 ` Gareth Rees
2023-03-10 12:05                   ` Eli Zaretskii
2023-03-10 12:58                     ` Gareth Rees
2023-03-13 17:17                     ` Andrew Burgess
2023-03-16 12:28                       ` Gareth Rees
2023-03-11 11:58                   ` Gareth Rees
2023-04-11 13:15                     ` Pedro Alves
2023-03-11 11:49               ` [PATCH v6] [gdb/mi] Don't treat references to compound values as "simple" Gareth Rees
2023-03-21  9:50                 ` [PING] " Gareth Rees
2023-03-26  9:56                   ` Gareth Rees
2023-04-03  9:22                     ` Gareth Rees
2023-05-04 15:08                       ` Tom Tromey
2023-04-18  9:23                   ` Gareth Rees
2023-04-24  9:53                   ` Gareth Rees
2023-05-02  9:13                   ` Gareth Rees
2023-03-27 14:34                 ` Tom Tromey
2023-03-29  9:14                   ` Gareth Rees
2023-04-06 17:18                   ` Gareth Rees
2022-10-20 17:58             ` [PATCH v4] [PR mi/29554] New PRINT-VALUES option '--scalar-values' Gareth Rees
2022-09-09  8:04       ` [PATCH v2] [gdb/mi] Don't treat references to compound values as "simple" Gareth Rees
2022-09-08 11:09   ` [PATCH] " Gareth Rees

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=877d2ema8q.fsf@redhat.com \
    --to=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=grees@undo.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).