[PATCH] Handle dprintf argument evaluation errors better (PR gdb/16551)

[PATCH] Handle dprintf argument evaluation errors better (PR gdb/16551)

[Simon Marchi]

The current behavior when GDB fails to evaluate the arguments of a
dynamic printf is not very good.

There was a previous attempt at fixing this here, but it did not went
through:
  https://sourceware.org/ml/gdb-patches/2015-02/msg00258.html

The issue can be reproduced by setting a dprintf referring to a variable
that doesn't exist or that triggers a memory error:

  dprintf hello, "hello %d\n", *((int*)0)
  dprintf hello, "hello %d\n", doesnt_exist

When an evaluation error occurs, an error is thrown at the stack trace
shown below and is caught in start_event_loop.  This leaves things in a
relatively bad shape:

- Printing the error in start_event_loop causes GDB to receive a SIGTTOU
  signal, because the terminal is still owned by the inferior at that
  point.
- There is an error message (e.g. No symbol "foo" in current context)
  and you are back to the GDB prompt, but nothing gives a clue about the
  context of the error.
- The thread that hit the dprintf is stopped.  If in all-stop mode on an
  all-stop-on-top-of-non-stop target, you end up with a single thread
  stopped and the others running, which is not good.
- With MI, the thread(s) is/are stopped but no *stopped event is sent,
  so frontends still show it as running.

I actually think it is nice that the program stops if there is an
error, so you can notice the problem and fix it.  It just needs to be
handled better.  This patch makes GDB catch the evaluation error in the
dprintf_after_condition_true function, and sets bpstat::stop to 1/true,
so that the dprintf will cause a stop similar to a breakpoint hit.  The
dprintf_print_it function defines how a "dprintf error hit" is printed.
The result looks like this:

  (gdb) c
  Continuing.

  Dprintf 2, failed to evaluate: No symbol "lalala" in current context.
  foo (n=1) at /home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/dprintf-error.c:30

When using MI, the stop is communicated using a new reason
"dprintf-error", and the error-message field gives the text of the
exception:

  *stopped,reason="dprintf-error",bkptno="2",error-message="No symbol \"lalala\" in current context.",frame=...

  #0  error (fmt=0x12bad88 "No symbol \"%s\" in current context.") at /home/emaisin/src/binutils-gdb/gdb/common/errors.c:39
  #1  0x00000000006f5d49 in c_yyparse () at /home/emaisin/src/binutils-gdb/gdb/c-exp.y:1090
  #2  0x00000000006fc082 in c_parse (par_state=0x7ffd3ed91c50) at /home/emaisin/src/binutils-gdb/gdb/c-exp.y:3273
  #3  0x0000000000979d88 in parse_exp_in_context_1 (stringptr=0x7ffd3ed91e00, pc=0, block=0x0, comma=1, void_context_p=0, out_subexp=0x0) at /home/emaisin/src/binutils-gdb/gdb/parse.c:1205
  #4  0x0000000000979aae in parse_exp_in_context (stringptr=0x7ffd3ed91e00, pc=0, block=0x0, comma=1, void_context_p=0, out_subexp=0x0) at /home/emaisin/src/binutils-gdb/gdb/parse.c:1108
  #5  0x0000000000979a2d in parse_exp_1 (stringptr=0x7ffd3ed91e00, pc=0, block=0x0, comma=1) at /home/emaisin/src/binutils-gdb/gdb/parse.c:1099
  #6  0x000000000089ec1d in parse_to_comma_and_eval (expp=0x7ffd3ed91e00) at /home/emaisin/src/binutils-gdb/gdb/eval.c:126
  #7  0x0000000000981a84 in ui_printf (arg=0x307f377 "\"hello %d\\n\", lalala", stream=0x2f34b90) at /home/emaisin/src/binutils-gdb/gdb/printcmd.c:2464
  #8  0x000000000098212a in printf_command (arg=0x307f377 "\"hello %d\\n\", lalala", from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/printcmd.c:2580
  #9  0x0000000000603808 in do_const_cfunc (c=0x2ee6b00, args=0x307f377 "\"hello %d\\n\", lalala", from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/cli/cli-decode.c:106
  #10 0x0000000000606900 in cmd_func (cmd=0x2ee6b00, args=0x307f377 "\"hello %d\\n\", lalala", from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/cli/cli-decode.c:1857
  #11 0x0000000000a61415 in execute_command (p=0x307f38a "a", from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/top.c:630
  #12 0x00000000006106f6 in execute_control_command_1 (cmd=0x2fd6f30, from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/cli/cli-script.c:525
  #13 0x0000000000610d2a in execute_control_command (cmd=0x2fd6f30, from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/cli/cli-script.c:694
  #14 0x000000000077e83f in bpstat_do_actions_1 (bsp=0x7ffd3ed922e8) at /home/emaisin/src/binutils-gdb/gdb/breakpoint.c:4433
  #15 0x00000000007920e1 in dprintf_after_condition_true (bs=0x2fe9260) at /home/emaisin/src/binutils-gdb/gdb/breakpoint.c:13035
  #16 0x000000000078057c in bpstat_stop_status (aspace=0x2f169e0, bp_addr=4195559, ptid=..., ws=0x7ffd3ed927a0, stop_chain=0x2fe9260) at /home/emaisin/src/binutils-gdb/gdb/breakpoint.c:5460
  #17 0x0000000000908760 in handle_signal_stop (ecs=0x7ffd3ed92780) at /home/emaisin/src/binutils-gdb/gdb/infrun.c:5946
  #18 0x0000000000907463 in handle_inferior_event_1 (ecs=0x7ffd3ed92780) at /home/emaisin/src/binutils-gdb/gdb/infrun.c:5375
  #19 0x00000000009075aa in handle_inferior_event (ecs=0x7ffd3ed92780) at /home/emaisin/src/binutils-gdb/gdb/infrun.c:5410
  #20 0x000000000090449a in fetch_inferior_event (client_data=0x0) at /home/emaisin/src/binutils-gdb/gdb/infrun.c:3924
  #21 0x00000000008efe47 in inferior_event_handler (event_type=INF_REG_EVENT, client_data=0x0) at /home/emaisin/src/binutils-gdb/gdb/inf-loop.c:43
  #22 0x000000000090ef4b in infrun_async_inferior_event_handler (data=0x0) at /home/emaisin/src/binutils-gdb/gdb/infrun.c:9164
  #23 0x00000000008aaa43 in check_async_event_handlers () at /home/emaisin/src/binutils-gdb/gdb/event-loop.c:1064
  #24 0x00000000008a9375 in gdb_do_one_event () at /home/emaisin/src/binutils-gdb/gdb/event-loop.c:326
  #25 0x00000000008a9426 in start_event_loop () at /home/emaisin/src/binutils-gdb/gdb/event-loop.c:371
  #26 0x000000000093d38f in captured_command_loop () at /home/emaisin/src/binutils-gdb/gdb/main.c:330
  #27 0x000000000093e87a in captured_main (data=0x7ffd3ed929e0) at /home/emaisin/src/binutils-gdb/gdb/main.c:1157
  #28 0x000000000093e945 in gdb_main (args=0x7ffd3ed929e0) at /home/emaisin/src/binutils-gdb/gdb/main.c:1173
  #29 0x0000000000411f5c in main (argc=10, argv=0x7ffd3ed92ae8) at /home/emaisin/src/binutils-gdb/gdb/gdb.c:32

