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

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

[Magne Hov]

Evaluating expressions from within an inferior exit event handler can
cause a crash:

    echo "int main() { return 0; }" > repro.cc
    g++ -g repro.cc -o repro
    ./gdb -q --ex "start" --ex "python gdb.events.exited.connect(lambda _: gdb.execute('set \$_a=0'))" --ex "continue" repro

    Reading symbols from repro...
    Temporary breakpoint 1 at 0x1131: file repro.cc, line 1.
    Starting program: /home/mhov/repos/binutils-gdb-master/install/bin/repro

    Temporary breakpoint 1, main () at repro.cc:1
    1	int main() { return 0; }
    Continuing.
    [Inferior 1 (process 75524) 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

This is the same assertion site as in
<https://sourceware.org/bugzilla/show_bug.cgi?id=26761>, but I'm not
sure if it is the same problem.

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

Diffing results before and after the patch shows a few XFAIL in
gdb.threads/attach-many-short-lived-threads.exp, and one unexpected
failure in gdb.base/run-attach-while-running.exp which seems to be
flaky on master as well.

gdb/ChangeLog:

2021-05-05  Magne Hov  <mhov@undo.io>

    * eval.c (expression::evaluate): Check inferior_ptid.

gdb/testsuite/ChangeLog:

2021-05-05  Magne Hov  <mhov@undo.io>

    * gdb.python/py-events.exp: Extend inferior exit tests.
    * gdb.python/py-events.py: Print inferior exit PID.
    * gdb.python/py-events.cc: New test.
---
 gdb/eval.c                             |  2 +-
 gdb/testsuite/gdb.python/py-events.cc  | 21 ++++++++++++++
 gdb/testsuite/gdb.python/py-events.exp | 39 ++++++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-events.py  |  1 +
 4 files changed, 62 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.python/py-events.cc

diff --git a/gdb/eval.c b/gdb/eval.c
index 1b3c974009a..34c6c6160d4 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.cc b/gdb/testsuite/gdb.python/py-events.cc
new file mode 100644
index 00000000000..b865a5a9652
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-events.cc
@@ -0,0 +1,21 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 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/>.  */
+
+int main(void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/py-events.exp b/gdb/testsuite/gdb.python/py-events.exp
index e89cd8b021b..bec3c9f2cab 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
+    global process_id
+    gdb_test_multiple "info proc" $test {
+	-re "process (\\d+).*$gdb_prompt $" {
+	    set process_id $expect_out(1,string)
+	    pass $test
+	}
+    }
+}
+
+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.*"
+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,27 @@ 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 C++ expression were evaluated.
+standard_testfile .cc
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+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"
+gdb_test "run" "exited normally.*"
+gdb_test "print \$_foo" "= 1" "check foo after run"
+gdb_test "start" "Temporary breakpoint .* main .*" "stop on a C++ frame"
+gdb_test "continue" "exited normally.*"
+gdb_test "print \$_foo" "= 2" "check foo after start continue"
+
+>>>>>>> 2d04478f2ca... gdb: check inferior_ptid before calling inferior_thread in eval.c
diff --git a/gdb/testsuite/gdb.python/py-events.py b/gdb/testsuite/gdb.python/py-events.py
index bd1dd749452..53c7f9f20a4 100644
--- a/gdb/testsuite/gdb.python/py-events.py
+++ b/gdb/testsuite/gdb.python/py-events.py
@@ -44,6 +44,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)))
 
 def continue_handler (event):
-- 
2.25.1

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

[Simon Marchi]

y
On 2021-05-05 11:56 a.m., Magne Hov via Gdb-patches wrote:
> Evaluating expressions from within an inferior exit event handler can
> cause a crash:
> 
>     echo "int main() { return 0; }" > repro.cc
>     g++ -g repro.cc -o repro
>     ./gdb -q --ex "start" --ex "python gdb.events.exited.connect(lambda _: gdb.execute('set \$_a=0'))" --ex "continue" repro
> 
>     Reading symbols from repro...
>     Temporary breakpoint 1 at 0x1131: file repro.cc, line 1.
>     Starting program: /home/mhov/repos/binutils-gdb-master/install/bin/repro
> 
>     Temporary breakpoint 1, main () at repro.cc:1
>     1	int main() { return 0; }
>     Continuing.
>     [Inferior 1 (process 75524) 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
> 
> This is the same assertion site as in
> <https://sourceware.org/bugzilla/show_bug.cgi?id=26761>, but I'm not
> sure if it is the same problem.

I don't think so, this assertion in inferior_thread (trying to get the
current thread while there is not current thread) is kind of a generic
symptom, but the root cause can be any thing.

