[Bug gdb/28275] New: commit_resumed_state assertion failure when killing running inferior and running again

[Bug gdb/28275] New: commit_resumed_state assertion failure when killing running inferior and running again

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

            Bug ID: 28275
           Summary: commit_resumed_state assertion failure when killing
                    running inferior and running again
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: gdb
          Assignee: unassigned at sourceware dot org
          Reporter: simark at simark dot ca
  Target Milestone: ---

$ ./gdb -nx -q --data-directory=data-directory /usr/bin/sleep 
Reading symbols from /usr/bin/sleep...
(No debugging symbols found in /usr/bin/sleep)
(gdb) r 123 &
Starting program: /usr/bin/sleep 123
(gdb) kill
Kill the program being debugged? (y or n) y
[Inferior 1 (process 2995416) killed]
(gdb) r
Starting program: /usr/bin/sleep 123
/home/simark/src/binutils-gdb/gdb/target.c:2603: internal-error: ptid_t
target_wait(ptid_t, target_waitstatus*, target_wait_flags): Assertion
`!proc_target->commit_resumed_state' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) 

When we kill, commit_resumed_state is and stays "true".  When we run again, the
linux nat target is not considered by scoped_disable_commit_resumed, because it
has no non-exited inferior.  So when we wait here, it's still true:

#10 0x000055c06ee19510 in target_wait (ptid=..., status=0x7fffb62cf0a0,
options=...) at /home/simark/src/binutils-gdb/gdb/target.c:2603
#11 0x000055c06e74d1ce in startup_inferior (proc_target=0x55c072e373c0
<the_amd64_linux_nat_target>, pid=2995427, ntraps=1, last_waitstatus=0x0,
last_ptid=0x0) at /home/simark/src/binutils-gdb/gdb/nat/fork-inferior.c:483
#12 0x000055c06e1f8e89 in gdb_startup_inferior (pid=2995427, num_traps=1) at
/home/simark/src/binutils-gdb/gdb/fork-child.c:129
#13 0x000055c06e38ffa3 in inf_ptrace_target::create_inferior
(this=0x55c072e373c0 <the_amd64_linux_nat_target>, exec_file=0x6020000918b0
"/usr/bin/sleep", allargs="123", env=0x61500001cc80, from_tty=1) at
/home/simark/src/binutils-gdb/gdb/inf-ptrace.c:102
#14 0x000055c06e4f04b2 in linux_nat_target::create_inferior
(this=0x55c072e373c0 <the_amd64_linux_nat_target>, exec_file=0x6020000918b0
"/usr/bin/sleep", allargs="123", env=0x61500001cc80, from_tty=1) at
/home/simark/src/binutils-gdb/gdb/linux-nat.c:1075
#15 0x000055c06e3aba38 in run_command_1 (args=0x0, from_tty=1,
run_how=RUN_NORMAL) at /home/simark/src/binutils-gdb/gdb/infcmd.c:448
#16 0x000055c06e3ac1f9 in run_command (args=0x0, from_tty=1) at
/home/simark/src/binutils-gdb/gdb/infcmd.c:504

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[andrew.burgess at embecosm dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

Andrew Burgess <andrew.burgess at embecosm dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrew.burgess at embecosm dot com

--- Comment #1 from Andrew Burgess <andrew.burgess at embecosm dot com> ---
Created attachment 13630
  --> https://sourceware.org/bugzilla/attachment.cgi?id=13630&action=edit
Patch to add a related issue into the testsuite.

I ran into the same assertion while debugging some other issue.  My steps to
reproduce are slightly different, but I thought I'd add the details here as it
feels like these two cases must be related:

  $ /tmp/gdb/testsuite/outputs/gdb.threads/detach-step-over/detach-step-over
  $ gdb
  (gdb) attach 608289
  Attaching to program:
/tmp/gdb/testsuite/outputs/gdb.threads/detach-step-over/detach-step-over,
process 608289
  ... snip ...
  [Thread debugging using libthread_db enabled]
  Using host libthread_db library "/lib64/libthread_db.so.1".
  0x00007f84068f4215 in nanosleep () from /lib64/libc.so.6
  (gdb) break detach-step-over.c:54 if 0
  Breakpoint 1 at 0x40119c: file
/tmp/gdb/testsuite/../../../src/gdb/testsuite/gdb.threads/detach-step-over.c,
line 54.
  (gdb) continue &
  Continuing.
  (gdb) q
  A debugging session is active.

        Inferior 1 [process 608289] will be detached.

  Quit anyway? (y or n) y
  ../../src/gdb/target.c:3778: internal-error: void target_stop(ptid_t):
Assertion `!proc_target->commit_resumed_state' failed.

