From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com [IPv6:2a00:1450:4864:20::42d]) by sourceware.org (Postfix) with ESMTPS id 718D63858D35 for ; Sun, 27 Feb 2022 12:39:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 718D63858D35 Received: by mail-wr1-x42d.google.com with SMTP id x15so11343893wrg.8 for ; Sun, 27 Feb 2022 04:39:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=6XbK5JjmPJmo2iFKMLN3MgKxLL5zh/GUKynjNUVVMZU=; b=nYaGqM6gepBIaH0gCFv4uW7icZ57Qm2ZcvFLZLwCdFkJMgDUDOF26lR6cd45lQGiHs cfD5h5tzoy2vK/YJWs50eJjGTjZEdXhCVt83pQ39RDnmPRUe/0UKy4bJ0uepMtnF8h/w OwshNcxcwgmfhipI/DZBlP6Ep4z2/ongb+gZx26bKOi3nVQmwynEMM1SPi8CTmhqxTFF Nx8BCiLacZvt8V6dannj4UKZNxD62qbmCj2VE5eGluuLF4lLJtSHwPFpouG1FJ2yfcPw 3wuRLzAVeT24zufiyQSrqOIK/u/H1WY+j9uMW9UoNEa5ouxic5txGI0lBHmihAJMaV4g O/XA== X-Gm-Message-State: AOAM531L/4iPAucIiaZuz8Kk4b2CZvIKxAehD7V92fCr13oFp7z/+QWy hT1o+1ON0kajihOktzpT5I+0ZufIc57M X-Google-Smtp-Source: ABdhPJwtJi5qGix+pmtwMHH0Zp5WuHmj+SitSvoDV6RPcC+jfa+KBFhb07sqDKwb0RQwvz7ufRATYA== X-Received: by 2002:adf:ed42:0:b0:1ef:9203:55d2 with SMTP id u2-20020adfed42000000b001ef920355d2mr3347037wro.320.1645965560418; Sun, 27 Feb 2022 04:39:20 -0800 (PST) Received: from takamaka.home (lfbn-reu-1-503-119.w92-130.abo.wanadoo.fr. [92.130.90.119]) by smtp.gmail.com with ESMTPSA id bg18-20020a05600c3c9200b0037c2ef07493sm8888815wmb.3.2022.02.27.04.39.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 27 Feb 2022 04:39:19 -0800 (PST) Received: by takamaka.home (Postfix, from userid 1000) id 8A2C3A49F4; Sun, 27 Feb 2022 16:39:16 +0400 (+04) Date: Sun, 27 Feb 2022 16:39:16 +0400 From: Joel Brobecker To: Bruno Larsen via Gdb-patches Cc: Joel Brobecker Subject: Re: [PATCH] [gdb/testsuite] test a function call by hand fron pretty printer Message-ID: References: <20220223185729.25609-1-blarsen@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220223185729.25609-1-blarsen@redhat.com> X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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: Sun, 27 Feb 2022 12:39:24 -0000 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. > --- > .../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). > + > +# 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? > + 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. > + 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 > -- Joel