[PATCH 3/3] btrace: Remove ui_out cleanups

[PATCH 3/3] btrace: Remove ui_out cleanups

[Simon Marchi]

This patch replaces the cleanups that close the list and tuple of the
btrace instruction history output with ui_out_emit_tuple and
ui_out_emit_list.

This allows removing make_cleanup_ui_out_tuple_begin_end and
make_cleanup_ui_out_list_begin_end.

This patch (along with the previous ones in the series) was regtested on
the buildbot.

gdb/ChangeLog:

	* record-btrace.c (btrace_print_lines): Replace cleanup
	parameter with RAII equivalents.
	(btrace_insn_history): Replace cleanup with RAII equivalents.
	* ui-out.h (make_cleanup_ui_out_list_begin_end,
	make_cleanup_ui_out_tuple_begin_end): Remove.
	* ui-out.c (struct ui_out_end_cleanup_data, do_cleanup_end,
	make_cleanup_ui_out_end, make_cleanup_ui_out_tuple_begin_end,
	make_cleanup_ui_out_list_begin_end): Remove.
---
 gdb/record-btrace.c | 55 ++++++++++++++++++++++++-----------------------------
 gdb/ui-out.c        | 44 ------------------------------------------
 gdb/ui-out.h        |  8 --------
 3 files changed, 25 insertions(+), 82 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 15ce760f5a..ddd15c4781 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -620,26 +620,25 @@ btrace_find_line_range (CORE_ADDR pc)
 
 static void
 btrace_print_lines (struct btrace_line_range lines, struct ui_out *uiout,
-		    struct cleanup **ui_item_chain, int flags)
+		    gdb::optional<ui_out_emit_tuple> *src_and_asm_tuple,
+		    gdb::optional<ui_out_emit_list> *asm_list,
+		    int flags)
 {
   print_source_lines_flags psl_flags;
-  int line;
 
-  psl_flags = 0;
   if (flags & DISASSEMBLY_FILENAME)
     psl_flags |= PRINT_SOURCE_LINES_FILENAME;
 
-  for (line = lines.begin; line < lines.end; ++line)
+  for (int line = lines.begin; line < lines.end; ++line)
     {
-      if (*ui_item_chain != NULL)
-	do_cleanups (*ui_item_chain);
+      asm_list->reset ();
+      src_and_asm_tuple->reset ();
 
-      *ui_item_chain
-	= make_cleanup_ui_out_tuple_begin_end (uiout, "src_and_asm_line");
+      src_and_asm_tuple->emplace (uiout, "src_and_asm_line");
 
       print_source_lines (lines.symtab, line, line + 1, psl_flags);
 
-      make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
+      asm_list->emplace (uiout, "line_asm_insn");
     }
 }
 