The patch I've attached modifies gdb.threads/detach-step-over.exp to show this
bug, this patch isn't ready to upstream in its current state.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[nyanpasu64 at tuta dot io]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

nyanpasu64 at tuta dot io changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nyanpasu64 at tuta dot io

--- Comment #2 from nyanpasu64 at tuta dot io ---
I found another procedure to reach the same crash:

- Run a multithreaded GUI app (eg. gdb kwrite).
- Start the application (type run).
- In another terminal, call `killall kwrite` or similar. GDB will say "Thread 1
"kwrite" received signal SIGTERM, Terminated."
- Type continue into gdb. gdb will say [Thread ... (LWP ...) exited], but not
[Inferior 1 (process ...) exited ...], even though the PID no longer exists in
/proc/NUMBER.
- Type kill. When gdb asks "Kill the program being debugged? (y or n)", say y.
gdb will say [Inferior 1 (process ...) killed].
- Type run.

This results in the same error: target.c:2603: internal-error: ptid_t
target_wait(ptid_t, target_waitstatus*, target_wait_flags): Assertion
`!proc_target->commit_resumed_state' failed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #3 from Simon Marchi <simark at simark dot ca> ---
Something like this should fix the original issue (when re-running):

>From 0bc259c5c7640fb9bb655b7b279eb0b2ec87c28b Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Fri, 29 Oct 2021 08:36:00 -0400
Subject: [PATCH] gdb: fix commit resumed failed assertion when re-running

Change-Id: Ic7ed0524751d0e5524936b2890048cd4a3d92765
---
 gdb/infcmd.c |  4 ++--
 gdb/target.c | 46 +++++++++++++++++++++-------------------------
 gdb/target.h |  8 ++++----
 3 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 302db421a21f..57726650f155 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -371,7 +371,6 @@ run_command_1 (const char *args, int from_tty, enum run_how
run_how)
 {
   const char *exec_file;
   struct ui_out *uiout = current_uiout;
-  struct target_ops *run_target;
   int async_exec;

   dont_repeat ();
@@ -403,7 +402,7 @@ run_command_1 (const char *args, int from_tty, enum run_how
run_how)
   /* Do validation and preparation before possibly changing anything
      in the inferior.  */

-  run_target = find_run_target ();
+  process_stratum_target *run_target = find_run_target ();

   prepare_execution_command (run_target, async_exec);

@@ -445,6 +444,7 @@ run_command_1 (const char *args, int from_tty, enum run_how
run_how)
       uiout->flush ();
     }

+  run_target->commit_resumed_state = false;
   run_target->create_inferior (exec_file,
                               current_inferior ()->args (),
                               current_inferior ()->environment.envp (),
diff --git a/gdb/target.c b/gdb/target.c
index 6cff56474873..967551a6ad8e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -84,7 +84,7 @@ static int default_verify_memory (struct target_ops *self,

 static void tcomplain (void) ATTRIBUTE_NORETURN;

-static struct target_ops *find_default_run_target (const char *);
+static process_stratum_target *find_default_run_target (const char *);

 static int dummy_find_memory_regions (struct target_ops *self,
                                      find_memory_region_ftype ignore1,
@@ -2880,12 +2880,12 @@ show_auto_connect_native_target (struct ui_file *file,
int from_tty,
 /* A pointer to the target that can respond to "run" or "attach".
    Native targets are always singletons and instantiated early at GDB
    startup.  */
-static target_ops *the_native_target;
+static process_stratum_target *the_native_target;

 /* See target.h.  */

 void
-set_native_target (target_ops *target)
+set_native_target (process_stratum_target *target)
 {
   if (the_native_target != NULL)
     internal_error (__FILE__, __LINE__,
@@ -2897,7 +2897,7 @@ set_native_target (target_ops *target)

 /* See target.h.  */

-target_ops *
+process_stratum_target *
 get_native_target ()
 {
   return the_native_target;
@@ -2910,7 +2910,7 @@ get_native_target ()
    If DO_MESG is not NULL, the result is always valid (error() is
    called for errors); else, return NULL on error.  */

-static struct target_ops *
+static process_stratum_target *
 find_default_run_target (const char *do_mesg)
 {
   if (auto_connect_native_target && the_native_target != NULL)
@@ -2923,17 +2923,15 @@ find_default_run_target (const char *do_mesg)

 /* See target.h.  */

-struct target_ops *
-find_attach_target (void)
+process_stratum_target *
+find_attach_target ()
 {
-  /* If a target on the current stack can attach, use it.  */
-  for (target_ops *t = current_inferior ()->top_target ();
-       t != NULL;
-       t = t->beneath ())
-    {
-      if (t->can_attach ())
-       return t;
-    }
+  /* If the current process stratum target can attach, use it.  */
+  process_stratum_target *current_proc_target
+    = current_inferior ()->process_target ();
+  if (current_proc_target != nullptr
+      && current_proc_target->can_attach ())
+    return current_proc_target;

   /* Otherwise, use the default run target for attaching.  */
   return find_default_run_target ("attach");
@@ -2941,17 +2939,15 @@ find_attach_target (void)

 /* See target.h.  */

-struct target_ops *
-find_run_target (void)
+process_stratum_target *
+find_run_target ()
 {
-  /* If a target on the current stack can run, use it.  */
-  for (target_ops *t = current_inferior ()->top_target ();
-       t != NULL;
-       t = t->beneath ())
-    {
-      if (t->can_create_inferior ())
-       return t;
-    }
+  /* If the current process stratum target can run, use it.  */
+  process_stratum_target *current_proc_target
+    = current_inferior ()->process_target ();
+  if (current_proc_target != nullptr
+      && current_proc_target->can_run ())
+    return current_proc_target;

   /* Otherwise, use the default run target.  */
   return find_default_run_target ("run");
diff --git a/gdb/target.h b/gdb/target.h
index 92fc1381db2b..5750a7655670 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1364,11 +1364,11 @@ typedef gdb::ref_ptr<target_ops, target_ops_ref_policy>
target_ops_ref;
 /* Native target backends call this once at initialization time to
    inform the core about which is the target that can respond to "run"
    or "attach".  Note: native targets are always singletons.  */
-extern void set_native_target (target_ops *target);
+extern void set_native_target (process_stratum_target *target);

 /* Get the registered native target, if there's one.  Otherwise return
    NULL.  */
-extern target_ops *get_native_target ();
+extern process_stratum_target *get_native_target ();

 /* Type that manages a target stack.  See description of target stacks
    and strata at the top of the file.  */
@@ -1430,13 +1430,13 @@ void target_close (struct target_ops *targ);
    current stack supports attaching, then it is returned.  Otherwise,
    the default run target is returned.  */

-extern struct target_ops *find_attach_target (void);
+extern process_stratum_target *find_attach_target (void);

 /* Find the correct target to use for "run".  If a target on the
    current stack supports creating a new inferior, then it is
    returned.  Otherwise, the default run target is returned.  */

-extern struct target_ops *find_run_target (void);
+extern process_stratum_target *find_run_target ();

 /* Some targets don't generate traps when attaching to the inferior,
    or their target_attach implementation takes care of the waiting.
-- 
2.33.1

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vries at gcc dot gnu.org

--- Comment #4 from Tom de Vries <vries at gcc dot gnu.org> ---
I ran into this today on an aarch64 machine:
...
tdevries@ampere3:~/gdb> grep -a -i 'internal-error:.*assertion' gdb.log | sed
's/^(gdb) //' | sort | uniq -c
     44 /suse/tdevries/gdb/src/gdb/target.c:2590: internal-error: target_wait:
Assertion `!proc_target->commit_resumed_state' failed.
...
but at first glance that looks related to forgetting to install libexpat-devel.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #5 from Simon Marchi <simark at simark dot ca> ---
(In reply to Tom de Vries from comment #4)
> I ran into this today on an aarch64 machine:
> ...
> tdevries@ampere3:~/gdb> grep -a -i 'internal-error:.*assertion' gdb.log |
> sed 's/^(gdb) //' | sort | uniq -c
>      44 /suse/tdevries/gdb/src/gdb/target.c:2590: internal-error:
> target_wait: Assertion `!proc_target->commit_resumed_state' failed.
> ...
> but at first glance that looks related to forgetting to install
> libexpat-devel.

Can you give specific test names?  I can try to build a GDB without expat and
run those tests.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #6 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Simon Marchi from comment #5)
> Can you give specific test names?  I can try to build a GDB without expat
> and run those tests.

I've deinstalled libexpat-devel and I'm doing a rerun.  Will let you known.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #7 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom de Vries from comment #6)
> (In reply to Simon Marchi from comment #5)
> > Can you give specific test names?  I can try to build a GDB without expat
> > and run those tests.
> 
> I've deinstalled libexpat-devel and I'm doing a rerun.  Will let you known.


In this particular run I get only 26 hits, all in gdb.server/ext-attach.exp:
...
$ egrep -i -a "^Running /|internal-error.*assertion" gdb.log \
  | sed 's/^(gdb) //' \
  | uniq -c \
  | grep -i -B1 "internal-error.*assertion"
      1 Running /suse/tdevries/gdb/src/gdb/testsuite/gdb.server/ext-attach.exp
...
     26 /suse/tdevries/gdb/src/gdb/target.c:2590: internal-error: target_wait:
Assertion `!proc_target->commit_resumed_state' failed.
...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #8 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom de Vries from comment #7)
> (In reply to Tom de Vries from comment #6)
> > (In reply to Simon Marchi from comment #5)
> > > Can you give specific test names?  I can try to build a GDB without expat
> > > and run those tests.
> > 
> > I've deinstalled libexpat-devel and I'm doing a rerun.  Will let you known.
> 
> 
> In this particular run I get only 26 hits, all in gdb.server/ext-attach.exp:
> ...
> $ egrep -i -a "^Running /|internal-error.*assertion" gdb.log \
>   | sed 's/^(gdb) //' \
>   | uniq -c \
>   | grep -i -B1 "internal-error.*assertion"
>       1 Running
> /suse/tdevries/gdb/src/gdb/testsuite/gdb.server/ext-attach.exp ...
>      26 /suse/tdevries/gdb/src/gdb/target.c:2590: internal-error:
> target_wait: Assertion `!proc_target->commit_resumed_state' failed.
> ...

Hmm, and we're running the test-case because aarch64 is not skipped in
skip_gdbserver_tests:
...
    # If GDB is lack of XML support, and targets, like arm, have                
    # multiple target descriptions, GDB doesn't know which target               
    # description GDBserver uses, and may fail to parse 'g' packet              
    # after connection.                                                         
    if { [gdb_skip_xml_test]
         && ([istarget "arm*-*-linux*"]
             || [istarget "mips*-*-linux*"]
             || [istarget "powerpc*-*-linux*"]
             || [istarget "s390*-*-linux*"]
             || [istarget "x86_64-*-linux*"]
             || [istarget "i\[34567\]86-*-linux*"]) } {
        return 1
    }
...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #9 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom de Vries from comment #8)
> Hmm, and we're running the test-case because aarch64 is not skipped in
> skip_gdbserver_tests:
> ...
>     # If GDB is lack of XML support, and targets, like arm, have            
> 
>     # multiple target descriptions, GDB doesn't know which target           
> 
>     # description GDBserver uses, and may fail to parse 'g' packet          
> 
>     # after connection.                                                     
> 
>     if { [gdb_skip_xml_test]
>          && ([istarget "arm*-*-linux*"]
>              || [istarget "mips*-*-linux*"]
>              || [istarget "powerpc*-*-linux*"]
>              || [istarget "s390*-*-linux*"]
>              || [istarget "x86_64-*-linux*"]
>              || [istarget "i\[34567\]86-*-linux*"]) } {
>         return 1
>     }
> ...

https://sourceware.org/pipermail/gdb-patches/2022-November/193819.html

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[aburgess at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

Andrew Burgess <aburgess at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #13630|0                           |1
        is obsolete|                            |

--- Comment #10 from Andrew Burgess <aburgess at redhat dot com> ---
Created attachment 14458
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14458&action=edit
Patch including test and possible fix (for issue in comment #1)

I took a look at the issue I raised in comment #1.  This updated patch includes
a test that I think is now ready for upstream, and a possible fix.

I've not posted this to the m/l as I believe Simon is currently looking at
wider commit_resumed_state issues, so I don't want to conflict with any work
he's doing.

Hopefully the new test at least will be included with anything Simon posts
upstream, and the fix I propose might be helpful.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[brobecker at gnat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

Joel Brobecker <brobecker at gnat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.1
                 CC|                            |brobecker at gnat dot com

--- Comment #11 from Joel Brobecker <brobecker at gnat dot com> ---
Adding target milestone to GDB 13.1 to mark this as needed for that release
(requested by SimonM, and I agree it would be good to have this fixed, since
this is a crash).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #12 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Simon Marchi <simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ddff2a2dea5261789e1f281f3ee1b33dd5fd8892

commit ddff2a2dea5261789e1f281f3ee1b33dd5fd8892
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Mon Nov 21 12:12:11 2022 -0500

    gdb: fix assert when quitting GDB while a thread is stepping

    This commit addresses one of the issues identified in PR gdb/28275.

    Bug gdb/28275 identifies a number of situations in which this assert:

      Assertion `!proc_target->commit_resumed_state' failed.

    could be triggered.  There's actually a number of similar places where
    this assert is found in GDB, the two of interest in gdb/28275 are in
    target_wait and target_stop.

    In one of the comments:

      https://sourceware.org/bugzilla/show_bug.cgi?id=28275#c1

    steps to trigger the assertion within target_stop were identified when
    using a modified version of the gdb.threads/detach-step-over.exp test
    script.

    In the gdb.threads/detach-step-over.exp test, we attach to a
    multi-threaded inferior, and continue the inferior in asynchronous
    (background) mode.  Each thread is continuously hitting a conditional
    breakpoint where the condition is always false.  While the inferior is
    running we detach.  The goal is that we detach while GDB is performing a
    step-over for the conditional breakpoint in at least one thread.

    While detaching, if a step-over is in progress, then GDB has to
    complete the step over before we can detach.  This involves calling
    target_stop and target_wait (see prepare_for_detach).

    As far as gdb/28275 is concerned, the interesting part here, is the
    the process_stratum_target::commit_resumed_state variable must be
    false when target_stop and target_wait are called.

    This is currently ensured because, in detach_command (infrun.c), we
    create an instance of scoped_disable_commit_resumed, this ensures that
    when target_detach is called, ::commit_resumed_state will be false.

    The modification to the test that I propose here, and which exposed
    the bug, is that, instead of using "detach" to detach from the
    inferior, we instead use "quit".  Quitting GDB after attaching to an
    inferior will cause GDB to first detach, and then exit.

    When we quit GDB we end up calling target_detach via a different code
    path, the stack looks like:

      #0 target_detach
      #1 kill_or_detach
      #2 quit_force
      #3 quit_command

    Along this path there is no scoped_disable_commit_resumed created.
    ::commit_resumed_state can be true when we reach prepare_for_detach,
    which calls target_wait and target_stop, so the assertion will trigger.

    In this commit, I propose fixing this by adding the creation of a
    scoped_disable_commit_resumed into target_detach.  This will ensure
    that ::commit_resumed_state is false when calling prepare_for_detach
    from within target_detach.

    I did consider placing the scoped_disable_commit_resumed in
    prepare_for_detach, however, placing it in target_detach ensures that
    the target's commit_resumed_state flag is left to false if the detached
    inferior was the last one for that target.  It's the same rationale as
    for patch "gdb: disable commit resumed in target_kill" that comes later
    in this series, but for detach instead of kill.

    detach_command still includes a scoped_disable_commit_resumed too, but I
    think it is still relevant to cover the resumption at the end of the
    function.

    Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28275
    Change-Id: Ie128f7aba6ef0e018859275eca372e6ea738e96f

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #13 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Simon Marchi <simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ed14d866a31625c6f8c2cb6d4a445a8372b46161

commit ed14d866a31625c6f8c2cb6d4a445a8372b46161
Author: Simon Marchi <simon.marchi@efficios.com>
Date:   Mon Nov 21 12:12:13 2022 -0500

    gdb: disable commit resumed in target_kill

    New in this version:

     - Better comment in target_kill
     - Uncomment line in test to avoid hanging when exiting, when testing on
       native-extended-gdbserver

    PR 28275 shows that doing a sequence of:

     - Run inferior in background (run &)
     - kill that inferior
     - Run again

    We get into this assertion:

        /home/smarchi/src/binutils-gdb/gdb/target.c:2590: internal-error:
target_wait: Assertion `!proc_target->commit_resumed_state' failed.

        #0  internal_error_loc (file=0x5606b344e740
"/home/smarchi/src/binutils-gdb/gdb/target.c", line=2590, fmt=0x5606b344d6a0
"%s: Assertion `%s' failed.") at
/home/smarchi/src/binutils-gdb/gdbsupport/errors.cc:54
        #1  0x00005606b6296475 in target_wait (ptid=..., status=0x7fffb9390630,
options=...) at /home/smarchi/src/binutils-gdb/gdb/target.c:2590
        #2  0x00005606b5767a98 in startup_inferior (proc_target=0x5606bfccb2a0
<the_amd64_linux_nat_target>, pid=3884857, ntraps=1, last_waitstatus=0x0,
last_ptid=0x0) at /home/smarchi/src/binutils-gdb/gdb/nat/fork-inferior.c:482
        #3  0x00005606b4e6c9c5 in gdb_startup_inferior (pid=3884857,
num_traps=1) at /home/smarchi/src/binutils-gdb/gdb/fork-child.c:132
        #4  0x00005606b50f14a5 in inf_ptrace_target::create_inferior
