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

[Simon Marchi <simon.marchi@polymtl.ca>]

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