gdb/ChangeLog:

	PR gdb/16551
	* breakpoint.h (struct bpstats) <dprintf_error>: New field.
	* breakpoint.c (bpstat_what): Make dprintf stops noisy.
	(dprintf_print_it): New function.
	(dprintf_after_condition_true): Catch evaluation errors, set
	bs->stop on error.
	* mi/mi-common.h (enum async_reply_reason): Add
	EXEC_ASYNC_DPRINTF_ERROR.
	* mi/mi-common.c (async_reason_string_lookup): Add
	"dprintf-error".
	* NEWS: Mention behavior changes.

gdb/testsuite/ChangeLog:

	PR gdb/16551
	* gdb.base/dprintf-error.exp: New file.
	* gdb.base/dprintf-error.c: New file.
	* gdb.mi/mi-dprintf-error.exp: New file.
	* gdb.mi/mi-dprintf-error.c: New file.

gdb/doc/ChangeLog:

	PR gdb/16551
	* gdb.texinfo (Dynamic Printf): Mention that dprintf evaluation
	errors cause stops.
	(GDB/MI Async Records): Add the "dprintf-error" stop reason.
---
 gdb/NEWS                                  |  6 +++++
 gdb/breakpoint.c                          | 43 ++++++++++++++++++++++++++++---
 gdb/breakpoint.h                          |  4 +++
 gdb/doc/gdb.texinfo                       |  9 +++++++
 gdb/mi/mi-common.c                        |  1 +
 gdb/mi/mi-common.h                        |  1 +
 gdb/testsuite/gdb.base/dprintf-error.c    | 27 +++++++++++++++++++
 gdb/testsuite/gdb.base/dprintf-error.exp  | 27 +++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-dprintf-error.c   | 27 +++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-dprintf-error.exp | 38 +++++++++++++++++++++++++++
 10 files changed, 180 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dprintf-error.c
 create mode 100644 gdb/testsuite/gdb.base/dprintf-error.exp
 create mode 100644 gdb/testsuite/gdb.mi/mi-dprintf-error.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-dprintf-error.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 13da2f1..d0786b0 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -84,6 +84,12 @@ SH-5/SH64 running OpenBSD 	SH-5/SH64 support in sh*-*-openbsd*
   the tradeoff that there is a possibility of false hits being
   reported.
 