@@ -652,28 +651,23 @@ btrace_insn_history (struct ui_out *uiout,
 		     const struct btrace_insn_iterator *end,
 		     gdb_disassembly_flags flags)
 {
-  struct cleanup *cleanups, *ui_item_chain;
-  struct gdbarch *gdbarch;
-  struct btrace_insn_iterator it;
-  struct btrace_line_range last_lines;
-
   DEBUG ("itrace (0x%x): [%u; %u)", (unsigned) flags,
 	 btrace_insn_number (begin), btrace_insn_number (end));
 
   flags |= DISASSEMBLY_SPECULATIVE;
 
-  gdbarch = target_gdbarch ();
-  last_lines = btrace_mk_line_range (NULL, 0, 0);
+  struct gdbarch *gdbarch = target_gdbarch ();
+  btrace_line_range last_lines = btrace_mk_line_range (NULL, 0, 0);
 
-  cleanups = make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
+  ui_out_emit_list list_emitter (uiout, "asm_insns");
 
-  /* UI_ITEM_CHAIN is a cleanup chain for the last source line and the
-     instructions corresponding to that line.  */
-  ui_item_chain = NULL;
+  gdb::optional<ui_out_emit_tuple> src_and_asm_tuple;
+  gdb::optional<ui_out_emit_list> asm_list;
 
   gdb_pretty_print_disassembler disasm (gdbarch);
 
-  for (it = *begin; btrace_insn_cmp (&it, end) != 0; btrace_insn_next (&it, 1))
+  for (btrace_insn_iterator it = *begin; btrace_insn_cmp (&it, end) != 0;
+         btrace_insn_next (&it, 1))
     {
       const struct btrace_insn *insn;
 
@@ -708,19 +702,22 @@ btrace_insn_history (struct ui_out *uiout,
 	      if (!btrace_line_range_is_empty (lines)
 		  && !btrace_line_range_contains_range (last_lines, lines))
 		{
-		  btrace_print_lines (lines, uiout, &ui_item_chain, flags);
+		  btrace_print_lines (lines, uiout, &src_and_asm_tuple,
+				      &asm_list, flags);
 		  last_lines = lines;
 		}
-	      else if (ui_item_chain == NULL)
+	      else if (!src_and_asm_tuple.has_value ())
 		{
-		  ui_item_chain
-		    = make_cleanup_ui_out_tuple_begin_end (uiout,
-							   "src_and_asm_line");
+		  gdb_assert (!asm_list.has_value ());
+
+		  src_and_asm_tuple.emplace (uiout, "src_and_asm_line");
+
 		  /* No source information.  */
-		  make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
+		  asm_list.emplace (uiout, "line_asm_insn");
 		}
 
-	      gdb_assert (ui_item_chain != NULL);
+	      gdb_assert (src_and_asm_tuple.has_value ());
+	      gdb_assert (asm_list.has_value ());
 	    }
 
 	  memset (&dinsn, 0, sizeof (dinsn));
@@ -733,8 +730,6 @@ btrace_insn_history (struct ui_out *uiout,
 	  disasm.pretty_print_insn (uiout, &dinsn, flags);
 	}
     }
-
-  do_cleanups (cleanups);
 }
 
 /* The to_insn_history method of target record-btrace.  */
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 0340a44a83..3648815090 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -438,50 +438,6 @@ ui_out::end (ui_out_type type)
   do_end (type);
 }
 
-struct ui_out_end_cleanup_data
-{
-  struct ui_out *uiout;
-  enum ui_out_type type;
-};
-
-static void
-do_cleanup_end (void *data)
-{
-  struct ui_out_end_cleanup_data *end_cleanup_data
-    = (struct ui_out_end_cleanup_data *) data;
-
-  end_cleanup_data->uiout->end (end_cleanup_data->type);
-  xfree (end_cleanup_data);
-}
-
-static struct cleanup *
-make_cleanup_ui_out_end (struct ui_out *uiout,
-			 enum ui_out_type type)
-{
-  struct ui_out_end_cleanup_data *end_cleanup_data;
-
-  end_cleanup_data = XNEW (struct ui_out_end_cleanup_data);
-  end_cleanup_data->uiout = uiout;
-  end_cleanup_data->type = type;
-  return make_cleanup (do_cleanup_end, end_cleanup_data);
-}
-
-struct cleanup *
-make_cleanup_ui_out_tuple_begin_end (struct ui_out *uiout,
-				     const char *id)
-{
-  uiout->begin (ui_out_type_tuple, id);
-  return make_cleanup_ui_out_end (uiout, ui_out_type_tuple);
-}
-
-struct cleanup *
-make_cleanup_ui_out_list_begin_end (struct ui_out *uiout,
-				    const char *id)
-{
-  uiout->begin (ui_out_type_list, id);
-  return make_cleanup_ui_out_end (uiout, ui_out_type_list);
-}
-
 void
 ui_out::field_int (const char *fldname, int value)
 {
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 1708542e7e..a415100d7e 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -66,14 +66,6 @@ enum ui_out_type
     ui_out_type_list
   };
 
