Re: [PATCH] handle dprintf error more gracefully

[Pedro Alves <palves at redhat dot com>]

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