[RFAv2] Fix internal error and improve 'set debug infrun 1'/target wait kind trace

[RFAv2] Fix internal error and improve 'set debug infrun 1'/target wait kind trace

[Philippe Waroquiers]

The test  gdb.threads/watchthreads-reorder.exp verifies that the
'set debug infrun 1' debug output does not crash GDB.

Under high load, the test can still cause a GDB internal error (see details
below).

This patch fixes this crash, and improves/factorises some wait kind traces.

Tested on debian/amd64 + run one test with 'set debug infrun 1'.

Changes compared to the first version:
  * Handles the suggestions of Kevin to trace the relevant elements
    of the wait status (this is done by calling target_waitstatus_to_string).
  * Some other changes to factorise wait status tracing.

Note that using target_waitstatus_to_string instead of the 'locally printed'
status kind strings means that debug trace that was using strings such as:
   "EXITED" or "TARGET_WAITKIND_EXITED"
will now use what is printed by target_waitstatus_to_string e.g.
   "exited".

gdb/ChangeLog
2019-03-30  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* infrun.c (stop_all_threads): If debug_infrun, always
	trace the wait status after wait_one, using
	target_waitstatus_to_string and target_pid_to_str.
	(handle_inferior_event): Replace various trace of
	wait status kind by a single trace.
	* gdb/gnu-nat.c (gnu_nat_target::wait): Replace local
	wait status kind image by target_waitstatus_to_string.
	* target/waitstatus.c (target_waitstatus_to_string): Fix
	obsolete comment.

  (top-gdb) bt
  #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
  #1  0x00007f3d54a0642a in __GI_abort () at abort.c:89
  #2  0x0000555c24c60e66 in dump_core () at ../../fixleaks/gdb/utils.c:201
  #3  0x0000555c24c63d49 in internal_vproblem(internal_problem *, const char *, int, const char *, typedef __va_list_tag __va_list_tag *) (problem=problem@entry=0x555c25338d40 <internal_error_problem>, file=<optimized out>, line=287,
      fmt=<optimized out>, ap=<optimized out>) at ../../fixleaks/gdb/utils.c:411
  #4  0x0000555c24c63eab in internal_verror (file=<optimized out>, line=<optimized out>, fmt=<optimized out>,
      ap=<optimized out>) at ../../fixleaks/gdb/utils.c:436
  #5  0x0000555c249e8c22 in internal_error (file=file@entry=0x555c24e0f2ad "../../fixleaks/gdb/inferior.c",
      line=line@entry=287, fmt=<optimized out>) at ../../fixleaks/gdb/common/errors.c:55
  #6  0x0000555c247d3f5c in find_inferior_pid (pid=<optimized out>) at ../../fixleaks/gdb/inferior.c:287
  #7  0x0000555c24ad2248 in find_inferior_pid (pid=<optimized out>) at ../../fixleaks/gdb/inferior.c:302
  #8  find_inferior_ptid (ptid=...) at ../../fixleaks/gdb/inferior.c:301
  #9  0x0000555c24c35f25 in find_thread_ptid (ptid=...) at ../../fixleaks/gdb/thread.c:522
  #10 0x0000555c24b0ab4d in thread_db_target::pid_to_str[abi:cxx11](ptid_t) (
      this=0x555c2532e3e0 <the_thread_db_target>, ptid=...) at ../../fixleaks/gdb/linux-thread-db.c:1637
  #11 0x0000555c24c2f420 in target_pid_to_str[abi:cxx11](ptid_t) (ptid=...) at ../../fixleaks/gdb/target.c:2083
  #12 0x0000555c24ad9cab in stop_all_threads () at ../../fixleaks/gdb/infrun.c:4373
  #13 0x0000555c24ada00f in stop_waiting (ecs=<optimized out>) at ../../fixleaks/gdb/infrun.c:7464
  #14 0x0000555c24adc401 in process_event_stop_test (ecs=ecs@entry=0x7ffc9402d9d0) at ../../fixleaks/gdb/infrun.c:6181
  ...
  (top-gdb) fr 12
  #12 0x0000555c24ad9cab in stop_all_threads () at ../../fixleaks/gdb/infrun.c:4373
  (top-gdb) p event_ptid
  $5 = {m_pid = 25419, m_lwp = 25427, m_tid = 0}
  (top-gdb) p ptid
  $6 = {m_pid = 0, m_lwp = 0, m_tid = 0}
  (top-gdb) p ws
  $7 = {kind = TARGET_WAITKIND_THREAD_EXITED, value = {integer = 0, sig = GDB_SIGNAL_0, related_pid = {m_pid = 0,
        m_lwp = 0, m_tid = 0}, execd_pathname = 0x0, syscall_number = 0}}
  (top-gdb)