-/* Compatibility wrappers.  */
-
-extern struct cleanup *make_cleanup_ui_out_list_begin_end (struct ui_out *uiout,
-							   const char *id);
-
-extern struct cleanup *make_cleanup_ui_out_tuple_begin_end (struct ui_out *uiout,
-							    const char *id);
-
 class ui_out
 {
  public:
-- 
2.16.1

[PATCH 2/3] btrace: Remove VEC cleanups

[Simon Marchi]

This patch replaces two VEC(tp_t) with std::vector<thread_info *>, which
allows to remove two cleanups.  To make it easier to map the old code to
the new code, I added the ordered_remove and unordered_remove functions,
which operate on std::vector and do the same as VEC's
ordered_remove/unordered_remove.

gdb/ChangeLog:

	* record-btrace.c (record_btrace_maybe_mark_async_event): Change
	parameter types to std::vector.  Use bool.
	(record_btrace_wait): Replace VEC(tp_t) with
	std::vector<thread_info *>.
	* common/gdb_vecs.h (unordered_remove, ordered_remove): New.
---
 gdb/common/gdb_vecs.h | 31 +++++++++++++++++++++++
 gdb/record-btrace.c   | 68 +++++++++++++++++++++++++--------------------------
 2 files changed, 64 insertions(+), 35 deletions(-)

diff --git a/gdb/common/gdb_vecs.h b/gdb/common/gdb_vecs.h
index 29db27a892..7318e53132 100644
--- a/gdb/common/gdb_vecs.h
+++ b/gdb/common/gdb_vecs.h
@@ -50,4 +50,35 @@ extern void dirnames_to_char_ptr_vec_append
 extern std::vector<gdb::unique_xmalloc_ptr<char>>
   dirnames_to_char_ptr_vec (const char *dirnames);
 
+/* Remove the element at position IX from VEC, not preserving the order of the
+   remaining elements.  Return the removed element.  */
+
+template <typename T>
+T
+unordered_remove (std::vector<T> &vec, typename std::vector<T>::size_type ix)
+{
+  gdb_assert (ix < vec.size ());
+
+  T removed = std::move (vec[ix]);
+  vec[ix] = std::move (vec.back ());
+  vec.pop_back ();
+
+  return removed;
+}
+
+/* Remove the element at position IX from VEC, preserving the order the
+   remaining elements.  Return the removed element.  */
+
+template <typename T>
+T
+ordered_remove (std::vector<T> &vec, typename std::vector<T>::size_type ix)
+{
+  gdb_assert (ix < vec.size ());
+
+  T removed = std::move (vec[ix]);
+  vec.erase (vec.begin () + ix);
+
+  return removed;
+}
+
 #endif /* GDB_VECS_H */
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index b04a7e5049..15ce760f5a 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2428,13 +2428,12 @@ DEF_VEC_P (tp_t);
 /* Announce further events if necessary.  */
 
 static void
-record_btrace_maybe_mark_async_event (const VEC (tp_t) *moving,
-				      const VEC (tp_t) *no_history)
+record_btrace_maybe_mark_async_event
+  (const std::vector<thread_info *> &moving,
+   const std::vector<thread_info *> &no_history)
 {
-  int more_moving, more_no_history;
-
-  more_moving = !VEC_empty (tp_t, moving);
-  more_no_history = !VEC_empty (tp_t, no_history);
+  bool more_moving = !moving.empty ();
+  bool more_no_history = !no_history.empty ();;
 
   if (!more_moving && !more_no_history)
     return;
@@ -2454,9 +2453,8 @@ static ptid_t
 record_btrace_wait (struct target_ops *ops, ptid_t ptid,
 		    struct target_waitstatus *status, int options)
 {
-  VEC (tp_t) *moving, *no_history;
-  struct thread_info *tp, *eventing;
-  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
+  std::vector<thread_info *> moving;
+  std::vector<thread_info *> no_history;
 
   DEBUG ("wait %s (0x%x)", target_pid_to_str (ptid), options);
 
@@ -2468,26 +2466,25 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
       return ops->to_wait (ops, ptid, status, options);
     }
 
-  moving = NULL;
-  no_history = NULL;
-
-  make_cleanup (VEC_cleanup (tp_t), &moving);
-  make_cleanup (VEC_cleanup (tp_t), &no_history);
-
   /* Keep a work list of moving threads.  */
-  ALL_NON_EXITED_THREADS (tp)
-    if (ptid_match (tp->ptid, ptid)
-	&& ((tp->btrace.flags & (BTHR_MOVE | BTHR_STOP)) != 0))
-      VEC_safe_push (tp_t, moving, tp);
+  {
+    thread_info *tp;
+
+    ALL_NON_EXITED_THREADS (tp)
+      {
+	if (ptid_match (tp->ptid, ptid)
+	    && ((tp->btrace.flags & (BTHR_MOVE | BTHR_STOP)) != 0))
+	  moving.push_back (tp);
+      }
+  }
 
-  if (VEC_empty (tp_t, moving))
+  if (moving.empty ())
     {
       *status = btrace_step_no_resumed ();
 
       DEBUG ("wait ended by %s: %s", target_pid_to_str (null_ptid),
 	     target_waitstatus_to_string (status).c_str ());
 
-      do_cleanups (cleanups);
       return null_ptid;
     }
 
@@ -2508,14 +2505,13 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
      nothing else to report.  By this time, all threads should have moved to
      either the beginning or the end of their execution history.  There will
      be a single user-visible stop.  */
-  eventing = NULL;
-  while ((eventing == NULL) && !VEC_empty (tp_t, moving))
+  struct thread_info *eventing = NULL;
+  while ((eventing == NULL) && !moving.empty ())
     {
-      unsigned int ix;
-
-      ix = 0;
-      while ((eventing == NULL) && VEC_iterate (tp_t, moving, ix, tp))
+      for (unsigned int ix = 0; eventing == NULL && ix < moving.size ();)
 	{
+	  thread_info *tp = moving[ix];
+
 	  *status = record_btrace_step_thread (tp);
 
 	  switch (status->kind)
@@ -2525,12 +2521,11 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
 	      break;
 
 	    case TARGET_WAITKIND_NO_HISTORY:
-	      VEC_safe_push (tp_t, no_history,
-			     VEC_ordered_remove (tp_t, moving, ix));
+	      no_history.push_back (ordered_remove (moving, ix));
 	      break;
 
 	    default:
-	      eventing = VEC_unordered_remove (tp_t, moving, ix);
+	      eventing = unordered_remove (moving, ix);
 	      break;
 	    }
 	}
@@ -2543,11 +2538,11 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
 
 	 In the former case, EVENTING must not be NULL.
 	 In the latter case, NO_HISTORY must not be empty.  */
-      gdb_assert (!VEC_empty (tp_t, no_history));
+      gdb_assert (!no_history.empty ());
 
       /* We kept threads moving at the end of their execution history.  Stop
 	 EVENTING now that we are going to report its stop.  */
-      eventing = VEC_unordered_remove (tp_t, no_history, 0);
+      eventing = unordered_remove (no_history, 0);
       eventing->btrace.flags &= ~BTHR_MOVE;
 
       *status = btrace_step_no_history ();
@@ -2561,8 +2556,12 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
 
   /* Stop all other threads. */
   if (!target_is_non_stop_p ())
-    ALL_NON_EXITED_THREADS (tp)
-      record_btrace_cancel_resume (tp);
+    {
+      thread_info *tp;
+
+      ALL_NON_EXITED_THREADS (tp)
+	record_btrace_cancel_resume (tp);
+    }
 
   /* In async mode, we need to announce further events.  */
   if (target_is_async_p ())
@@ -2579,7 +2578,6 @@ record_btrace_wait (struct target_ops *ops, ptid_t ptid,
 	 target_pid_to_str (eventing->ptid),
 	 target_waitstatus_to_string (status).c_str ());
 
-  do_cleanups (cleanups);
   return eventing->ptid;
 }
 
-- 
2.16.1

[PATCH 1/3] btrace: Remove btrace disable cleanup

[Simon Marchi]

This patch removes a cleanup that disables btrace on threads in case of
failure, so we don't leave it enabled for some the threads and disabled
for the rest.

gdb/ChangeLog:

	* record-btrace.c (record_btrace_disable_callback): Remove.
	(struct scoped_btrace_disable): New.
	(record_btrace_open): Use scoped_btrace_disable.
---
 gdb/record-btrace.c | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index d9a1dc593f..b04a7e5049 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -159,16 +159,6 @@ record_btrace_enable_warn (struct thread_info *tp)
   END_CATCH
 }
 
