[PATCH 4/6] gdb: Remove final cleanup from find_overload_match

[PATCH 4/6] gdb: Remove final cleanup from find_overload_match

[Andrew Burgess]

This patch removes the setup of a null_cleanup in
valops.c:find_overload_match, and all the calls to do_cleanups.

gdb/ChangeLog:

	* valops.c (find_overload_match): Remove use of null_cleanup, and
	calls to do_cleanups.
---
 gdb/ChangeLog | 5 +++++
 gdb/valops.c  | 6 ------
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/gdb/valops.c b/gdb/valops.c
index 1a9d6a6f958..75ff7058b7c 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -2516,8 +2516,6 @@ find_overload_match (gdb::array_view<value *> args,
   struct type *basetype = NULL;
   LONGEST boffset;
 
-  struct cleanup *all_cleanups = make_cleanup (null_cleanup, NULL);
-
   const char *obj_type_name = NULL;
   const char *func_name = NULL;
   gdb::unique_xmalloc_ptr<char> temp_func;
@@ -2547,7 +2545,6 @@ find_overload_match (gdb::array_view<value *> args,
 	  if (*valp)
 	    {
 	      *staticp = 1;
-	      do_cleanups (all_cleanups);
 	      return 0;
 	    }
 	}
@@ -2693,7 +2690,6 @@ find_overload_match (gdb::array_view<value *> args,
       if (func_name == NULL)
         {
 	  *symp = fsym;
-	  do_cleanups (all_cleanups);
           return 0;
         }
 
@@ -2820,8 +2816,6 @@ find_overload_match (gdb::array_view<value *> args,
       *objp = temp;
     }
 
-  do_cleanups (all_cleanups);
-
   switch (match_quality)
     {
     case INCOMPATIBLE:
-- 
2.14.5

[PATCH 1/6] gdb/testsuite: Better detection of auto-response at y/n prompts

[Andrew Burgess]

I noticed that when running this test:

  make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"

I would occasionally see some UNRESOLVED test results like this:

  (gdb)
  PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
  Expecting: ^(kill[
  ]+)?(.*[
  ]+[(]gdb[)]
  [ ]*)
  kill
  &"kill\n"
  ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
  =thread-group-exited,id="i1"
  ERROR: Got interactive prompt.
  UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:

The problem appears to be that the expect buffer fills up to include
the '(y or n)' prompt without including the following lines.

The pattern supplied by the outer test script is looking for the
following lines.  As the following lines are not present then expect
matches on the interactive prompt case rather than the case for the
user supplied pattern.

The problem with this is that we are not really at an interactive
prompt, GDB is providing an answer for us and then moving on.  When I
examine a successful run of the test the output from GDB is identical,
the only difference is where expect happens to buffer the output from
GDB.

This patch introduces a second check inside the 'y or n' prompt case,
which gives expect a chance to refill its buffers and catches the
'answered Y; input ...' text.

This second check is on a short 1 second timeout, I'm currently
assuming that the auto-answer text will either already be in expect,
or is waiting to be read in.  If after 1 second the auto-answer text
is not seen then we assume that GDB really is waiting at an
interactive prompt.

With this patch in place I can now leave the following loop running
indefinitely, where before it would fail usually after ~10
iterations.

  while make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"; \
  do /bin/true; \
  done

gdb/testsuite/ChangeLog:

	* lib/mi-support.exp (mi_gdb_test): When detecting a 'y or n'
	interactive prompt, look for the auto-answer before declaring a
	fail.
---
 gdb/testsuite/ChangeLog          |  6 ++++++
 gdb/testsuite/lib/mi-support.exp | 21 ++++++++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index d193592a843..48ea45d62c7 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -834,9 +834,24 @@ proc mi_gdb_test { args } {
 	     fail "$message"
 	}
 	 -re "\\(y or n\\) " {
-	    send_gdb "n\n"
-	    perror "Got interactive prompt."
-	     fail "$message"
+	    # If the expect buffer just happens to fill up to the 'y
+	    # or n' prompt then we can end up in this case, even
+	    # though GDB will automatically provide a response for us.
+	    # We give expect another chance here to look for the auto
+	    # answer text before declaring a fail.
+	    set auto_response_seen 0
+	    gdb_expect 1 {
+		-re ".answered Y; input not from terminal." {
+		    set auto_response_seen 1
+		}
+	    }
+	    if { ! $auto_response_seen } {
+		send_gdb "n\n"
+		perror "Got interactive prompt."
+		fail "$message"
+	    } else {
+		exp_continue
+	    }
 	}
 	 eof {
 	     perror "Process no longer exists"
-- 
2.14.5

[PATCH 2/6] gdb/remote: Remove a cleanup in remote_check_symbols

[Andrew Burgess]

Convert one of the variables that requires a cleanup from a 'char *'
to a 'gdb::char_vector' in remote_target::remote_check_symbols.

Tested on x86-64/Linux with target_board native-gdbserver and
native-extended-gdbserver.

gdb/ChangeLog:

	* remote.c (remote_target::remote_check_symbols): Convert `msg` to
	gdb::char_vector, remove cleanup, and update uses of `msg`.
---
 gdb/ChangeLog |  5 +++++
 gdb/remote.c  | 21 +++++++++++----------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index efed99855d4..324ed46809e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4883,7 +4883,7 @@ init_all_packet_configs (void)
 void
 remote_target::remote_check_symbols ()
 {
-  char *msg, *reply, *tmp;
+  char *reply, *tmp;
   int end;
   long reply_size;
   struct cleanup *old_chain;
@@ -4905,10 +4905,9 @@ remote_target::remote_check_symbols ()
 
   /* Allocate a message buffer.  We can't reuse the input buffer in RS,
      because we need both at the same time.  */
-  msg = (char *) xmalloc (get_remote_packet_size ());
-  old_chain = make_cleanup (xfree, msg);
+  gdb::char_vector msg (get_remote_packet_size ());
   reply = (char *) xmalloc (get_remote_packet_size ());
-  make_cleanup (free_current_contents, &reply);
+  old_chain = make_cleanup (free_current_contents, &reply);
   reply_size = get_remote_packet_size ();
 
   /* Invite target to request symbol lookups.  */
@@ -4922,11 +4921,13 @@ remote_target::remote_check_symbols ()
       struct bound_minimal_symbol sym;
 
       tmp = &reply[8];
-      end = hex2bin (tmp, (gdb_byte *) msg, strlen (tmp) / 2);
+      end = hex2bin (tmp, reinterpret_cast <gdb_byte *> (msg.data ()),
+		     strlen (tmp) / 2);
       msg[end] = '\0';
-      sym = lookup_minimal_symbol (msg, NULL, NULL);
+      sym = lookup_minimal_symbol (msg.data (), NULL, NULL);
       if (sym.minsym == NULL)
-	xsnprintf (msg, get_remote_packet_size (), "qSymbol::%s", &reply[8]);
+	xsnprintf (msg.data (), get_remote_packet_size (), "qSymbol::%s",
+		   &reply[8]);
       else
 	{
 	  int addr_size = gdbarch_addr_bit (target_gdbarch ()) / 8;
@@ -4938,11 +4939,11 @@ remote_target::remote_check_symbols ()
 							 sym_addr,
 							 current_top_target ());
 
-	  xsnprintf (msg, get_remote_packet_size (), "qSymbol:%s:%s",
+	  xsnprintf (msg.data (), get_remote_packet_size (), "qSymbol:%s:%s",
 		     phex_nz (sym_addr, addr_size), &reply[8]);
 	}
-  
-      putpkt (msg);
+
+      putpkt (msg.data ());
       getpkt (&reply, &reply_size, 0);
     }
 
-- 
2.14.5

[PATCH 0/6] Set of cleanup removal patches

[Andrew Burgess]

A set of patches that remove some cleanups.  The first patch isn't
cleanup related, but is a testsuite fix I found while testing the
remaining patches.

The series has been tested on X86-64/Linux against native gdb as well
as the native-gdbserver and native-extended-gdbserver board files.

--

Andrew Burgess (6):
  gdb/testsuite: Better detection of auto-response at y/n prompts
  gdb/remote: Remove a cleanup in remote_check_symbols
  gdb: Remove a cleanup from find_overload_match
  gdb: Remove final cleanup from find_overload_match
  gdb: Remove cleanup from linux-fork.c:inferior_call_waitpid
  gdb: Remove cleanup from linux_nat_target::follow_fork

 gdb/ChangeLog                     |  35 +++++++++++++
 gdb/compile/compile-cplus-types.c |   4 +-
 gdb/cp-support.c                  |   9 ++--
 gdb/cp-support.h                  |   2 +-
 gdb/linux-fork.c                  | 101 ++++++++++++++++++++++----------------
 gdb/linux-nat.c                   |  27 +++++-----
 gdb/remote.c                      |  21 ++++----
 gdb/testsuite/ChangeLog           |   6 +++
 gdb/testsuite/lib/mi-support.exp  |  21 ++++++--
 gdb/valops.c                      |  16 ++----
 10 files changed, 157 insertions(+), 85 deletions(-)

-- 
2.14.5

[PATCH 5/6] gdb: Remove cleanup from linux-fork.c:inferior_call_waitpid

[Andrew Burgess]

Replace cleanup in linux-fork.c:inferior_call_waitpid with a RAII
object.

gdb/ChangeLog:

	* linux-fork.c (class scoped_switch_fork_info): New class.
	(inferior_call_waitpid): Update to use scoped_switch_fork_info.
---
 gdb/ChangeLog    |   5 +++
 gdb/linux-fork.c | 101 +++++++++++++++++++++++++++++++++----------------------
 2 files changed, 65 insertions(+), 41 deletions(-)

diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index 39ab74e2deb..f3231bae048 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -437,45 +437,64 @@ linux_fork_detach (int from_tty)
     delete_fork (inferior_ptid);
 }
 
-static void
-inferior_call_waitpid_cleanup (void *fp)
+/* Temporarily switch to the infrun state stored on the fork_info
+   identified by a given ptid_t.  When this object goes out of scope,
+   restore the currently selected infrun state.   */
+
+class scoped_switch_fork_info
 {
-  struct fork_info *oldfp = (struct fork_info *) fp;
+public:
+  /* Switch to the infrun state held on the fork_info identified by
+     PPTID.  If PPTID is the current inferior then no switch is done.  */
+  scoped_switch_fork_info (ptid_t pptid)
+    : m_oldfp (nullptr)
+  {
+    if (pptid != inferior_ptid)
+      {
+	struct fork_info *newfp = nullptr;
+
+	/* Switch to pptid.  */
+	m_oldfp = find_fork_ptid (inferior_ptid);
+	gdb_assert (m_oldfp != nullptr);
+	newfp = find_fork_ptid (pptid);
+	gdb_assert (newfp != nullptr);
+	fork_save_infrun_state (m_oldfp, 1);
+	remove_breakpoints ();
+	fork_load_infrun_state (newfp);
+	insert_breakpoints ();
+      }
+  }
 
-  if (oldfp)
-    {
-      /* Switch back to inferior_ptid.  */
-      remove_breakpoints ();
-      fork_load_infrun_state (oldfp);
-      insert_breakpoints ();
-    }
-}
+  /* Restore the previously selected infrun state.  If the constructor
+     didn't need to switch states, then nothing is done here either.  */
+  ~scoped_switch_fork_info ()
+  {
+    if (m_oldfp != nullptr)
+      {
+	/* Switch back to inferior_ptid.  */
+	remove_breakpoints ();
+	fork_load_infrun_state (m_oldfp);
+	insert_breakpoints ();
+      }
+  }
+
+  DISABLE_COPY_AND_ASSIGN (scoped_switch_fork_info);
+
+private:
+  /* The fork_info for the previously selected infrun state, or nullptr if
+     we were already in the desired state, and nothing needs to be
+     restored.  */
+  struct fork_info *m_oldfp;
+};
 
 static int
 inferior_call_waitpid (ptid_t pptid, int pid)
 {
   struct objfile *waitpid_objf;
   struct value *waitpid_fn = NULL;
-  struct value *argv[3], *retv;
-  struct gdbarch *gdbarch = get_current_arch ();
-  struct fork_info *oldfp = NULL, *newfp = NULL;
-  struct cleanup *old_cleanup;
   int ret = -1;
 
-  if (pptid != inferior_ptid)
-    {
-      /* Switch to pptid.  */
-      oldfp = find_fork_ptid (inferior_ptid);
-      gdb_assert (oldfp != NULL);
-      newfp = find_fork_ptid (pptid);
-      gdb_assert (newfp != NULL);
-      fork_save_infrun_state (oldfp, 1);
-      remove_breakpoints ();
-      fork_load_infrun_state (newfp);
-      insert_breakpoints ();
-    }
-
-  old_cleanup = make_cleanup (inferior_call_waitpid_cleanup, oldfp);
+  scoped_switch_fork_info switch_fork_info (pptid);
 
   /* Get the waitpid_fn.  */
   if (lookup_minimal_symbol ("waitpid", NULL, NULL).minsym != NULL)
@@ -483,22 +502,22 @@ inferior_call_waitpid (ptid_t pptid, int pid)
   if (!waitpid_fn
       && lookup_minimal_symbol ("_waitpid", NULL, NULL).minsym != NULL)
     waitpid_fn = find_function_in_inferior ("_waitpid", &waitpid_objf);
-  if (!waitpid_fn)
-    goto out;
+  if (waitpid_fn != nullptr)
+    {
+      struct gdbarch *gdbarch = get_current_arch ();
+      struct value *argv[3], *retv;
 
-  /* Get the argv.  */
-  argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int, pid);
-  argv[1] = value_from_pointer (builtin_type (gdbarch)->builtin_data_ptr, 0);
-  argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
+      /* Get the argv.  */
+      argv[0] = value_from_longest (builtin_type (gdbarch)->builtin_int, pid);
+      argv[1] = value_from_pointer (builtin_type (gdbarch)->builtin_data_ptr, 0);
+      argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
 
-  retv = call_function_by_hand (waitpid_fn, NULL, argv);
-  if (value_as_long (retv) < 0)
-    goto out;
+      retv = call_function_by_hand (waitpid_fn, NULL, argv);
 
-  ret = 0;
+      if (value_as_long (retv) >= 0)
+	ret = 0;
+    }
 
-out:
-  do_cleanups (old_cleanup);
   return ret;
 }
 
-- 
2.14.5

[PATCH 3/6] gdb: Remove a cleanup from find_overload_match

[Andrew Burgess]

This patch changes cp-support.c:cp_func_name to return a
'gdb::unique_xmalloc_ptr<char>' instead of a 'char *'.  This allows a
cleanup to be removed from valops.c:find_overload_match.

gdb/ChangeLog:

	* compile/compile-cplus-types.c
	(compile_cplus_instance::decl_name): Handle changes to
	cp_func_name.
	* cp-support.c (cp_func_name): Update header comment, update
	return type.
	* cp-support.h (cp_func_name): Update return type in declaration.
	* valops.c (find_overload_match): Move temp_func local to top
	level of function and change its type.  Use temp_func to hold and
	delete temporary string obtained from cp_func_name.
---
 gdb/ChangeLog                     | 12 ++++++++++++
 gdb/compile/compile-cplus-types.c |  4 ++--
 gdb/cp-support.c                  |  9 ++++-----
 gdb/cp-support.h                  |  2 +-
 gdb/valops.c                      | 10 ++++------
 5 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/gdb/compile/compile-cplus-types.c b/gdb/compile/compile-cplus-types.c
index 3de0d8eee8c..910a874550d 100644
--- a/gdb/compile/compile-cplus-types.c
+++ b/gdb/compile/compile-cplus-types.c
@@ -63,9 +63,9 @@ compile_cplus_instance::decl_name (const char *natural)
   if (natural == nullptr)
     return nullptr;
 
-  char *name = cp_func_name (natural);
+  gdb::unique_xmalloc_ptr<char> name = cp_func_name (natural);
   if (name != nullptr)
-    return gdb::unique_xmalloc_ptr<char> (name);
+    return name;
 
   return gdb::unique_xmalloc_ptr<char> (xstrdup (natural));
 }
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 2024f87c44d..489bcca2b8d 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -808,10 +808,9 @@ method_name_from_physname (const char *physname)
 /* If FULL_NAME is the demangled name of a C++ function (including an
    arg list, possibly including namespace/class qualifications),
    return a new string containing only the function name (without the
-   arg list/class qualifications).  Otherwise, return NULL.  The
-   caller is responsible for freeing the memory in question.  */
+   arg list/class qualifications).  Otherwise, return NULL.  */
 