(this=0x5606bfccb2a0 <the_amd64_linux_nat_target>, exec_file=0x604000039f50
"/home/smarchi/build/binutils-gdb/gdb/test", allargs="", env=0x61500000a580,
from_tty=1)
            at /home/smarchi/src/binutils-gdb/gdb/inf-ptrace.c:105
        #5  0x00005606b53b6d23 in linux_nat_target::create_inferior
(this=0x5606bfccb2a0 <the_amd64_linux_nat_target>, exec_file=0x604000039f50
"/home/smarchi/build/binutils-gdb/gdb/test", allargs="", env=0x61500000a580,
from_tty=1)
            at /home/smarchi/src/binutils-gdb/gdb/linux-nat.c:978
        #6  0x00005606b512b79b in run_command_1 (args=0x0, from_tty=1,
run_how=RUN_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:468
        #7  0x00005606b512c236 in run_command (args=0x0, from_tty=1) at
/home/smarchi/src/binutils-gdb/gdb/infcmd.c:526

    When running the kill command, commit_resumed_state for the
    process_stratum_target (linux-nat, here) is true.  After the kill, when
    there are no more threads, commit_resumed_state is still true, as
    nothing touches this flag during the kill operation.  During the
    subsequent run command, run_command_1 does:

        scoped_disable_commit_resumed disable_commit_resumed ("running");

    We would think that this would clear the commit_resumed_state flag of
    our native target, but that's not the case, because
    scoped_disable_commit_resumed iterates on non-exited inferiors in order
    to find active process targets.  And after the kill, the inferior is
    exited, and the native target was unpushed from it anyway.  So
    scoped_disable_commit_resumed doesn't touch the commit_resumed_state
    flag of the native target, it stays true.  When reaching target_wait, in
    startup_inferior (to consume the initial expect stop events while the
    inferior is starting up and working its way through the shell),
    commit_resumed_state is true, breaking the contract saying that
    commit_resumed_state is always false when calling the targets' wait
    method.

    (note: to be correct, I think that startup_inferior should toggle
    commit_resumed between the target_wait and target_resume calls, but I'll
    ignore that for now)

    I can see multiple ways to fix this.  In the end, we need
    commit_resumed_state to be cleared by the time we get to that
    target_wait.  It could be done at the end of the kill command, or at the
    beginning of the run command.

    To keep things in a coherent state, I'd like to make it so that after
    the kill command, when the target is left with no threads, its
    commit_resumed_state flag is left to false.  This way, we can keep
    working with the assumption that a target with no threads (and therefore
    no running threads) has commit_resumed_state == false.

    Do this by adding a scoped_disable_commit_resumed in target_kill.  It
    clears the target's commit_resumed_state on entry, and leaves it false
    if the target does not have any resumed thread on exit.  That means,
    even if the target has another inferior with stopped threads,
    commit_resumed_state will be left to false, which makes sense.

    Add a test that tries to cover various combinations of actions done
    while an inferior is running (and therefore while commit_resumed_state
    is true on the process target).

    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28275
    Change-Id: I8e6fe6dc1f475055921520e58cab68024039a1e9
    Approved-By: Andrew Burgess <aburgess@redhat.com>

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