-/* Callback function to disable branch tracing for one thread.  */
-
-static void
-record_btrace_disable_callback (void *arg)
-{
-  struct thread_info *tp = (struct thread_info *) arg;
-
-  btrace_disable (tp);
-}
-
 /* Enable automatic tracing of new threads.  */
 
 static void
@@ -223,12 +213,42 @@ record_btrace_push_target (void)
   observer_notify_record_changed (current_inferior (), 1, "btrace", format);
 }
 
+/* Disable btrace on a set of threads on scope exit.  */
+
+struct scoped_btrace_disable
+{
+  scoped_btrace_disable () = default;
+
+  DISABLE_COPY_AND_ASSIGN (scoped_btrace_disable);
+
+  ~scoped_btrace_disable ()
+  {
+    for (thread_info *tp : m_threads)
+      btrace_disable (tp);
+  }
+
+  void add_thread (thread_info *thread)
+  {
+    m_threads.push_front (thread);
+  }
+
+  void discard ()
+  {
+    m_threads.clear ();
+  }
+
+private:
+  std::forward_list<thread_info *> m_threads;
+};
+
 /* The to_open method of target record-btrace.  */
 
 static void
 record_btrace_open (const char *args, int from_tty)
 {
-  struct cleanup *disable_chain;
+  /* If we fail to enable btrace for one thread, disable it for the threads for
+     which it was successfully enabled.  */
+  scoped_btrace_disable btrace_disable;
   struct thread_info *tp;
 
   DEBUG ("open");
@@ -240,18 +260,17 @@ record_btrace_open (const char *args, int from_tty)
 
   gdb_assert (record_btrace_thread_observer == NULL);
 
-  disable_chain = make_cleanup (null_cleanup, NULL);
   ALL_NON_EXITED_THREADS (tp)
     if (args == NULL || *args == 0 || number_is_in_list (args, tp->global_num))
       {
 	btrace_enable (tp, &record_btrace_conf);
 
-	make_cleanup (record_btrace_disable_callback, tp);
+	btrace_disable.add_thread (tp);
       }
 
   record_btrace_push_target ();
 
-  discard_cleanups (disable_chain);
+  btrace_disable.discard ();
 }
 
 /* The to_stop_recording method of target record-btrace.  */