+* Dynamic printf argument evaluation error changes
+
+  When GDB fails to evaluate an expression passed as an argument to a dynamic
+  printf, it will print an error and the dynamic printf will cause a stop.  In
+  MI, this stop is reported using the new "dprintf-error" stop reason.
+
 *** Changes in GDB 8.1
 
 * GDB now supports dynamically creating arbitrary register groups specified
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f20bc50..db44bf8 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5677,8 +5677,10 @@ bpstat_what (bpstat bs_head)
 	  break;
 
 	case bp_dprintf:
+	  /* We may stop on a dprintf if we failed to evaluate the expressions to
+	     print.  */
 	  if (bs->stop)
-	    this_action = BPSTAT_WHAT_STOP_SILENT;
+	    this_action = BPSTAT_WHAT_STOP_NOISY;
 	  else
 	    this_action = BPSTAT_WHAT_SINGLE;
 	  break;
@@ -12993,6 +12995,31 @@ dprintf_re_set (struct breakpoint *b)
     update_dprintf_command_list (b);
 }
 
+/* Implement the "print_it" breakpoint_ops method for dprintf.  */
+
+static enum print_stop_action
+dprintf_print_it (bpstat bs)
+{
+  struct ui_out *uiout = current_uiout;
+  gdb_assert (bs->bp_location_at != NULL);
+  breakpoint *bp = bs->breakpoint_at;
+
+  maybe_print_thread_hit_breakpoint (uiout);
+
+  uiout->text ("Dprintf ");
+  if (uiout->is_mi_like_p ())
+    {
+      uiout->field_string ("reason",
+			   async_reason_lookup (EXEC_ASYNC_DPRINTF_ERROR));
+    }
+  uiout->field_int ("bkptno", bp->number);
+  uiout->text (", failed to evaluate: ");
+  uiout->field_string ("error-message", bs->dprintf_error);
+  uiout->text ("\n");
+
+  return PRINT_SRC_AND_LOC;
+}
+
 /* Implement the "print_recreate" breakpoint_ops method for dprintf.  */
 
 static void
@@ -13032,7 +13059,17 @@ dprintf_after_condition_true (struct bpstats *bs)
   tmp_bs.commands = bs->commands;
   bs->commands = NULL;
 
-  bpstat_do_actions_1 (&tmp_bs_p);
+  TRY
+    {
+      bpstat_do_actions_1 (&tmp_bs_p);
+    }
+  CATCH (ex, RETURN_MASK_ALL)
+    {
+      /* Stop the inferior if we fail to evaluate an expression.  */
+      bs->stop = 1;
+      bs->dprintf_error = ex.message;
+    }
+  END_CATCH
 
   /* 'tmp_bs.commands' will usually be NULL by now, but
      bpstat_do_actions_1 may return early without processing the whole
@@ -15504,7 +15541,7 @@ initialize_breakpoint_ops (void)
   *ops = bkpt_base_breakpoint_ops;
   ops->re_set = dprintf_re_set;
   ops->resources_needed = bkpt_resources_needed;
-  ops->print_it = bkpt_print_it;
+  ops->print_it = dprintf_print_it;
   ops->print_mention = bkpt_print_mention;
   ops->print_recreate = dprintf_print_recreate;
   ops->after_condition_true = dprintf_after_condition_true;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 4223158..2aead5c 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1167,6 +1167,10 @@ struct bpstats
     /* Tell bpstat_print and print_bp_stop_message how to print stuff
        associated with this element of the bpstat chain.  */
     enum bp_print_how print_it;
+
+    /* When we stop due to a failed dprintf evaluation (e.g. referenced an
+       unknown symbol), this contains a meaningful error message.  */
+    std::string dprintf_error;
   };
 
 enum inf_context
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 2b56b5a..da15f53 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -5170,6 +5170,12 @@ may be the values of local variables, but if that is the case, then
 all enabled dynamic prints must be at locations within the scope of
 those locals.  If evaluation fails, @value{GDBN} will report an error.
 
+When using @code{dprintf-style} values @code{gdb} or @code{call}, if
+@value{GDBN} fails to evaluate an expression passed as an argument (a reference
+to an unknown symbol or an access to an invalid memory location, for example),
+it will report the error and the dynamic printf will cause a stop, as if it was
+a breakpoint.
+
 @node Save Breakpoints
 @subsection How to save breakpoints to a file
 
@@ -27338,6 +27344,9 @@ The inferior returned from a system call.  This is reported when
 @item exec
 The inferior called @code{exec}.  This is reported when @code{catch exec}
 (@pxref{Set Catchpoints}) has been used.