Simon Marchi <simark at simark dot ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #14 from Simon Marchi <simark at simark dot ca> ---
Should be fixed by the commits shown above.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #15 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The gdb-12-branch branch has been updated by Simon Marchi
<simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=0fd7bbc90133138d12914608ebb68bab16259b78

commit 0fd7bbc90133138d12914608ebb68bab16259b78
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Mon Nov 21 12:12:11 2022 -0500

    gdb: fix assert when quitting GDB while a thread is stepping

    This commit addresses one of the issues identified in PR gdb/28275.

    Bug gdb/28275 identifies a number of situations in which this assert:

      Assertion `!proc_target->commit_resumed_state' failed.

    could be triggered.  There's actually a number of similar places where
    this assert is found in GDB, the two of interest in gdb/28275 are in
    target_wait and target_stop.

    In one of the comments:

      https://sourceware.org/bugzilla/show_bug.cgi?id=28275#c1

    steps to trigger the assertion within target_stop were identified when
    using a modified version of the gdb.threads/detach-step-over.exp test
    script.

    In the gdb.threads/detach-step-over.exp test, we attach to a
    multi-threaded inferior, and continue the inferior in asynchronous
    (background) mode.  Each thread is continuously hitting a conditional
    breakpoint where the condition is always false.  While the inferior is
    running we detach.  The goal is that we detach while GDB is performing a
    step-over for the conditional breakpoint in at least one thread.

    While detaching, if a step-over is in progress, then GDB has to
    complete the step over before we can detach.  This involves calling
    target_stop and target_wait (see prepare_for_detach).

    As far as gdb/28275 is concerned, the interesting part here, is the
    the process_stratum_target::commit_resumed_state variable must be
    false when target_stop and target_wait are called.

    This is currently ensured because, in detach_command (infrun.c), we
    create an instance of scoped_disable_commit_resumed, this ensures that
    when target_detach is called, ::commit_resumed_state will be false.

    The modification to the test that I propose here, and which exposed
    the bug, is that, instead of using "detach" to detach from the
    inferior, we instead use "quit".  Quitting GDB after attaching to an
    inferior will cause GDB to first detach, and then exit.

    When we quit GDB we end up calling target_detach via a different code
    path, the stack looks like:

      #0 target_detach
      #1 kill_or_detach
      #2 quit_force
      #3 quit_command

    Along this path there is no scoped_disable_commit_resumed created.
    ::commit_resumed_state can be true when we reach prepare_for_detach,
    which calls target_wait and target_stop, so the assertion will trigger.

    In this commit, I propose fixing this by adding the creation of a
    scoped_disable_commit_resumed into target_detach.  This will ensure
    that ::commit_resumed_state is false when calling prepare_for_detach
    from within target_detach.

    I did consider placing the scoped_disable_commit_resumed in
    prepare_for_detach, however, placing it in target_detach ensures that
    the target's commit_resumed_state flag is left to false if the detached
    inferior was the last one for that target.  It's the same rationale as
    for patch "gdb: disable commit resumed in target_kill" that comes later
    in this series, but for detach instead of kill.

    detach_command still includes a scoped_disable_commit_resumed too, but I
    think it is still relevant to cover the resumption at the end of the
    function.

    Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28275
    Change-Id: Ie128f7aba6ef0e018859275eca372e6ea738e96f

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #16 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The gdb-12-branch branch has been updated by Simon Marchi
<simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1aefce82fdf8834c7440747b80656e0d030aa2fd

commit 1aefce82fdf8834c7440747b80656e0d030aa2fd
Author: Simon Marchi <simon.marchi@efficios.com>
Date:   Mon Nov 21 12:12:13 2022 -0500

    gdb: disable commit resumed in target_kill

    New in this version:

     - Better comment in target_kill
     - Uncomment line in test to avoid hanging when exiting, when testing on
       native-extended-gdbserver

    PR 28275 shows that doing a sequence of:

     - Run inferior in background (run &)
     - kill that inferior
     - Run again

    We get into this assertion:

        /home/smarchi/src/binutils-gdb/gdb/target.c:2590: internal-error:
target_wait: Assertion `!proc_target->commit_resumed_state' failed.

        #0  internal_error_loc (file=0x5606b344e740
