handle dprintf error more gracefully

  If a dprintf generates an error, handle it so that we leave
the program as stopped at location as if a real breakpoint was hit.
On the mi interface this sends a *stopped event  and avoids
breaking frontend functionality. On cli it prints as a breakpoint
would.

Here's an example of what is returned on the mi interface :

&"No symbol \"invalidvar1\" in current context.\n"
~"\nBreakpoint "
~"5, invalid () at ../../../gdb/testsuite/gdb.mi/mi-dprintf.c:37\n"
~"37\t  ++i; /* set dprintf 2 here */\n"
*stopped,reason="breakpoint-hit",disp="keep",bkptno="5",frame={addr="0x0000000000400665",
func="invalid",args=[],file="../../../gdb/testsuite/gdb.mi/mi-dprintf.c",
fullname="/home/eantotr/src/binutils-gdb/gdb/testsuite/gdb.mi/mi-dprintf.c",line="37"},
thread-id="1",stopped-threads="all",core="2"

Here's an example for the cli :

No symbol "asdf" in current context.

Breakpoint 1, main () at testdprintf.c:8
8	    printf("i is %d\n",i);

Note we could also treat the error as a warning and continue
the program's execution by leaving bs->stop at 0 and continuing.

I sided with the stop on error for now since I though an error
should be treated as soon as possible, but continuing also
seems like a possiblity. I'm unsure between the behavior of
stopping at error or dprintf not stopping...

Feedback is welcome, on the best way to handle this...

gdb/ChangeLog:
	PR breakpoints/16551
	* breakpoint.c (dprintf_after_condition_true): Handle dprintf error.
	(bpstat_what) : Enable dprintf to print on stop.

gdb/testsuite/ChangeLog:
	PR breakpoints/16551
	* gdb.mi/mi-dprintf.c (invalid): New function to test invalid dprintf.
	* gdb.mi/mi-dprintf.exp: Add invalid dprintf test.
---
 gdb/breakpoint.c                    |   21 +++++++++++++++++++--
 gdb/testsuite/gdb.mi/mi-dprintf.c   |   12 +++++++++++-
 gdb/testsuite/gdb.mi/mi-dprintf.exp |   15 +++++++++++++++
 3 files changed, 45 insertions(+), 3 deletions(-)
  

ping...

