Bug 29554 - --simple-values does not take reference types into account
Summary: --simple-values does not take reference types into account
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: mi (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 14.1
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-09-07 15:46 UTC by Gareth Rees
Modified: 2023-05-04 16:32 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Patch against 154f2735ad4 (3.33 KB, patch)
2022-09-07 15:46 UTC, Gareth Rees
Details | Diff
Corrected patch against 154f2735ad (3.33 KB, patch)
2022-09-08 06:55 UTC, Gareth Rees
Details | Diff
Patch against 154f2735ad implementing solution (2) (8.17 KB, patch)
2022-09-09 08:09 UTC, Gareth Rees
Details | Diff
Corrected patch against 154f2735ad implementing solution (2) (8.16 KB, patch)
2022-10-04 09:05 UTC, Gareth Rees
Details | Diff
Corrected patch against 154f2735ad implementing solution (2) (9.01 KB, patch)
2022-10-20 17:49 UTC, Gareth Rees
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gareth Rees 2022-09-07 15:46:27 UTC
Created attachment 14321 [details]
Patch against 154f2735ad4

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

The attached 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.
Comment 1 Gareth Rees 2022-09-08 06:55:56 UTC
Created attachment 14324 [details]
Corrected patch against 154f2735ad

My first patch was backwards due to getting the "git diff" command wrong. New patch is the right way round.
Comment 2 Gareth Rees 2022-09-09 08:09:58 UTC
Created attachment 14329 [details]
Patch against 154f2735ad implementing solution (2)

Here's an alternative patch implementing solution (2), adding a
'--scalar-values' option for the PRINT-VALUES argument to
'-stack-list-arguments' and similar commands. This option prints the
value only for scalars and so matches the behaviour of the 'scalars'
argument to the 'set print frame-arguments' command. References to
structures are not scalars, and so the option is suitable for use by
IDEs.
Comment 3 Gareth Rees 2022-10-04 09:05:20 UTC
Created attachment 14379 [details]
Corrected patch against 154f2735ad implementing solution (2)

Here's a patch implementing solution (2), corrected after review by Eli Zaretskii.
Comment 4 Gareth Rees 2022-10-20 17:49:32 UTC
Created attachment 14408 [details]
Corrected patch against 154f2735ad implementing solution (2)

Here's a patch implementing solution (2), corrected after review by Eli Zaretskii and Andrew Burgess.
Comment 5 Sourceware Commits 2023-05-04 15:08:30 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=51f8dafba8175a8d59716f220a46de4e626e5073

commit 51f8dafba8175a8d59716f220a46de4e626e5073
Author: Gareth Rees <grees@undo.io>
Date:   Sat Mar 11 11:49:34 2023 +0000

    Don't treat references to compound values as "simple".
    
    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, '--scalar-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 '--scalar-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
    contrary to reasonable expectation.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29554
Comment 6 Tom Tromey 2023-05-04 16:32:24 UTC
I think this is fixed now.