"/home/smarchi/src/binutils-gdb/gdb/target.c", line=2590, fmt=0x5606b344d6a0
"%s: Assertion `%s' failed.") at
/home/smarchi/src/binutils-gdb/gdbsupport/errors.cc:54
        #1  0x00005606b6296475 in target_wait (ptid=..., status=0x7fffb9390630,
options=...) at /home/smarchi/src/binutils-gdb/gdb/target.c:2590
        #2  0x00005606b5767a98 in startup_inferior (proc_target=0x5606bfccb2a0
<the_amd64_linux_nat_target>, pid=3884857, ntraps=1, last_waitstatus=0x0,
last_ptid=0x0) at /home/smarchi/src/binutils-gdb/gdb/nat/fork-inferior.c:482
        #3  0x00005606b4e6c9c5 in gdb_startup_inferior (pid=3884857,
num_traps=1) at /home/smarchi/src/binutils-gdb/gdb/fork-child.c:132
        #4  0x00005606b50f14a5 in inf_ptrace_target::create_inferior
(this=0x5606bfccb2a0 <the_amd64_linux_nat_target>, exec_file=0x604000039f50
"/home/smarchi/build/binutils-gdb/gdb/test", allargs="", env=0x61500000a580,
from_tty=1)
            at /home/smarchi/src/binutils-gdb/gdb/inf-ptrace.c:105
        #5  0x00005606b53b6d23 in linux_nat_target::create_inferior