On 02/10/2015 11:27 AM, Antoine Tremblay wrote:
> If a dprintf generates an error, handle it so that we leave
> the program as stopped at location as if a real breakpoint was hit.
> On the mi interface this sends a *stopped event  and avoids
> breaking frontend functionality. On cli it prints as a breakpoint
> would.
>
> Here's an example of what is returned on the mi interface :
>
> &"No symbol \"invalidvar1\" in current context.\n"
> ~"\nBreakpoint "
> ~"5, invalid () at ../../../gdb/testsuite/gdb.mi/mi-dprintf.c:37\n"
> ~"37\t  ++i; /* set dprintf 2 here */\n"
> *stopped,reason="breakpoint-hit",disp="keep",bkptno="5",frame={addr="0x0000000000400665",
> func="invalid",args=[],file="../../../gdb/testsuite/gdb.mi/mi-dprintf.c",
> fullname="/home/eantotr/src/binutils-gdb/gdb/testsuite/gdb.mi/mi-dprintf.c",line="37"},
> thread-id="1",stopped-threads="all",core="2"
>
> Here's an example for the cli :
>
> No symbol "asdf" in current context.
>
> Breakpoint 1, main () at testdprintf.c:8
> 8	    printf("i is %d\n",i);
>
> Note we could also treat the error as a warning and continue
> the program's execution by leaving bs->stop at 0 and continuing.
>
> I sided with the stop on error for now since I though an error
> should be treated as soon as possible, but continuing also
> seems like a possiblity. I'm unsure between the behavior of
> stopping at error or dprintf not stopping...
>
> Feedback is welcome, on the best way to handle this...
>
> gdb/ChangeLog:
> 	PR breakpoints/16551
> 	* breakpoint.c (dprintf_after_condition_true): Handle dprintf error.
> 	(bpstat_what) : Enable dprintf to print on stop.
>
> gdb/testsuite/ChangeLog:
> 	PR breakpoints/16551
> 	* gdb.mi/mi-dprintf.c (invalid): New function to test invalid dprintf.
> 	* gdb.mi/mi-dprintf.exp: Add invalid dprintf test.
> ---
>   gdb/breakpoint.c                    |   21 +++++++++++++++++++--
>   gdb/testsuite/gdb.mi/mi-dprintf.c   |   12 +++++++++++-
>   gdb/testsuite/gdb.mi/mi-dprintf.exp |   15 +++++++++++++++
>   3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 2804453..b51d17c 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5818,7 +5818,12 @@ bpstat_what (bpstat bs_head)
>   
>   	case bp_dprintf:
>   	  if (bs->stop)
> -	    this_action = BPSTAT_WHAT_STOP_SILENT;
> +	    {
> +	      if (bs->print)
> +		this_action = BPSTAT_WHAT_STOP_NOISY;
> +	      else
> +		this_action = BPSTAT_WHAT_STOP_SILENT;
> +	    }
>   	  else
>   	    this_action = BPSTAT_WHAT_SINGLE;
>   	  break;
> @@ -13885,6 +13890,7 @@ dprintf_after_condition_true (struct bpstats *bs)
>     struct cleanup *old_chain;
>     struct bpstats tmp_bs = { NULL };
>     struct bpstats *tmp_bs_p = &tmp_bs;
> +  volatile struct gdb_exception e;
>   
>     /* dprintf's never cause a stop.  This wasn't set in the
>        check_status hook instead because that would make the dprintf's
> @@ -13900,7 +13906,18 @@ dprintf_after_condition_true (struct bpstats *bs)
>     bs->commands = NULL;
>     old_chain = make_cleanup_decref_counted_command_line (&tmp_bs.commands);
>   
> -  bpstat_do_actions_1 (&tmp_bs_p);
> +  TRY_CATCH (e, RETURN_MASK_ERROR)
> +    {
> +      bpstat_do_actions_1 (&tmp_bs_p);
> +    }
> +  if (e.reason < 0)
> +    {
> +      /* Do stop if we have an error executing a dprintf command.  */
> +      bs->stop = 1;
> +      /* Print the breakpoint information.  */
> +      bs->print = 1;
> +      exception_print (gdb_stderr, e);
> +    }
>   
>     /* 'tmp_bs.commands' will usually be NULL by now, but
>        bpstat_do_actions_1 may return early without processing the whole
> diff --git a/gdb/testsuite/gdb.mi/mi-dprintf.c b/gdb/testsuite/gdb.mi/mi-dprintf.c
> index 0b8fc82..ed7d87e 100644
> --- a/gdb/testsuite/gdb.mi/mi-dprintf.c
> +++ b/gdb/testsuite/gdb.mi/mi-dprintf.c
> @@ -29,6 +29,14 @@ foo (int arg)
>     g /= 2.5; /* set breakpoint 1 here */
>   }
>   
> +/* Function to test invalid symbols used in dprinf.  */
> +void
> +invalid (void)
> +{
> +  int i = 0;
> +  ++i; /* set dprintf 2 here */
> +}
> +
>   int
>   main (int argc, char *argv[])
>   {
> @@ -40,7 +48,9 @@ main (int argc, char *argv[])
>   
>     foo (loc++);
>     foo (loc++);
> -  foo (loc++);
> +
> +  invalid ();
> +
>     return g;
>   }
>   
> diff --git a/gdb/testsuite/gdb.mi/mi-dprintf.exp b/gdb/testsuite/gdb.mi/mi-dprintf.exp
> index ea198bd..0cdf2b5 100644
> --- a/gdb/testsuite/gdb.mi/mi-dprintf.exp
> +++ b/gdb/testsuite/gdb.mi/mi-dprintf.exp
> @@ -33,6 +33,7 @@ mi_delete_breakpoints
>   
>   set bp_location1 [gdb_get_line_number "set breakpoint 1 here"]
>   set dp_location1 [gdb_get_line_number "set dprintf 1 here"]
> +set dp_location2 [gdb_get_line_number "set dprintf 2 here"]
>   
>   mi_run_to_main
>   
> @@ -60,12 +61,22 @@ set bps [mi_make_breakpoint -type dprintf -func foo \
>   mi_gdb_test "[incr i]-dprintf-insert $dp_location1 \"arg=%d, g=%d\\n\" arg g" \
>   		   "$i\\^done,$bps" "mi insert dprintf dp_location1"
>   
> +set bps [mi_make_breakpoint -type dprintf -func invalid \
> +		   -file ".*mi-dprintf.c" -fullname ".*mi-dprintf.c" \
> +		   -line $dp_location2]
> +mi_gdb_test "[incr i]-dprintf-insert $dp_location2 \"g=%d\\n\" invalidvar1" \
> +		   "$i\\^done,$bps" "mi insert dprintf dp_location2"
> +
>   set bps {}
>   lappend bps [mi_make_breakpoint -number 3 -type dprintf -func foo \
>   		 -file ".*mi-dprintf.c" -fullname ".*mi-dprintf.c"]
>   lappend bps [mi_make_breakpoint -type dprintf -func foo \
>   		 -file ".*mi-dprintf.c" -fullname ".*mi-dprintf.c" \
>   		 -line $dp_location1]
> +lappend bps [mi_make_breakpoint -type dprintf -func invalid \
> +		 -file ".*mi-dprintf.c" -fullname ".*mi-dprintf.c" \
> +		 -line $dp_location2]
> +
>   mi_gdb_test "[incr i]-break-info" \
>       "$i\\^done,[mi_make_breakpoint_table $bps]" \
>       "mi info dprintf"
> @@ -111,6 +122,10 @@ proc mi_continue_dprintf {args} {
>               }
>   	}
>   	mi_expect_stop ".*" ".*" ".*" ".*" ".*" "" "$msg 2nd stop"
> +
> +	set msg "mi 3nd dprintf"
> +	mi_send_resuming_command "exec-continue" "$msg continue"
> +	mi_expect_stop ".*" ".*" ".*" ".*" ".*" "" "$msg 3nd stop"
>       }
>   }
>
  