The gdb.log corresponding to the above crash is:
  (gdb) PASS: gdb.threads/watchthreads-reorder.exp: reorder1: set debug infrun 1
  continue
  Continuing.
  infrun: clear_proceed_status_thread (Thread 0x7ffff7fcfb40 (LWP 25419))
  infrun: clear_proceed_status_thread (Thread 0x7ffff7310700 (LWP 25427))
  infrun: clear_proceed_status_thread (Thread 0x7ffff6b0f700 (LWP 25428))
  infrun: proceed (addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT)
  infrun: proceed: resuming Thread 0x7ffff7fcfb40 (LWP 25419)
  infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [Thread 0x7ffff7fcfb40 (LWP 25419)] at 0x7ffff7344317
  infrun: infrun_async(1)
  infrun: prepare_to_wait
  infrun: proceed: resuming Thread 0x7ffff7310700 (LWP 25427)
  infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [Thread 0x7ffff7310700 (LWP 25427)] at 0x5555555553d7
  infrun: prepare_to_wait
  infrun: proceed: resuming Thread 0x7ffff6b0f700 (LWP 25428)
  infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread [Thread 0x7ffff6b0f700 (LWP 25428)] at 0x5555555554c8
  infrun: prepare_to_wait
  infrun: target_wait (-1.0.0, status) =
  infrun:   -1.0.0 [process -1],
  infrun:   status->kind = ignore
  infrun: TARGET_WAITKIND_IGNORE
  infrun: prepare_to_wait
  Joining the threads.
  [Thread 0x7ffff6b0f700 (LWP 25428) exited]
  infrun: target_wait (-1.0.0, status) =
  infrun:   -1.0.0 [process -1],
  infrun:   status->kind = ignore
  infrun: TARGET_WAITKIND_IGNORE
  infrun: prepare_to_wait
  infrun: target_wait (-1.0.0, status) =
  infrun:   25419.25419.0 [Thread 0x7ffff7fcfb40 (LWP 25419)],
  infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
  infrun: TARGET_WAITKIND_STOPPED
  infrun: stop_pc = 0x555555555e50
  infrun: context switch
  infrun: Switching context from Thread 0x7ffff6b0f700 (LWP 25428) to Thread 0x7ffff7fcfb40 (LWP 25419)
  infrun: BPSTAT_WHAT_STOP_NOISY
  infrun: stop_waiting
  infrun: stop_all_threads
  infrun: stop_all_threads, pass=0, iterations=0
  infrun:   Thread 0x7ffff7fcfb40 (LWP 25419) not executing
  infrun:   Thread 0x7ffff7310700 (LWP 25427) executing, need stop
  [Thread 0x7ffff7310700 (LWP 25427) exited]
  infrun: target_wait (-1.0.0, status) =
  infrun:   25419.25427.0 [LWP 25427],
  infrun:   status->kind = thread exited, status = 0
  infrun: infrun_async(0)
  ../../fixleaks/gdb/inferior.c:287: internal-error: inferior* find_inferior_pid(int): Assertion `pid != 0' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  Quit this debugging session? (y or n) FAIL: gdb.threads/watchthreads-reorder.exp: reorder1: continue to breakpoint: break-at-exit (GDB internal error)
  Resyncing due to internal error.
  n
  infrun: infrun_async(1)

  This is a bug, please report it.  For instructions, see:
  <http://www.gnu.org/software/gdb/bugs/>.

  infrun: infrun_async(0)
  ../../fixleaks/gdb/inferior.c:287: internal-error: inferior* find_inferior_pid(int): Assertion `pid != 0' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  Create a core file of GDB? (y or n) y
---
 gdb/gnu-nat.c           | 10 ++----
 gdb/infrun.c            | 78 +++++++++--------------------------------
 gdb/target/waitstatus.c |  3 +-
 3 files changed, 19 insertions(+), 72 deletions(-)

diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index c1fafed156..654a652717 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -1647,15 +1647,9 @@ rewait:
       inf_update_suspends (inf);
     }
 
-  inf_debug (inf, "returning ptid = %s, status = %s (%d)",
+  inf_debug (inf, "returning ptid = %s, %s",
 	     target_pid_to_str (ptid).c_str (),
-	     status->kind == TARGET_WAITKIND_EXITED ? "EXITED"
-	     : status->kind == TARGET_WAITKIND_STOPPED ? "STOPPED"
-	     : status->kind == TARGET_WAITKIND_SIGNALLED ? "SIGNALLED"
-	     : status->kind == TARGET_WAITKIND_LOADED ? "LOADED"
-	     : status->kind == TARGET_WAITKIND_SPURIOUS ? "SPURIOUS"
-	     : "?",
-	     status->value.integer);
+	     target_waitstatus_to_string (&status).c_str ());
 
   return ptid;
 }
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 0cfa2d6825..2826741169 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4358,24 +4358,21 @@ stop_all_threads (void)
 	    pass = -1;
 
 	  event_ptid = wait_one (&ws);
-
-	  if (ws.kind == TARGET_WAITKIND_NO_RESUMED)
+	  if (debug_infrun)
 	    {
-	      /* All resumed threads exited.  */
+	      fprintf_unfiltered (gdb_stdlog,
+				  "infrun: stop_all_threads %s %s\n",
+				  target_waitstatus_to_string (&ws).c_str (),
+				  target_pid_to_str (event_ptid).c_str ());
 	    }
