[PATCH] [gdb/testsuite] test a function call by hand fron pretty printer

[PATCH] [gdb/testsuite] test a function call by hand fron pretty printer

[Bruno Larsen]

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.
---
 .../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{
+    char *x;
+};
+
+void rec (int i){
+    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
+
+# 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 {} {
+    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_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
+}
+
+proc start_test_pretty {} {
+    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"
+
+    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

Re: [PATCH] [gdb/testsuite] test a function call by hand fron pretty printer

[Joel Brobecker]

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  <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).

> +
> +# 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?

> +    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

Re: [PATCH] [gdb/testsuite] test a function call by hand fron pretty printer

[Bruno Larsen]

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

Re: [PATCH] [gdb/testsuite] test a function call by hand fron pretty printer

[Joel Brobecker]

> > > +# 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?

Grumpf. This is a bug in skip_python_tests_prompt, in my opinion,
which seems to make an unwarranted assumption about a GDB being
available. It looks like a simple call to "gdb_start" at the start
of that function should be sufficient to fix the issue, as gdb_start
only starts GDB if one hasn't already been started.

But I don't want this fix to be a precondition to your patch.
In the meantime, I think a comment explaining why is good enough.
I keep flip-flopping between the two options and can't decide,
so I went with the simplest and less wasteful (avoiding to do
build a test program if the testcase is not going to run).

> > 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.

Alright! Thanks for the explanation.


-- 
Joel

[PATCH v2] [gdb/testsuite] test a function call by hand fron pretty printer

[Bruno Larsen]

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.
---

Changes from v1:
    * make .c follow GDB's coding guidelines more closely
    * Documented .exp file better
    * renamed and refactored TCL proc to make it less confusing
    * General style changes

Thanks Joel for your comments!