-- 
2.16.1

RE: [PATCH 2/3] btrace: Remove VEC cleanups

[Metzger, Markus T]

Hello Simon,

> This patch replaces two VEC(tp_t) with std::vector<thread_info *>, which
> allows to remove two cleanups.  To make it easier to map the old code to
> the new code, I added the ordered_remove and unordered_remove functions,
> which operate on std::vector and do the same as VEC's
> ordered_remove/unordered_remove.

Looks good to me.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 3/3] btrace: Remove ui_out cleanups

[Metzger, Markus T]

Hello Simon,


> This patch replaces the cleanups that close the list and tuple of the
> btrace instruction history output with ui_out_emit_tuple and
> ui_out_emit_list.
> 
> This allows removing make_cleanup_ui_out_tuple_begin_end and
> make_cleanup_ui_out_list_begin_end.
> 
> This patch (along with the previous ones in the series) was regtested on
> the buildbot.


> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 15ce760f5a..ddd15c4781 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -620,26 +620,25 @@ btrace_find_line_range (CORE_ADDR pc)
> 
>  static void
>  btrace_print_lines (struct btrace_line_range lines, struct ui_out *uiout,
> -		    struct cleanup **ui_item_chain, int flags)
> +		    gdb::optional<ui_out_emit_tuple> *src_and_asm_tuple,
> +		    gdb::optional<ui_out_emit_list> *asm_list,