> 
> 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 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
> 
> Diffing results before and after the patch shows a few XFAIL in
> gdb.threads/attach-many-short-lived-threads.exp, and one unexpected
> failure in gdb.base/run-attach-while-running.exp which seems to be
> flaky on master as well.

First, let me say that your commit message is fantastic.

I do agree with your analysis and the fix.  inferior_ptid is always
supposed to be in sync with current_thread_.

> diff --git a/gdb/testsuite/gdb.python/py-events.exp b/gdb/testsuite/gdb.python/py-events.exp
> index e89cd8b021b..bec3c9f2cab 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
> +    global process_id
> +    gdb_test_multiple "info proc" $test {
> +	-re "process (\\d+).*$gdb_prompt $" {
> +	    set process_id $expect_out(1,string)
> +	    pass $test
> +	}
> +    }
> +}

Insted of a proc setting a global variable (process_id), I would prefer
if the proc returned the value.  And then you could do:

set process_id [get_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.*"
> +get_process_id "get inferior 2 process id"
>  gdb_test "continue" ".*event type: continue.*

I get:

    DUPLICATE: gdb.python/py-events.exp: continue

We try to avoid duplicate test names, to make it easier to track
down failures.  A handy way to make test names unique is to use
with_test_prefix:

with_test_prefix "inferior 1" {
    ...
}

with_test_prefix "inferior 2" {
    .
}

That will add "inferior 1: " and "inferior 2: " to the test names of
whatever is within these { }.

>  .*event type: exit.*
>  .*exit code: 12.*
>  .*exit inf: 2.*
> +.*exit pid: $process_id.*
>  dir ok: True.*" "Inferior 2 terminated."
>  
>  
> @@ -235,3 +250,27 @@ 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 C++ expression were evaluated.

Wrap the comment to 80 columns, and use two spaces after the period
(this is a common rule in this codebase).

> +standard_testfile .cc

Is C++ really required?  I was able to reproduce the bug with your
reproducer, but compiled as C.  If we can re-use the existing test
program for this test (just let it run to completion), it would be
simpler and preferable.

> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
> +    return -1
> +}
> +
> +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"
> +gdb_test "run" "exited normally.*"
> +gdb_test "print \$_foo" "= 1" "check foo after run"
> +gdb_test "start" "Temporary breakpoint .* main .*" "stop on a C++ frame"
> +gdb_test "continue" "exited normally.*"

We want the tests to also work when using the native-gdbserver
(gdbserver using the remote protocol) and native-extended-gdbserver
(gdbserver using the extended-remote protocol) boards.  For this reason,
it's generally not good to use "run" and "start" literally in the tests.

Instead, try to use the procedures runto_main, gdb_run_cmd,
gdb_start_cmd, etc.

Unfortunately, I can see that this test is completely skipped when
running with the native-gdbserver board.  I don't understand why, Python
events are still relevant then.  And it's broken with the
native-extended-gdbserver board.  But let's at least try to do the right
thing and use the right procedures.  It will help for when someone goes
and tries to fix that test.

> +gdb_test "print \$_foo" "= 2" "check foo after start continue"
> +
> +>>>>>>> 2d04478f2ca... gdb: check inferior_ptid before calling inferior_thread in eval.c

I guess this last line is not meant to be included.

Simon

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

[Magne Hov]

Thank you for reviewing the patch so quickly. I will follow up with a
revised patch shortly.

On  5 May 2021 21:59, Simon Marchi <simon.marchi@polymtl.ca> wrote:

> y
> On 2021-05-05 11:56 a.m., Magne Hov via Gdb-patches wrote:
>> Evaluating expressions from within an inferior exit event handler can
>> cause a crash:
>> 
>>     echo "int main() { return 0; }" > repro.cc
>>     g++ -g repro.cc -o repro
>>     ./gdb -q --ex "start" --ex "python gdb.events.exited.connect(lambda _: gdb.execute('set \$_a=0'))" --ex "continue" repro
>> 
>>     Reading symbols from repro...
>>     Temporary breakpoint 1 at 0x1131: file repro.cc, line 1.
>>     Starting program: /home/mhov/repos/binutils-gdb-master/install/bin/repro
>> 
>>     Temporary breakpoint 1, main () at repro.cc:1
>>     1	int main() { return 0; }
>>     Continuing.
>>     [Inferior 1 (process 75524) 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
>> 
>> This is the same assertion site as in
>> <https://sourceware.org/bugzilla/show_bug.cgi?id=26761>, but I'm not
>> sure if it is the same problem.
>
> I don't think so, this assertion in inferior_thread (trying to get the
> current thread while there is not current thread) is kind of a generic
> symptom, but the root cause can be any thing.
>

In that case I will omit mentioning it in the the commit message.

>> 
>> 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 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
>> 
>> Diffing results before and after the patch shows a few XFAIL in
>> gdb.threads/attach-many-short-lived-threads.exp, and one unexpected
>> failure in gdb.base/run-attach-while-running.exp which seems to be
>> flaky on master as well.
>
> First, let me say that your commit message is fantastic.
>
> I do agree with your analysis and the fix.  inferior_ptid is always
> supposed to be in sync with current_thread_.
>
>> diff --git a/gdb/testsuite/gdb.python/py-events.exp b/gdb/testsuite/gdb.python/py-events.exp
>> index e89cd8b021b..bec3c9f2cab 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
>> +    global process_id
>> +    gdb_test_multiple "info proc" $test {
>> +	-re "process (\\d+).*$gdb_prompt $" {
>> +	    set process_id $expect_out(1,string)
>> +	    pass $test
>> +	}
>> +    }
>> +}
>
> Insted of a proc setting a global variable (process_id), I would prefer
> if the proc returned the value.  And then you could do:
>
> set process_id [get_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.*"
>> +get_process_id "get inferior 2 process id"
>>  gdb_test "continue" ".*event type: continue.*
>
> I get:
>
>     DUPLICATE: gdb.python/py-events.exp: continue
>
> We try to avoid duplicate test names, to make it easier to track
> down failures.  A handy way to make test names unique is to use
> with_test_prefix:
>
> with_test_prefix "inferior 1" {
>     ...
> }
>
> with_test_prefix "inferior 2" {
>     .
> }
>
> That will add "inferior 1: " and "inferior 2: " to the test names of
> whatever is within these { }.
>

That's a nice utility - thanks!

>>  .*event type: exit.*
>>  .*exit code: 12.*
>>  .*exit inf: 2.*
>> +.*exit pid: $process_id.*
>>  dir ok: True.*" "Inferior 2 terminated."
>>  
>>  
>> @@ -235,3 +250,27 @@ 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 C++ expression were evaluated.
>
> Wrap the comment to 80 columns, and use two spaces after the period
> (this is a common rule in this codebase).
>
>> +standard_testfile .cc
>
> Is C++ really required?  I was able to reproduce the bug with your
> reproducer, but compiled as C.  If we can re-use the existing test
> program for this test (just let it run to completion), it would be
> simpler and preferable.
>

I agree that it would be nice to reuse the existing test program. The only
requirement is that `language_defn->la_language == language_cplus` evaluates to
true in order to prevent the asserting condition in `eval.c` from being
short-circuited. We can force this by executing `set language c++`. In my
revised patch I do this and I have removed the unnecessary program.

>> +
>> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
>> +    return -1
>> +}
>> +
>> +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"
>> +gdb_test "run" "exited normally.*"
>> +gdb_test "print \$_foo" "= 1" "check foo after run"
>> +gdb_test "start" "Temporary breakpoint .* main .*" "stop on a C++ frame"
>> +gdb_test "continue" "exited normally.*"
>
> We want the tests to also work when using the native-gdbserver
> (gdbserver using the remote protocol) and native-extended-gdbserver
> (gdbserver using the extended-remote protocol) boards.  For this reason,
> it's generally not good to use "run" and "start" literally in the tests.
>
> Instead, try to use the procedures runto_main, gdb_run_cmd,
> gdb_start_cmd, etc.
>