---
 .../gdb.python/pretty-print-call-by-hand.c    |  48 ++++++++
 .../gdb.python/pretty-print-call-by-hand.exp  | 115 ++++++++++++++++++
 .../gdb.python/pretty-print-call-by-hand.py   |  25 ++++
 3 files changed, 188 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..c215c78541b
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
@@ -0,0 +1,48 @@
+/* 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
+{
+  char *x;
+};
+
+void rec (int i)
+{
+  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: final frame */
+  g (mt, depth - 1); /* TAG: first frame */
+}
+
+int main (){
+  struct mytype mt;
+  mt.x = "hello world";
+  g (mt, 10); /* TAG: outside the frame */
+  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..7508be20a03
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
@@ -0,0 +1,115 @@
+# 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 a pretty printer that
+# calls an inferior function by hand, triggering a Use-after-Free bug
+# (PR gdb/28856).
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+# gdb needs to be started here for skip_python_tests to work.
+# prepare_for_testing could be used instead, but it could compile the program
+# unnecessarily, so starting GDB like this is preferable.
+gdb_start
+
+# 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
+}
+
+# This proc restarts GDB, makes the inferior reach the desired spot - marked
+# by a comment in the .c file - and turns on the pretty printer for testing.
+# Starting with a new GDB is important because the test may crash GDB.  The
+# return values are here to avoid us trying to test the pretty printer if
+# there was a problem getting to main.
+proc start_test { breakpoint_comment } {
+    global srcdir subdir testfile binfile
+
+    # Start with a fresh gdb.
+    # This is important because the test can crash GDB.
+
+    clean_restart ${binfile}
+
+    if { ![runto_main] } then {
+	untested "couldn't run to breakpoint"
+	return -1
+    }
+
+    # Let GDB get to the return line.
+    gdb_breakpoint [gdb_get_line_number ${breakpoint_comment} ${testfile}.c ]
+    gdb_continue_to_breakpoint ${breakpoint_comment} ".*"
+
+    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 "TAG: final frame"] == 0 } {
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_test "backtrace -frame-arguments all" ".*"
+    }
+}
+
+# Testing the down command.
+with_test_prefix "frame movement down" {
+    if { [start_test "TAG: first frame"] == 0 } {
+	gdb_test "up" ".*"
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_test "down" ".*"
+    }
+}
+
+# Testing the up command.
+with_test_prefix "frame movement up" {
+    if { [start_test "TAG: final frame"] == 0 } {
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_test "up" ".*"
+    }
+}
+
+# Testing the finish command.
+with_test_prefix "frame exit through finish" {
+    if { [start_test "TAG: first frame"] == 0 } {
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_test "finish" ".*"
+    }
+}
+
+# Testing the step command.
+with_test_prefix "frame enter through step" {
+    if { [start_test "TAG: outside the frame"] == 0 } {
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_test "step" ".*"
+    }
+}
+
+# Testing the continue command.
+with_test_prefix "frame enter through continue" {
+    if { [start_test "TAG: outside the frame"] == 0 } {
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_breakpoint [gdb_get_line_number "TAG: first frame" ${testfile}.c ]
+	gdb_continue_to_breakpoint "TAG: first frame" ".*TAG: first 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..627011a9ac9
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
@@ -0,0 +1,25 @@
+class MytypePrinter:
+    """pretty print my type"""
+
+    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

Re: [PATCH v2] [gdb/testsuite] test a function call by hand fron pretty printer

[Joel Brobecker]

On Mon, Mar 07, 2022 at 10:09:24AM -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.

> ---
> 
> Changes from v1:
>     * make .c follow GDB's coding guidelines more closely
>     * Documented .exp file better
>     * renamed and refactored TCL proc to make it less confusing

Thanks for this new version.

I noticed a typo in the subject "fron" -> "from".

A few more (minor) comments below.

> ---
>  .../gdb.python/pretty-print-call-by-hand.c    |  48 ++++++++
>  .../gdb.python/pretty-print-call-by-hand.exp  | 115 ++++++++++++++++++
>  .../gdb.python/pretty-print-call-by-hand.py   |  25 ++++
>  3 files changed, 188 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..c215c78541b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
> @@ -0,0 +1,48 @@
> +/* 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
> +{
> +  char *x;
> +};
> +
> +void rec (int i)

Can you put "rec" at the start of the next line?

    | void
    | rec (int i)

The GCS explains that this allows "diff -p" to print the function name
when showing hunks.

Same comment for the other functions below.

> +{
> +  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: final frame */
> +  g (mt, depth - 1); /* TAG: first frame */
> +}
> +
> +int main (){

Same here. And also, can you put the opening curly brace on
the next line, please?

> +  struct mytype mt;
> +  mt.x = "hello world";
> +  g (mt, 10); /* TAG: outside the frame */
> +  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..7508be20a03
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
> @@ -0,0 +1,115 @@
> +# 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 a pretty printer that
> +# calls an inferior function by hand, triggering a Use-after-Free bug
> +# (PR gdb/28856).
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +# gdb needs to be started here for skip_python_tests to work.
> +# prepare_for_testing could be used instead, but it could compile the program
> +# unnecessarily, so starting GDB like this is preferable.
> +gdb_start
> +
> +# 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
> +}
> +
> +# This proc restarts GDB, makes the inferior reach the desired spot - marked
> +# by a comment in the .c file - and turns on the pretty printer for testing.
> +# Starting with a new GDB is important because the test may crash GDB.  The
> +# return values are here to avoid us trying to test the pretty printer if
> +# there was a problem getting to main.
> +proc start_test { breakpoint_comment } {
> +    global srcdir subdir testfile binfile
> +
> +    # Start with a fresh gdb.
> +    # This is important because the test can crash GDB.
> +
> +    clean_restart ${binfile}
> +
> +    if { ![runto_main] } then {
> +	untested "couldn't run to breakpoint"
> +	return -1
> +    }
> +
> +    # Let GDB get to the return line.
> +    gdb_breakpoint [gdb_get_line_number ${breakpoint_comment} ${testfile}.c ]
> +    gdb_continue_to_breakpoint ${breakpoint_comment} ".*"
> +
> +    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 "TAG: final frame"] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "backtrace -frame-arguments all" ".*"

Even though this is not critical for this testcase, I'd like to
suggest we do some minimal verification that the output seems
about right. What do you think?

(here, and all the tests below).


> +    }
> +}
> +
> +# Testing the down command.
> +with_test_prefix "frame movement down" {
> +    if { [start_test "TAG: first frame"] == 0 } {
> +	gdb_test "up" ".*"
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "down" ".*"
> +    }
> +}
> +
> +# Testing the up command.
> +with_test_prefix "frame movement up" {
> +    if { [start_test "TAG: final frame"] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "up" ".*"
> +    }
> +}
> +
> +# Testing the finish command.
> +with_test_prefix "frame exit through finish" {
> +    if { [start_test "TAG: first frame"] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "finish" ".*"
> +    }
> +}
> +
> +# Testing the step command.
> +with_test_prefix "frame enter through step" {
> +    if { [start_test "TAG: outside the frame"] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "step" ".*"
> +    }
> +}
> +
> +# Testing the continue command.
> +with_test_prefix "frame enter through continue" {
> +    if { [start_test "TAG: outside the frame"] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_breakpoint [gdb_get_line_number "TAG: first frame" ${testfile}.c ]
> +	gdb_continue_to_breakpoint "TAG: first frame" ".*TAG: first 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..627011a9ac9
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
> @@ -0,0 +1,25 @@
> +class MytypePrinter:

Can you add a copyright notice to this file, please?

> +    """pretty print my type"""
> +
> +    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)

-- 
Joel

[PATCH v3] [gdb/testsuite] test a function call by hand from pretty printer

[Bruno Larsen]

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.
---

Changes from v2:
    * Make .c follow GDB's coding style 2 - electric boogaloo 
    * .exp tests outputs, not just if GDB breaks or not
    * added copyright header to .py file

Changes from v1:
    * make .c follow GDB's coding guidelines more closely
    * Documented .exp file better
    * renamed and refactored TCL proc to make it less confusing

---
 .../gdb.python/pretty-print-call-by-hand.c    |  53 ++++++++
 .../gdb.python/pretty-print-call-by-hand.exp  | 127 ++++++++++++++++++
 .../gdb.python/pretty-print-call-by-hand.py   |  41 ++++++
 3 files changed, 221 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..3be5675b096
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
@@ -0,0 +1,53 @@
+/* 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
+{
+  char *x;
+};
+
+void
+rec (int i)
+{
+  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: final frame */
+  g (mt, depth - 1); /* TAG: first frame */
+}
+
+int
+main ()
+{
+  struct mytype mt;
+  mt.x = "hello world";
+  g (mt, 10); /* TAG: outside the frame */
+  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..0a6d014cf12
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
@@ -0,0 +1,127 @@
+# 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 a pretty printer that
+# calls an inferior function by hand, triggering a Use-after-Free bug
+# (PR gdb/28856).
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+# gdb needs to be started here for skip_python_tests to work.
+# prepare_for_testing could be used instead, but it could compile the program
+# unnecessarily, so starting GDB like this is preferable.
+gdb_start
+
+# 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
+}
+
+# This proc restarts GDB, makes the inferior reach the desired spot - marked
+# by a comment in the .c file - and turns on the pretty printer for testing.
+# Starting with a new GDB is important because the test may crash GDB.  The
+# return values are here to avoid us trying to test the pretty printer if
+# there was a problem getting to main.
+proc start_test { breakpoint_comment } {
+    global srcdir subdir testfile binfile
+
+    # Start with a fresh gdb.
+    # This is important because the test can crash GDB.
+
+    clean_restart ${binfile}
+
+    if { ![runto_main] } then {
+	untested "couldn't run to breakpoint"
+	return -1
+    }
+
+    # Let GDB get to the return line.
+    gdb_breakpoint [gdb_get_line_number ${breakpoint_comment} ${testfile}.c ]
+    gdb_continue_to_breakpoint ${breakpoint_comment} ".*"
+
+    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 "TAG: final frame"] == 0 } {
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_test "backtrace -frame-arguments all" [multi_line \
+	"#0 .*g \\(mt=mytype is .*\\).*"\
+	"#1 .*g \\(mt=mytype is .*\\).*"\
+	"#2 .*g \\(mt=mytype is .*\\).*"\
+	"#3 .*g \\(mt=mytype is .*\\).*"\
+	"#4 .*g \\(mt=mytype is .*\\).*"\
+	"#5 .*g \\(mt=mytype is .*\\).*"\
+	"#6 .*g \\(mt=mytype is .*\\).*"\
+	"#7 .*g \\(mt=mytype is .*\\).*"\
+	"#8 .*g \\(mt=mytype is .*\\).*"\
+	"#9 .*g \\(mt=mytype is .*\\).*"\
+	"#10 .*g \\(mt=mytype is .*\\).*"\
+	"#11 .*main \\(\\).*"] \
+	"backtrace test"
+    }
+}
+# Testing the down command.
+with_test_prefix "frame movement down" {
+    if { [start_test "TAG: first frame"] == 0 } {
+	gdb_test "up" [multi_line "#1 .*in main \\(\\) at .*" ".*outside the frame.*"]
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_test "down" [multi_line "#0\\s+g \\(mt=mytype is .*\\).*" ".*first frame.*"]
+    }
+}
+
+# Testing the up command.
+with_test_prefix "frame movement up" {
+    if { [start_test "TAG: final frame"] == 0 } {
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_test "up" [multi_line "#1 .*in g \\(mt=mytype is .*\\).*" ".*first frame.*"]
+    }
+}
+
+# Testing the finish command.
+with_test_prefix "frame exit through finish" {
+    if { [start_test "TAG: final frame"] == 0 } {
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_test "finish" [multi_line "Run till exit from #0 .*" ".* g \\(mt=mytype is .*\\, depth=1\\).*" ".*first frame.*"]
+    }
+}
+
+# Testing the step command.
+with_test_prefix "frame enter through step" {
+    if { [start_test "TAG: outside the frame"] == 0 } {
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_test "step" [multi_line "g \\(mt=mytype is .*\\, depth=10\\).*" "41.*if \\(depth \\<= 0\\)"]
+    }
+}
+
+# Testing the continue command.
+with_test_prefix "frame enter through continue" {
+    if { [start_test "TAG: outside the frame"] == 0 } {
+	setup_kfail gdb/28856 "*-*-*"
+	gdb_breakpoint [gdb_get_line_number "TAG: first frame" ${testfile}.c ]
+	gdb_continue_to_breakpoint "TAG: first frame" ".*TAG: first 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..f8f5df678f8
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
@@ -0,0 +1,41 @@
+# 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/>.
+
+
+class MytypePrinter:
+    """pretty print my type"""
+
+    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

Re: [PATCH v3] [gdb/testsuite] test a function call by hand from pretty printer

[Joel Brobecker]

On Mon, Mar 14, 2022 at 05:39:26PM -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.
> ---
> 
> Changes from v2:
>     * Make .c follow GDB's coding style 2 - electric boogaloo 
>     * .exp tests outputs, not just if GDB breaks or not
>     * added copyright header to .py file
> 
> Changes from v1:
>     * make .c follow GDB's coding guidelines more closely
>     * Documented .exp file better
>     * renamed and refactored TCL proc to make it less confusing

Thanks you Bruno. This version of the patch looks good to me.

> ---
>  .../gdb.python/pretty-print-call-by-hand.c    |  53 ++++++++
>  .../gdb.python/pretty-print-call-by-hand.exp  | 127 ++++++++++++++++++
>  .../gdb.python/pretty-print-call-by-hand.py   |  41 ++++++
>  3 files changed, 221 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..3be5675b096
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
> @@ -0,0 +1,53 @@
> +/* 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
> +{
> +  char *x;
> +};
> +
> +void
> +rec (int i)
> +{
> +  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: final frame */
> +  g (mt, depth - 1); /* TAG: first frame */
> +}
> +
> +int
> +main ()
> +{
> +  struct mytype mt;
> +  mt.x = "hello world";
> +  g (mt, 10); /* TAG: outside the frame */
> +  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..0a6d014cf12
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
> @@ -0,0 +1,127 @@
> +# 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 a pretty printer that
> +# calls an inferior function by hand, triggering a Use-after-Free bug
> +# (PR gdb/28856).
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +# gdb needs to be started here for skip_python_tests to work.
> +# prepare_for_testing could be used instead, but it could compile the program
> +# unnecessarily, so starting GDB like this is preferable.
> +gdb_start
> +
> +# 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
> +}
> +
> +# This proc restarts GDB, makes the inferior reach the desired spot - marked
> +# by a comment in the .c file - and turns on the pretty printer for testing.
> +# Starting with a new GDB is important because the test may crash GDB.  The
> +# return values are here to avoid us trying to test the pretty printer if
> +# there was a problem getting to main.
> +proc start_test { breakpoint_comment } {
> +    global srcdir subdir testfile binfile
> +
> +    # Start with a fresh gdb.
> +    # This is important because the test can crash GDB.
> +
> +    clean_restart ${binfile}
> +
> +    if { ![runto_main] } then {
> +	untested "couldn't run to breakpoint"
> +	return -1
> +    }
> +
> +    # Let GDB get to the return line.
> +    gdb_breakpoint [gdb_get_line_number ${breakpoint_comment} ${testfile}.c ]
> +    gdb_continue_to_breakpoint ${breakpoint_comment} ".*"
> +
> +    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 "TAG: final frame"] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "backtrace -frame-arguments all" [multi_line \
> +	"#0 .*g \\(mt=mytype is .*\\).*"\
> +	"#1 .*g \\(mt=mytype is .*\\).*"\
> +	"#2 .*g \\(mt=mytype is .*\\).*"\
> +	"#3 .*g \\(mt=mytype is .*\\).*"\
> +	"#4 .*g \\(mt=mytype is .*\\).*"\
> +	"#5 .*g \\(mt=mytype is .*\\).*"\
> +	"#6 .*g \\(mt=mytype is .*\\).*"\
> +	"#7 .*g \\(mt=mytype is .*\\).*"\
> +	"#8 .*g \\(mt=mytype is .*\\).*"\
> +	"#9 .*g \\(mt=mytype is .*\\).*"\
> +	"#10 .*g \\(mt=mytype is .*\\).*"\
> +	"#11 .*main \\(\\).*"] \
> +	"backtrace test"
> +    }
> +}
> +# Testing the down command.
> +with_test_prefix "frame movement down" {
> +    if { [start_test "TAG: first frame"] == 0 } {
> +	gdb_test "up" [multi_line "#1 .*in main \\(\\) at .*" ".*outside the frame.*"]
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "down" [multi_line "#0\\s+g \\(mt=mytype is .*\\).*" ".*first frame.*"]
> +    }
> +}
> +
> +# Testing the up command.
> +with_test_prefix "frame movement up" {
> +    if { [start_test "TAG: final frame"] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "up" [multi_line "#1 .*in g \\(mt=mytype is .*\\).*" ".*first frame.*"]
> +    }
> +}
> +
> +# Testing the finish command.
> +with_test_prefix "frame exit through finish" {
> +    if { [start_test "TAG: final frame"] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "finish" [multi_line "Run till exit from #0 .*" ".* g \\(mt=mytype is .*\\, depth=1\\).*" ".*first frame.*"]
> +    }
> +}
> +
> +# Testing the step command.
> +with_test_prefix "frame enter through step" {
> +    if { [start_test "TAG: outside the frame"] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_test "step" [multi_line "g \\(mt=mytype is .*\\, depth=10\\).*" "41.*if \\(depth \\<= 0\\)"]
> +    }
> +}
> +
> +# Testing the continue command.
> +with_test_prefix "frame enter through continue" {
> +    if { [start_test "TAG: outside the frame"] == 0 } {
> +	setup_kfail gdb/28856 "*-*-*"
> +	gdb_breakpoint [gdb_get_line_number "TAG: first frame" ${testfile}.c ]
> +	gdb_continue_to_breakpoint "TAG: first frame" ".*TAG: first 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..f8f5df678f8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
> @@ -0,0 +1,41 @@
> +# 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/>.
> +
> +
> +class MytypePrinter:
> +    """pretty print my type"""
> +
> +    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

Re: [PATCH v3] [gdb/testsuite] test a function call by hand from pretty printer

[Simon Marchi]

On 2022-03-14 16:39, 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.
> ---
>
> Changes from v2:
>     * Make .c follow GDB's coding style 2 - electric boogaloo
>     * .exp tests outputs, not just if GDB breaks or not
>     * added copyright header to .py file
>
> Changes from v1:
>     * make .c follow GDB's coding guidelines more closely
>     * Documented .exp file better
>     * renamed and refactored TCL proc to make it less confusing

Hi,

Having the test without the fix is annoying, because it crashes GDB on
an ASan-enabled build, and causes failures that did not exist before,
and the setup_kfails don't help for that.  This is something we
generally don't want, as we try to strictly progress towards less
failures.

Does the test actually test anything right now, given that everything is
broken?  If not, I'd suggest attaching the test case to the bug, and it
can be merged along with the fix.


Simon

Re: [PATCH v3] [gdb/testsuite] test a function call by hand from pretty printer

[Bruno Larsen]

On 3/21/22 17:21, Simon Marchi wrote:
> On 2022-03-14 16:39, 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.
>> ---
>>
>> Changes from v2:
>>      * Make .c follow GDB's coding style 2 - electric boogaloo
>>      * .exp tests outputs, not just if GDB breaks or not
>>      * added copyright header to .py file
>>
>> Changes from v1:
>>      * make .c follow GDB's coding guidelines more closely
>>      * Documented .exp file better
>>      * renamed and refactored TCL proc to make it less confusing
> 
> Hi,
> 
> Having the test without the fix is annoying, because it crashes GDB on
> an ASan-enabled build, and causes failures that did not exist before,
> and the setup_kfails don't help for that.  This is something we
> generally don't want, as we try to strictly progress towards less
> failures.
> 
> Does the test actually test anything right now, given that everything is
> broken?  If not, I'd suggest attaching the test case to the bug, and it
> can be merged along with the fix.
> 
> 
> Simon
> 

Hi

Oh sorry, I didn't think about that. My thinking was that this looked like a pretty complex issue, so I could at least get some sort conversation going in case anyone knew how to fix the problem, and could help me find the origin of the problems. It doesn't test anything new that doesn't fail, but unfortunately, I had already pushed the change when your email arrived. If you want to remove it until I have a fix, I understand it. Sorry again.

-- 
Cheers!
Bruno Larsen

Re: [PATCH v3] [gdb/testsuite] test a function call by hand from pretty printer

[Simon Marchi]

> Oh sorry, I didn't think about that. My thinking was that this looked
> like a pretty complex issue, so I could at least get some sort
> conversation going in case anyone knew how to fix the problem, and
> could help me find the origin of the problems. It doesn't test
> anything new that doesn't fail, but unfortunately, I had already
> pushed the change when your email arrived. If you want to remove it
> until I have a fix, I understand it. Sorry again.

Ok, I'll remove the test and attach it to the bug.  This will help bring
back my little CI closer to being green.

Simon