From: Bruno Larsen <blarsen@redhat.com>
To: Joel Brobecker <brobecker@adacore.com>,
Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>
Subject: Re: [PATCH] [gdb/testsuite] test a function call by hand fron pretty printer
Date: Mon, 28 Feb 2022 09:13:30 -0300 [thread overview]
Message-ID: <edcc19f5-51d1-0815-23a4-b7a35fa76141@redhat.com> (raw)
In-Reply-To: <Yhtw9P11f8drAv5s@adacore.com>
Hello Joel
On 2/27/22 09:39, Joel Brobecker via Gdb-patches wrote:
> Hello Bruno,
>
> On Wed, Feb 23, 2022 at 03:57:29PM -0300, Bruno Larsen via Gdb-patches wrote:
>> The test case added here is testing the bug gdb/28856, where calling a
>> function by hand from a pretty printer makes GDB crash. There are 6
>> mechanisms to trigger this crash in the current test, using the commands
>> backtrace, up, down, finish, step and continue. Since the failure happens
>> because of use-after-free (more details below) the tests will always
>> have a chance of passing through sheer luck, but anecdotally they seem
>> to fail all of the time.
>>
>> The reason GDB is crashing is a use-after-free problem. The above
>> mentioned functions save a pointer to the current frame's information,
>> then calls the pretty printer, and uses the saved pointer for different
>> reasons, depending on the function. The issue happens because
>> call_function_by_hand needs to reset the obstack to get the current
>> frame, invalidating the saved pointer.
>
> Thanks for this new testcase.
>
> A few comments below.
I'll take care of most of your comments, and I have answers for the other ones
>
>> ---
>> .../gdb.python/pretty-print-call-by-hand.c | 44 ++++++
>> .../gdb.python/pretty-print-call-by-hand.exp | 148 ++++++++++++++++++
>> .../gdb.python/pretty-print-call-by-hand.py | 25 +++
>> 3 files changed, 217 insertions(+)
>> create mode 100644 gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
>> create mode 100644 gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
>> create mode 100644 gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
>>
>> diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
>> new file mode 100644
>> index 00000000000..0dcddc9c1e6
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
>> @@ -0,0 +1,44 @@
>> +/* This testcase 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/>. */
>> +
>> +struct mytype{
>
> Can you place the opening curly brace to the start of the next line,
> please?
>
> Same remark throughout the rest of this code: Although it is less
> important for testcases, we made the decision that, whenever possible,
> we would try to keep our testcases adhering to the same coding standards
> as with the rest of the code, wheneve
>
>> + char *x;
>
> Also part of our coding style: Can you use an indentation level of
> 2 characters, rather than 4?
>
>> +};
>> +
>> +void rec (int i){
>
> Same as above, for the avoidance of doubt.
>
>> + if (i <= 0)
>
>
>
>> + return;
>> + rec (i-1);
>> +}
>> +
>> +int f (){
>> + rec (100);
>> + return 2;
>> +}
>> +
>> +void g (struct mytype mt, int depth) {
>> + if (depth <= 0)
>> + return; /*TAG: inside the frame */
>> + g (mt, depth - 1);
>> +}
>> +
>> +int main (){
>> + struct mytype mt;
>> + mt.x = "hello world";
>> + g (mt, 10); /* TAG: break here */
>> + return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
>> new file mode 100644
>> index 00000000000..6ba3fdcc7b3
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
>> @@ -0,0 +1,148 @@
>> +# Copyright (C) 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/>.
>> +
>> +# This file is part of the GDB testsuite. It tests Python-based
>> +# pretty-printing for the CLI.
>> +
>> +load_lib gdb-python.exp
>> +
>> +standard_testfile
>> +
>> +# Start with a fresh gdb.
>> +gdb_exit
>> +gdb_start
>
> I think you can drop the gdb_exit/gdb_start part (it is taken care
> of by prepare_for_testing).
If I remove this, skip_python_tests throws a TCL error, complaining about not having a use_gdb_stub variable. If I change the order of the 2 if statements, the error isn't there anymore but we compile the code and don't use it. Do I keep the exit+start here and document why I need it, or do I change the order of the if statements?
>
>> +
>> +# Skip all tests if Python scripting is not enabled.
>> +if { [skip_python_tests] } { continue }
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
>> + untested "failed to compile"
>> + return -1
>> +}
>> +
>> +proc start_test_no_pretty {} {
>
> Can you please document this function the same way we document
> functions in GDB's actual code? We're normally somewhat more
> relaxed about this in our testsuite, but in this case, I spent
> some time asking that you remove some "return" statements because
> these are typically not used in our testsuite, only to understand
> a bit later why you are using them. A comment would have avoided
> me spending time commenting on something, and so I am thinking
> it might help others as well.
>
>> + global srcdir subdir testfile binfile
>> +
>> + # Start with a fresh gdb.
>> + # This is important because the test can crash GDB.
>> +
>> + gdb_exit
>> + gdb_start
>> + gdb_reinitialize_dir $srcdir/$subdir
>> + gdb_load ${binfile}
>
> You can use clean_restart <executable> the 4 function calls above.
>
>> + if { ![runto_main] } then {
>> + perror "couldn't run to breakpoint"
>
> Can you use "untested" rather than "perror"? For reference,
> our testcase cookbook wiki page references the following file
> as a template you can take a look at:
>
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/testsuite/gdb.base/template.exp;h=007a1472fee6866e76c3c6d991251502f278a615;hb=HEAD
>
>> + return -1
>> + }
>> + #gdb_test_no_output "source ${testfile}.py" "load python file"
>
> Can you remove this commented out code, please?
>
>> + #let GDB get to the return line
>
> Can you format this with a space after the "#", an uppercase letter
> to start the sentence, and then a period at the end?
>
>> + gdb_breakpoint [gdb_get_line_number "TAG: break here" ${testfile}.c ]
>> + gdb_continue_to_breakpoint "TAG: break here" ".*TAG: break here.*"
>> +
>> + gdb_test_no_output "set print pretty on" "starting to pretty print"
>
> There is an oddity here; the name of the function seems to indicate
> that this should be "off" rather than "on", here. Can you confirm
> whether this is correct or not?
It is, the name of the function is wrong. More details below
>
>> + set remote_python_file [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
>> + gdb_test_no_output "source ${remote_python_file}" "load python file"
>> +
>> + return 0
>> +}
>> +
>> +proc start_test_pretty {} {
>
> This function seems to be identical to the start_test_no_pretty.
> Can you perhaps factorize this into a single function, possibly
> with one argument being the "on/off" value to be used for
> the "print pretty" setting?
>
> If, for some reason I missed, it is better not to factorize in
> this case, the same comments I made above should apply here.
The difference is when the pretty printer is turned on, actually. The "no_pretty" option doesn't enter the frame that creates our problems, just turns the pretty printer on and waits by the door - setup for step and continue, whereas the "pretty" option enters the frame and then turns the pretty printer on - setup for backtrace, finish, up and down.
I'll change the function to receive the breaking location instead. Sorry about the confusion.
>
>> + global srcdir subdir testfile binfile
>> +
>> + # Start with a fresh gdb.
>> + # This is important because the test can crash GDB.
>> +
>> + gdb_exit
>> + gdb_start
>> + gdb_reinitialize_dir $srcdir/$subdir
>> + gdb_load ${binfile}
>> +
>> + if { ![runto_main] } then {
>> + perror "couldn't run to breakpoint"
>> + return -1
>> + }
>> + #gdb_test_no_output "source ${testfile}.py" "load python file"
>> +
>> + #let GDB get to the return line
>> + gdb_breakpoint [gdb_get_line_number "TAG: break here" ${testfile}.c ]
>> + gdb_continue_to_breakpoint "TAG: break here" ".*TAG: break here.*"
>> +
>> + gdb_test "step" ".*" #enter the problematic frame
>> +
>> + gdb_test_no_output "set print pretty on" "starting to pretty print"
>> +
>> + set remote_python_file [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
>> + gdb_test_no_output "source ${remote_python_file}" "load python file"
>> +
>> + return 0
>> +}
>> +
>> +# Testing the backtrace command
>> +with_test_prefix "frame print" {
>> + if { [start_test_pretty] == 0 } {
>> + setup_kfail gdb/28856 "*-*-*"
>> + gdb_test "backtrace -frame-arguments all" ".*"
>> + }
>> +}
>> +
>> +# Testing the down command
>> +with_test_prefix "frame movement up" {
>> + if { [start_test_pretty] == 0 } {
>> + gdb_test "up" ".*"
>> + setup_kfail gdb/28856 "*-*-*"
>> + gdb_test "down" ".*"
>> + }
>> +}
>> +
>> +# Testing the up command
>> +with_test_prefix "frame movement down" {
>> + if { [start_test_pretty] == 0 } {
>> + gdb_test_no_output "set print pretty off" "removing pretty printing"
>> + gdb_breakpoint [gdb_get_line_number "TAG: inside the frame" ${testfile}.c ]
>> + gdb_continue_to_breakpoint "TAG: inside the frame" ".*TAG: inside the frame.*"
>> + gdb_test_no_output "set print pretty on" "removing pretty printing"
>> + setup_kfail gdb/28856 "*-*-*"
>> + gdb_test "up" ".*"
>> + }
>> +}
>> +
>> +# Testing the finish command
>> +with_test_prefix "frame exit through finish" {
>> + if { [start_test_pretty] == 0 } {
>> + setup_kfail gdb/28856 "*-*-*"
>> + gdb_test "finish" ".*"
>> + }
>> +}
>> +
>> +# Testing the step command
>> +with_test_prefix "frame enter through step" {
>> + if { [start_test_no_pretty] == 0 } {
>> + setup_kfail gdb/28856 "*-*-*"
>> + gdb_test "step" ".*"
>> + }
>> +}
>> +
>> +# Testing the continue command
>> +with_test_prefix "frame enter through continue" {
>> + if { [start_test_no_pretty] == 0 } {
>> + gdb_test "backtrace -frame-arguments all" ".*"
>> + setup_kfail gdb/28856 "*-*-*"
>> + gdb_breakpoint [gdb_get_line_number "TAG: inside the frame" ${testfile}.c ]
>> + gdb_continue_to_breakpoint "TAG: inside the frame" ".*TAG: inside the frame.*"
>> + }
>> +}
>> diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
>> new file mode 100644
>> index 00000000000..978876723fc
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
>> @@ -0,0 +1,25 @@
>> +class MytypePrinter:
>> + "pretty print my type"
>
> Can you use """ for this docstring? This is what's typical and it will
> help those who don't read Python everyday and are more used to seeing
> """xxx""" strings rather than "x" ones when it comes to documentation.
>
>> +
>> + def __init__(self, val):
>> + self.val = val
>> +
>> + def to_string(self):
>> + calls = gdb.parse_and_eval('f()')
>> + return "mytype is %s" % self.val['x']
>> +
>> +def ec_lookup_function(val):
>> + typ = val.type
>> + if typ.code == gdb.TYPE_CODE_REF:
>> + typ = typ.target()
>> + if str(typ) == 'struct mytype':
>> + return MytypePrinter(val)
>> + return None
>> +
>> +def disable_lookup_function():
>> + ec_lookup_function.enabled = False
>> +
>> +def enable_lookup_function():
>> + ec_lookup_function.enabled = True
>> +
>> +gdb.pretty_printers.append(ec_lookup_function)
>> --
>> 2.31.1
>>
>
--
Cheers!
Bruno Larsen
next prev parent reply other threads:[~2022-02-28 12:13 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-23 18:57 Bruno Larsen
2022-02-27 12:39 ` Joel Brobecker
2022-02-28 12:13 ` Bruno Larsen [this message]
2022-03-06 10:43 ` Joel Brobecker
2022-03-07 13:09 ` [PATCH v2] " Bruno Larsen
2022-03-13 6:13 ` Joel Brobecker
2022-03-14 20:39 ` [PATCH v3] [gdb/testsuite] test a function call by hand from " Bruno Larsen
2022-03-19 14:41 ` Joel Brobecker
2022-03-21 20:21 ` Simon Marchi
2022-03-23 15:37 ` Bruno Larsen
2022-03-24 14:18 ` Simon Marchi
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=edcc19f5-51d1-0815-23a4-b7a35fa76141@redhat.com \
--to=blarsen@redhat.com \
--cc=brobecker@adacore.com \
--cc=gdb-patches@sourceware.org \
/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).