(this=0x5606bfccb2a0 <the_amd64_linux_nat_target>, exec_file=0x604000039f50
"/home/smarchi/build/binutils-gdb/gdb/test", allargs="", env=0x61500000a580,
from_tty=1)
            at /home/smarchi/src/binutils-gdb/gdb/linux-nat.c:978
        #6  0x00005606b512b79b in run_command_1 (args=0x0, from_tty=1,
run_how=RUN_NORMAL) at /home/smarchi/src/binutils-gdb/gdb/infcmd.c:468
        #7  0x00005606b512c236 in run_command (args=0x0, from_tty=1) at
/home/smarchi/src/binutils-gdb/gdb/infcmd.c:526

    When running the kill command, commit_resumed_state for the
    process_stratum_target (linux-nat, here) is true.  After the kill, when
    there are no more threads, commit_resumed_state is still true, as
    nothing touches this flag during the kill operation.  During the
    subsequent run command, run_command_1 does:

        scoped_disable_commit_resumed disable_commit_resumed ("running");

    We would think that this would clear the commit_resumed_state flag of
    our native target, but that's not the case, because
    scoped_disable_commit_resumed iterates on non-exited inferiors in order
    to find active process targets.  And after the kill, the inferior is
    exited, and the native target was unpushed from it anyway.  So
    scoped_disable_commit_resumed doesn't touch the commit_resumed_state
    flag of the native target, it stays true.  When reaching target_wait, in
    startup_inferior (to consume the initial expect stop events while the
    inferior is starting up and working its way through the shell),
    commit_resumed_state is true, breaking the contract saying that
    commit_resumed_state is always false when calling the targets' wait
    method.

    (note: to be correct, I think that startup_inferior should toggle
    commit_resumed between the target_wait and target_resume calls, but I'll
    ignore that for now)

    I can see multiple ways to fix this.  In the end, we need
    commit_resumed_state to be cleared by the time we get to that
    target_wait.  It could be done at the end of the kill command, or at the
    beginning of the run command.

    To keep things in a coherent state, I'd like to make it so that after
    the kill command, when the target is left with no threads, its
    commit_resumed_state flag is left to false.  This way, we can keep
    working with the assumption that a target with no threads (and therefore
    no running threads) has commit_resumed_state == false.

    Do this by adding a scoped_disable_commit_resumed in target_kill.  It
    clears the target's commit_resumed_state on entry, and leaves it false
    if the target does not have any resumed thread on exit.  That means,
    even if the target has another inferior with stopped threads,
    commit_resumed_state will be left to false, which makes sense.

    Add a test that tries to cover various combinations of actions done
    while an inferior is running (and therefore while commit_resumed_state
    is true on the process target).

    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28275
    Change-Id: I8e6fe6dc1f475055921520e58cab68024039a1e9
    Approved-By: Andrew Burgess <aburgess@redhat.com>

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #17 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to cvs-commit@gcc.gnu.org from comment #15)
> The gdb-12-branch branch has been updated by Simon Marchi
> <simark@sourceware.org>:
> 

At https://sourceware.org/gdb/wiki/GDB_12_Release there's a  'see fixed bugs
marked "Target Milestone 12.2"' link, which doesn't show this PR, because the
target milestone is set to 13.1, which might be because there's no 12.2 target
milestone.  Should we add such a milestone, and update this PR's target
milestone?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/28275] commit_resumed_state assertion failure when killing running inferior and running again

[brobecker at gnat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28275

--- Comment #18 from Joel Brobecker <brobecker at gnat dot com> ---
IMO, I think it would be confusing to create 12.2 release that hasn't been
created.

-- 
You are receiving this mail because:
You are on the CC list for the bug.