From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x131.google.com (mail-lf1-x131.google.com [IPv6:2a00:1450:4864:20::131]) by sourceware.org (Postfix) with ESMTPS id 7976C3857801 for ; Thu, 3 Jun 2021 21:07:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7976C3857801 Received: by mail-lf1-x131.google.com with SMTP id i10so10913515lfj.2 for ; Thu, 03 Jun 2021 14:07:56 -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=TvUX56VP3Yu90abdAb/HujWqGalaRqQ//NXC37pihDw=; b=aSJsCJWQMmhulqLaIWCTiHIPTDeHUJn+IRNgoaleK/q5w0yU59TMmst36o6vUEzAQ7 h++RJtTAkdE95PMf8wHtcqKgxbzk09kL3MROVZSqjXh9OgpN3OLEgP9rb+kiWKFbaXRR od2mXHZVJvf4EC963Pr/K69+5MQrKiIVQW6OPUFHXAvGap/34dqt0aGceg20b8zidVHV O/Oqml2c14TLMCo5WlCwe3ZWsxHXz9y4Q14vlF2yUEF4qrgGCSoEtjTPgZmI2HM3ALU2 jjZMq6Ot3eiA7JHzoysYZ9yvKVPdHdtmJ7OQXxMgKqvjMBgYdRsA36CCPDtyYdNgzP8E nukA== X-Gm-Message-State: AOAM530PbpMz0RSZdsUrXnIOIyMk6O/FNRFH/jm74OnE8nGYDNepmsQO c/QprqozfJLbNfSxwZCzRBi08tfLAeTv6fLUpM15jmTWPIqbY6uymvEVC/lDEbyFB89KhOjJ3Q9 nf64EbR5HYkd8b8S/eKB7TRijDrOmwgbKL53V7rsWAf7LdOblvxBwvKdjj2mVuZL3uA== X-Google-Smtp-Source: ABdhPJwtheHk4DxHWibIlFTioSSy7bcB62UbkFPQtHxwQD968NrXjB/8DsAZBBgiliZFuEpe5N31HQ== X-Received: by 2002:a05:6512:2390:: with SMTP id c16mr537514lfv.183.1622754475016; Thu, 03 Jun 2021 14:07:55 -0700 (PDT) Received: from bucheron-thinkpad (telia-590881-181.connect.netcom.no. [89.8.129.181]) by smtp.gmail.com with ESMTPSA id r195sm421749lff.69.2021.06.03.14.07.54 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Jun 2021 14:07:54 -0700 (PDT) From: Magne Hov To: gdb-patches@sourceware.org Subject: Re: [PATCH v3] gdb: fix eval.c assert during inferior exit event In-Reply-To: <20210526130233.986846-1-mhov@undo.io> References: <20210505155627.3850386-1-mhov@undo.io> <20210526130233.986846-1-mhov@undo.io> Date: Thu, 03 Jun 2021 22:07:53 +0100 Message-ID: <5sbl8m3bdi.fsf@undo.io> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_BARRACUDACENTRAL, 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: Thu, 03 Jun 2021 21:07:58 -0000 This has now been pushed. Thank you everyone for comments on the patch, and an extra thanks to Simon for helping me with the push process. Kind regards, Magne On Wed, May 26 2021, Magne Hov 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 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 > > PR python/27841 > * eval.c (expression::evaluate): Check inferior_ptid. > > gdb/testsuite/ChangeLog: > > 2021-05-26 Magne Hov > > 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 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