Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Lancelot SIX <Lancelot.Six@amd.com>
To: Andrew Burgess <aburgess@redhat.com>, gdb-patches@sourceware.org
Subject: Re: [PATCHv2] gdb/python: fix 'exited' event when GDB exits from core file debugging
Date: Fri, 12 Jun 2026 14:49:53 +0100	[thread overview]
Message-ID: <038a1fc7-8627-4030-8fc5-e02cafced42a@amd.com> (raw)
In-Reply-To: <7db5bc5d0e3c7189881540ab2c775ea20fc03b5f.1780767159.git.aburgess@redhat.com>



On 06/06/2026 18:32, Andrew Burgess wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> This fixes an issue that was reported here:
> 
>    https://inbox.sourceware.org/gdb-patches/v3x4md2dg6rflq35ymzwrmmqf5uaem5exrnlbsp5dmhph2vihy@lq22ncu774yu
> 
> After commit:
> 
>    commit 3780b9993c973a2b68b496b80eddb820c0932cc0
>    Date:   Fri Mar 27 11:29:07 2026 +0000
> 
>      gdb: refactor core_target ::close and ::detach functions
> 
> it was observed that the Python 'exited' event was no longer being
> emitted when debugging a core file, and then exiting GDB.
> 
> The problem is that, when GDB is exiting we eventually end up in
> quit_force (in top.c), which calls kill_or_detach for every inferior.
> 
> In kill_or_detach we call either target_detach or target_kill, but
> only for non-core file targets.  For core file targets, neither of
> these is called and kill_or_detach does nothing of interest.
> 
> After the call to kill_or_detach, we call inferior::pop_all_targets,
> which calls inferior::pop_all_targets_above the dummy_stratum target,
> which means popping all targets.
> 
> In inferior::pop_all_targets_above (in inferior.c), we call
> switch_to_inferior_no_thread, which ensures the correct inferior is
> selected, but makes it so that no thread is selected.  Switching to no
> thread sets inferior_ptid to null_ptid.
> 
> Now popping the core_target calls core_target::close, and within
> core_target::close we currently check inferior_ptid in order to
> determine if exit_core_file_inferior has already been called or not.
> We only call exit_core_file_inferior if inferior_ptid is not
> null_ptid, so in this case we will not call exit_core_file_inferior.
> 
> The only other place that exit_core_file_inferior can be called from
> is core_target::detach, but remember we specifically avoided calling
> target_detach earlier in kill_or_detach.  This means that
> exit_core_file_inferior ends up never being called.
> 
> It is exit_core_file_inferior that calls exit_inferior, and it is from
> here that the Python 'exited' event is emitted.
> 
> I don't see any reason why kill_or_detach couldn't call target_detach
> for a core file target, but I don't propose making that change in this
> commit.
> 
> The check against inferior_ptid in core_target::close is clearly
> incorrect, checking this requires that a suitable thread within the
> inferior be selected, and that is not really a requirement for closing
> a core_target.  Instead, we can just check the inferior::pid field.
> When we open a core_target we always set inferior::pid, even if we
> just assign a fake CORELOW_PID value, so checking inferior::pid
> against zero will tell us if the inferior has already been exited.
> Fixing this check is enough to resolve the reported bug and ensure
> that the 'exited' event is always emitted, which is why I don't
> propose changing kill_or_detach in this commit.
> 
> An assert in core_target::exit_core_file_inferior has to go too for
> the same reason, the assert is checking that a thread is currently
> selected, and as discussed above, this is not always the case.
> 
> There's a new test which checks that the 'exited' event is emitted for
> both a core file debug session, and a live inferior debug session.
> Only the core file case was broken before this commit, but more
> testing is always a good thing.
> ---
>   gdb/corelow.c                                 |  13 +-
>   .../gdb.python/py-inf-exited-at-exit.c        |  29 ++++
>   .../gdb.python/py-inf-exited-at-exit.exp      | 125 ++++++++++++++++++
>   .../gdb.python/py-inf-exited-at-exit.py       |  20 +++
>   4 files changed, 179 insertions(+), 8 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.python/py-inf-exited-at-exit.c
>   create mode 100644 gdb/testsuite/gdb.python/py-inf-exited-at-exit.exp
>   create mode 100644 gdb/testsuite/gdb.python/py-inf-exited-at-exit.py
> 
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 819e7cae6f9..185b8da90de 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -629,10 +629,6 @@ core_target::build_file_mappings ()
>   void
>   core_target::exit_core_file_inferior ()
>   {
> -  /* Opening a core file ensures that some thread, even if it's just a
> -     "fake" thread, will have been selected.  */
> -  gdb_assert (inferior_ptid != null_ptid);
> -
>     /* Avoid confusion from thread stuff.  */
>     switch_to_no_thread ();
> 
> @@ -665,10 +661,11 @@ core_target::close ()
>        mostly harmless except it causes two 'exited' events to be emitted in
>        the Python API, which isn't ideal.
> 
> -     As opening a core_target always ensures that some thread is selected,
> -     then we can tell if exit_core_file_inferior has already been called by
> -     checking if no thread is now selected.  */
> -  if (inferior_ptid != null_ptid)
> +     As opening a core_target always ensures that a pid is assigned to the
> +     core file inferior, even if it is the fake CORELOW_PID, then we can
> +     tell if exit_core_file_inferior has already been called by checking if
> +     the inferior has a non-zero pid or not.  */
> +  if (current_inferior ()->pid != 0)
>       exit_core_file_inferior ();
> 
>     /* Core targets are heap-allocated (see core_target_open), so here
> diff --git a/gdb/testsuite/gdb.python/py-inf-exited-at-exit.c b/gdb/testsuite/gdb.python/py-inf-exited-at-exit.c
> new file mode 100644
> index 00000000000..068e2e6a509
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-inf-exited-at-exit.c
> @@ -0,0 +1,29 @@
> +/* Copyright 2026 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +void
> +foo (void)
> +{
> +  /* Nothing.  */
> +}
> +
> +int
> +main (void)
> +{
> +  foo ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.python/py-inf-exited-at-exit.exp b/gdb/testsuite/gdb.python/py-inf-exited-at-exit.exp
> new file mode 100644
> index 00000000000..53e2a2b782c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-inf-exited-at-exit.exp
> @@ -0,0 +1,125 @@
> +# Copyright (C) 2026 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Check that the 'exited' event triggers when GDB exits.  Test for
> +# both live inferiors, and for core files.
> +
> +require allow_python_tests
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +if {[build_executable "build executable" $testfile $srcfile] == -1} {
> +    return
> +}
> +
> +set remote_python_file \
> +    [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> +
> +# Load the Python script for this test.  Record the string
> +# representation of the current inferior.  Then exit GDB.  Ensure that
> +# during the exit we see a single Python 'exited' event associated
> +# with the expected inferior.
> +proc source_py_script_and_exit_checking_event {} {
> +    gdb_test_no_output "source $::remote_python_file" \
> +       "load python script"
> +
> +    set expected_inferior_string \
> +       [capture_command_output \
> +            "python print(str(gdb.selected_inferior()))" ""]
> +
> +    set inferior_string ""
> +    set event_count 0
> +    gdb_test_multiple "with confirm off -- exit" "exit gdb" {
> +       -re "^EVENT: inferior exited event\\.  Inferior is (\[^\r\n\]+)\r\n" {
> +           set inferior_string $expect_out(1,string)
> +           incr event_count
> +           exp_continue
> +       }
> +
> +       eof {
> +           verbose -log "GDB has now exited"
> +           gdb_assert { $expected_inferior_string eq $inferior_string \
> +                            && $event_count == 1 } $gdb_test_name
> +       }
> +
> +       -re "^\[^\r\n\]*\r\n" {
> +           exp_continue
> +       }
> +    }
> +}
> +
> +# Clean restart using global TESTFILE as the executable, then run to
> +# 'foo'.  Return true on success, otherwise, return false.
> +proc clean_restart_and_runto_foo {} {
> +    if {[clean_restart $::testfile] == -1} {
> +       return false
> +    }
> +
> +    if {![runto foo]} {
> +       return false
> +    }
> +
> +    return true

Hi Andrew,

Just one minor comment:

Can't this be simplified as

     return [runto foo]

?

Otherwise, this looks good to me, and I can confirm it fixes the issue 
I reported initially.

Best,
Lancelot

Reviewed-by: Lancelot Six <lancelot.six@amd.com>

> +}
> +
> +# Check that the current inferior's backtrace is 'main -> foo'.
> +proc check_backtrace { testname } {
> +    gdb_test "bt" \
> +       [multi_line \
> +            "#0  (?:$::hex in )?foo \\(\\) at \[^\r\n\]+" \
> +            "#1  (?:$::hex in )?main \\(\\) at \[^\r\n\]+"] \
> +       $testname
> +}
> +
> +# Create a core file.  Start GDB and load the core file.  Exit GDB.
> +# Check that we see an 'exited' event, and that it is associated with
> +# the correct gdb.Inferior.
> +proc_with_prefix check_with_corefile {} {
> +    if {![clean_restart_and_runto_foo]} {
> +       return
> +    }
> +
> +    check_backtrace "backtrace before generating core file"
> +
> +    set corefile "$::binfile.core"
> +    if {![gdb_gcore_cmd $corefile "dump core file"]} {
> +       return
> +    }
> +
> +    clean_restart $::testfile
> +
> +    gdb_core_cmd $corefile "load corefile"
> +
> +    check_backtrace "backtrace after loading core file"
> +
> +    source_py_script_and_exit_checking_event
> +}
> +
> +# Start a running inferior.  Exit GDB.  Check that we see an 'exited'
> +# event, and that it is associated with the correct gdb.Inferior.
> +proc_with_prefix check_with_live {} {
> +    if {![clean_restart_and_runto_foo]} {
> +       return
> +    }
> +
> +    check_backtrace "backtrace before exiting"
> +
> +    source_py_script_and_exit_checking_event
> +}
> +
> +check_with_live
> +check_with_corefile
> diff --git a/gdb/testsuite/gdb.python/py-inf-exited-at-exit.py b/gdb/testsuite/gdb.python/py-inf-exited-at-exit.py
> new file mode 100644
> index 00000000000..b6fe39e4061
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-inf-exited-at-exit.py
> @@ -0,0 +1,20 @@
> +# Copyright (C) 2026 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +def exit_event_handler(event):
> +    inf = event.inferior
> +    print("EVENT: inferior exited event.  Inferior is " + str(inf))
> +
> +gdb.events.exited.connect(exit_event_handler)
> 
> base-commit: bd64797371d27c766d551d0bf115d9090f1d0594
> --
> 2.25.4
> 


      parent reply	other threads:[~2026-06-12 13:50 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-04 16:47 [PATCH] " Andrew Burgess
2026-06-04 20:11 ` Lancelot SIX
2026-06-06 17:35   ` Andrew Burgess
2026-06-06 17:32 ` [PATCHv2] " Andrew Burgess
2026-06-09 10:19   ` [PATCHv3] " Andrew Burgess
2026-06-12 14:38     ` Pedro Alves
2026-06-14 21:06       ` Andrew Burgess
2026-06-12 13:49   ` Lancelot SIX [this message]

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=038a1fc7-8627-4030-8fc5-e02cafced42a@amd.com \
    --to=lancelot.six@amd.com \
    --cc=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    /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