-char *
+gdb::unique_xmalloc_ptr<char>
 cp_func_name (const char *full_name)
 {
   gdb::unique_xmalloc_ptr<char> ret;
@@ -820,14 +819,14 @@ cp_func_name (const char *full_name)
 
   info = cp_demangled_name_to_comp (full_name, NULL);
   if (!info)
-    return NULL;
+    return nullptr;
 
   ret_comp = unqualified_name_from_comp (info->tree);
 
   if (ret_comp != NULL)
     ret = cp_comp_to_string (ret_comp, 10);
 
-  return ret.release ();
+  return ret;
 }
 
 /* Helper for cp_remove_params.  DEMANGLED_NAME is the name of a
diff --git a/gdb/cp-support.h b/gdb/cp-support.h
index 32fafe4a5a7..2677e1bfcaf 100644
--- a/gdb/cp-support.h
+++ b/gdb/cp-support.h
@@ -96,7 +96,7 @@ extern unsigned int cp_find_first_component (const char *name);
 
 extern unsigned int cp_entire_prefix_len (const char *name);
 
-extern char *cp_func_name (const char *full_name);
+extern gdb::unique_xmalloc_ptr<char> cp_func_name (const char *full_name);
 
 extern gdb::unique_xmalloc_ptr<char> cp_remove_params
   (const char *demanged_name);
diff --git a/gdb/valops.c b/gdb/valops.c
index caf31629482..1a9d6a6f958 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -2520,6 +2520,7 @@ find_overload_match (gdb::array_view<value *> args,
 
   const char *obj_type_name = NULL;
   const char *func_name = NULL;
+  gdb::unique_xmalloc_ptr<char> temp_func;
   enum oload_classification match_quality;
   enum oload_classification method_match_quality = INCOMPATIBLE;
   enum oload_classification src_method_match_quality = INCOMPATIBLE;
@@ -2666,20 +2667,17 @@ find_overload_match (gdb::array_view<value *> args,
               && TYPE_CODE (check_typedef (SYMBOL_TYPE (fsym)))
 	      == TYPE_CODE_FUNC)
             {
-	      char *temp_func;
-
 	      temp_func = cp_func_name (qualified_name);
 
 	      /* If cp_func_name did not remove anything, the name of the
 	         symbol did not include scope or argument types - it was
 	         probably a C-style function.  */
-	      if (temp_func)
+	      if (temp_func != nullptr)
 		{
-		  make_cleanup (xfree, temp_func);
-		  if (strcmp (temp_func, qualified_name) == 0)
+		  if (strcmp (temp_func.get (), qualified_name) == 0)
 		    func_name = NULL;
 		  else
-		    func_name = temp_func;
+		    func_name = temp_func.get ();
 		}
             }
         }
