Re: [PATCH v3] gdb: fix eval.c assert during inferior exit event

[Magne Hov <mhov@undo.io>]

This has now been pushed. Thank you everyone for comments on the patch,
and an extra thanks to Simon for helping me with the push process.

Kind regards,
Magne

On Wed, May 26 2021, Magne Hov wrote:

> Evaluating expressions from within an inferior exit event handler can
> cause a crash:
>
>     echo "int main() { return 0; }" > repro.c
>     gcc -g repro.c -o repro
>     ./gdb -q --ex "set language c++" --ex "python gdb.events.exited.connect(lambda _: gdb.execute('set \$_a=0'))" --ex "run" repro
>
>     Reading symbols from repro...
>     Starting program: /home/mhov/repos/binutils-gdb-master/install-bad/bin/repro
>     [Inferior 1 (process 1974779) exited normally]
>     ../../gdb/thread.c:72: internal-error: thread_info* inferior_thread(): Assertion `current_thread_ != nullptr' failed.
>     A problem internal to GDB has been detected,
>     further debugging may prove unreliable.
>     Quit this debugging session? (y or n) [answered Y; input not from terminal]
>
>     This is a bug, please report it.  For instructions, see:
>     <https://www.gnu.org/software/gdb/bugs/>.
>
> Backtrace
>     0  in internal_error of ../../gdbsupport/errors.cc:51
>     1  in inferior_thread of ../../gdb/thread.c:72
>     2  in expression::evaluate of ../../gdb/eval.c:98
>     3  in evaluate_expression of ../../gdb/eval.c:115
>     4  in set_command of ../../gdb/printcmd.c:1502
>     5  in do_const_cfunc of ../../gdb/cli/cli-decode.c:101
>     6  in cmd_func of ../../gdb/cli/cli-decode.c:2181
>     7  in execute_command of ../../gdb/top.c:670
>     ...
>     22 in python_inferior_exit of ../../gdb/python/py-inferior.c:182
>
> In `expression::evaluate (...)' there is a call to `inferior_thread
> ()' that is guarded by `target_has_execution ()':
>
>     struct value *
>     expression::evaluate (struct type *expect_type, enum noside noside)
>     {
>       gdb::optional<enable_thread_stack_temporaries> stack_temporaries;
>       if (target_has_execution ()
>           && language_defn->la_language == language_cplus
>           && !thread_stack_temporaries_enabled_p (inferior_thread ()))
>         stack_temporaries.emplace (inferior_thread ());
>
> The `target_has_execution ()' guard maps onto `inf->pid' and the
> `inferior_thread ()' call assumes that `current_thread_' is set to
> something meaningful:
>
>     struct thread_info*
>     inferior_thread (void)
>     {
>       gdb_assert (current_thread_ != nullptr);
>       return current_thread_;
>     }
>
> In other words, it is assumed that if `inf->pid' is set then
> `current_thread_' must also be set. This does not hold at the point
> where inferior exit observers are notified:
> - `generic_mourn_inferior (...)'
>   - `switch_to_no_thread ()'
>     - `current_thread_ = nullptr;'
>   - `exit_inferior (...)'
>     - `gdb::observers::inferior_exit.notify (...)'
>     - `inf->pid = 0'
>
> The inferior exit notification means that a Python handler can get a
> chance to run while `current_thread' has been cleared and the
> `inf->pid' has not been cleared. Since the Python handler can call any
> GDB command with `gdb.execute(...)' (in my case `gdb.execute("set
> $_a=0")' we can end up evaluating expressions and asserting in
> `evaluate_subexp (...)'.
>
> This patch adds a test in `evaluate_subexp (...)' to check the global
> `inferior_ptid' which is reset at the same time as `current_thread_'.
> Checking `inferior_ptid' at the same time as `target_has_execution ()'
> seems to be a common pattern:
>
>     $ git grep -n -e inferior_ptid --and -e target_has_execution
>     gdb/breakpoint.c:2998:    && (inferior_ptid == null_ptid || !target_has_execution ()))
>     gdb/breakpoint.c:3054:    && (inferior_ptid == null_ptid || !target_has_execution ()))
>     gdb/breakpoint.c:4587:  if (inferior_ptid == null_ptid || !target_has_execution ())
>     gdb/infcmd.c:360:  if (inferior_ptid != null_ptid && target_has_execution ())
>     gdb/infcmd.c:2380:  /* FIXME:  This should not really be inferior_ptid (or target_has_execution).
>     gdb/infrun.c:3438:  if (!target_has_execution () || inferior_ptid == null_ptid)
>     gdb/remote.c:11961:  if (!target_has_execution () || inferior_ptid == null_ptid)
>     gdb/solib.c:725:  if (target_has_execution () && inferior_ptid != null_ptid)
>
> The testsuite has been run on 5.4.0-59-generic x86_64 GNU/Linux:
> - Ubuntu 20.04.1 LTS
> - gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> - DejaGnu version 1.6.2
>   - Expect version 5.45.4
>   - Tcl version 8.6
> - Native configuration: x86_64-pc-linux-gnu
> - Target: unix
>
> Results show a few XFAIL in
> gdb.threads/attach-many-short-lived-threads.exp. The existing
> py-events.exp tests are skipped for native-gdbserver and fail for
> native-extended-gdbserver, but the new tests pass with
> native-extended-gdbserver when run without the existing tests.
>
> gdb/ChangeLog:
>
> 2021-05-26  Magne Hov  <mhov@undo.io>
>
> 	PR python/27841
> 	* eval.c (expression::evaluate): Check inferior_ptid.
>
> gdb/testsuite/ChangeLog:
>
> 2021-05-26  Magne Hov  <mhov@undo.io>
>
> 	PR python/27841
> 	* gdb.python/py-events.exp: Extend inferior exit tests.
> 	* gdb.python/py-events.py: Print inferior exit PID.
> ---
>  gdb/eval.c                             |  2 +-
>  gdb/testsuite/gdb.python/py-events.exp | 41 ++++++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-events.py  |  1 +
>  3 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/eval.c b/gdb/eval.c
> index 1b3c974009a..659493c8237 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -93,7 +93,7 @@ struct value *
>  expression::evaluate (struct type *expect_type, enum noside noside)
>  {
>    gdb::optional<enable_thread_stack_temporaries> stack_temporaries;
> -  if (target_has_execution ()
> +  if (target_has_execution () && inferior_ptid != null_ptid
>        && language_defn->la_language == language_cplus
>        && !thread_stack_temporaries_enabled_p (inferior_thread ()))
>      stack_temporaries.emplace (inferior_thread ());
> diff --git a/gdb/testsuite/gdb.python/py-events.exp b/gdb/testsuite/gdb.python/py-events.exp
> index e89cd8b021b..753709361f5 100644
> --- a/gdb/testsuite/gdb.python/py-events.exp
> +++ b/gdb/testsuite/gdb.python/py-events.exp
> @@ -197,18 +197,33 @@ gdb_test_multiple "continue" $test {
>  gdb_test_no_output "delete $second_breakpoint"
>  
>  #test exited event.
> +proc get_process_id {test} {
> +    global gdb_prompt
> +    gdb_test_multiple "info proc" $test {
> +	-re "process (\\d+).*$gdb_prompt $" {
> +	    set process_id $expect_out(1,string)
> +	    pass $gdb_test_name
> +	}
> +    }
> +    return ${process_id}
> +}
> +
> +set process_id [get_process_id "get inferior 1 process id"]
>  gdb_test "continue" ".*event type: continue.*
>  .*clear_objfiles\[\r\n\]*progspace: .*py-events.*
>  .*event type: exit.*
>  .*exit code: 12.*
>  .*exit inf: 1.*
> +.*exit pid: $process_id.*
>  dir ok: True.*" "Inferior 1 terminated."
>  
>  gdb_test "inferior 2" ".*Switching to inferior 2.*"
> +set process_id [get_process_id "get inferior 2 process id"]
>  gdb_test "continue" ".*event type: continue.*
>  .*event type: exit.*
>  .*exit code: 12.*
>  .*exit inf: 2.*
> +.*exit pid: $process_id.*
>  dir ok: True.*" "Inferior 2 terminated."
>  
>  
> @@ -235,3 +250,29 @@ gdb_test "python print(count)" 2 "check for before_prompt event"
>  
>  gdb_test_no_output "xxz" "run a canned sequence"
>  gdb_test "python print(count)" 4 "check for before_prompt event again"
> +
> +# Test evaluating expressions from within an inferior exit event handler.  This
> +# used to cause a crash when expression were evaluated as C++.
> +gdb_test_no_output "set language c++"
> +
> +gdb_test_multiline "add exited listener" \
> +    "python" "" \
> +    "def increment_foo(_):" "" \
> +    "  gdb.execute('set \$_foo=\$_foo+1')" "" \
> +    "gdb.events.exited.connect(increment_foo)" "" \
> +    "end" ""
> +gdb_test "set \$_foo=0" "" "initialize foo variable"
> +gdb_test "print \$_foo" "= 0" "check foo initialized"
> +
> +with_test_prefix "inferior run exit" {
> +    gdb_run_cmd
> +    gdb_test "" "exited with code.*" "run to exit"
> +    gdb_test "print \$_foo" "= 1" "check foo after run"
> +}
> +
> +with_test_prefix "inferior continue exit" {
> +    gdb_start_cmd
> +    gdb_test "" "Temporary breakpoint .* main .*" "stop on a frame"
> +    gdb_test "continue" "exited with code.*" "continue to exit"
> +    gdb_test "print \$_foo" "= 2" "check foo after start continue"
> +}
> diff --git a/gdb/testsuite/gdb.python/py-events.py b/gdb/testsuite/gdb.python/py-events.py
> index 6a676271b41..1524267117d 100644
> --- a/gdb/testsuite/gdb.python/py-events.py
> +++ b/gdb/testsuite/gdb.python/py-events.py
> @@ -47,6 +47,7 @@ def exit_handler(event):
>      print("event type: exit")
>      print("exit code: %d" % (event.exit_code))
>      print("exit inf: %d" % (event.inferior.num))
> +    print("exit pid: %d" % (event.inferior.pid))
>      print("dir ok: %s" % str("exit_code" in dir(event)))
>  
>  
> -- 
> 2.25.1