RE: [PATCH 4/5] [func_call] Add function-call-history-length command to MI.

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Ari,

>diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
>index cd7cabdda9..945a439d3a 100644
>--- a/gdb/mi/mi-cmds.c
>+++ b/gdb/mi/mi-cmds.c
>@@ -227,11 +227,11 @@ add_builtin_mi_commands ()
>   add_mi_cmd_mi ("catch-unload", mi_cmd_catch_unload,
> 		 &mi_suppress_notification.breakpoint);
>   add_mi_cmd_mi ("catch-throw", mi_cmd_catch_throw,
>-                 &mi_suppress_notification.breakpoint),
>+                 &mi_suppress_notification.breakpoint);
>   add_mi_cmd_mi ("catch-rethrow", mi_cmd_catch_rethrow,
>-                 &mi_suppress_notification.breakpoint),
>+                 &mi_suppress_notification.breakpoint);
>   add_mi_cmd_mi ("catch-catch", mi_cmd_catch_catch,
>-                 &mi_suppress_notification.breakpoint),
>+                 &mi_suppress_notification.breakpoint);

Those changes appear to be unrelated.  You may want to
send them in a separate patch.

>   add_mi_cmd_mi ("complete", mi_cmd_complete);
>   add_mi_cmd_mi ("data-disassemble", mi_cmd_disassemble);
>   add_mi_cmd_mi ("data-evaluate-expression",
>mi_cmd_data_evaluate_expression);
>@@ -275,10 +275,12 @@ add_builtin_mi_commands ()
>   add_mi_cmd_mi ("file-list-exec-source-files",
> 		 mi_cmd_file_list_exec_source_files);
>   add_mi_cmd_mi ("file-list-shared-libraries",
>-     mi_cmd_file_list_shared_libraries),
>+     mi_cmd_file_list_shared_libraries);

Those, as well.

>@@ -1221,6 +1222,29 @@ btrace_call_history (struct ui_out *uiout,
>     }
> }
>
>+/* The call_history_length method of target record-btrace.  */
>+
>+void
>+record_btrace_target::call_history_length ()
>+{
>+  struct btrace_call_iterator end;
>+  btrace_call_end (&end, require_btrace ());
>+
>+  const int length = btrace_call_number (&end) - 1;

You'd want to check that the trace is not empty.

Also, the end iterator points one beyond the last function.

There is similar code in record_btrace_target::info_record().
Let's extract that into a separate function so we're sure we
report the same numbers in 'info record' and here.


>+
>+  if (current_uiout->is_mi_like_p ())
>+    {
>+      ui_out_emit_list list_emitter (current_uiout, "func history length");
>+      ui_out_emit_tuple tuple_emitter (current_uiout, nullptr);
>+      current_uiout->field_unsigned ("end", length);
>+    }
>+  else
>+    {
>+      ui_out_emit_tuple tuple_emitter (current_uiout, "func history length");
>+      current_uiout->field_unsigned ("end", length);
>+    }

Could we use the same code for both MI and CLI?
Should the field be called 'length' instead of 'end'?


>diff --git a/gdb/record.h b/gdb/record.h
>index 0fbca9d44c..174b71c67c 100644
>--- a/gdb/record.h
>+++ b/gdb/record.h
>@@ -114,4 +114,10 @@ extern void record_start (const char *method, const
>char *format,
> /* Stop recording.  Throw on failure.  */
> extern void record_stop (int from_tty);
>
>+/* The MI version of the command to get the length of the function call history
>+   for record targets.  */
>+extern void mi_cmd_record_function_call_history_length (char *command,
>+							char **argv,
>+							int argc);
>+
> #endif /* RECORD_H */

Why would this go into record.[hc] and not, say, some gdb/mi/mi-cmd-*.[hc]?


>diff --git a/gdb/testsuite/gdb.mi/mi-function_call_history.exp
>b/gdb/testsuite/gdb.mi/mi-function_call_history.exp
>new file mode 100644
>index 0000000000..948a04a5ed
>--- /dev/null
>+++ b/gdb/testsuite/gdb.mi/mi-function_call_history.exp
>@@ -0,0 +1,88 @@
>+# This testcase is part of GDB, the GNU debugger.
>+#
>+# Copyright 2013-2022 Free Software Foundation, Inc.
>+#
>+# This program is free software; you can redistribute it and/or modify
>+# it under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# This program is distributed in the hope that it will be useful,
>+# but WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with this program.  If not, see <http://www.gnu.org/licenses/>.

Would you please briefly comment what this test does?

>+set errorInfo
>+
>+if [skip_btrace_tests] {

There should be an untested message.

>+    return 0
>+}
>+
>+load_lib mi-support.exp
>+set MIFLAGS "-i=mi"
>+
>+gdb_exit
>+if [mi_gdb_start] {
>+    continue
>+}
>+
>+# start inferior
>+standard_testfile mi-function_call_history.c
>+
>+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}]
>!= "" } {

I think the default should be to use prepare_for_testing.

>+     untested mi-function_call_history.exp
>+     return -1
>+}
>+
>+mi_delete_breakpoints
>+mi_gdb_reinitialize_dir $srcdir/$subdir
>+mi_gdb_load ${binfile}
>+
>+if {[mi_runto_main] < 0} {
>+    return -1
>+}
>+
>+# start btrace
>+send_gdb "record btrace\n"
>+
>+gdb_expect {
>+    -re "=record-started,thread-group=\".*\"\r\n\\^done\r\n$mi_gdb_prompt$" {
>+	# Done
>+    }
>+    -re "\\^error,msg=\"Target does not support branch tracing.\"" {
>+	perror "Branch tracing not supported"
>+	return -1
>+    }
>+    timeout {
>+	perror "Unable to start branch tracing"
>+	return -1
>+    }
>+}

Would this work with gdb_test_multiple instead of separate send and expect?

Looks like this might be used more often if we want to test btrace over MI.
Would it make sense to put this into some mi_gdb_record function?

>+
>+mi_create_breakpoint "-t $testfile.c:41" "insert temp breakpoint at
>$testfile.c:41" \
>+    -number 2 -disp del -func main -file "$srcdir/$subdir/$testfile.c" -line 41
>+
>+mi_send_resuming_command "exec-continue" "continue to bp.1"
>+
>+mi_expect_stop "breakpoint-hit" "main" ".*" "$testfile.c" 41 \
>+    {"" "disp=\".*\"" } "run to breakpoint bp.1"
>+
>+mi_gdb_test "125-function-call-history-length" \
>+    "125\\^done,func history length=\\\[\{end=\"21\"\}\\\]" \
>+    "125 function call history length"
>+
>+mi_create_breakpoint "-t $testfile.c:42" "insert temp breakpoint at
>$testfile.c:42" \
>+    -number 3 -disp del -func main -file "$srcdir/$subdir/$testfile.c" -line 42
>+
>+mi_send_resuming_command "exec-continue" "continue to bp.2"
>+
>+mi_expect_stop "breakpoint-hit" "main" ".*" "$testfile.c" 42 \
>+    {"" "disp=\".*\"" } "run to breakpoint bp.2"
>+
>+mi_gdb_test "225-function-call-history-length" \
>+    "225\\^done,func history length=\\\[\{end=\"375\"\}\\\]" \
>+    "225 function call history length"
>+
>+mi_gdb_exit
>--
>2.25.1

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928