From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) by sourceware.org (Postfix) with ESMTPS id C06663943426 for ; Fri, 14 May 2021 16:24:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C06663943426 Received: by mail-wr1-x42e.google.com with SMTP id s8so30578520wrw.10 for ; Fri, 14 May 2021 09:24:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version; bh=K8+25/pSxIFEQWwnEavZlQQksac6WiPtJAxYnFbJuUs=; b=k7fvrUHLZ2et5Qnl4b6o7xwMwOu6OY44mdvVts/4is6XWPkxLidwnigARTCvXfd+tq ZETQFmoKs6C2sXalCJkbi/Kg5/vAVyJGIv/LnnxjbHDAHTkpP2H0vWTK3cx1jyKf+T4s YcGjz1+XKyaiVRyWCyQGromcWwfcX+1biIsV/vHzAhldBC4N9Xj6EHuuvnSKLtSRf6yD +FVC0t/xBcODobUkLUF4Fybw8uS3EWDgbbmtdNPC7T+WjTjTJJkCwAKhmS7AaG93Tvis U+sYJOW2zSHzhMG3Wiko5aYmWXM8Kbl2tdpjTwJPwZaiP7GnM0KCX3icEcUUNQEhtElq r29w== X-Gm-Message-State: AOAM533WqYLWVMl0avM3bww+hyGKVNpeVSrQuLJpVI7Uf2S6zISy/n8H f1Mo1Jcp09xJoSHxbPorr+/ijqnHb9Ez4vblasZjCvHEzjqRICMDiNbKfhxKUmBJz1MfhkEXWGz XW8SnMYMGHIUqEnV5TtYu2TaHB5Tb9grqWZ0D2W8UhfZcFT2iOv9Igfsf2UdwsQvUTQ== X-Google-Smtp-Source: ABdhPJwhn444mHMlNdWhT9oKjdVOK15Oj5zBQl7dmbAP++FNVq/JXKtRtMmTLMOspTUPYGUrn1R9tQ== X-Received: by 2002:a5d:6e11:: with SMTP id h17mr58465739wrz.331.1621009477821; Fri, 14 May 2021 09:24:37 -0700 (PDT) Received: from bucheron-thinkpad (cpc120850-nrwh12-2-0-cust139.4-4.cable.virginm.net. [82.32.180.140]) by smtp.gmail.com with ESMTPSA id p1sm7141312wrs.50.2021.05.14.09.24.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 May 2021 09:24:37 -0700 (PDT) From: Magne Hov To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH v2] gdb: fix eval.c assert during inferior exit event In-Reply-To: <64c998a8-c267-aecc-370b-b651a4aecd7e@polymtl.ca> References: <20210505155627.3850386-1-mhov@undo.io> <20210510172121.2123009-1-mhov@undo.io> <64c998a8-c267-aecc-370b-b651a4aecd7e@polymtl.ca> Date: Fri, 14 May 2021 17:24:36 +0100 Message-ID: <5sk0o1jnaz.fsf@undo.io> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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: Fri, 14 May 2021 16:24:41 -0000 On 10 May 2021 13:44, Simon Marchi 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: >> . >> >> 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 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