+@item dprintf-error
+GDB failed to evaluate the argument of a dynamic printf (@xref{Dynamic Printf}).
+An error message is available in the @code{error-message} field.
 @end table
 
 The @var{id} field identifies the global thread ID of the thread
diff --git a/gdb/mi/mi-common.c b/gdb/mi/mi-common.c
index 1724f32..d4c95e3 100644
--- a/gdb/mi/mi-common.c
+++ b/gdb/mi/mi-common.c
@@ -39,6 +39,7 @@ static const char * const async_reason_string_lookup[] =
   "syscall-entry",
   "syscall-return",
   "exec",
+  "dprintf-error",
   NULL
 };
 
diff --git a/gdb/mi/mi-common.h b/gdb/mi/mi-common.h
index c8ad0d2..4af43eb 100644
--- a/gdb/mi/mi-common.h
+++ b/gdb/mi/mi-common.h
@@ -46,6 +46,7 @@ enum async_reply_reason
   EXEC_ASYNC_SYSCALL_ENTRY,
   EXEC_ASYNC_SYSCALL_RETURN,
   EXEC_ASYNC_EXEC,
+  EXEC_ASYNC_DPRINTF_ERROR,
   /* This is here only to represent the number of enums.  */
   EXEC_ASYNC_LAST
 };
diff --git a/gdb/testsuite/gdb.base/dprintf-error.c b/gdb/testsuite/gdb.base/dprintf-error.c
new file mode 100644
index 0000000..7203f7a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-error.c
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2018 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/>.  */
+
+static void
+foo (void)
+{
+}
+
+int
+main (void)
+{
+  foo ();
+}
diff --git a/gdb/testsuite/gdb.base/dprintf-error.exp b/gdb/testsuite/gdb.base/dprintf-error.exp
new file mode 100644
index 0000000..7841362
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-error.exp
@@ -0,0 +1,27 @@
+#   Copyright (C) 2018 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/>.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+if ![runto main] {
+    return -1
+}
+
+gdb_test "dprintf foo, \"Hello %d\", doesnt_exist" "Dprintf $decimal at .*"
+gdb_test "continue" ".*Dprintf $decimal, failed to evaluate: No symbol \"doesnt_exist\" in current context.*"
diff --git a/gdb/testsuite/gdb.mi/mi-dprintf-error.c b/gdb/testsuite/gdb.mi/mi-dprintf-error.c
new file mode 100644
index 0000000..7203f7a
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-dprintf-error.c
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 2018 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/>.  */
+
+static void
+foo (void)
+{
+}
+
+int
+main (void)
+{
+  foo ();
+}
diff --git a/gdb/testsuite/gdb.mi/mi-dprintf-error.exp b/gdb/testsuite/gdb.mi/mi-dprintf-error.exp
new file mode 100644
index 0000000..551350a
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-dprintf-error.exp
@@ -0,0 +1,38 @@
+#   Copyright (C) 2018 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/>.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+standard_testfile
+
+if {[build_executable $testfile.exp $testfile $srcfile {debug}] == -1} {
+    untested "failed to compile"
+    return -1
+}
+
+mi_delete_breakpoints
+mi_run_to_main
+
+mi_gdb_test "-dprintf-insert foo \"Hello %d\" doesnt_exist" \
+    "\\^done,.*" "insert dprintf"
+
+set extra [list "" {bkptno="[0-9]+",error-message="No symbol \\"doesnt_exist\\" in current context.".*}]
+mi_execute_to exec-continue dprintf-error foo "" "mi-dprintf-error.c" $decimal $extra "continue to failed dprintf"
-- 
2.7.4

Re: [PATCH] Handle dprintf argument evaluation errors better (PR gdb/16551)

[Simon Marchi]

