public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Magne Hov <mhov@undo.io>
To: gdb-patches@sourceware.org
Subject: [PATCH v3] gdb: fix eval.c assert during inferior exit event
Date: Wed, 26 May 2021 14:02:33 +0100	[thread overview]
Message-ID: <20210526130233.986846-1-mhov@undo.io> (raw)
In-Reply-To: <20210505155627.3850386-1-mhov@undo.io>

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


  parent reply	other threads:[~2021-05-26 13:02 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-05 15:56 [RFC PATCH] " Magne Hov
2021-05-06  1:59 ` Simon Marchi
2021-05-10 16:57   ` Magne Hov
2021-05-10 17:33     ` Simon Marchi
2021-05-10 17:21 ` [PATCH v2] " Magne Hov
2021-05-10 17:44   ` Simon Marchi
2021-05-14 16:24     ` Magne Hov
2021-05-11  7:16   ` Aktemur, Tankut Baris
2021-05-14 16:22     ` Magne Hov
2021-05-24 19:23   ` Joel Brobecker
2021-05-26 13:02 ` Magne Hov [this message]
2021-05-29  2:25   ` [PATCH v3] " Simon Marchi
2021-06-02 11:34     ` Magne Hov
2021-06-03 21:07   ` Magne Hov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210526130233.986846-1-mhov@undo.io \
    --to=mhov@undo.io \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).