I'm not entirely used to the test system conventions yet so I really appreciate
these comments. I've adopted gdb_run_cmd and gdb_start_cmd. 

> Unfortunately, I can see that this test is completely skipped when
> running with the native-gdbserver board.  I don't understand why, Python
> events are still relevant then.  And it's broken with the
> native-extended-gdbserver board.  But let's at least try to do the right
> thing and use the right procedures.  It will help for when someone goes
> and tries to fix that test.
>

I see something similar, native-gdbserver appears to be skipped entirely and
native-extended-gdbserver fails with `FAIL: gdb.python/py-events.exp: get
current thread`. I've checked that the new tests pass with
native-extended-gdbserver on my machine by temporarily commenting out all the
other test.

>> +gdb_test "print \$_foo" "= 2" "check foo after start continue"
>> +
>> +>>>>>>> 2d04478f2ca... gdb: check inferior_ptid before calling inferior_thread in eval.c
>
> I guess this last line is not meant to be included.
>

That's right - thanks for spotting that.

> Simon

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

[Magne Hov]

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 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, and one unexpected
failure in gdb.base/run-attach-while-running.exp which seems to be
flaky on master as well. 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-10  Magne Hov  <mhov@undo.io>

	* eval.c (expression::evaluate): Check inferior_ptid.

gdb/testsuite/ChangeLog:

2021-05-10  Magne Hov  <mhov@undo.io>

	* 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..34c6c6160d4 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..9aa0680fd0e 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 $test
+	}
+    }
+    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 bd1dd749452..53c7f9f20a4 100644
--- a/gdb/testsuite/gdb.python/py-events.py
+++ b/gdb/testsuite/gdb.python/py-events.py
@@ -44,6 +44,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)))
 
 def continue_handler (event):
-- 
2.25.1

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

[Simon Marchi]

>>> +standard_testfile .cc
>>
>> Is C++ really required?  I was able to reproduce the bug with your
>> reproducer, but compiled as C.  If we can re-use the existing test
>> program for this test (just let it run to completion), it would be
>> simpler and preferable.
>>
> 
> I agree that it would be nice to reuse the existing test program. The only
> requirement is that `language_defn->la_language == language_cplus` evaluates to
> true in order to prevent the asserting condition in `eval.c` from being
> short-circuited. We can force this by executing `set language c++`. In my
> revised patch I do this and I have removed the unnecessary program.

Sorry, I thought I tried with the source file as C and saw the bug, but
I no longer see it.  I probably tested with the wrong file or something.

Using "set language c++" sounds good.

Simon

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

[Simon Marchi]

On 2021-05-10 1:21 p.m., Magne Hov via Gdb-patches 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 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, and one unexpected
> failure in gdb.base/run-attach-while-running.exp which seems to be
> flaky on master as well. 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-10  Magne Hov  <mhov@undo.io>
> 
> 	* eval.c (expression::evaluate): Check inferior_ptid.
> 
> gdb/testsuite/ChangeLog:
> 
> 2021-05-10  Magne Hov  <mhov@undo.io>
> 
> 	* gdb.python/py-events.exp: Extend inferior exit tests.
> 	* gdb.python/py-events.py: Print inferior exit PID.

The patch LGTM, but I forgot to ask, do you have a copyright assignment
on file?  It is necessary, since your patch is a bit above the "trivial"
threshold.  I couldn't find anything with your name or your company's
name.  If you don't have one and would like to have one in your personal
name, this would be the form to use:

    https://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future

Let me know once that's done, it can take a bit of time for the FSF to
process it.

I filed this with the 11.1 target milestone so it doesn't fall through
the cracks:

    https://sourceware.org/bugzilla/show_bug.cgi?id=27841

Simon

RE: [PATCH v2] gdb: fix eval.c assert during inferior exit event

[Aktemur, Tankut Baris]

On Monday, May 10, 2021 7:21 PM, Magne Hov wrote:
> diff --git a/gdb/eval.c b/gdb/eval.c
> index 1b3c974009a..34c6c6160d4 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

