[v2,PR,cli/22573] Honour 'print pretty' when printing result of finish command
Commit Message
On Wed, Jun 13, 2018 at 04:50:06PM +0100, Pedro Alves wrote:
> On 06/09/2018 06:07 PM, Tom de Vries wrote:
> > Hi,
> >
> > this fixes PR22573 - "finish" doesn't respect "set print pretty".
> >
> >
> > Consider this testcase:
> > ...
> > struct s {
> > int a;
> > int b;
> > };
> >
> > struct s foo ()
> > {
> > struct s r;
> > r.a = 1;
> > r.b = 2;
> > return r;
> > }
> >
> > int
> > main (void)
> > {
> > struct s v;
> > v = foo ();
> > return v.a + v.b;
> > }
> > ...
> >
> > When we compile it with -g, load the exec with gdb, and run till the end of foo,
> > we can print r:
> > ...
> > (gdb) p r
> > $1 = {a = 1, b = 2}
> > ...
> >
> > and by setting pretty printing to on, we can get the fields of r printed each
> > on its own line:
> > ...
> > (gdb) set print pretty
> > (gdb) p r
> > $2 = {
> > a = 1,
> > b = 2
> > }
> > ...
> >
> > However, when we finish foo, the printed function result value is not using
> > the pretty printing setting:
> > ...
> > (gdb) finish
> > Run till exit from #0 foo () at test.c:11
> > 0x00000000004004c1 in main () at test.c:18
> > 18 v = foo ();
> > Value returned is $3 = {a = 1, b = 2}
> > ...
> >
> > This patch fixes that by using get_user_print_options instead of
> > get_no_prettyformat_print_options in print_return_value_1, which gives us:
> > ...
> > (gdb) finish
> > Run till exit from #0 foo () at test.c:11
> > 0x00000000004004c1 in main () at test.c:18
> > 18 v = foo ();
> > Value returned is $2 = {
> > a = 1,
> > b = 2
> > }
> > ...
> >
> > Build & reg-tested on x86_64.
> >
> > OK for trunk?
> >
> > Thanks,
> > - Tom
> >
> > [gdb/cli] Honour 'print pretty' when printing result of finish command
> >
> > 2018-06-08 Tom de Vries <tdevries@suse.de>
> >
> > PR cli/22573
> > * infcmd.c (print_return_value_1): Use get_user_print_options instead of
> > get_no_prettyformat_print_options.
> >
> > * gdb.base/finish-pretty.c: New test.
> > * gdb.base/finish-pretty.exp: New file.
>
> Just in case, note that gdb/testsuite/ has its own ChangeLog.
>
Hi,
Indeed. For gcc I've been using a pre-commit script (
https://github.com/vries/bin-scripts/blob/master/git-prepare-gnu-commit.sh )
that:
- updates the date in the log message
- copies all ChangeLog hunks to the appropriate ChangeLog files, prefixing
each hunk with the date/author line and the PR line
So, the log message is not explicit about which files the ChangeLog hunks are
meant for, but the script makes sure they end up where they should.
> > diff --git a/gdb/testsuite/gdb.base/finish-pretty.c b/gdb/testsuite/gdb.base/finish-pretty.c
> > new file mode 100644
> > index 0000000000..5d30b1c16d
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/finish-pretty.c
> > @@ -0,0 +1,24 @@
>
> Please add a copyright header.
>
Done.
> > +struct s
> > +{
> > + int a;
> > + int b;
> > +};
> > +
> > +static struct s __attribute((noinline, noclone))
>
> Do we need that? (Tests are built with -O0.)
>
Removed.
> > +foo (void)
> > +{
> > + struct s r;
> > + r.a = 1;
> > + r.b = 2;
> > + return r;
> > +}
> > +
> > +int
> > +main (void)
> > +{
> > + struct s v;
> > +
> > + v = foo ();
> > +
> > + return v.a + v.b;
> > +}
> > diff --git a/gdb/testsuite/gdb.base/finish-pretty.exp b/gdb/testsuite/gdb.base/finish-pretty.exp
> > new file mode 100644
> > index 0000000000..52852c7bad
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/finish-pretty.exp
> > @@ -0,0 +1,34 @@
> > +# Copyright 2018 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/>.
> > +
>
> Please add small intro comment describing what the testcase is about.
>
Done.
> > +if { [prepare_for_testing "failed to prepare" finish-pretty finish-pretty.c] } {
> > + return -1
> > +}
>
> Write:
>
> standard_testfile
>
> if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> return -1
> }
>
Done.
>
>
> > +
> > +proc finish_pretty { } {
> > + global gdb_prompt
>
> Not used, AFAICT.
>
Removed.
> > +
> > + gdb_test "break foo" "Breakpoint \[0123456789\].*" \
> > + "set break on foo"
> > + gdb_test "run" "Breakpoint.* foo.*" \
> > + "run to foo"
>
> Please don't use naked "run" unless required for the testcase.
> It won't work when testing against "target remote".
>
Aha, I see.
> You can try that with:
>
> make check-parallel \
> RUNTESTFLAGS="--target_board=native-gdbserver" TESTS="*/finish-pretty.exp"
>
That's useful, thanks.
> Instead, use "runto_main", or better, since you want to run to "foo",
> use "runto foo", like:
>
> if ![runto foo] {
> fail "can't run to foo"
> return
> }
>
Done.
[ FWIW, I've just found GDBTestcaseCookbook on the wiki, and saw that
template.exp uses untested instead of fail in such a situation, I'm not sure
how relevant the difference is. ]
> > + gdb_test "set print pretty" ".*" \
> > + "pretty printing switched on"
>
> Use gdb_test_no_output.
>
Done.
Thanks for the review.
- Tom
> Please resend a v2 with the issue addressed, and it should be
> ready to go.
[gdb/cli] Honour 'print pretty' when printing result of finish command
2018-06-08 Tom de Vries <tdevries@suse.de>
PR cli/22573
* infcmd.c (print_return_value_1): Use get_user_print_options instead of
get_no_prettyformat_print_options.
* gdb.base/finish-pretty.c: New test.
* gdb.base/finish-pretty.exp: New file.
---
gdb/infcmd.c | 2 +-
gdb/testsuite/gdb.base/finish-pretty.c | 41 ++++++++++++++++++++++++++++++++
gdb/testsuite/gdb.base/finish-pretty.exp | 37 ++++++++++++++++++++++++++++
3 files changed, 79 insertions(+), 1 deletion(-)
Comments
On 06/14/2018 10:47 AM, Tom de Vries wrote:
> Indeed. For gcc I've been using a pre-commit script (
> https://github.com/vries/bin-scripts/blob/master/git-prepare-gnu-commit.sh )
> that:
> - updates the date in the log message
> - copies all ChangeLog hunks to the appropriate ChangeLog files, prefixing
> each hunk with the date/author line and the PR line
>
> So, the log message is not explicit about which files the ChangeLog hunks are
> meant for, but the script makes sure they end up where they should.
I see. FYI, most folks here follow the convention at
<https://sourceware.org/gdb/wiki/ContributionChecklist>
("In your patch email (...)").
> [ FWIW, I've just found GDBTestcaseCookbook on the wiki, and saw that
> template.exp uses untested instead of fail in such a situation, I'm not sure
> how relevant the difference is. ]
Not much.
>> Please resend a v2 with the issue addressed, and it should be
>> ready to go.
>
> [gdb/cli] Honour 'print pretty' when printing result of finish command
OK with the typo below fixed. Please make sure the git commit log
includes the rationale [1], and push.
>
> 2018-06-08 Tom de Vries <tdevries@suse.de>
>
> PR cli/22573
> * infcmd.c (print_return_value_1): Use get_user_print_options instead of
> get_no_prettyformat_print_options.
>
> * gdb.base/finish-pretty.c: New test.
> * gdb.base/finish-pretty.exp: New file.
> +
> +# Check whether finish respect the print pretty user setting when printing the
> +# function result.
"respect" -> "respects"
Thanks,
Pedro Alves
[1] - sorry to be a nag, but your previous push did not include it. :-)
On 06/14/2018 12:55 PM, Pedro Alves wrote:
> OK with the typo below fixed. Please make sure the git commit log
> includes the rationale [1], and push.
>
> [1] - sorry to be a nag, but your previous push did not include it. :-)
No problem, much appreciated.
Currently I add the rationale manually pre-commit, so it's something
that I might forget. I need to update my pre-commit script to handle
this log format, and then I'll try to train myself to add the rationale
in the log before submission.
Thanks,
- Tom
@@ -1687,7 +1687,7 @@ print_return_value_1 (struct ui_out *uiout, struct return_value_info *rv)
uiout->field_fmt ("gdb-result-var", "$%d",
rv->value_history_index);
uiout->text (" = ");
- get_no_prettyformat_print_options (&opts);
+ get_user_print_options (&opts);
string_file stb;
new file mode 100644
@@ -0,0 +1,41 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2018 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/>. */
+
+struct s
+{
+ int a;
+ int b;
+};
+
+static struct s
+foo (void)
+{
+ struct s r;
+ r.a = 1;
+ r.b = 2;
+ return r;
+}
+
+int
+main (void)
+{
+ struct s v;
+
+ v = foo ();
+
+ return v.a + v.b;
+}
new file mode 100644
@@ -0,0 +1,37 @@
+# Copyright 2018 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/>.
+
+# Check whether finish respect the print pretty user setting when printing the
+# function result.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+ return -1
+}
+
+proc finish_pretty { } {
+ if ![runto foo] {
+ fail "can't run to foo"
+ return
+ }
+ gdb_test_no_output "set print pretty" \
+ "pretty printing switched on"
+ gdb_test "finish" \
+ {.*Value returned is \$1 = \{\r\n a = 1, \r\n b = 2\r\n\}} \
+ "finish foo prettyprinted function result"
+}
+
+finish_pretty