On 02/10/2015 04:27 PM, Antoine Tremblay wrote:
> If a dprintf generates an error, handle it so that we leave
> the program as stopped at location as if a real breakpoint was hit.
> On the mi interface this sends a *stopped event  and avoids
> breaking frontend functionality. 

In general, frontends should not be broken by errors.  If they
are, they are mishandling errors.

From GDB/MI General Design in the manual:

 There's no guarantee that whenever an MI command reports an error,
 @value{GDBN} or the target are in any specific state, and especially,
 the state is not reverted to the state before the MI command was
 processed.  Therefore, whenever an MI command results in an error,
 we recommend that the frontend refreshes all the information shown in
 the user interface.

> On cli it prints as a breakpoint would.

I'm not sure that makes sense.

There's a general problem to solve here; dprintf is just a special
case.  There are many reasons that can lead to an exception thrown
while handling a target event.  All will have the same result.
We should intercept all errors, and handle them consistently.

It seems that since target-async was flipped on by default, we no
longer get the "^error" as reported in the PR.  With
"maint set target-async off", we get it.

With "maint set target-async on" (the default) :

 (gdb)
 dprintf main,"%d\n",badvariable
 ...
 (gdb)
 -exec-run
 ...
 *running,thread-id="all"
 ...
 &"No symbol \"badvariable\" in current context.\n"

With "maint set target-async off" :

 -exec-run
 ...
 ^error,msg="No symbol \"badvariable\" in current context."


Now, that seems an unfortunate change I missed before, but in any
case, with MI async ("set mi-async on" + "maint set target-async on"),
there's really no command associated with the error anymore,
so ^error wouldn't be good.  So at least for async, *stopped would be
better.  Probably with a new "error" reason or some such,
like *stopped,reason="error".