Reference instead of pointer?

 
> -  for (line = lines.begin; line < lines.end; ++line)
> +  for (int line = lines.begin; line < lines.end; ++line)
>      {
> -      if (*ui_item_chain != NULL)
> -	do_cleanups (*ui_item_chain);
> +      asm_list->reset ();
> +      src_and_asm_tuple->reset ();

The SRC_AND_ASM_TUPLE reset () shouldn't be necessary; it is reset () in emplace ().


> -      *ui_item_chain
> -	= make_cleanup_ui_out_tuple_begin_end (uiout, "src_and_asm_line");
> +      src_and_asm_tuple->emplace (uiout, "src_and_asm_line");
> 
>        print_source_lines (lines.symtab, line, line + 1, psl_flags);
> 
> -      make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
> +      asm_list->emplace (uiout, "line_asm_insn");
>      }
>  }

Looks good to me, otherwise.

I have not tested it, though.  Did you run the gdb.btrace test suite?  You said
you ran buildbot but that might skip gdb.btrace when run on VMs or on old
hardware.

Please let me know if you want me to run the gdb.btrace tests for you.  A user
branch would be nice in that case.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 1/3] btrace: Remove btrace disable cleanup

[Metzger, Markus T]

Hello Simon,


> This patch removes a cleanup that disables btrace on threads in case of
> failure, so we don't leave it enabled for some the threads and disabled
> for the rest.
> 
> gdb/ChangeLog:
> 
> 	* record-btrace.c (record_btrace_disable_callback): Remove.
> 	(struct scoped_btrace_disable): New.
> 	(record_btrace_open): Use scoped_btrace_disable.

Looks good to me.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 3/3] btrace: Remove ui_out cleanups

[Simon Marchi]

On 2018-03-05 07:39, Metzger, Markus T wrote:
> Hello Simon,
> 
> 
>> This patch replaces the cleanups that close the list and tuple of the
>> btrace instruction history output with ui_out_emit_tuple and
>> ui_out_emit_list.
>> 
>> This allows removing make_cleanup_ui_out_tuple_begin_end and
>> make_cleanup_ui_out_list_begin_end.
>> 
>> This patch (along with the previous ones in the series) was regtested 
>> on
>> the buildbot.
> 
> 
>> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
>> index 15ce760f5a..ddd15c4781 100644
>> --- a/gdb/record-btrace.c
>> +++ b/gdb/record-btrace.c
>> @@ -620,26 +620,25 @@ btrace_find_line_range (CORE_ADDR pc)
>> 
>>  static void
>>  btrace_print_lines (struct btrace_line_range lines, struct ui_out 
>> *uiout,
>> -		    struct cleanup **ui_item_chain, int flags)
>> +		    gdb::optional<ui_out_emit_tuple> *src_and_asm_tuple,
>> +		    gdb::optional<ui_out_emit_list> *asm_list,
> 
> Reference instead of pointer?

I once pointed this out on one of Tom's patches, and he said that in the 
caller code, it's more obvious that object is meant to be modified if 
you do:

   function_that_modifies_object (&object);

instead of

   function_that_modifies_object (object);

And I kind of agree with that it is, which is why I've been using 
pointers when references would have worked (ok, here the function name 
makes it obvious but it's not always the case).  In the implementation 
of the function, since you use object->field instead of object.field, it 
also hints that you're not modifying a local object.  But I also agree 
that it's really not C++-y to do it this way, so I'll happily change it.

>> -  for (line = lines.begin; line < lines.end; ++line)
>> +  for (int line = lines.begin; line < lines.end; ++line)
>>      {
>> -      if (*ui_item_chain != NULL)
>> -	do_cleanups (*ui_item_chain);
>> +      asm_list->reset ();
>> +      src_and_asm_tuple->reset ();
> 
> The SRC_AND_ASM_TUPLE reset () shouldn't be necessary; it is reset ()
> in emplace ().

Ok.

>> -      *ui_item_chain
>> -	= make_cleanup_ui_out_tuple_begin_end (uiout, "src_and_asm_line");
>> +      src_and_asm_tuple->emplace (uiout, "src_and_asm_line");
>> 
>>        print_source_lines (lines.symtab, line, line + 1, psl_flags);
>> 
>> -      make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
>> +      asm_list->emplace (uiout, "line_asm_insn");
>>      }
>>  }
> 
> Looks good to me, otherwise.
> 
> I have not tested it, though.  Did you run the gdb.btrace test suite?  
> You said
> you ran buildbot but that might skip gdb.btrace when run on VMs or on 
> old
> hardware.
> 
> Please let me know if you want me to run the gdb.btrace tests for you.  
> A user
> branch would be nice in that case.