-- 
2.14.5

[PATCH 6/6] gdb: Remove cleanup from linux_nat_target::follow_fork

[Andrew Burgess]

Remove cleanup from linux_nat_target::follow_fork, instead add a new
unique_ptr specialisation for holding lwp_info pointers and use this
to ensure the pointer is cleaned up when needed.

gdb/ChangeLog:

	* linux-nat.c (delete_lwp_cleanup): Delete.
	(struct lwp_deleter): New struct.
	(lwp_info_up): New typedef.
	(linux_nat_target::follow_fork): Delete cleanup, and make use of
	lwp_info_up.
---
 gdb/ChangeLog   |  8 ++++++++
 gdb/linux-nat.c | 27 ++++++++++++++++-----------
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 7286207dd49..73d4fd193ae 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -425,15 +425,19 @@ num_lwps (int pid)
   return count;
 }
 
-/* Call delete_lwp with prototype compatible for make_cleanup.  */
+/* Deleter for lwp_info unique_ptr specialisation.  */
 
-static void
-delete_lwp_cleanup (void *lp_voidp)
+struct lwp_deleter
 {
-  struct lwp_info *lp = (struct lwp_info *) lp_voidp;
+  void operator() (struct lwp_info *lwp) const
+  {
+    delete_lwp (lwp->ptid);
+  }
+};
 