Alternatively, a new *error asynchronous MI event could be added,
and then the frontend would be responsible for refresh all it's state
when it got that, just like it should when it gets a synchronous ^error
command result.  (I haven't thought this options fully through.)

Note that making sure the frontend ends up with the correct thread
state on error is already the job of finish_thread_state_cleanup
currently.  fetch_inferior_event does install that cleanup.  However,
finish_thread_state_cleanup does not handle *running -> *stopped,
only *stopped -> *running...  So the fix could/should be around here.

Thanks,
Pedro Alves
  

> Now, that seems an unfortunate change I missed before, but in any
> case, with MI async ("set mi-async on" + "maint set target-async on"),
> there's really no command associated with the error anymore,
> so ^error wouldn't be good.  So at least for async, *stopped would be
> better.  Probably with a new "error" reason or some such,
> like *stopped,reason="error".
>
> Alternatively, a new *error asynchronous MI event could be added,
> and then the frontend would be responsible for refresh all it's state
> when it got that, just like it should when it gets a synchronous ^error
> command result.  (I haven't thought this options fully through.)
>
> Note that making sure the frontend ends up with the correct thread
> state on error is already the job of finish_thread_state_cleanup
> currently.  fetch_inferior_event does install that cleanup.  However,
> finish_thread_state_cleanup does not handle *running -> *stopped,
> only *stopped -> *running...  So the fix could/should be around here.
>
> Thanks,
> Pedro Alves
>

I've been checking how to implement this with 
finish_thread_state_cleanup and
I ran into a few issues that I'm a bit unsure how to solve.

First problem is that finish_thread_state_cleanup gets explicitely called
by normal_stop around infrun.c:6527. So in the normal case and in the 
failure
case this cleanup gets called.

This means that if I were to use observer_notify_normal_stop(NULL, 
false); for
example in the case of *running -> *stopped in finish_thread_state, to 
handle
a possible failure we would get two *stopped events.

Maybe I could avoid calling this when I know it's a success case but
finish_thread_state_cleanup and finish_thread_sate should not be tied
to an error case. It's legitimate to call these in the normal case.
Like it is done for  observer_notify_target_resumed. This is a normal
case. If I understand correctly ?

Then I tought ok let's always call finish_thread_state_cleanup and rely 
on it
on the success case too like for resume but I need to be able to call
observer_notify_normal_stop with it's proper arguments from the normal_stop
call.

It's too bad that finish_thread_state can't handle all the states, and 
actually
this led me to believe that we should not use it in the success case for 
resume
and have a new cleanup only for the error cases or rename the function.
Since it does not really sync the front end state.

But the implications of that seem large for the corner case it handles.

So I don't think finish_thread_state_cleanup is a good place to fix the 
issue.
I think it would be better to be directly in some catch probably around
handle_signal_stop.. not sure where exactly yet...

However I think the way of doing a observer_notify_normal_stop with 
reason error
is much better then what I had done at first !! :) And so use that.

Does it sound like a plan ?

Also, for the global *error.. I'm not sure, I think it's better even if 
we give
no guarantee on the state, that we try to advertise it to the frontend 
as much
as possible even in case of error... *stopped,reason does a better job 
at that
then *error... even if *error would be more general....

Regards,

Antoine Tremblay
  

> Pedro:
 > Alternatively, a new *error asynchronous MI event could be added,
 > and then the frontend would be responsible for refresh all it's state
 > when it got that, just like it should when it gets a synchronous
 > ^error command result.  (I haven't thought this options fully
 > through.)
...

 > Antoine:
> So I don't think finish_thread_state_cleanup is a good place to fix the
> issue.
> I think it would be better to be directly in some catch probably around
> handle_signal_stop.. not sure where exactly yet...
>
> However I think the way of doing a observer_notify_normal_stop with
> reason error is much better then what I had done at first !! :) And so use that.
>
> Does it sound like a plan ?
>
> Also, for the global *error.. I'm not sure, I think it's better even if
> we give no guarantee on the state, that we try to advertise it to the frontend
> as much as possible even in case of error... *stopped,reason does a better job
> at that then *error... even if *error would be more general....

After thinking about this more, in order to be consistent, I think the
new *error asynchronous MI event is a better approach since we need to 
make this consistent...

And having a special error handling case for each type of event did not 
feel right...

I will rethink the patch with that in mind...

Regards,

Antoine