On 2018-06-13 16:01, Simon Marchi wrote:
> The current behavior when GDB fails to evaluate the arguments of a
> dynamic printf is not very good.
> 
> There was a previous attempt at fixing this here, but it did not went
> through:
>   https://sourceware.org/ml/gdb-patches/2015-02/msg00258.html
> 
> The issue can be reproduced by setting a dprintf referring to a 
> variable
> that doesn't exist or that triggers a memory error:
> 
>   dprintf hello, "hello %d\n", *((int*)0)
>   dprintf hello, "hello %d\n", doesnt_exist
> 
> When an evaluation error occurs, an error is thrown at the stack trace
> shown below and is caught in start_event_loop.  This leaves things in a
> relatively bad shape:
> 
> - Printing the error in start_event_loop causes GDB to receive a 
> SIGTTOU
>   signal, because the terminal is still owned by the inferior at that
>   point.
> - There is an error message (e.g. No symbol "foo" in current context)
>   and you are back to the GDB prompt, but nothing gives a clue about 
> the
>   context of the error.
> - The thread that hit the dprintf is stopped.  If in all-stop mode on 
> an
>   all-stop-on-top-of-non-stop target, you end up with a single thread
>   stopped and the others running, which is not good.
> - With MI, the thread(s) is/are stopped but no *stopped event is sent,
>   so frontends still show it as running.
> 
> I actually think it is nice that the program stops if there is an
> error, so you can notice the problem and fix it.  It just needs to be
> handled better.  This patch makes GDB catch the evaluation error in the
> dprintf_after_condition_true function, and sets bpstat::stop to 1/true,
> so that the dprintf will cause a stop similar to a breakpoint hit.  The
> dprintf_print_it function defines how a "dprintf error hit" is printed.
> The result looks like this:
> 
>   (gdb) c
>   Continuing.
> 
>   Dprintf 2, failed to evaluate: No symbol "lalala" in current context.
>   foo (n=1) at
> /home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/dprintf-error.c:30
> 
> When using MI, the stop is communicated using a new reason
> "dprintf-error", and the error-message field gives the text of the
> exception:
> 
>   *stopped,reason="dprintf-error",bkptno="2",error-message="No symbol
> \"lalala\" in current context.",frame=...
> 
>   #0  error (fmt=0x12bad88 "No symbol \"%s\" in current context.") at
> /home/emaisin/src/binutils-gdb/gdb/common/errors.c:39
>   #1  0x00000000006f5d49 in c_yyparse () at
> /home/emaisin/src/binutils-gdb/gdb/c-exp.y:1090
>   #2  0x00000000006fc082 in c_parse (par_state=0x7ffd3ed91c50) at
> /home/emaisin/src/binutils-gdb/gdb/c-exp.y:3273
>   #3  0x0000000000979d88 in parse_exp_in_context_1
> (stringptr=0x7ffd3ed91e00, pc=0, block=0x0, comma=1, void_context_p=0,
> out_subexp=0x0) at /home/emaisin/src/binutils-gdb/gdb/parse.c:1205
>   #4  0x0000000000979aae in parse_exp_in_context
> (stringptr=0x7ffd3ed91e00, pc=0, block=0x0, comma=1, void_context_p=0,
> out_subexp=0x0) at /home/emaisin/src/binutils-gdb/gdb/parse.c:1108
>   #5  0x0000000000979a2d in parse_exp_1 (stringptr=0x7ffd3ed91e00,
> pc=0, block=0x0, comma=1) at
> /home/emaisin/src/binutils-gdb/gdb/parse.c:1099
>   #6  0x000000000089ec1d in parse_to_comma_and_eval
> (expp=0x7ffd3ed91e00) at /home/emaisin/src/binutils-gdb/gdb/eval.c:126
>   #7  0x0000000000981a84 in ui_printf (arg=0x307f377 "\"hello %d\\n\",
> lalala", stream=0x2f34b90) at
> /home/emaisin/src/binutils-gdb/gdb/printcmd.c:2464
>   #8  0x000000000098212a in printf_command (arg=0x307f377 "\"hello
> %d\\n\", lalala", from_tty=0) at
> /home/emaisin/src/binutils-gdb/gdb/printcmd.c:2580
>   #9  0x0000000000603808 in do_const_cfunc (c=0x2ee6b00,
> args=0x307f377 "\"hello %d\\n\", lalala", from_tty=0) at
> /home/emaisin/src/binutils-gdb/gdb/cli/cli-decode.c:106
>   #10 0x0000000000606900 in cmd_func (cmd=0x2ee6b00, args=0x307f377
> "\"hello %d\\n\", lalala", from_tty=0) at
> /home/emaisin/src/binutils-gdb/gdb/cli/cli-decode.c:1857
>   #11 0x0000000000a61415 in execute_command (p=0x307f38a "a",
> from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/top.c:630
>   #12 0x00000000006106f6 in execute_control_command_1 (cmd=0x2fd6f30,
> from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/cli/cli-script.c:525
>   #13 0x0000000000610d2a in execute_control_command (cmd=0x2fd6f30,
> from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/cli/cli-script.c:694
>   #14 0x000000000077e83f in bpstat_do_actions_1 (bsp=0x7ffd3ed922e8)
> at /home/emaisin/src/binutils-gdb/gdb/breakpoint.c:4433
>   #15 0x00000000007920e1 in dprintf_after_condition_true
> (bs=0x2fe9260) at
> /home/emaisin/src/binutils-gdb/gdb/breakpoint.c:13035
>   #16 0x000000000078057c in bpstat_stop_status (aspace=0x2f169e0,
> bp_addr=4195559, ptid=..., ws=0x7ffd3ed927a0, stop_chain=0x2fe9260) at
> /home/emaisin/src/binutils-gdb/gdb/breakpoint.c:5460
>   #17 0x0000000000908760 in handle_signal_stop (ecs=0x7ffd3ed92780) at
> /home/emaisin/src/binutils-gdb/gdb/infrun.c:5946
>   #18 0x0000000000907463 in handle_inferior_event_1
> (ecs=0x7ffd3ed92780) at
> /home/emaisin/src/binutils-gdb/gdb/infrun.c:5375
>   #19 0x00000000009075aa in handle_inferior_event (ecs=0x7ffd3ed92780)
> at /home/emaisin/src/binutils-gdb/gdb/infrun.c:5410
>   #20 0x000000000090449a in fetch_inferior_event (client_data=0x0) at
> /home/emaisin/src/binutils-gdb/gdb/infrun.c:3924
>   #21 0x00000000008efe47 in inferior_event_handler
> (event_type=INF_REG_EVENT, client_data=0x0) at
> /home/emaisin/src/binutils-gdb/gdb/inf-loop.c:43
>   #22 0x000000000090ef4b in infrun_async_inferior_event_handler
> (data=0x0) at /home/emaisin/src/binutils-gdb/gdb/infrun.c:9164
>   #23 0x00000000008aaa43 in check_async_event_handlers () at
> /home/emaisin/src/binutils-gdb/gdb/event-loop.c:1064
>   #24 0x00000000008a9375 in gdb_do_one_event () at
> /home/emaisin/src/binutils-gdb/gdb/event-loop.c:326
>   #25 0x00000000008a9426 in start_event_loop () at
> /home/emaisin/src/binutils-gdb/gdb/event-loop.c:371
>   #26 0x000000000093d38f in captured_command_loop () at
> /home/emaisin/src/binutils-gdb/gdb/main.c:330
>   #27 0x000000000093e87a in captured_main (data=0x7ffd3ed929e0) at
> /home/emaisin/src/binutils-gdb/gdb/main.c:1157
>   #28 0x000000000093e945 in gdb_main (args=0x7ffd3ed929e0) at
> /home/emaisin/src/binutils-gdb/gdb/main.c:1173
>   #29 0x0000000000411f5c in main (argc=10, argv=0x7ffd3ed92ae8) at
> /home/emaisin/src/binutils-gdb/gdb/gdb.c:32

