From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x435.google.com (mail-wr1-x435.google.com [IPv6:2a00:1450:4864:20::435]) by sourceware.org (Postfix) with ESMTPS id 12B59385E02F for ; Mon, 10 May 2021 16:57:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 12B59385E02F Received: by mail-wr1-x435.google.com with SMTP id z6so17363289wrm.4 for ; Mon, 10 May 2021 09:57:15 -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=2OC5su0WLf1biUZpFE3FzD4XPDoNjtNhfTXW9G8ln6w=; b=nwJDTImk8oAKEkoqOfUnXsx7Og/nWjt/BR3TyhAITFZV4a04Oc7mBzbc6meRphmvc8 93zei0eqDqR0KaHo0RTyzbhrJLaz/clVLutZbRuXf9PPpFTQL3QnzM4ef4mpdiq/igcO tS2rjkB1Ub3J2xWHWJAGpjExwwFc8eKtbHSeulgLv4N4MHIijnbXAAA230JEvWPhNoEv jIx3DW5r6SS35kfMjzDCrqOczpjFyE+dCmQHm5tAbyNYH4okot/5OEvKZl6grR2xvqO8 OVsX9z9GQfcgG0ndm3biAv85DlukhrtrcK9zpOKqzGSu0kmd4+s4mPYJW+NRPieCoNEG T67Q== X-Gm-Message-State: AOAM533t1DBsLXMHIIk0UYm8W65710/CAs+Ymq7d9LyiIJ/Kj4SCzUb3 66vJb1ajpS5mJk1o7+0bU2tgkFHWj1mNbMNUOvJ46W50rWvCMeQauo8rsOj7W7JTaV3DTWgfdFZ NlFNPg6fX0HB4X/BStSS7/rKTvNdf+IQJ0DAt81gKFlz4ZuLq3TM+b/3DupO4K+wKbg== X-Google-Smtp-Source: ABdhPJzEeUFwFgdhQalLJgm4mfmvZaCIsYPAzFhlhfCbgnA6NSsErzC4z+2ZaulZL9bwxDpTldmaWw== X-Received: by 2002:adf:edd0:: with SMTP id v16mr31825991wro.3.1620665833922; Mon, 10 May 2021 09:57:13 -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 f8sm18467704wmg.43.2021.05.10.09.57.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 May 2021 09:57:13 -0700 (PDT) From: Magne Hov To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [RFC PATCH] gdb: fix eval.c assert during inferior exit event In-Reply-To: <2dcd95bb-4262-4a4a-e851-472f977f15c9@polymtl.ca> References: <20210505155627.3850386-1-mhov@undo.io> <2dcd95bb-4262-4a4a-e851-472f977f15c9@polymtl.ca> Date: Mon, 10 May 2021 17:57:13 +0100 Message-ID: <5swns6le6u.fsf@undo.io> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: Mon, 10 May 2021 16:57:17 -0000 Thank you for reviewing the patch so quickly. I will follow up with a revised patch shortly. On 5 May 2021 21:59, Simon Marchi wrote: > y > On 2021-05-05 11:56 a.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.cc >> g++ -g repro.cc -o repro >> ./gdb -q --ex "start" --ex "python gdb.events.exited.connect(lambda _: gdb.execute('set \$_a=0'))" --ex "continue" repro >> >> Reading symbols from repro... >> Temporary breakpoint 1 at 0x1131: file repro.cc, line 1. >> Starting program: /home/mhov/repos/binutils-gdb-master/install/bin/repro >> >> Temporary breakpoint 1, main () at repro.cc:1 >> 1 int main() { return 0; } >> Continuing. >> [Inferior 1 (process 75524) 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 >> >> This is the same assertion site as in >> , but I'm not >> sure if it is the same problem. > > I don't think so, this assertion in inferior_thread (trying to get the > current thread while there is not current thread) is kind of a generic > symptom, but the root cause can be any thing. > In that case I will omit mentioning it in the the commit message. >> >> 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 >> >> Diffing results before and after the patch shows 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. > > First, let me say that your commit message is fantastic. > > I do agree with your analysis and the fix. inferior_ptid is always > supposed to be in sync with current_thread_. > >> diff --git a/gdb/testsuite/gdb.python/py-events.exp b/gdb/testsuite/gdb.python/py-events.exp >> index e89cd8b021b..bec3c9f2cab 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 >> + global process_id >> + gdb_test_multiple "info proc" $test { >> + -re "process (\\d+).*$gdb_prompt $" { >> + set process_id $expect_out(1,string) >> + pass $test >> + } >> + } >> +} > > Insted of a proc setting a global variable (process_id), I would prefer > if the proc returned the value. And then you could do: > > set process_id [get_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.*" >> +get_process_id "get inferior 2 process id" >> gdb_test "continue" ".*event type: continue.* > > I get: > > DUPLICATE: gdb.python/py-events.exp: continue > > We try to avoid duplicate test names, to make it easier to track > down failures. A handy way to make test names unique is to use > with_test_prefix: > > with_test_prefix "inferior 1" { > ... > } > > with_test_prefix "inferior 2" { > . > } > > That will add "inferior 1: " and "inferior 2: " to the test names of > whatever is within these { }. > That's a nice utility - thanks! >> .*event type: exit.* >> .*exit code: 12.* >> .*exit inf: 2.* >> +.*exit pid: $process_id.* >> dir ok: True.*" "Inferior 2 terminated." >> >> >> @@ -235,3 +250,27 @@ 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 C++ expression were evaluated. > > Wrap the comment to 80 columns, and use two spaces after the period > (this is a common rule in this codebase). > >> +standard_testfile .cc > > Is C++ really required? I was able to reproduce the bug with your > reproducer, but compiled as C. If we can re-use the existing test > program for this test (just let it run to completion), it would be > simpler and preferable. > I agree that it would be nice to reuse the existing test program. The only requirement is that `language_defn->la_language == language_cplus` evaluates to true in order to prevent the asserting condition in `eval.c` from being short-circuited. We can force this by executing `set language c++`. In my revised patch I do this and I have removed the unnecessary program. >> + >> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} { >> + return -1 >> +} >> + >> +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" >> +gdb_test "run" "exited normally.*" >> +gdb_test "print \$_foo" "= 1" "check foo after run" >> +gdb_test "start" "Temporary breakpoint .* main .*" "stop on a C++ frame" >> +gdb_test "continue" "exited normally.*" > > We want the tests to also work when using the native-gdbserver > (gdbserver using the remote protocol) and native-extended-gdbserver > (gdbserver using the extended-remote protocol) boards. For this reason, > it's generally not good to use "run" and "start" literally in the tests. > > Instead, try to use the procedures runto_main, gdb_run_cmd, > gdb_start_cmd, etc. > I'm not entirely used to the test system conventions yet so I really appreciate these comments. I've adopted gdb_run_cmd and gdb_start_cmd. > Unfortunately, I can see that this test is completely skipped when > running with the native-gdbserver board. I don't understand why, Python > events are still relevant then. And it's broken with the > native-extended-gdbserver board. But let's at least try to do the right > thing and use the right procedures. It will help for when someone goes > and tries to fix that test. > I see something similar, native-gdbserver appears to be skipped entirely and native-extended-gdbserver fails with `FAIL: gdb.python/py-events.exp: get current thread`. I've checked that the new tests pass with native-extended-gdbserver on my machine by temporarily commenting out all the other test. >> +gdb_test "print \$_foo" "= 2" "check foo after start continue" >> + >> +>>>>>>> 2d04478f2ca... gdb: check inferior_ptid before calling inferior_thread in eval.c > > I guess this last line is not meant to be included. > That's right - thanks for spotting that. > Simon