-  delete_lwp (lp->ptid);
-}
+/* A unique_ptr specialisation for lwp_info.  */
+
+typedef std::unique_ptr<struct lwp_info, lwp_deleter> lwp_info_up;
 
 /* Target hook for follow_fork.  On entry inferior_ptid must be the
    ptid of the followed inferior.  At return, inferior_ptid will be
@@ -466,10 +470,13 @@ linux_nat_target::follow_fork (int follow_child, int detach_fork)
 	{
 	  int child_stop_signal = 0;
 	  bool detach_child = true;
-	  struct cleanup *old_chain = make_cleanup (delete_lwp_cleanup,
-						    child_lp);
 
-	  linux_target->low_prepare_to_resume (child_lp);
+	  /* Move CHILD_LP into a unique_ptr and clear the source pointer
+	     to prevent us doing anything stupid with it.  */
+	  lwp_info_up child_lp_ptr (child_lp);
+	  child_lp = nullptr;
+
+	  linux_target->low_prepare_to_resume (child_lp_ptr.get ());
 
 	  /* When debugging an inferior in an architecture that supports
 	     hardware single stepping on a kernel without commit
@@ -508,8 +515,6 @@ linux_nat_target::follow_fork (int follow_child, int detach_fork)
 		signo = 0;
 	      ptrace (PTRACE_DETACH, child_pid, 0, signo);
 	    }
-
-	  do_cleanups (old_chain);
 	}
       else
 	{
-- 
2.14.5

Re: [PATCH 2/6] gdb/remote: Remove a cleanup in remote_check_symbols

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> gdb/ChangeLog:
Andrew> 	* remote.c (remote_target::remote_check_symbols): Convert `msg` to
Andrew> 	gdb::char_vector, remove cleanup, and update uses of `msg`.

Thanks, this is ok.

Tom

Re: [PATCH 3/6] gdb: Remove a cleanup from find_overload_match

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> This patch changes cp-support.c:cp_func_name to return a
Andrew> 'gdb::unique_xmalloc_ptr<char>' instead of a 'char *'.  This allows a
Andrew> cleanup to be removed from valops.c:find_overload_match.

Andrew> gdb/ChangeLog:

Andrew> 	* compile/compile-cplus-types.c
Andrew> 	(compile_cplus_instance::decl_name): Handle changes to
Andrew> 	cp_func_name.
Andrew> 	* cp-support.c (cp_func_name): Update header comment, update
Andrew> 	return type.
Andrew> 	* cp-support.h (cp_func_name): Update return type in declaration.
Andrew> 	* valops.c (find_overload_match): Move temp_func local to top
Andrew> 	level of function and change its type.  Use temp_func to hold and
Andrew> 	delete temporary string obtained from cp_func_name.

Thank you.  This is ok.

Tom

Re: [PATCH 4/6] gdb: Remove final cleanup from find_overload_match

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> This patch removes the setup of a null_cleanup in
Andrew> valops.c:find_overload_match, and all the calls to do_cleanups.

Andrew> gdb/ChangeLog:

Andrew> 	* valops.c (find_overload_match): Remove use of null_cleanup, and
Andrew> 	calls to do_cleanups.

Ok.  Thank you.

Tom

Re: [PATCH 6/6] gdb: Remove cleanup from linux_nat_target::follow_fork

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Remove cleanup from linux_nat_target::follow_fork, instead add a new
Andrew> unique_ptr specialisation for holding lwp_info pointers and use this
Andrew> to ensure the pointer is cleaned up when needed.

Andrew> gdb/ChangeLog:

Andrew> 	* linux-nat.c (delete_lwp_cleanup): Delete.
Andrew> 	(struct lwp_deleter): New struct.
Andrew> 	(lwp_info_up): New typedef.
Andrew> 	(linux_nat_target::follow_fork): Delete cleanup, and make use of
Andrew> 	lwp_info_up.

Thanks, this is ok.

Tom

Re: [PATCH 5/6] gdb: Remove cleanup from linux-fork.c:inferior_call_waitpid

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Replace cleanup in linux-fork.c:inferior_call_waitpid with a RAII
Andrew> object.

Andrew> gdb/ChangeLog:

Andrew> 	* linux-fork.c (class scoped_switch_fork_info): New class.
Andrew> 	(inferior_call_waitpid): Update to use scoped_switch_fork_info.

Thanks, this is ok.

Tom

Re: [PATCH 0/6] Set of cleanup removal patches

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> A set of patches that remove some cleanups.  The first patch isn't
Andrew> cleanup related, but is a testsuite fix I found while testing the
Andrew> remaining patches.

I can't really look into patch #1 right now -- maybe someone else can do
it in a more timely way.  The rest seemed fine to me, though, and IMO
can go in without that one if you'd like.

Tom

Re: [PATCH 1/6] gdb/testsuite: Better detection of auto-response at y/n prompts

[Simon Marchi]

On 2019-01-01 5:45 p.m., Andrew Burgess wrote:
> I noticed that when running this test:
> 
>   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"
> 
> I would occasionally see some UNRESOLVED test results like this:
> 
>   (gdb)
>   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
>   Expecting: ^(kill[
>   ]+)?(.*[
>   ]+[(]gdb[)]
>   [ ]*)
>   kill
>   &"kill\n"
>   ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
>   =thread-group-exited,id="i1"
>   ERROR: Got interactive prompt.
>   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
> 
> The problem appears to be that the expect buffer fills up to include
> the '(y or n)' prompt without including the following lines.
> 
> The pattern supplied by the outer test script is looking for the
> following lines.  As the following lines are not present then expect
> matches on the interactive prompt case rather than the case for the
> user supplied pattern.
> 
> The problem with this is that we are not really at an interactive
> prompt, GDB is providing an answer for us and then moving on.  When I
> examine a successful run of the test the output from GDB is identical,
> the only difference is where expect happens to buffer the output from
> GDB.
> 
> This patch introduces a second check inside the 'y or n' prompt case,
> which gives expect a chance to refill its buffers and catches the
> 'answered Y; input ...' text.
> 
> This second check is on a short 1 second timeout, I'm currently
> assuming that the auto-answer text will either already be in expect,
> or is waiting to be read in.  If after 1 second the auto-answer text
> is not seen then we assume that GDB really is waiting at an
> interactive prompt.
> 
> With this patch in place I can now leave the following loop running
> indefinitely, where before it would fail usually after ~10
> iterations.

For me it fails consistently the way you describe.

> diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
> index d193592a843..48ea45d62c7 100644
> --- a/gdb/testsuite/lib/mi-support.exp
> +++ b/gdb/testsuite/lib/mi-support.exp
> @@ -834,9 +834,24 @@ proc mi_gdb_test { args } {
>  	     fail "$message"
>  	}
>  	 -re "\\(y or n\\) " {
> -	    send_gdb "n\n"
> -	    perror "Got interactive prompt."
> -	     fail "$message"
> +	    # If the expect buffer just happens to fill up to the 'y
> +	    # or n' prompt then we can end up in this case, even
> +	    # though GDB will automatically provide a response for us.
> +	    # We give expect another chance here to look for the auto
> +	    # answer text before declaring a fail.
> +	    set auto_response_seen 0
> +	    gdb_expect 1 {
> +		-re ".answered Y; input not from terminal." {
> +		    set auto_response_seen 1
> +		}
> +	    }
> +	    if { ! $auto_response_seen } {
> +		send_gdb "n\n"
> +		perror "Got interactive prompt."
> +		fail "$message"
> +	    } else {
> +		exp_continue
> +	    }
>  	}
>  	 eof {
>  	     perror "Process no longer exists"

Do we need this stanza at all?  I understand that it can save some time (avoid having
to wait for the timeout) if the MI interpreter suddenly starts asking interactive
"y or n" questions (which it should not do), but since it's causing this kind of trouble,
maybe we can just get rid of it?

Simon

Re: [PATCH 1/6] gdb/testsuite: Better detection of auto-response at y/n prompts

[Andrew Burgess]

* Simon Marchi <simon.marchi@ericsson.com> [2019-01-02 21:48:06 +0000]:

> On 2019-01-01 5:45 p.m., Andrew Burgess wrote:
> > I noticed that when running this test:
> > 
> >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"
> > 
> > I would occasionally see some UNRESOLVED test results like this:
> > 
> >   (gdb)
> >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
> >   Expecting: ^(kill[
> >   ]+)?(.*[
> >   ]+[(]gdb[)]
> >   [ ]*)
> >   kill
> >   &"kill\n"
> >   ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
> >   =thread-group-exited,id="i1"
> >   ERROR: Got interactive prompt.
> >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
> > 
> > The problem appears to be that the expect buffer fills up to include
> > the '(y or n)' prompt without including the following lines.
> > 
> > The pattern supplied by the outer test script is looking for the
> > following lines.  As the following lines are not present then expect
> > matches on the interactive prompt case rather than the case for the
> > user supplied pattern.
> > 
> > The problem with this is that we are not really at an interactive
> > prompt, GDB is providing an answer for us and then moving on.  When I
> > examine a successful run of the test the output from GDB is identical,
> > the only difference is where expect happens to buffer the output from
> > GDB.
> > 
> > This patch introduces a second check inside the 'y or n' prompt case,
> > which gives expect a chance to refill its buffers and catches the
> > 'answered Y; input ...' text.
> > 
> > This second check is on a short 1 second timeout, I'm currently
> > assuming that the auto-answer text will either already be in expect,
> > or is waiting to be read in.  If after 1 second the auto-answer text
> > is not seen then we assume that GDB really is waiting at an
> > interactive prompt.
> > 
> > With this patch in place I can now leave the following loop running
> > indefinitely, where before it would fail usually after ~10
> > iterations.
> 
> For me it fails consistently the way you describe.
> 
> > diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
> > index d193592a843..48ea45d62c7 100644
> > --- a/gdb/testsuite/lib/mi-support.exp
> > +++ b/gdb/testsuite/lib/mi-support.exp
> > @@ -834,9 +834,24 @@ proc mi_gdb_test { args } {
> >  	     fail "$message"
> >  	}
> >  	 -re "\\(y or n\\) " {
> > -	    send_gdb "n\n"
> > -	    perror "Got interactive prompt."
> > -	     fail "$message"
> > +	    # If the expect buffer just happens to fill up to the 'y
> > +	    # or n' prompt then we can end up in this case, even
> > +	    # though GDB will automatically provide a response for us.
> > +	    # We give expect another chance here to look for the auto
> > +	    # answer text before declaring a fail.
> > +	    set auto_response_seen 0
> > +	    gdb_expect 1 {
> > +		-re ".answered Y; input not from terminal." {
> > +		    set auto_response_seen 1
> > +		}
> > +	    }
> > +	    if { ! $auto_response_seen } {
> > +		send_gdb "n\n"
> > +		perror "Got interactive prompt."
> > +		fail "$message"
> > +	    } else {
> > +		exp_continue
> > +	    }
> >  	}
> >  	 eof {
> >  	     perror "Process no longer exists"
> 
> Do we need this stanza at all?  I understand that it can save some time (avoid having
> to wait for the timeout) if the MI interpreter suddenly starts asking interactive
> "y or n" questions (which it should not do), but since it's causing this kind of trouble,
> maybe we can just get rid of it?

I don't see any real reason to keep it.  I tried deleting it and no
tests seem to regress, and the previously unstable test is now stable.

Is this OK to apply?

Thanks,
Andrew

--

gdb/testsuite: Remove interactive prompt case from mi_gdb_test

I noticed that when running this test:

  make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"

I would occasionally see some UNRESOLVED test results like this:

  (gdb)
  PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
  Expecting: ^(kill[
  ]+)?(.*[
  ]+[(]gdb[)]
  [ ]*)
  kill
  &"kill\n"
  ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
  =thread-group-exited,id="i1"
  ERROR: Got interactive prompt.
  UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:

The problem appears to be that the expect buffer fills up to include
the '(y or n)' prompt without including the following lines.

The pattern supplied by the outer test script is looking for the
following lines.  As the following lines are not present then expect
matches on the interactive prompt case rather than the case for the
user supplied pattern.

The problem with this is that we are not really at an interactive
prompt, GDB is providing an answer for us and then moving on.  When I
examine a successful run of the test the output from GDB is identical,
the only difference is where expect happens to buffer the output from
GDB.

This patch remove all special handling of the interactive prompt
case.  This means that if we ever break GDB and start seeing an
unexpected interactive prompt then tests will rely on a timeout to
fail, instead of having dedicated interactive prompt detection, but
this solves the problem that an auto-answered prompt looks very
similar to an interactive prompt.

With this patch in place I can now leave the following loop running
indefinitely, where before it would fail usually after ~10
iterations.

  while make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"; \
  do /bin/true; \
  done

gdb/testsuite/ChangeLog:

	* lib/mi-support.exp (mi_gdb_test): Remove interactive prompt
	case.
---
 gdb/testsuite/ChangeLog          | 5 +++++
 gdb/testsuite/lib/mi-support.exp | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index d193592a843..a58c4f6e119 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -832,11 +832,6 @@ proc mi_gdb_test { args } {
 	    send_gdb "\n"
 	    perror "Window too small."
 	     fail "$message"
-	}
-	 -re "\\(y or n\\) " {
-	    send_gdb "n\n"
-	    perror "Got interactive prompt."
-	     fail "$message"
 	}
 	 eof {
 	     perror "Process no longer exists"
-- 
2.14.5

Re: [PATCH 1/6] gdb/testsuite: Better detection of auto-response at y/n prompts

[Simon Marchi]

On 2019-01-07 04:00, Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@ericsson.com> [2019-01-02 21:48:06 +0000]:
> 
>> On 2019-01-01 5:45 p.m., Andrew Burgess wrote:
>> > I noticed that when running this test:
>> >
>> >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"
>> >
>> > I would occasionally see some UNRESOLVED test results like this:
>> >
>> >   (gdb)
>> >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
>> >   Expecting: ^(kill[
>> >   ]+)?(.*[
>> >   ]+[(]gdb[)]
>> >   [ ]*)
>> >   kill
>> >   &"kill\n"
>> >   ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
>> >   =thread-group-exited,id="i1"
>> >   ERROR: Got interactive prompt.
>> >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
>> >
>> > The problem appears to be that the expect buffer fills up to include
>> > the '(y or n)' prompt without including the following lines.
>> >
>> > The pattern supplied by the outer test script is looking for the
>> > following lines.  As the following lines are not present then expect
>> > matches on the interactive prompt case rather than the case for the
>> > user supplied pattern.
>> >
>> > The problem with this is that we are not really at an interactive
>> > prompt, GDB is providing an answer for us and then moving on.  When I
>> > examine a successful run of the test the output from GDB is identical,
>> > the only difference is where expect happens to buffer the output from
>> > GDB.
>> >
>> > This patch introduces a second check inside the 'y or n' prompt case,
>> > which gives expect a chance to refill its buffers and catches the
>> > 'answered Y; input ...' text.
>> >
>> > This second check is on a short 1 second timeout, I'm currently
>> > assuming that the auto-answer text will either already be in expect,
>> > or is waiting to be read in.  If after 1 second the auto-answer text
>> > is not seen then we assume that GDB really is waiting at an
>> > interactive prompt.
>> >
>> > With this patch in place I can now leave the following loop running
>> > indefinitely, where before it would fail usually after ~10
>> > iterations.
>> 
>> For me it fails consistently the way you describe.
>> 
>> > diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
>> > index d193592a843..48ea45d62c7 100644
>> > --- a/gdb/testsuite/lib/mi-support.exp
>> > +++ b/gdb/testsuite/lib/mi-support.exp
>> > @@ -834,9 +834,24 @@ proc mi_gdb_test { args } {
>> >  	     fail "$message"
>> >  	}
>> >  	 -re "\\(y or n\\) " {
>> > -	    send_gdb "n\n"
>> > -	    perror "Got interactive prompt."
>> > -	     fail "$message"
>> > +	    # If the expect buffer just happens to fill up to the 'y
>> > +	    # or n' prompt then we can end up in this case, even
>> > +	    # though GDB will automatically provide a response for us.
>> > +	    # We give expect another chance here to look for the auto
>> > +	    # answer text before declaring a fail.
>> > +	    set auto_response_seen 0
>> > +	    gdb_expect 1 {
>> > +		-re ".answered Y; input not from terminal." {
>> > +		    set auto_response_seen 1
>> > +		}
>> > +	    }
>> > +	    if { ! $auto_response_seen } {
>> > +		send_gdb "n\n"
>> > +		perror "Got interactive prompt."
>> > +		fail "$message"
>> > +	    } else {
>> > +		exp_continue
>> > +	    }
>> >  	}
>> >  	 eof {
>> >  	     perror "Process no longer exists"
>> 
>> Do we need this stanza at all?  I understand that it can save some 
>> time (avoid having
>> to wait for the timeout) if the MI interpreter suddenly starts asking 
>> interactive
>> "y or n" questions (which it should not do), but since it's causing 
>> this kind of trouble,
>> maybe we can just get rid of it?
> 
> I don't see any real reason to keep it.  I tried deleting it and no
> tests seem to regress, and the previously unstable test is now stable.
> 
> Is this OK to apply?
> 
> Thanks,
> Andrew
> 
> --
> 
> gdb/testsuite: Remove interactive prompt case from mi_gdb_test
> 
> I noticed that when running this test:
> 
>   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
> gdb.mi/mi-break.exp"
> 
> I would occasionally see some UNRESOLVED test results like this:
> 
>   (gdb)
>   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
>   Expecting: ^(kill[
>   ]+)?(.*[
>   ]+[(]gdb[)]
>   [ ]*)
>   kill
>   &"kill\n"
>   ~"Kill the program being debugged? (y or n) [answered Y; input not
> from terminal]\n"
>   =thread-group-exited,id="i1"
>   ERROR: Got interactive prompt.
>   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
> 
> The problem appears to be that the expect buffer fills up to include
> the '(y or n)' prompt without including the following lines.
> 
> The pattern supplied by the outer test script is looking for the
> following lines.  As the following lines are not present then expect
> matches on the interactive prompt case rather than the case for the
> user supplied pattern.
> 
> The problem with this is that we are not really at an interactive
> prompt, GDB is providing an answer for us and then moving on.  When I
> examine a successful run of the test the output from GDB is identical,
> the only difference is where expect happens to buffer the output from
> GDB.
> 
> This patch remove all special handling of the interactive prompt
> case.  This means that if we ever break GDB and start seeing an
> unexpected interactive prompt then tests will rely on a timeout to
> fail, instead of having dedicated interactive prompt detection, but
> this solves the problem that an auto-answered prompt looks very
> similar to an interactive prompt.
> 
> With this patch in place I can now leave the following loop running
> indefinitely, where before it would fail usually after ~10
> iterations.
> 
>   while make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
> gdb.mi/mi-break.exp"; \
>   do /bin/true; \
>   done
> 
> gdb/testsuite/ChangeLog:
> 
> 	* lib/mi-support.exp (mi_gdb_test): Remove interactive prompt
> 	case.
> ---
>  gdb/testsuite/ChangeLog          | 5 +++++
>  gdb/testsuite/lib/mi-support.exp | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/mi-support.exp 
> b/gdb/testsuite/lib/mi-support.exp
> index d193592a843..a58c4f6e119 100644
> --- a/gdb/testsuite/lib/mi-support.exp
> +++ b/gdb/testsuite/lib/mi-support.exp
> @@ -832,11 +832,6 @@ proc mi_gdb_test { args } {
>  	    send_gdb "\n"
>  	    perror "Window too small."
>  	     fail "$message"
> -	}
> -	 -re "\\(y or n\\) " {
> -	    send_gdb "n\n"
> -	    perror "Got interactive prompt."
> -	     fail "$message"
>  	}
>  	 eof {
>  	     perror "Process no longer exists"


Thanks, this LGTM.

Btw, what you think about those "fail"s following "perror"s?  It's my 
understanding that perror throws an error, interrupting the execution 
flow, so the fail is not recorded, right?

And a note, maybe you are already aware of its existence, but I think 
that "make check-read1" could have been useful to reproduce the issue 
reliably here.  It forces the read syscall to read 1 byte at the time, 
so you are certain to reach a point where -re "\\(y or n\\) " matches.

Simon

Re: [PATCH 1/6] gdb/testsuite: Better detection of auto-response at y/n prompts

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2019-01-09 00:03:49 -0500]:

> On 2019-01-07 04:00, Andrew Burgess wrote:
> > * Simon Marchi <simon.marchi@ericsson.com> [2019-01-02 21:48:06 +0000]:
> > 
> > > On 2019-01-01 5:45 p.m., Andrew Burgess wrote:
> > > > I noticed that when running this test:
> > > >
> > > >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"
> > > >
> > > > I would occasionally see some UNRESOLVED test results like this:
> > > >
> > > >   (gdb)
> > > >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
> > > >   Expecting: ^(kill[
> > > >   ]+)?(.*[
> > > >   ]+[(]gdb[)]
> > > >   [ ]*)
> > > >   kill
> > > >   &"kill\n"
> > > >   ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
> > > >   =thread-group-exited,id="i1"
> > > >   ERROR: Got interactive prompt.
> > > >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
> > > >
> > > > The problem appears to be that the expect buffer fills up to include
> > > > the '(y or n)' prompt without including the following lines.
> > > >
> > > > The pattern supplied by the outer test script is looking for the
> > > > following lines.  As the following lines are not present then expect
> > > > matches on the interactive prompt case rather than the case for the
> > > > user supplied pattern.
> > > >
> > > > The problem with this is that we are not really at an interactive
> > > > prompt, GDB is providing an answer for us and then moving on.  When I
> > > > examine a successful run of the test the output from GDB is identical,
> > > > the only difference is where expect happens to buffer the output from
> > > > GDB.
> > > >
> > > > This patch introduces a second check inside the 'y or n' prompt case,
> > > > which gives expect a chance to refill its buffers and catches the
> > > > 'answered Y; input ...' text.
> > > >
> > > > This second check is on a short 1 second timeout, I'm currently
> > > > assuming that the auto-answer text will either already be in expect,
> > > > or is waiting to be read in.  If after 1 second the auto-answer text
> > > > is not seen then we assume that GDB really is waiting at an
> > > > interactive prompt.
> > > >
> > > > With this patch in place I can now leave the following loop running
> > > > indefinitely, where before it would fail usually after ~10
> > > > iterations.
> > > 
> > > For me it fails consistently the way you describe.
> > > 
> > > > diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
> > > > index d193592a843..48ea45d62c7 100644
> > > > --- a/gdb/testsuite/lib/mi-support.exp
> > > > +++ b/gdb/testsuite/lib/mi-support.exp
> > > > @@ -834,9 +834,24 @@ proc mi_gdb_test { args } {
> > > >  	     fail "$message"
> > > >  	}
> > > >  	 -re "\\(y or n\\) " {
> > > > -	    send_gdb "n\n"
> > > > -	    perror "Got interactive prompt."
> > > > -	     fail "$message"
> > > > +	    # If the expect buffer just happens to fill up to the 'y
> > > > +	    # or n' prompt then we can end up in this case, even
> > > > +	    # though GDB will automatically provide a response for us.
> > > > +	    # We give expect another chance here to look for the auto
> > > > +	    # answer text before declaring a fail.
> > > > +	    set auto_response_seen 0
> > > > +	    gdb_expect 1 {
> > > > +		-re ".answered Y; input not from terminal." {
> > > > +		    set auto_response_seen 1
> > > > +		}
> > > > +	    }
> > > > +	    if { ! $auto_response_seen } {
> > > > +		send_gdb "n\n"
> > > > +		perror "Got interactive prompt."
> > > > +		fail "$message"
> > > > +	    } else {
> > > > +		exp_continue
> > > > +	    }
> > > >  	}
> > > >  	 eof {
> > > >  	     perror "Process no longer exists"
> > > 
> > > Do we need this stanza at all?  I understand that it can save some
> > > time (avoid having
> > > to wait for the timeout) if the MI interpreter suddenly starts
> > > asking interactive
> > > "y or n" questions (which it should not do), but since it's causing
> > > this kind of trouble,
> > > maybe we can just get rid of it?
> > 
> > I don't see any real reason to keep it.  I tried deleting it and no
> > tests seem to regress, and the previously unstable test is now stable.
> > 
> > Is this OK to apply?
> > 
> > Thanks,
> > Andrew
> > 
> > --
> > 
> > gdb/testsuite: Remove interactive prompt case from mi_gdb_test
> > 
> > I noticed that when running this test:
> > 
> >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
> > gdb.mi/mi-break.exp"
> > 
> > I would occasionally see some UNRESOLVED test results like this:
> > 
> >   (gdb)
> >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
> >   Expecting: ^(kill[
> >   ]+)?(.*[
> >   ]+[(]gdb[)]
> >   [ ]*)
> >   kill
> >   &"kill\n"
> >   ~"Kill the program being debugged? (y or n) [answered Y; input not
> > from terminal]\n"
> >   =thread-group-exited,id="i1"
> >   ERROR: Got interactive prompt.
> >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
> > 
> > The problem appears to be that the expect buffer fills up to include
> > the '(y or n)' prompt without including the following lines.
> > 
> > The pattern supplied by the outer test script is looking for the
> > following lines.  As the following lines are not present then expect
> > matches on the interactive prompt case rather than the case for the
> > user supplied pattern.
> > 
> > The problem with this is that we are not really at an interactive
> > prompt, GDB is providing an answer for us and then moving on.  When I
> > examine a successful run of the test the output from GDB is identical,
> > the only difference is where expect happens to buffer the output from
> > GDB.
> > 
> > This patch remove all special handling of the interactive prompt
> > case.  This means that if we ever break GDB and start seeing an
> > unexpected interactive prompt then tests will rely on a timeout to
> > fail, instead of having dedicated interactive prompt detection, but
> > this solves the problem that an auto-answered prompt looks very
> > similar to an interactive prompt.
> > 
> > With this patch in place I can now leave the following loop running
> > indefinitely, where before it would fail usually after ~10
> > iterations.
> > 
> >   while make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
> > gdb.mi/mi-break.exp"; \
> >   do /bin/true; \
> >   done
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* lib/mi-support.exp (mi_gdb_test): Remove interactive prompt
> > 	case.
> > ---
> >  gdb/testsuite/ChangeLog          | 5 +++++
> >  gdb/testsuite/lib/mi-support.exp | 5 -----
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/gdb/testsuite/lib/mi-support.exp
> > b/gdb/testsuite/lib/mi-support.exp
> > index d193592a843..a58c4f6e119 100644
> > --- a/gdb/testsuite/lib/mi-support.exp
> > +++ b/gdb/testsuite/lib/mi-support.exp
> > @@ -832,11 +832,6 @@ proc mi_gdb_test { args } {
> >  	    send_gdb "\n"
> >  	    perror "Window too small."
> >  	     fail "$message"
> > -	}
> > -	 -re "\\(y or n\\) " {
> > -	    send_gdb "n\n"
> > -	    perror "Got interactive prompt."
> > -	     fail "$message"
> >  	}
> >  	 eof {
> >  	     perror "Process no longer exists"
> 
> 
> Thanks, this LGTM.

Thanks, I pushed this patch.
> 
> Btw, what you think about those "fail"s following "perror"s?  It's my
> understanding that perror throws an error, interrupting the execution flow,
> so the fail is not recorded, right?

That's not what I see.  As an experiment I created the file
gdb.base/perror-test.exp like this:

  perror "this is an error"
  pass "this is a pass"

With this I see:

  ERROR: this is an error
  
  		=== gdb Summary ===
  
  # of unresolved testcases	1

With the unresolved being instead of the 'PASS'.  Also the return
value from runtest is non-zero.

If I change perror-test.exp to this:

  perror "this is an error"
  fail "this is a fail"

Then the results are similar:

  ERROR: this is an error
  
  		=== gdb Summary ===
  
  # of unresolved testcases	1

Again, the unresolved replaces the 'FAIL', and the exit value is
non-zero.

Where it gets interesting is if I change perror-test.exp to this:

  perror "this is an error"

Then I get this result:

  ERROR: this is an error
  
  		=== gdb Summary ===

And alarmingly (maybe?) the exit value from runtest is 0, so it looks
like the test has passed.

My conclusions then:

  - perror doesn't interrupt control flow,
  - perror doesn't itself cause the test script to appear to fail,
  - perror causes the next pass/fail (at least) to become unresolved,

Given that the perror calls we're looking at here are in mi_gdb_test,
the entire pass/fail printing is wrapped up within this method, I
don't think the unresolved-ness should leak out into the next test,
so, in this case I think perror/fail is correct.

> 
> And a note, maybe you are already aware of its existence, but I think that
> "make check-read1" could have been useful to reproduce the issue reliably
> here.  It forces the read syscall to read 1 byte at the time, so you are
> certain to reach a point where -re "\\(y or n\\) " matches.

I did not know this.  Might be worth testing the whole testsuite
against this and look for regressions..... I'll add it to my (long)
list.

Thanks,
Andrew

Re: [PATCH 5/6] gdb: Remove cleanup from linux-fork.c:inferior_call_waitpid

[Pedro Alves]

On 01/01/2019 10:45 PM, Andrew Burgess wrote:

> +/* Temporarily switch to the infrun state stored on the fork_info
> +   identified by a given ptid_t.  When this object goes out of scope,
> +   restore the currently selected infrun state.   */
> +
> +class scoped_switch_fork_info
>  {
> -  struct fork_info *oldfp = (struct fork_info *) fp;
> +public:
> +  /* Switch to the infrun state held on the fork_info identified by
> +     PPTID.  If PPTID is the current inferior then no switch is done.  */
> +  scoped_switch_fork_info (ptid_t pptid)

(Nit, "explicit scoped_switch_fork_info")

> +    : m_oldfp (nullptr)
> +  {
> +    if (pptid != inferior_ptid)
> +      {
> +	struct fork_info *newfp = nullptr;
> +
> +	/* Switch to pptid.  */
> +	m_oldfp = find_fork_ptid (inferior_ptid);
> +	gdb_assert (m_oldfp != nullptr);
> +	newfp = find_fork_ptid (pptid);
> +	gdb_assert (newfp != nullptr);
> +	fork_save_infrun_state (m_oldfp, 1);
> +	remove_breakpoints ();
> +	fork_load_infrun_state (newfp);
> +	insert_breakpoints ();
> +      }
> +  }
>  
> -  if (oldfp)
> -    {
> -      /* Switch back to inferior_ptid.  */
> -      remove_breakpoints ();
> -      fork_load_infrun_state (oldfp);
> -      insert_breakpoints ();
> -    }
> -}
> +  /* Restore the previously selected infrun state.  If the constructor
> +     didn't need to switch states, then nothing is done here either.  */
> +  ~scoped_switch_fork_info ()
> +  {
> +    if (m_oldfp != nullptr)
> +      {
> +	/* Switch back to inferior_ptid.  */
> +	remove_breakpoints ();
> +	fork_load_infrun_state (m_oldfp);
> +	insert_breakpoints ();
> +      }

When writing destructors, we need to keep in mind that if an
exception escapes, gdb is terminated on the spot.

Things aren't usually correct in the cleanup version either,
since an exception escaping while running cleanups leaves
the cleanup chain with dangling cleanups, but I think that
these conversions are the ideal time to fix it up.

The solution will usually be to swallow exceptions in the
dtor with try/catch(...) and try to limp along.

Thanks,
Pedro Alves

Re: [PATCH 5/6] gdb: Remove cleanup from linux-fork.c:inferior_call_waitpid

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2019-01-09 12:54:59 +0000]:

> On 01/01/2019 10:45 PM, Andrew Burgess wrote:
> 
> > +/* Temporarily switch to the infrun state stored on the fork_info
> > +   identified by a given ptid_t.  When this object goes out of scope,
> > +   restore the currently selected infrun state.   */
> > +
> > +class scoped_switch_fork_info
> >  {
> > -  struct fork_info *oldfp = (struct fork_info *) fp;
> > +public:
> > +  /* Switch to the infrun state held on the fork_info identified by
> > +     PPTID.  If PPTID is the current inferior then no switch is done.  */
> > +  scoped_switch_fork_info (ptid_t pptid)
> 
> (Nit, "explicit scoped_switch_fork_info")
> 
> > +    : m_oldfp (nullptr)
> > +  {
> > +    if (pptid != inferior_ptid)
> > +      {
> > +	struct fork_info *newfp = nullptr;
> > +
> > +	/* Switch to pptid.  */
> > +	m_oldfp = find_fork_ptid (inferior_ptid);
> > +	gdb_assert (m_oldfp != nullptr);
> > +	newfp = find_fork_ptid (pptid);
> > +	gdb_assert (newfp != nullptr);
> > +	fork_save_infrun_state (m_oldfp, 1);
> > +	remove_breakpoints ();
> > +	fork_load_infrun_state (newfp);
> > +	insert_breakpoints ();
> > +      }
> > +  }
> >  
> > -  if (oldfp)
> > -    {
> > -      /* Switch back to inferior_ptid.  */
> > -      remove_breakpoints ();
> > -      fork_load_infrun_state (oldfp);
> > -      insert_breakpoints ();
> > -    }
> > -}
> > +  /* Restore the previously selected infrun state.  If the constructor
> > +     didn't need to switch states, then nothing is done here either.  */
> > +  ~scoped_switch_fork_info ()
> > +  {
> > +    if (m_oldfp != nullptr)
> > +      {
> > +	/* Switch back to inferior_ptid.  */
> > +	remove_breakpoints ();
> > +	fork_load_infrun_state (m_oldfp);
> > +	insert_breakpoints ();
> > +      }
> 
> When writing destructors, we need to keep in mind that if an
> exception escapes, gdb is terminated on the spot.
> 
> Things aren't usually correct in the cleanup version either,
> since an exception escaping while running cleanups leaves
> the cleanup chain with dangling cleanups, but I think that
> these conversions are the ideal time to fix it up.
> 
> The solution will usually be to swallow exceptions in the
> dtor with try/catch(...) and try to limp along.

Is it worth using `internal_warning` rather than silently dropping the
exception?

Thanks,
Andrew

Re: [PATCH 5/6] gdb: Remove cleanup from linux-fork.c:inferior_call_waitpid

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2019-01-09 13:33:23 +0000]:

> * Pedro Alves <palves@redhat.com> [2019-01-09 12:54:59 +0000]:
> 
> > On 01/01/2019 10:45 PM, Andrew Burgess wrote:
> > 
> > > +/* Temporarily switch to the infrun state stored on the fork_info
> > > +   identified by a given ptid_t.  When this object goes out of scope,
> > > +   restore the currently selected infrun state.   */
> > > +
> > > +class scoped_switch_fork_info
> > >  {
> > > -  struct fork_info *oldfp = (struct fork_info *) fp;
> > > +public:
> > > +  /* Switch to the infrun state held on the fork_info identified by
> > > +     PPTID.  If PPTID is the current inferior then no switch is done.  */
> > > +  scoped_switch_fork_info (ptid_t pptid)
> > 
> > (Nit, "explicit scoped_switch_fork_info")
> > 
> > > +    : m_oldfp (nullptr)
> > > +  {
> > > +    if (pptid != inferior_ptid)
> > > +      {
> > > +	struct fork_info *newfp = nullptr;
> > > +
> > > +	/* Switch to pptid.  */
> > > +	m_oldfp = find_fork_ptid (inferior_ptid);
> > > +	gdb_assert (m_oldfp != nullptr);
> > > +	newfp = find_fork_ptid (pptid);
> > > +	gdb_assert (newfp != nullptr);
> > > +	fork_save_infrun_state (m_oldfp, 1);
> > > +	remove_breakpoints ();
> > > +	fork_load_infrun_state (newfp);
> > > +	insert_breakpoints ();
> > > +      }
> > > +  }
> > >  
> > > -  if (oldfp)
> > > -    {
> > > -      /* Switch back to inferior_ptid.  */
> > > -      remove_breakpoints ();
> > > -      fork_load_infrun_state (oldfp);
> > > -      insert_breakpoints ();
> > > -    }
> > > -}
> > > +  /* Restore the previously selected infrun state.  If the constructor
> > > +     didn't need to switch states, then nothing is done here either.  */
> > > +  ~scoped_switch_fork_info ()
> > > +  {
> > > +    if (m_oldfp != nullptr)
> > > +      {
> > > +	/* Switch back to inferior_ptid.  */
> > > +	remove_breakpoints ();
> > > +	fork_load_infrun_state (m_oldfp);
> > > +	insert_breakpoints ();
> > > +      }
> > 
> > When writing destructors, we need to keep in mind that if an
> > exception escapes, gdb is terminated on the spot.
> > 
> > Things aren't usually correct in the cleanup version either,
> > since an exception escaping while running cleanups leaves
> > the cleanup chain with dangling cleanups, but I think that
> > these conversions are the ideal time to fix it up.
> > 
> > The solution will usually be to swallow exceptions in the
> > dtor with try/catch(...) and try to limp along.
> 
> Is it worth using `internal_warning` rather than silently dropping the
> exception?

Here's a patch using internal_warning.  If you don't think that's a
good idea then I can commit without internal_warning and place this
comment in the catch instead:

    CATCH (ex, RETURN_MASK_ERROR)
      {
        /* It's not clear how we should recover from an exception at
           this point, so for now ignore the error and push on.  */
      }

--

gdb: Improve scoped_switch_fork_info class

After committing this patch I got this feedback:

   https://sourceware.org/ml/gdb-patches/2019-01/msg00181.html

This patch makes the constructor of scoped_switch_fork_info explicit,
and wraps the core of the destructor in a TRY/CATCH block.

I've run this through the testsuite on X86-64/GNU Linux, however, this
code is not exercised, so this patch is untested.

gdb/ChangeLog:

	* linux-fork.c (scoped_switch_fork_info)
	<scoped_switch_fork_info>: Make explicit.
	<~scoped_switch_fork_info>: Wrap core in try/catch.
---
 gdb/ChangeLog    |  6 ++++++
 gdb/linux-fork.c | 18 ++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index f3231bae048..a71a429678c 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -446,7 +446,7 @@ class scoped_switch_fork_info
 public:
   /* Switch to the infrun state held on the fork_info identified by
      PPTID.  If PPTID is the current inferior then no switch is done.  */
-  scoped_switch_fork_info (ptid_t pptid)
+  explicit scoped_switch_fork_info (ptid_t pptid)
     : m_oldfp (nullptr)
   {
     if (pptid != inferior_ptid)
@@ -472,9 +472,19 @@ public:
     if (m_oldfp != nullptr)
       {
 	/* Switch back to inferior_ptid.  */
-	remove_breakpoints ();
-	fork_load_infrun_state (m_oldfp);
-	insert_breakpoints ();
+	TRY
+	  {
+	    remove_breakpoints ();
+	    fork_load_infrun_state (m_oldfp);
+	    insert_breakpoints ();
+	  }
+	CATCH (ex, RETURN_MASK_ERROR)
+	  {
+	    internal_warning (__FILE__, __LINE__,
+			      "error during scoped_switch_fork_info cleanup: %s",
+			      ex.message);
+	  }
+	END_CATCH
       }
   }
 
-- 
2.14.5

Re: [PATCH 1/6] gdb/testsuite: Better detection of auto-response at y/n prompts

[Simon Marchi]

On 2019-01-09 06:24, Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@polymtl.ca> [2019-01-09 00:03:49 -0500]:
> 
>> On 2019-01-07 04:00, Andrew Burgess wrote:
>> > * Simon Marchi <simon.marchi@ericsson.com> [2019-01-02 21:48:06 +0000]:
>> >
>> > > On 2019-01-01 5:45 p.m., Andrew Burgess wrote:
>> > > > I noticed that when running this test:
>> > > >
>> > > >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"
>> > > >
>> > > > I would occasionally see some UNRESOLVED test results like this:
>> > > >
>> > > >   (gdb)
>> > > >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
>> > > >   Expecting: ^(kill[
>> > > >   ]+)?(.*[
>> > > >   ]+[(]gdb[)]
>> > > >   [ ]*)
>> > > >   kill
>> > > >   &"kill\n"
>> > > >   ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
>> > > >   =thread-group-exited,id="i1"
>> > > >   ERROR: Got interactive prompt.
>> > > >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
>> > > >
>> > > > The problem appears to be that the expect buffer fills up to include
>> > > > the '(y or n)' prompt without including the following lines.
>> > > >
>> > > > The pattern supplied by the outer test script is looking for the
>> > > > following lines.  As the following lines are not present then expect
>> > > > matches on the interactive prompt case rather than the case for the
>> > > > user supplied pattern.
>> > > >
>> > > > The problem with this is that we are not really at an interactive
>> > > > prompt, GDB is providing an answer for us and then moving on.  When I
>> > > > examine a successful run of the test the output from GDB is identical,
>> > > > the only difference is where expect happens to buffer the output from
>> > > > GDB.
>> > > >
>> > > > This patch introduces a second check inside the 'y or n' prompt case,
>> > > > which gives expect a chance to refill its buffers and catches the
>> > > > 'answered Y; input ...' text.
>> > > >
>> > > > This second check is on a short 1 second timeout, I'm currently
>> > > > assuming that the auto-answer text will either already be in expect,
>> > > > or is waiting to be read in.  If after 1 second the auto-answer text
>> > > > is not seen then we assume that GDB really is waiting at an
>> > > > interactive prompt.
>> > > >
>> > > > With this patch in place I can now leave the following loop running
>> > > > indefinitely, where before it would fail usually after ~10
>> > > > iterations.
>> > >
>> > > For me it fails consistently the way you describe.
>> > >
>> > > > diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
>> > > > index d193592a843..48ea45d62c7 100644
>> > > > --- a/gdb/testsuite/lib/mi-support.exp
>> > > > +++ b/gdb/testsuite/lib/mi-support.exp
>> > > > @@ -834,9 +834,24 @@ proc mi_gdb_test { args } {
>> > > >  	     fail "$message"
>> > > >  	}
>> > > >  	 -re "\\(y or n\\) " {
>> > > > -	    send_gdb "n\n"
>> > > > -	    perror "Got interactive prompt."
>> > > > -	     fail "$message"
>> > > > +	    # If the expect buffer just happens to fill up to the 'y
>> > > > +	    # or n' prompt then we can end up in this case, even
>> > > > +	    # though GDB will automatically provide a response for us.
>> > > > +	    # We give expect another chance here to look for the auto
>> > > > +	    # answer text before declaring a fail.
>> > > > +	    set auto_response_seen 0
>> > > > +	    gdb_expect 1 {
>> > > > +		-re ".answered Y; input not from terminal." {
>> > > > +		    set auto_response_seen 1
>> > > > +		}
>> > > > +	    }
>> > > > +	    if { ! $auto_response_seen } {
>> > > > +		send_gdb "n\n"
>> > > > +		perror "Got interactive prompt."
>> > > > +		fail "$message"
>> > > > +	    } else {
>> > > > +		exp_continue
>> > > > +	    }
>> > > >  	}
>> > > >  	 eof {
>> > > >  	     perror "Process no longer exists"
>> > >
>> > > Do we need this stanza at all?  I understand that it can save some
>> > > time (avoid having
>> > > to wait for the timeout) if the MI interpreter suddenly starts
>> > > asking interactive
>> > > "y or n" questions (which it should not do), but since it's causing
>> > > this kind of trouble,
>> > > maybe we can just get rid of it?
>> >
>> > I don't see any real reason to keep it.  I tried deleting it and no
>> > tests seem to regress, and the previously unstable test is now stable.
>> >
>> > Is this OK to apply?
>> >
>> > Thanks,
>> > Andrew
>> >
>> > --
>> >
>> > gdb/testsuite: Remove interactive prompt case from mi_gdb_test
>> >
>> > I noticed that when running this test:
>> >
>> >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
>> > gdb.mi/mi-break.exp"
>> >
>> > I would occasionally see some UNRESOLVED test results like this:
>> >
>> >   (gdb)
>> >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
>> >   Expecting: ^(kill[
>> >   ]+)?(.*[
>> >   ]+[(]gdb[)]
>> >   [ ]*)
>> >   kill
>> >   &"kill\n"
>> >   ~"Kill the program being debugged? (y or n) [answered Y; input not
>> > from terminal]\n"
>> >   =thread-group-exited,id="i1"
>> >   ERROR: Got interactive prompt.
>> >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
>> >
>> > The problem appears to be that the expect buffer fills up to include
>> > the '(y or n)' prompt without including the following lines.
>> >
>> > The pattern supplied by the outer test script is looking for the
>> > following lines.  As the following lines are not present then expect
>> > matches on the interactive prompt case rather than the case for the
>> > user supplied pattern.
>> >
>> > The problem with this is that we are not really at an interactive
>> > prompt, GDB is providing an answer for us and then moving on.  When I
>> > examine a successful run of the test the output from GDB is identical,
>> > the only difference is where expect happens to buffer the output from
>> > GDB.
>> >
>> > This patch remove all special handling of the interactive prompt
>> > case.  This means that if we ever break GDB and start seeing an
>> > unexpected interactive prompt then tests will rely on a timeout to
>> > fail, instead of having dedicated interactive prompt detection, but
>> > this solves the problem that an auto-answered prompt looks very
>> > similar to an interactive prompt.
>> >
>> > With this patch in place I can now leave the following loop running
>> > indefinitely, where before it would fail usually after ~10
>> > iterations.
>> >
>> >   while make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
>> > gdb.mi/mi-break.exp"; \
>> >   do /bin/true; \
>> >   done
>> >
>> > gdb/testsuite/ChangeLog:
>> >
>> > 	* lib/mi-support.exp (mi_gdb_test): Remove interactive prompt
>> > 	case.
>> > ---
>> >  gdb/testsuite/ChangeLog          | 5 +++++
>> >  gdb/testsuite/lib/mi-support.exp | 5 -----
>> >  2 files changed, 5 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/gdb/testsuite/lib/mi-support.exp
>> > b/gdb/testsuite/lib/mi-support.exp
>> > index d193592a843..a58c4f6e119 100644
>> > --- a/gdb/testsuite/lib/mi-support.exp
>> > +++ b/gdb/testsuite/lib/mi-support.exp
>> > @@ -832,11 +832,6 @@ proc mi_gdb_test { args } {
>> >  	    send_gdb "\n"
>> >  	    perror "Window too small."
>> >  	     fail "$message"
>> > -	}
>> > -	 -re "\\(y or n\\) " {
>> > -	    send_gdb "n\n"
>> > -	    perror "Got interactive prompt."
>> > -	     fail "$message"
>> >  	}
>> >  	 eof {
>> >  	     perror "Process no longer exists"
>> 
>> 
>> Thanks, this LGTM.
> 
> Thanks, I pushed this patch.
>> 
>> Btw, what you think about those "fail"s following "perror"s?  It's my
>> understanding that perror throws an error, interrupting the execution 
>> flow,
>> so the fail is not recorded, right?
> 
> That's not what I see.  As an experiment I created the file
> gdb.base/perror-test.exp like this:
> 
>   perror "this is an error"
>   pass "this is a pass"
> 
> With this I see:
> 
>   ERROR: this is an error
> 
>   		=== gdb Summary ===
> 
>   # of unresolved testcases	1
> 
> With the unresolved being instead of the 'PASS'.  Also the return
> value from runtest is non-zero.
> 
> If I change perror-test.exp to this:
> 
>   perror "this is an error"
>   fail "this is a fail"
> 
> Then the results are similar:
> 
>   ERROR: this is an error
> 
>   		=== gdb Summary ===
> 
>   # of unresolved testcases	1
> 
> Again, the unresolved replaces the 'FAIL', and the exit value is
> non-zero.
> 
> Where it gets interesting is if I change perror-test.exp to this:
> 
>   perror "this is an error"
> 
> Then I get this result:
> 
>   ERROR: this is an error
> 
>   		=== gdb Summary ===
> 
> And alarmingly (maybe?) the exit value from runtest is 0, so it looks
> like the test has passed.
> 
> My conclusions then:
> 
>   - perror doesn't interrupt control flow,
>   - perror doesn't itself cause the test script to appear to fail,
>   - perror causes the next pass/fail (at least) to become unresolved,
> 
> Given that the perror calls we're looking at here are in mi_gdb_test,
> the entire pass/fail printing is wrapped up within this method, I
> don't think the unresolved-ness should leak out into the next test,
> so, in this case I think perror/fail is correct.

Ah, this is clearer when reading the documentation :)

https://www.gnu.org/software/dejagnu/manual/perror-procedure.html

It's a bit confusing, since it overrides the following pass/fail 
message.  If you don't know what perror does, it looks like the 
pass/fail is not reached...  Thanks for looking into it.

>> And a note, maybe you are already aware of its existence, but I think 
>> that
>> "make check-read1" could have been useful to reproduce the issue 
>> reliably
>> here.  It forces the read syscall to read 1 byte at the time, so you 
>> are
>> certain to reach a point where -re "\\(y or n\\) " matches.
> 
> I did not know this.  Might be worth testing the whole testsuite
> against this and look for regressions..... I'll add it to my (long)
> list.

At least, it can be a good practice to check the tests we are working 
with it.

Simon

Re: [PATCH 5/6] gdb: Remove cleanup from linux-fork.c:inferior_call_waitpid

[Pedro Alves]

On 01/09/2019 02:10 PM, Andrew Burgess wrote:
> * Andrew Burgess <andrew.burgess@embecosm.com> [2019-01-09 13:33:23 +0000]:
> 
>> * Pedro Alves <palves@redhat.com> [2019-01-09 12:54:59 +0000]:
>>
>>> On 01/01/2019 10:45 PM, Andrew Burgess wrote:

>>> When writing destructors, we need to keep in mind that if an
>>> exception escapes, gdb is terminated on the spot.
>>>
>>> Things aren't usually correct in the cleanup version either,
>>> since an exception escaping while running cleanups leaves
>>> the cleanup chain with dangling cleanups, but I think that
>>> these conversions are the ideal time to fix it up.
>>>
>>> The solution will usually be to swallow exceptions in the
>>> dtor with try/catch(...) and try to limp along.
>>
>> Is it worth using `internal_warning` rather than silently dropping the
>> exception?
> 
> Here's a patch using internal_warning.  If you don't think that's a
> good idea then I can commit without internal_warning and place this
> comment in the catch instead:

I don't think this would be an internal issue?  For example, something
external to GDB could SIGKILL the inferior process, and then calling lseek
in the inferior within fork_load_infrun_state, could throw, for example.

If we print something, it'd be pedantically better to not talk about
gdb functions.

Maybe something around:

    warning (_("Couldn't restore checkpoint state in %s: %s",
	     target_pid_to_str (fp->ptid), ex.message);

?

> 
>     CATCH (ex, RETURN_MASK_ERROR)

Use RETURN_MASK_ALL, since if a Quit/Ctrl-C escapes, RETURN_MASK_ERROR
won't catch it, and GDB dies.

Pedantically, raw C++ try/catch(...), would catch any kind of exception, even
non-GDB ones, but that would mean losing the ex object, unless you complicate
this some more.  We don't throw around non-GDB exceptions (maybe a
std::logic_error could be possible somewhere, but I'd call that a bug),
so I think
  CATCH (ex, RETURN_MASK_ALL) 
is good enough.

>       {
>         /* It's not clear how we should recover from an exception at
>            this point, so for now ignore the error and push on.  */
>       }
That's fine too.  It's what I done in other similar spots, though I confess
that I had assumed that warning() writes to gdb_stdout, and thus could
paginate and cause another exception (ctrl-c/Quit), but now that I look,
I see that warning() writes to gdb_stderr which doesn't paginate.
There's even a comment about that:

/* Print a warning message.  The first argument STRING is the warning
   message, used as an fprintf format string, the second is the
   va_list of arguments for that string.  A warning is unfiltered (not
   paginated) so that the user does not need to page through each
   screen full of warnings when there are lots of them.  */

Eh.  So yeah, a warning seems good.

Thanks,
Pedro Alves

Re: [PATCH 5/6] gdb: Remove cleanup from linux-fork.c:inferior_call_waitpid

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2019-01-09 18:44:54 +0000]:

> On 01/09/2019 02:10 PM, Andrew Burgess wrote:
> > * Andrew Burgess <andrew.burgess@embecosm.com> [2019-01-09 13:33:23 +0000]:
> > 
> >> * Pedro Alves <palves@redhat.com> [2019-01-09 12:54:59 +0000]:
> >>
> >>> On 01/01/2019 10:45 PM, Andrew Burgess wrote:
> 
> >>> When writing destructors, we need to keep in mind that if an
> >>> exception escapes, gdb is terminated on the spot.
> >>>
> >>> Things aren't usually correct in the cleanup version either,
> >>> since an exception escaping while running cleanups leaves
> >>> the cleanup chain with dangling cleanups, but I think that
> >>> these conversions are the ideal time to fix it up.
> >>>
> >>> The solution will usually be to swallow exceptions in the
> >>> dtor with try/catch(...) and try to limp along.
> >>
> >> Is it worth using `internal_warning` rather than silently dropping the
> >> exception?
> > 
> > Here's a patch using internal_warning.  If you don't think that's a
> > good idea then I can commit without internal_warning and place this
> > comment in the catch instead:
> 
> I don't think this would be an internal issue?  For example, something
> external to GDB could SIGKILL the inferior process, and then calling lseek
> in the inferior within fork_load_infrun_state, could throw, for example.
> 
> If we print something, it'd be pedantically better to not talk about
> gdb functions.
> 
> Maybe something around:
> 
>     warning (_("Couldn't restore checkpoint state in %s: %s",
> 	     target_pid_to_str (fp->ptid), ex.message);
> 
> ?
> 
> > 
> >     CATCH (ex, RETURN_MASK_ERROR)
> 
> Use RETURN_MASK_ALL, since if a Quit/Ctrl-C escapes, RETURN_MASK_ERROR
> won't catch it, and GDB dies.
> 
> Pedantically, raw C++ try/catch(...), would catch any kind of exception, even
> non-GDB ones, but that would mean losing the ex object, unless you complicate
> this some more.  We don't throw around non-GDB exceptions (maybe a
> std::logic_error could be possible somewhere, but I'd call that a bug),
> so I think
>   CATCH (ex, RETURN_MASK_ALL) 
> is good enough.
> 
> >       {
> >         /* It's not clear how we should recover from an exception at
> >            this point, so for now ignore the error and push on.  */
> >       }
> That's fine too.  It's what I done in other similar spots, though I confess
> that I had assumed that warning() writes to gdb_stdout, and thus could
> paginate and cause another exception (ctrl-c/Quit), but now that I look,
> I see that warning() writes to gdb_stderr which doesn't paginate.
> There's even a comment about that:
> 
> /* Print a warning message.  The first argument STRING is the warning
>    message, used as an fprintf format string, the second is the
>    va_list of arguments for that string.  A warning is unfiltered (not
>    paginated) so that the user does not need to page through each
>    screen full of warnings when there are lots of them.  */
> 
> Eh.  So yeah, a warning seems good.

I pushed this patch with the warning as you suggested.

General apology to the list - I fluffed the commit, and had to send a
small fix-up patch immediately afterwards.  Sorry for that.

Thanks,
Andrew