From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 8E0443858C83 for ; Mon, 28 Feb 2022 12:13:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8E0443858C83 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-385-tqN9c0rTMRerc2EU1l0EMg-1; Mon, 28 Feb 2022 07:13:38 -0500 X-MC-Unique: tqN9c0rTMRerc2EU1l0EMg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3A9871006AA5; Mon, 28 Feb 2022 12:13:37 +0000 (UTC) Received: from [10.97.116.21] (ovpn-116-21.gru2.redhat.com [10.97.116.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2C3B578872; Mon, 28 Feb 2022 12:13:34 +0000 (UTC) Message-ID: Date: Mon, 28 Feb 2022 09:13:30 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH] [gdb/testsuite] test a function call by hand fron pretty printer To: Joel Brobecker , Bruno Larsen via Gdb-patches References: <20220223185729.25609-1-blarsen@redhat.com> From: Bruno Larsen In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE, WEIRD_QUOTING autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Feb 2022 12:13:43 -0000 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 . */ >> + >> +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 . >> + >> +# 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 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