Yes, I have ran the gdb.btrace/*.exp tests locally on two different 
machines and saw no regressions.  However, the processors may be a bit 
old (Q6600 from 2007 and i5-4310U from 2014), so it's possible that not 
all required features are available, and therefore some tests may be 
skipped.  So if you want to be sure, here's a branch for you to test:

users/simark/btrace-cleanups
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/btrace-cleanups

Thanks!

Simon

RE: [PATCH 3/3] btrace: Remove ui_out cleanups

[Metzger, Markus T]

Hello Simon,

> >>  static void
> >>  btrace_print_lines (struct btrace_line_range lines, struct ui_out
> >> *uiout,
> >> -		    struct cleanup **ui_item_chain, int flags)
> >> +		    gdb::optional<ui_out_emit_tuple> *src_and_asm_tuple,
> >> +		    gdb::optional<ui_out_emit_list> *asm_list,
> >
> > Reference instead of pointer?
> 
> I once pointed this out on one of Tom's patches, and he said that in the
> caller code, it's more obvious that object is meant to be modified if
> you do:
> 
>    function_that_modifies_object (&object);
> 
> instead of
> 
>    function_that_modifies_object (object);
> 
> And I kind of agree with that it is, which is why I've been using
> pointers when references would have worked (ok, here the function name
> makes it obvious but it's not always the case).  In the implementation
> of the function, since you use object->field instead of object.field, it
> also hints that you're not modifying a local object.  But I also agree
> that it's really not C++-y to do it this way, so I'll happily change it.

I prefer consistency.  I we agreed to use pointers instead of references
in other parts of GDB, let's do so everywhere.


> Yes, I have ran the gdb.btrace/*.exp tests locally on two different
> machines and saw no regressions.  However, the processors may be a bit
> old (Q6600 from 2007 and i5-4310U from 2014), so it's possible that not
> all required features are available, and therefore some tests may be
> skipped.  So if you want to be sure, here's a branch for you to test:

You would get an "untested" if btrace tests are skipped.  As long as
you're not getting all "untested", you should be fine.  There is only
one test, tsx.exp, that requires recent hardware and compiler.

It would use the method that is available on your target preferring
PT over BTS.  But this change is not related to trace decode so it
shouldn't matter.

I ran the tests on recent hardware using PT and everything passes.

Thanks,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 3/3] btrace: Remove ui_out cleanups

[Simon Marchi]

On 2018-03-06 02:30, Metzger, Markus T wrote:
> I prefer consistency.  I we agreed to use pointers instead of 
> references
> in other parts of GDB, let's do so everywhere.

There was no formal decision, I would just say it's the current trend.  
But it would be a good idea to formalize it, so we don't have to wonder 
about it again, I'll send a proposal in a separate mail.  For reference, 
I checked the Google C++ style guide, and they forbid non-const 
reference:

https://google.github.io/styleguide/cppguide.html#Reference_Arguments

I'll push this patch with pointers then.

>> Yes, I have ran the gdb.btrace/*.exp tests locally on two different
>> machines and saw no regressions.  However, the processors may be a bit
>> old (Q6600 from 2007 and i5-4310U from 2014), so it's possible that 
>> not
>> all required features are available, and therefore some tests may be
>> skipped.  So if you want to be sure, here's a branch for you to test:
> 
> You would get an "untested" if btrace tests are skipped.  As long as
> you're not getting all "untested", you should be fine.  There is only
> one test, tsx.exp, that requires recent hardware and compiler.
> 
> It would use the method that is available on your target preferring
> PT over BTS.  But this change is not related to trace decode so it
> shouldn't matter.
> 
> I ran the tests on recent hardware using PT and everything passes.

Ok, thanks!

Simon