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

[Simon Marchi <simon.marchi@ericsson.com>]

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