Ping.  Since there would be multiple ways of handling this, I'd like to 
get a second opinion to know if this makes sense.

Simon

Re: [PATCH] Handle dprintf argument evaluation errors better (PR gdb/16551)

[Simon Marchi]

On 2018-06-27 10:31 AM, Simon Marchi wrote:
> On 2018-06-13 16:01, Simon Marchi wrote:
>> The current behavior when GDB fails to evaluate the arguments of a
>> dynamic printf is not very good.
>>
>> There was a previous attempt at fixing this here, but it did not went
>> through:
>>   https://sourceware.org/ml/gdb-patches/2015-02/msg00258.html
>>
>> The issue can be reproduced by setting a dprintf referring to a variable
>> that doesn't exist or that triggers a memory error:
>>
>>   dprintf hello, "hello %d\n", *((int*)0)
>>   dprintf hello, "hello %d\n", doesnt_exist
>>
>> When an evaluation error occurs, an error is thrown at the stack trace
>> shown below and is caught in start_event_loop.  This leaves things in a
>> relatively bad shape:
>>
>> - Printing the error in start_event_loop causes GDB to receive a SIGTTOU
>>   signal, because the terminal is still owned by the inferior at that
>>   point.
>> - There is an error message (e.g. No symbol "foo" in current context)
>>   and you are back to the GDB prompt, but nothing gives a clue about the
>>   context of the error.
>> - The thread that hit the dprintf is stopped.  If in all-stop mode on an
>>   all-stop-on-top-of-non-stop target, you end up with a single thread
>>   stopped and the others running, which is not good.
>> - With MI, the thread(s) is/are stopped but no *stopped event is sent,
>>   so frontends still show it as running.
>>
>> I actually think it is nice that the program stops if there is an
>> error, so you can notice the problem and fix it.  It just needs to be
>> handled better.  This patch makes GDB catch the evaluation error in the
>> dprintf_after_condition_true function, and sets bpstat::stop to 1/true,
>> so that the dprintf will cause a stop similar to a breakpoint hit.  The
>> dprintf_print_it function defines how a "dprintf error hit" is printed.
>> The result looks like this:
>>
>>   (gdb) c
>>   Continuing.
>>
>>   Dprintf 2, failed to evaluate: No symbol "lalala" in current context.
>>   foo (n=1) at
>> /home/emaisin/src/binutils-gdb/gdb/testsuite/gdb.base/dprintf-error.c:30
>>
>> When using MI, the stop is communicated using a new reason
>> "dprintf-error", and the error-message field gives the text of the
>> exception:
>>
>>   *stopped,reason="dprintf-error",bkptno="2",error-message="No symbol
>> \"lalala\" in current context.",frame=...
>>
>>   #0  error (fmt=0x12bad88 "No symbol \"%s\" in current context.") at
>> /home/emaisin/src/binutils-gdb/gdb/common/errors.c:39
>>   #1  0x00000000006f5d49 in c_yyparse () at
>> /home/emaisin/src/binutils-gdb/gdb/c-exp.y:1090
>>   #2  0x00000000006fc082 in c_parse (par_state=0x7ffd3ed91c50) at
>> /home/emaisin/src/binutils-gdb/gdb/c-exp.y:3273
>>   #3  0x0000000000979d88 in parse_exp_in_context_1
>> (stringptr=0x7ffd3ed91e00, pc=0, block=0x0, comma=1, void_context_p=0,
>> out_subexp=0x0) at /home/emaisin/src/binutils-gdb/gdb/parse.c:1205
>>   #4  0x0000000000979aae in parse_exp_in_context
>> (stringptr=0x7ffd3ed91e00, pc=0, block=0x0, comma=1, void_context_p=0,
>> out_subexp=0x0) at /home/emaisin/src/binutils-gdb/gdb/parse.c:1108
>>   #5  0x0000000000979a2d in parse_exp_1 (stringptr=0x7ffd3ed91e00,
>> pc=0, block=0x0, comma=1) at
>> /home/emaisin/src/binutils-gdb/gdb/parse.c:1099
>>   #6  0x000000000089ec1d in parse_to_comma_and_eval
>> (expp=0x7ffd3ed91e00) at /home/emaisin/src/binutils-gdb/gdb/eval.c:126
>>   #7  0x0000000000981a84 in ui_printf (arg=0x307f377 "\"hello %d\\n\",
>> lalala", stream=0x2f34b90) at
>> /home/emaisin/src/binutils-gdb/gdb/printcmd.c:2464
>>   #8  0x000000000098212a in printf_command (arg=0x307f377 "\"hello
>> %d\\n\", lalala", from_tty=0) at
>> /home/emaisin/src/binutils-gdb/gdb/printcmd.c:2580
>>   #9  0x0000000000603808 in do_const_cfunc (c=0x2ee6b00,
>> args=0x307f377 "\"hello %d\\n\", lalala", from_tty=0) at
>> /home/emaisin/src/binutils-gdb/gdb/cli/cli-decode.c:106
>>   #10 0x0000000000606900 in cmd_func (cmd=0x2ee6b00, args=0x307f377
>> "\"hello %d\\n\", lalala", from_tty=0) at
>> /home/emaisin/src/binutils-gdb/gdb/cli/cli-decode.c:1857
>>   #11 0x0000000000a61415 in execute_command (p=0x307f38a "a",
>> from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/top.c:630
>>   #12 0x00000000006106f6 in execute_control_command_1 (cmd=0x2fd6f30,
>> from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/cli/cli-script.c:525
>>   #13 0x0000000000610d2a in execute_control_command (cmd=0x2fd6f30,
>> from_tty=0) at /home/emaisin/src/binutils-gdb/gdb/cli/cli-script.c:694
>>   #14 0x000000000077e83f in bpstat_do_actions_1 (bsp=0x7ffd3ed922e8)
>> at /home/emaisin/src/binutils-gdb/gdb/breakpoint.c:4433
>>   #15 0x00000000007920e1 in dprintf_after_condition_true
>> (bs=0x2fe9260) at
>> /home/emaisin/src/binutils-gdb/gdb/breakpoint.c:13035
>>   #16 0x000000000078057c in bpstat_stop_status (aspace=0x2f169e0,
>> bp_addr=4195559, ptid=..., ws=0x7ffd3ed927a0, stop_chain=0x2fe9260) at
>> /home/emaisin/src/binutils-gdb/gdb/breakpoint.c:5460
>>   #17 0x0000000000908760 in handle_signal_stop (ecs=0x7ffd3ed92780) at
>> /home/emaisin/src/binutils-gdb/gdb/infrun.c:5946
>>   #18 0x0000000000907463 in handle_inferior_event_1
>> (ecs=0x7ffd3ed92780) at
>> /home/emaisin/src/binutils-gdb/gdb/infrun.c:5375
>>   #19 0x00000000009075aa in handle_inferior_event (ecs=0x7ffd3ed92780)
>> at /home/emaisin/src/binutils-gdb/gdb/infrun.c:5410
>>   #20 0x000000000090449a in fetch_inferior_event (client_data=0x0) at
>> /home/emaisin/src/binutils-gdb/gdb/infrun.c:3924
>>   #21 0x00000000008efe47 in inferior_event_handler
>> (event_type=INF_REG_EVENT, client_data=0x0) at
>> /home/emaisin/src/binutils-gdb/gdb/inf-loop.c:43
>>   #22 0x000000000090ef4b in infrun_async_inferior_event_handler
>> (data=0x0) at /home/emaisin/src/binutils-gdb/gdb/infrun.c:9164
>>   #23 0x00000000008aaa43 in check_async_event_handlers () at
>> /home/emaisin/src/binutils-gdb/gdb/event-loop.c:1064
>>   #24 0x00000000008a9375 in gdb_do_one_event () at
>> /home/emaisin/src/binutils-gdb/gdb/event-loop.c:326
>>   #25 0x00000000008a9426 in start_event_loop () at
>> /home/emaisin/src/binutils-gdb/gdb/event-loop.c:371
>>   #26 0x000000000093d38f in captured_command_loop () at
>> /home/emaisin/src/binutils-gdb/gdb/main.c:330
>>   #27 0x000000000093e87a in captured_main (data=0x7ffd3ed929e0) at
>> /home/emaisin/src/binutils-gdb/gdb/main.c:1157
>>   #28 0x000000000093e945 in gdb_main (args=0x7ffd3ed929e0) at
>> /home/emaisin/src/binutils-gdb/gdb/main.c:1173
>>   #29 0x0000000000411f5c in main (argc=10, argv=0x7ffd3ed92ae8) at
>> /home/emaisin/src/binutils-gdb/gdb/gdb.c:32
> 
> Ping.  Since there would be multiple ways of handling this, I'd like to get a second opinion to know if this makes sense.
> 
> Simon

