From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id E2DEB3857C59 for ; Mon, 10 May 2021 17:44:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E2DEB3857C59 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 14AHisA9009847 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 10 May 2021 13:44:59 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 14AHisA9009847 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 2C43A1E813; Mon, 10 May 2021 13:44:54 -0400 (EDT) Subject: Re: [PATCH v2] gdb: fix eval.c assert during inferior exit event To: Magne Hov , gdb-patches@sourceware.org References: <20210505155627.3850386-1-mhov@undo.io> <20210510172121.2123009-1-mhov@undo.io> From: Simon Marchi Message-ID: <64c998a8-c267-aecc-370b-b651a4aecd7e@polymtl.ca> Date: Mon, 10 May 2021 13:44:53 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210510172121.2123009-1-mhov@undo.io> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 10 May 2021 17:44:54 +0000 X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 May 2021 17:45:01 -0000 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: > . > > 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 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 > > * eval.c (expression::evaluate): Check inferior_ptid. > > gdb/testsuite/ChangeLog: > > 2021-05-10 Magne Hov > > * 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