Nit: there is an extra space before `&&`.

>        && 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..9aa0680fd0e 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 $test

Using `$gdb_test_name` is preferable here, I believe.

-Baris



Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH v2] gdb: fix eval.c assert during inferior exit event

[Magne Hov]

Thanks for these comments, I'll make sure to address them in a revised
patch. 

On 11 May 2021 07:16, "Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> wrote:

> On Monday, May 10, 2021 7:21 PM, Magne Hov wrote:
>> diff --git a/gdb/eval.c b/gdb/eval.c
>> index 1b3c974009a..34c6c6160d4 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
>
> Nit: there is an extra space before `&&`.
>
>>        && 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..9aa0680fd0e 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 $test
>
> Using `$gdb_test_name` is preferable here, I believe.
>
> -Baris
>
>
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928

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

[Magne Hov]

On 10 May 2021 13:44, Simon Marchi <simon.marchi@polymtl.ca> wrote:

> On 2021-05-10 1:21 p.m., Magne Hov via Gdb-patches 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 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, and one unexpected
>> failure in gdb.base/run-attach-while-running.exp which seems to be
>> flaky on master as well. 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-10  Magne Hov  <mhov@undo.io>
>> 
>> 	* eval.c (expression::evaluate): Check inferior_ptid.
>> 
>> gdb/testsuite/ChangeLog:
>> 
>> 2021-05-10  Magne Hov  <mhov@undo.io>
>> 
>> 	* gdb.python/py-events.exp: Extend inferior exit tests.
>> 	* gdb.python/py-events.py: Print inferior exit PID.
>
> The patch LGTM, but I forgot to ask, do you have a copyright assignment
> on file?  It is necessary, since your patch is a bit above the "trivial"
> threshold.  I couldn't find anything with your name or your company's
> name.  If you don't have one and would like to have one in your personal
> name, this would be the form to use:
>
>     https://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future
>
> Let me know once that's done, it can take a bit of time for the FSF to
> process it.

I don't have an existing copyright assignment, but I've submitted the
signed forms and so I am waiting for the final forms to be returned.

>
> I filed this with the 11.1 target milestone so it doesn't fall through
> the cracks:
>
>     https://sourceware.org/bugzilla/show_bug.cgi?id=27841

Thanks for doing that.

>
> Simon

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

[Joel Brobecker]

Hello Magne,

> 2021-05-10  Magne Hov  <mhov@undo.io>
> 
> 	* eval.c (expression::evaluate): Check inferior_ptid.
> 
> gdb/testsuite/ChangeLog:
> 
> 2021-05-10  Magne Hov  <mhov@undo.io>
> 
> 	* gdb.python/py-events.exp: Extend inferior exit tests.
> 	* gdb.python/py-events.py: Print inferior exit PID.

A quick request when the copyright assignment process is complete;
since Simon created a PR in bugzilla to track this issue, would
you mind adding...

        PR python/27841

... at the start of each ChangeLog entry above, when you push the
commit? This way, you commit gets automatically filed into that PR
in bugzilla as well.

If you look at the current ChangeLog files, you'll find lots of
examples.

Thank you!
-- 
Joel

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

[Magne Hov]

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

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

[Simon Marchi]

On 2021-05-26 9:02 a.m., Magne Hov via Gdb-patches wrote:
> Evaluating expressions from within an inferior exit event handler can
> cause a crash:
>
> ...

So, this was already approved and I can now see your copyright
assignment.  Do you already have push access?  If not, do you want it
(useful if you intend on sending more patches) or would you like me to
push the patch on your behalf?

Simon

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

[Magne Hov]

On Fri, May 28 2021, Simon Marchi wrote:

> On 2021-05-26 9:02 a.m., Magne Hov via Gdb-patches wrote:
>> Evaluating expressions from within an inferior exit event handler can
>> cause a crash:
>>
>> ...
>
> So, this was already approved and I can now see your copyright
> assignment.  Do you already have push access?  If not, do you want it
> (useful if you intend on sending more patches) or would you like me to
> push the patch on your behalf?

I don't push access at the moment, but as I'm hoping to contribute more
patches I would be good to have it. I'm

>
> Simon

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

[Magne Hov]

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