Eli, would it be possible to take a look at least at the NEWS/doc part of this patch?  If
there are no comments on the behavior change, I would be ready to merge this patch, but
I'd like to make sure that NEWS/doc is ok first.

Thanks,

Simon

Re: [PATCH] Handle dprintf argument evaluation errors better (PR gdb/16551)

[Eli Zaretskii]

> CC: <gdb-patches@sourceware.org>, Eli Zaretskii <eliz@gnu.org>
> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Wed, 22 Aug 2018 20:03:05 -0400
> 
> Eli, would it be possible to take a look at least at the NEWS/doc part of this patch?  If
> there are no comments on the behavior change, I would be ready to merge this patch, but
> I'd like to make sure that NEWS/doc is ok first.

Sorry I missed that: the documentation parts are OK.

Thanks.

Re: [PATCH] Handle dprintf argument evaluation errors better (PR gdb/16551)

[Pedro Alves]

On 06/13/2018 09:01 PM, Simon Marchi wrote:
> The current behavior when GDB fails to evaluate the arguments of a
> dynamic printf is not very good.
> 
> There was a previous attempt at fixing this here, but it did not went
> through:
>   https://sourceware.org/ml/gdb-patches/2015-02/msg00258.html
> 
> The issue can be reproduced by setting a dprintf referring to a variable
> that doesn't exist or that triggers a memory error:
> 
>   dprintf hello, "hello %d\n", *((int*)0)
>   dprintf hello, "hello %d\n", doesnt_exist
> 
> When an evaluation error occurs, an error is thrown at the stack trace
> shown below and is caught in start_event_loop.  This leaves things in a
> relatively bad shape:
> 
> - Printing the error in start_event_loop causes GDB to receive a SIGTTOU
>   signal, because the terminal is still owned by the inferior at that
>   point.
> - There is an error message (e.g. No symbol "foo" in current context)
>   and you are back to the GDB prompt, but nothing gives a clue about the
>   context of the error.
> - The thread that hit the dprintf is stopped.  If in all-stop mode on an
>   all-stop-on-top-of-non-stop target, you end up with a single thread
>   stopped and the others running, which is not good.
> - With MI, the thread(s) is/are stopped but no *stopped event is sent,
>   so frontends still show it as running.
> 
> I actually think it is nice that the program stops if there is an
> error, so you can notice the problem and fix it.  It just needs to be
> handled better.  

I'm not so sure about that.  Consider a dprintf like this:

  int *some_int_global;

  dprintf funct, "some_int_global: %d\n", *some_int_global

and during most of the run, some_int_global points somewhere
valid (or some more complex expression involving pointers,
like a->b->c->d, you get the idea).  However, at some point, maybe
after days of a long running hard-to-collect-data debug session,
the pointer points to NULL or to garbage.

If I understood correctly, that makes the dprintf stop the thread.
That doesn't seem desirable to me.

Also, if that dprintf is handled on the target side, then GDB is
not notified of the error either, and no thread stops, right?

It would seem to me better, in terms of the spirit of the
command (non-intrusive dynamic logging), to catch the error,
print something like "<error: xxxxx>" and let the thread/program
run.

Thanks,
Pedro Alves