From: Magne Hov <mhov@undo.io>
To: Simon Marchi <simon.marchi@polymtl.ca>, gdb-patches@sourceware.org
Subject: Re: [PATCH v2] gdb: fix eval.c assert during inferior exit event
Date: Fri, 14 May 2021 17:24:36 +0100 [thread overview]
Message-ID: <5sk0o1jnaz.fsf@undo.io> (raw)
In-Reply-To: <64c998a8-c267-aecc-370b-b651a4aecd7e@polymtl.ca>
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
next prev parent reply other threads:[~2021-05-14 16:24 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 [this message]
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 ` [PATCH v3] " Magne Hov
2021-05-29 2:25 ` 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=5sk0o1jnaz.fsf@undo.io \
--to=mhov@undo.io \
--cc=gdb-patches@sourceware.org \
--cc=simon.marchi@polymtl.ca \
/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).