-	  else if (ws.kind == TARGET_WAITKIND_THREAD_EXITED
-		   || ws.kind == TARGET_WAITKIND_EXITED
-		   || ws.kind == TARGET_WAITKIND_SIGNALLED)
-	    {
-	      if (debug_infrun)
-		{
-		  ptid_t ptid = ptid_t (ws.value.integer);
 
-		  fprintf_unfiltered (gdb_stdlog,
-				      "infrun: %s exited while "
-				      "stopping threads\n",
-				      target_pid_to_str (ptid).c_str ());
-		}
+	  if (ws.kind == TARGET_WAITKIND_NO_RESUMED
+	      || ws.kind == TARGET_WAITKIND_THREAD_EXITED
+	      || ws.kind == TARGET_WAITKIND_EXITED
+	      || ws.kind == TARGET_WAITKIND_SIGNALLED)
+	    {
+	      /* All resumed threads exited
+		 or one thread/process exited/signalled.  */
 	    }
 	  else
 	    {
@@ -4604,6 +4601,10 @@ handle_inferior_event (struct execution_control_state *ecs)
 
   enum stop_kind stop_soon;
 
+  if (debug_infrun)
+    fprintf_unfiltered (gdb_stdlog, "infrun: handle_inferior_event %s\n",
+			target_waitstatus_to_string (&ecs->ws).c_str ());
+
   if (ecs->ws.kind == TARGET_WAITKIND_IGNORE)
     {
       /* We had an event in the inferior, but we are not interested in
@@ -4615,16 +4616,12 @@ handle_inferior_event (struct execution_control_state *ecs)
 	 not stopped, and we are ignoring the event.  Another possible
 	 circumstance is any event which the lower level knows will be
 	 reported multiple times without an intervening resume.  */
-      if (debug_infrun)
-	fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_IGNORE\n");
       prepare_to_wait (ecs);
       return;
     }
 
   if (ecs->ws.kind == TARGET_WAITKIND_THREAD_EXITED)
     {
-      if (debug_infrun)
-	fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_THREAD_EXITED\n");
       prepare_to_wait (ecs);
       return;
     }
@@ -4643,9 +4640,6 @@ handle_inferior_event (struct execution_control_state *ecs)
     {
       /* No unwaited-for children left.  IOW, all resumed children
 	 have exited.  */
-      if (debug_infrun)
-	fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_NO_RESUMED\n");
-
       stop_print_frame = 0;
       stop_waiting (ecs);
       return;
@@ -4738,8 +4732,6 @@ handle_inferior_event (struct execution_control_state *ecs)
   switch (ecs->ws.kind)
     {
     case TARGET_WAITKIND_LOADED:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_LOADED\n");
       context_switch (ecs);
       /* Ignore gracefully during startup of the inferior, as it might
          be the shell which has just loaded some objects, otherwise
@@ -4817,8 +4809,6 @@ handle_inferior_event (struct execution_control_state *ecs)
 		      _("unhandled stop_soon: %d"), (int) stop_soon);
 
     case TARGET_WAITKIND_SPURIOUS:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_SPURIOUS\n");
       if (handle_stop_requested (ecs))
 	return;
       context_switch (ecs);
@@ -4827,8 +4817,6 @@ handle_inferior_event (struct execution_control_state *ecs)
       return;
 
     case TARGET_WAITKIND_THREAD_CREATED:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_THREAD_CREATED\n");
       if (handle_stop_requested (ecs))
 	return;
       context_switch (ecs);
@@ -4838,16 +4826,6 @@ handle_inferior_event (struct execution_control_state *ecs)
 
     case TARGET_WAITKIND_EXITED:
     case TARGET_WAITKIND_SIGNALLED:
-      if (debug_infrun)
-	{
-	  if (ecs->ws.kind == TARGET_WAITKIND_EXITED)
-	    fprintf_unfiltered (gdb_stdlog,
-				"infrun: TARGET_WAITKIND_EXITED\n");
-	  else
-	    fprintf_unfiltered (gdb_stdlog,
-				"infrun: TARGET_WAITKIND_SIGNALLED\n");
-	}
-
       inferior_ptid = ecs->ptid;
       set_current_inferior (find_inferior_ptid (ecs->ptid));
       set_current_program_space (current_inferior ()->pspace);
@@ -4912,14 +4890,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
          the above cases end in a continue or goto.  */
     case TARGET_WAITKIND_FORKED:
     case TARGET_WAITKIND_VFORKED:
-      if (debug_infrun)
-	{
-	  if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
-	    fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_FORKED\n");
-	  else
-	    fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_VFORKED\n");
-	}
-
       /* Check whether the inferior is displaced stepping.  */
       {
 	struct regcache *regcache = get_thread_regcache (ecs->event_thread);
@@ -5080,10 +5050,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
       /* Done with the shared memory region.  Re-insert breakpoints in
 	 the parent, and keep going.  */
 
-      if (debug_infrun)
-	fprintf_unfiltered (gdb_stdlog,
-			    "infrun: TARGET_WAITKIND_VFORK_DONE\n");
-
       context_switch (ecs);
 
       current_inferior ()->waiting_for_vfork_done = 0;
@@ -5098,8 +5064,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
       return;
 
     case TARGET_WAITKIND_EXECD:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_EXECD\n");
 
       /* Note we can't read registers yet (the stop_pc), because we
 	 don't yet know the inferior's post-exec architecture.
@@ -5148,9 +5112,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
       /* Be careful not to try to gather much state about a thread
          that's in a syscall.  It's frequently a losing proposition.  */
     case TARGET_WAITKIND_SYSCALL_ENTRY:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog,
-			    "infrun: TARGET_WAITKIND_SYSCALL_ENTRY\n");
       /* Getting the current syscall number.  */
       if (handle_syscall_event (ecs) == 0)
 	process_event_stop_test (ecs);
@@ -5162,22 +5123,15 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
          syscall.  Stepping one instruction seems to get it back
          into user code.)  */
     case TARGET_WAITKIND_SYSCALL_RETURN:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog,
-			    "infrun: TARGET_WAITKIND_SYSCALL_RETURN\n");
       if (handle_syscall_event (ecs) == 0)
 	process_event_stop_test (ecs);
       return;
 
     case TARGET_WAITKIND_STOPPED:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_STOPPED\n");
       handle_signal_stop (ecs);
       return;
 
     case TARGET_WAITKIND_NO_HISTORY:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_NO_HISTORY\n");
       /* Reverse execution: target ran out of history info.  */
 
       /* Switch to the stopped thread.  */
diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
index 4ac98e50f4..f86ce22373 100644
--- a/gdb/target/waitstatus.c
+++ b/gdb/target/waitstatus.c
@@ -20,8 +20,7 @@
 #include "common/common-defs.h"
 #include "waitstatus.h"
 
-/* Return a pretty printed form of target_waitstatus.
-   Space for the result is malloc'd, caller must free.  */
+/* Return a pretty printed form of target_waitstatus.  */
 
 std::string
 target_waitstatus_to_string (const struct target_waitstatus *ws)
-- 
2.20.1

Re: [RFAv2] Fix internal error and improve 'set debug infrun 1'/target wait kind trace

[Kevin Buettner]

On Sat, 30 Mar 2019 10:58:31 +0100
Philippe Waroquiers <philippe.waroquiers@skynet.be> wrote:

> The test  gdb.threads/watchthreads-reorder.exp verifies that the
> 'set debug infrun 1' debug output does not crash GDB.
> 
> Under high load, the test can still cause a GDB internal error (see details
> below).
> 
> This patch fixes this crash, and improves/factorises some wait kind traces.
> 
> Tested on debian/amd64 + run one test with 'set debug infrun 1'.
> 
> Changes compared to the first version:
>   * Handles the suggestions of Kevin to trace the relevant elements
>     of the wait status (this is done by calling target_waitstatus_to_string).
>   * Some other changes to factorise wait status tracing.
> 
> Note that using target_waitstatus_to_string instead of the 'locally printed'
> status kind strings means that debug trace that was using strings such as:
>    "EXITED" or "TARGET_WAITKIND_EXITED"
> will now use what is printed by target_waitstatus_to_string e.g.
>    "exited".
> 
> gdb/ChangeLog
> 2019-03-30  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* infrun.c (stop_all_threads): If debug_infrun, always
> 	trace the wait status after wait_one, using
> 	target_waitstatus_to_string and target_pid_to_str.
> 	(handle_inferior_event): Replace various trace of
> 	wait status kind by a single trace.
> 	* gdb/gnu-nat.c (gnu_nat_target::wait): Replace local
> 	wait status kind image by target_waitstatus_to_string.
> 	* target/waitstatus.c (target_waitstatus_to_string): Fix
> 	obsolete comment.

Okay.

Kevin

Re: [RFAv2] Fix internal error and improve 'set debug infrun 1'/target wait kind trace

[Philippe Waroquiers]

On Mon, 2019-04-01 at 09:32 -0700, Kevin Buettner wrote:
> > gdb/ChangeLog
> > 2019-03-30  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> > 
> > 	* infrun.c (stop_all_threads): If debug_infrun, always
> > 	trace the wait status after wait_one, using
> > 	target_waitstatus_to_string and target_pid_to_str.
> > 	(handle_inferior_event): Replace various trace of
> > 	wait status kind by a single trace.
> > 	* gdb/gnu-nat.c (gnu_nat_target::wait): Replace local
> > 	wait status kind image by target_waitstatus_to_string.
> > 	* target/waitstatus.c (target_waitstatus_to_string): Fix
> > 	obsolete comment.
> 
> Okay.
> 
> Kevin
Thanks for the reviews, pushed.

Philippe

Re: [RFAv2] Fix internal error and improve 'set debug infrun 1'/target wait kind trace

[Tom de Vries]

On 01-04-19 21:05, Philippe Waroquiers wrote:
> On Mon, 2019-04-01 at 09:32 -0700, Kevin Buettner wrote:
>>> gdb/ChangeLog
>>> 2019-03-30  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
>>>
>>> 	* infrun.c (stop_all_threads): If debug_infrun, always
>>> 	trace the wait status after wait_one, using
>>> 	target_waitstatus_to_string and target_pid_to_str.
>>> 	(handle_inferior_event): Replace various trace of
>>> 	wait status kind by a single trace.
>>> 	* gdb/gnu-nat.c (gnu_nat_target::wait): Replace local
>>> 	wait status kind image by target_waitstatus_to_string.
>>> 	* target/waitstatus.c (target_waitstatus_to_string): Fix
>>> 	obsolete comment.
>>
>> Okay.
>>
>> Kevin
> Thanks for the reviews, pushed.

Hi,

I ran today into the failure that this commit fixes:
...
FAIL: gdb.threads/watchthreads-reorder.exp: reorder1: continue to
breakpoint: break-at-exit (GDB internal error)
...
on the 8.3 branch.

My understanding from reading the rationale is that this is sufficiently
cornercase to not merit a backport, but perhaps someone thinks otherwise?

If we decide not to backport, we could perhaps mark this as as KFAIL in
the 8.3 branch?

Thanks,
- Tom

Re: [RFAv2] Fix internal error and improve 'set debug infrun 1'/target wait kind trace

[Kevin Buettner]

On Wed, 11 Sep 2019 19:05:19 +0200
Tom de Vries <tdevries@suse.de> wrote:

> I ran today into the failure that this commit fixes:
> ...
> FAIL: gdb.threads/watchthreads-reorder.exp: reorder1: continue to
> breakpoint: break-at-exit (GDB internal error)
> ...
> on the 8.3 branch.
> 
> My understanding from reading the rationale is that this is sufficiently
> cornercase to not merit a backport, but perhaps someone thinks otherwise?
> 
> If we decide not to backport, we could perhaps mark this as as KFAIL in
> the 8.3 branch?

Marking it as a KFAIL is okay with me...

Kevin

Re: [PATCH, 8.3][gdb/testsuite] Mark watchthreads-reorder.exp FAIL as KFAIL

[Tom de Vries]

[ resending, got "Undelivered Mail Returned to Sender" for
gdb-patches@sourceware.org ]

On 12-09-19 19:12, Tom de Vries wrote:
> [ was: Re: [RFAv2] Fix internal error and improve 'set debug infrun
> 1'/target wait kind trace ]
> On 12-09-19 13:30, Kevin Buettner wrote:
>> On Wed, 11 Sep 2019 19:05:19 +0200
>> Tom de Vries <tdevries@suse.de> wrote:
>>
>>> I ran today into the failure that this commit fixes:
>>> ...
>>> FAIL: gdb.threads/watchthreads-reorder.exp: reorder1: continue to
>>> breakpoint: break-at-exit (GDB internal error)
>>> ...
>>> on the 8.3 branch.
>>>
>>> My understanding from reading the rationale is that this is sufficiently
>>> cornercase to not merit a backport, but perhaps someone thinks otherwise?
>>>
>>> If we decide not to backport, we could perhaps mark this as as KFAIL in
>>> the 8.3 branch?
>> Marking it as a KFAIL is okay with me...
>>
> This patch implements the KFAIL, but does so by adding an extra argument
> to gdb_test_multiple, which is perhaps a bit intrusive for a release branch.
> 
> I'm also fine with just doing:
> ...
>     +    setup_kfail gdb/24995 "*-*-*"
>          gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
> ...
> or following up on any other suggestion.
> 
> Thanks,
> - Tom
> 
> 
> 0001-gdb-testsuite-Mark-watchthreads-reorder.exp-FAIL-as-KFAIL.patch
> 
> [gdb/testsuite] Mark watchthreads-reorder.exp FAIL as KFAIL
> 
> When running gdb.threads/watchthreads-reorder.exp in parallel with:
> ...
> $ n=$(grep -c processor /proc/cpuinfo); n=$((($n + 1) / 2)); stress -c $n
> ...
> there's a reasonable change to trigger an internal gdb error:
> ...
> $ for n in $(seq 1 10); do ./test.sh; done 2>&1 \
>   | grep "expected passes" \
>   | sort \
>   | uniq -c
>       1 # of expected passes            14
>       2 # of expected passes            15
>       1 # of expected passes            16
>       6 # of expected passes            17
> ...
> which look like this in gdb.sum:
> ...
> FAIL: gdb.threads/watchthreads-reorder.exp: reorder1: continue to breakpoint: \
>   break-at-exit (GDB internal error)
> ...
> 
> This FAIL is filed as PR gdb/24995 and fixed on master by commit c29705b71a
> "Fix internal error and improve 'set debug infrun 1'/target wait kind trace".
> 
> Mark this as KFAIL for the 8.3 branch.
> 
> It's trivial to do this by adding a setup_kfail:
> ...
> +    setup_kfail gdb/24995 "*-*-*"
>      gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
> ...
> but we'll get a fair amount of KPASSES:
> ...
> KPASS: gdb.threads/watchthreads-reorder.exp: reorder0: \
>   continue to breakpoint: break-at-exit (PRMS gdb/24995)
> KPASS: gdb.threads/watchthreads-reorder.exp: reorder1: \
>   continue to breakpoint: break-at-exit (PRMS gdb/24995)
> ...
> 
> Instead, do this more precise by only KFAILing in case the internal error is detected.
> 
> Tested on x86_64-linux.
> 
> gdb/testsuite/ChangeLog:
> 
> 2019-09-12  Tom de Vries  <tdevries@suse.de>
> 
> 	* gdb.threads/watchthreads-reorder.exp: Add PR gdb/24995 KFAIL.
> 	* lib/gdb.exp (prepare_user_code): New proc, factored out of ...
> 	(gdb_test_multiple): ... here.  Add and handle optional argument
> 	early_user_code.
> 
> ---
>  gdb/testsuite/gdb.threads/watchthreads-reorder.exp |  15 +-
>  gdb/testsuite/lib/gdb.exp                          | 167 ++++++++++++---------
>  2 files changed, 110 insertions(+), 72 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.threads/watchthreads-reorder.exp b/gdb/testsuite/gdb.threads/watchthreads-reorder.exp
> index 9bbbb6f2b8..efbaef1e63 100644
> --- a/gdb/testsuite/gdb.threads/watchthreads-reorder.exp
> +++ b/gdb/testsuite/gdb.threads/watchthreads-reorder.exp
> @@ -90,5 +90,18 @@ foreach reorder {0 1} { with_test_prefix "reorder$reorder" {
>      # found in the DEBUG_INFRUN code path.
>      gdb_test "set debug infrun 1"
>  
> -    gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
> +    # Do:
> +    #   gdb_continue_to_breakpoint "break-at-exit" ".*break-at-exit.*"
> +    # with setup_kfail.
> +    set msg "continue to breakpoint: break-at-exit"
> +    gdb_test_multiple "continue" $msg  {
> +	-re "internal-error: inferior\\* find_inferior_pid\\(int\\): Assertion .pid != 0. failed\\." {
> +	    setup_kfail gdb/24995 "*-*-*"
> +	    exp_continue
> +	}
> +    } {
> +	-re "(?:Breakpoint|Temporary breakpoint) .* (at|in) .*break-at-exit.*\r\n$gdb_prompt $" {
> +	    pass $msg
> +	}
> +    }
>  }}
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 3d5f8726f7..2cddd5cf60 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -694,14 +694,92 @@ proc gdb_internal_error_resync {} {
>      return 0
>  }
>  
> +# Prepare expect arguments for execution in gdb_test_multiple.
> +#
> +proc prepare_user_code { user_code } {
> +    # TCL/EXPECT WART ALERT
> +    # Expect does something very strange when it receives a single braced
> +    # argument.  It splits it along word separators and performs substitutions.
> +    # This means that { "[ab]" } is evaluated as "[ab]", but { "\[ab\]" } is
> +    # evaluated as "\[ab\]".  But that's not how TCL normally works; inside a
> +    # double-quoted list item, "\[ab\]" is just a long way of representing
> +    # "[ab]", because the backslashes will be removed by lindex.
> +
> +    # Unfortunately, there appears to be no easy way to duplicate the splitting
> +    # that expect will do from within TCL.  And many places make use of the
> +    # "\[0-9\]" construct, so we need to support that; and some places make use
> +    # of the "[func]" construct, so we need to support that too.  In order to
> +    # get this right we have to substitute quoted list elements differently
> +    # from braced list elements.
> +
> +    # We do this roughly the same way that Expect does it.  We have to use two
> +    # lists, because if we leave unquoted newlines in the argument to uplevel
> +    # they'll be treated as command separators, and if we escape newlines
> +    # we mangle newlines inside of command blocks.  This assumes that the
> +    # input doesn't contain a pattern which contains actual embedded newlines
> +    # at this point!
> +    uplevel {
> +	regsub -all {\n} ${user_code} { } subst_code
> +	set subst_code [uplevel list $subst_code]
> +
> +	set processed_code ""
> +	set patterns ""
> +	set expecting_action 0
> +	set expecting_arg 0
> +	foreach item $user_code subst_item $subst_code {
> +	    if { $item == "-n" || $item == "-notransfer" || $item == "-nocase" } {
> +		lappend processed_code $item
> +		continue
> +	    }
> +	    if { $item == "-indices" || $item == "-re" || $item == "-ex" } {
> +		lappend processed_code $item
> +		continue
> +	    }
> +	    if { $item == "-timeout" || $item == "-i" } {
> +		set expecting_arg 1
> +		lappend processed_code $item
> +		continue
> +	    }
> +	    if { $expecting_arg } {
> +		set expecting_arg 0
> +		lappend processed_code $subst_item
> +		continue
> +	    }
> +	    if { $expecting_action } {
> +		lappend processed_code "uplevel [list $item]"
> +		set expecting_action 0
> +		# Cosmetic, no effect on the list.
> +		append processed_code "\n"
> +		continue
> +	    }
> +	    set expecting_action 1
> +	    lappend processed_code $subst_item
> +	    if {$patterns != ""} {
> +		append patterns "; "
> +	    }
> +	    append patterns "\"$subst_item\""
> +	}
> +
> +	# Also purely cosmetic.
> +	regsub -all {\r} $patterns {\\r} patterns
> +	regsub -all {\n} $patterns {\\n} patterns
> +
> +	if $verbose>2 then {
> +	    send_user "Looking to match \"$patterns\"\n"
> +	}
> +	return $processed_code
> +    }
> +}
>  
> -# gdb_test_multiple COMMAND MESSAGE EXPECT_ARGUMENTS
> +# gdb_test_multiple COMMAND MESSAGE [EARLY_EXPECT_ARGUMENTS] EXPECT_ARGUMENTS
>  # Send a command to gdb; test the result.
>  #
>  # COMMAND is the command to execute, send to GDB with send_gdb.  If
>  #   this is the null string no command is sent.
>  # MESSAGE is a message to be printed with the built-in failure patterns
>  #   if one of them matches.  If MESSAGE is empty COMMAND will be used.
> +# EARLY_EXPECT_ARGUMENTS as EXPECT_ARGUMENTS, but will be fed to expect
> +#   before the standard patterns.
>  # EXPECT_ARGUMENTS will be fed to expect in addition to the standard
>  #   patterns.  Pattern elements will be evaluated in the caller's
>  #   context; action elements will be executed in the caller's context.
> @@ -744,7 +822,7 @@ proc gdb_internal_error_resync {} {
>  # expected from $gdb_spawn_id.  IOW, callers do not need to worry
>  # about resetting "-i" back to $gdb_spawn_id explicitly.
>  #
> -proc gdb_test_multiple { command message user_code } {
> +proc gdb_test_multiple { command message args } {
>      global verbose use_gdb_stub
>      global gdb_prompt pagination_prompt
>      global GDB
> @@ -754,6 +832,16 @@ proc gdb_test_multiple { command message user_code } {
>      upvar expect_out expect_out
>      global any_spawn_id
>  
> +    if { [llength $args] == 2 } {
> +	set early_user_code [lindex $args 0]
> +	set user_code [lindex $args 1]
> +    } elseif { [llength $args] == 1 } {
> +	set early_user_code {}
> +	set user_code [lindex $args 0]
> +    } else {
> +	error "Invalid number of arguments for gdb_test_multiple"
> +    }
> +
>      if { $message == "" } {
>  	set message $command
>      }
> @@ -772,76 +860,12 @@ proc gdb_test_multiple { command message user_code } {
>  	error "gdbserver does not support $command without extended-remote"
>      }
>  
> -    # TCL/EXPECT WART ALERT
> -    # Expect does something very strange when it receives a single braced
> -    # argument.  It splits it along word separators and performs substitutions.
> -    # This means that { "[ab]" } is evaluated as "[ab]", but { "\[ab\]" } is
> -    # evaluated as "\[ab\]".  But that's not how TCL normally works; inside a
> -    # double-quoted list item, "\[ab\]" is just a long way of representing
> -    # "[ab]", because the backslashes will be removed by lindex.
> -
> -    # Unfortunately, there appears to be no easy way to duplicate the splitting
> -    # that expect will do from within TCL.  And many places make use of the
> -    # "\[0-9\]" construct, so we need to support that; and some places make use
> -    # of the "[func]" construct, so we need to support that too.  In order to
> -    # get this right we have to substitute quoted list elements differently
> -    # from braced list elements.
> -
> -    # We do this roughly the same way that Expect does it.  We have to use two
> -    # lists, because if we leave unquoted newlines in the argument to uplevel
> -    # they'll be treated as command separators, and if we escape newlines
> -    # we mangle newlines inside of command blocks.  This assumes that the
> -    # input doesn't contain a pattern which contains actual embedded newlines
> -    # at this point!
> -
> -    regsub -all {\n} ${user_code} { } subst_code
> -    set subst_code [uplevel list $subst_code]
> -
> -    set processed_code ""
> -    set patterns ""
> -    set expecting_action 0
> -    set expecting_arg 0
> -    foreach item $user_code subst_item $subst_code {
> -	if { $item == "-n" || $item == "-notransfer" || $item == "-nocase" } {
> -	    lappend processed_code $item
> -	    continue
> -	}
> -	if { $item == "-indices" || $item == "-re" || $item == "-ex" } {
> -	    lappend processed_code $item
> -	    continue
> -	}
> -	if { $item == "-timeout" || $item == "-i" } {
> -	    set expecting_arg 1
> -	    lappend processed_code $item
> -	    continue
> -	}
> -	if { $expecting_arg } {
> -	    set expecting_arg 0
> -	    lappend processed_code $subst_item
> -	    continue
> -	}
> -	if { $expecting_action } {
> -	    lappend processed_code "uplevel [list $item]"
> -	    set expecting_action 0
> -	    # Cosmetic, no effect on the list.
> -	    append processed_code "\n"
> -	    continue
> -	}
> -	set expecting_action 1
> -	lappend processed_code $subst_item
> -	if {$patterns != ""} {
> -	    append patterns "; "
> -	}
> -	append patterns "\"$subst_item\""
> -    }
> -
> -    # Also purely cosmetic.
> -    regsub -all {\r} $patterns {\\r} patterns
> -    regsub -all {\n} $patterns {\\n} patterns
> -
>      if $verbose>2 then {
>  	send_user "Sending \"$command\" to gdb\n"
> -	send_user "Looking to match \"$patterns\"\n"
> +    }
> +    set processed_code [prepare_user_code $user_code]
> +    set early_processed_code [prepare_user_code $early_user_code]
> +    if $verbose>2 then {
>  	send_user "Message is \"$message\"\n"
>      }
>  
> @@ -891,7 +915,8 @@ proc gdb_test_multiple { command message user_code } {
>  	}
>      }
>  
> -    set code {
> +    set code $early_user_code
> +    append code {
>  	-re ".*A problem internal to GDB has been detected" {
>  	    fail "$message (GDB internal error)"
>  	    gdb_internal_error_resync
> 

Re: [RFAv2] Fix internal error and improve 'set debug infrun 1'/target wait kind trace

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

Hi!

On Sat, 30 Mar 2019 10:58:31 +0100, Philippe Waroquiers <philippe.waroquiers@skynet.be> wrote:
> This patch [...] improves/factorises some wait kind traces.

> --- a/gdb/gnu-nat.c
> +++ b/gdb/gnu-nat.c
> @@ -1647,15 +1647,9 @@ rewait:
>        inf_update_suspends (inf);
>      }
>  
> -  inf_debug (inf, "returning ptid = %s, status = %s (%d)",
> +  inf_debug (inf, "returning ptid = %s, %s",
>  	     target_pid_to_str (ptid).c_str (),
> -	     status->kind == TARGET_WAITKIND_EXITED ? "EXITED"
> -	     : status->kind == TARGET_WAITKIND_STOPPED ? "STOPPED"
> -	     : status->kind == TARGET_WAITKIND_SIGNALLED ? "SIGNALLED"
> -	     : status->kind == TARGET_WAITKIND_LOADED ? "LOADED"
> -	     : status->kind == TARGET_WAITKIND_SPURIOUS ? "SPURIOUS"
> -	     : "?",
> -	     status->value.integer);
> +	     target_waitstatus_to_string (&status).c_str ());

Thanks for that cleanup.  I pushed the attached commit
e9f8e3f109d57c119eaaa1e56378926f7f20863f "[GDB, Hurd] Fix build;
'target_waitstatus_to_string' call" to make it work.  ;-)


Grüße
 Thomas



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-GDB-Hurd-Fix-build-target_waitstatus_to_string-call.patch --]
[-- Type: text/x-diff, Size: 2172 bytes --]

From e9f8e3f109d57c119eaaa1e56378926f7f20863f Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Thu, 4 Apr 2019 11:20:09 +0200
Subject: [PATCH] [GDB, Hurd] Fix build; 'target_waitstatus_to_string' call

Recent commit c29705b71a8ec966478c0dc4712194a95291c6de removed an incomplete
local implementation in favor of 'target_waitstatus_to_string' (thanks!), but
introduced a small typing error:

    In file included from [...]/gdb/gnu-nat.c:24:0:
    [...]/gdb/gnu-nat.c: In member function 'virtual ptid_t gnu_nat_target::wait(ptid_t, target_waitstatus*, int)':
    [...]/gdb/gnu-nat.c:1652:43: error: cannot convert 'target_waitstatus**' to 'const target_waitstatus*' for argument '1' to 'std::__cxx11::string target_waitstatus_to_string(const target_waitstatus*)'
           target_waitstatus_to_string (&status).c_str ());
                                               ^
    [...]/gdb/gnu-nat.h:119:32: note: in definition of macro 'debug'
            __FILE__ , __LINE__ , ##args); } while (0)
                                    ^~~~
    [...]/gdb/gnu-nat.c:1650:3: note: in expansion of macro 'inf_debug'
       inf_debug (inf, "returning ptid = %s, %s",
       ^~~~~~~~~

	gdb/
	* gnu-nat.c (gnu_nat_target::wait): Fix
	target_waitstatus_to_string call.
---
 gdb/ChangeLog | 5 +++++
 gdb/gnu-nat.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index aee0d967e1..4a49d32277 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2019-04-04  Thomas Schwinge  <thomas@codesourcery.com>
+
+	* gnu-nat.c (gnu_nat_target::wait): Fix
+	target_waitstatus_to_string call.
+
 2019-04-01  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* eval.c (evaluate_subexp_standard): Handle internal functions
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 654a652717..5eeac9270a 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -1649,7 +1649,7 @@ rewait:
 
   inf_debug (inf, "returning ptid = %s, %s",
 	     target_pid_to_str (ptid).c_str (),
-	     target_waitstatus_to_string (&status).c_str ());
+	     target_waitstatus_to_string (status).c_str ());
 
   return ptid;
 }
-- 
2.19.2

Re: [RFAv2] Fix internal error and improve 'set debug infrun 1'/target wait kind trace

[Philippe Waroquiers]

On Thu, 2019-04-04 at 11:28 +0200, Thomas Schwinge wrote:
> I pushed the attached commit
> e9f8e3f109d57c119eaaa1e56378926f7f20863f "[GDB, Hurd] Fix build;
> 'target_waitstatus_to_string' call" to make it work.  ;-)
Oops, sorry, and thanks for fixing.
I have not yet taken the time to see how to use the buildbot to catch
such errors before pushing.
Is there a Hurd machine in the buildbot set of machines ?

Thanks

Philippe