[PATCH 0/4] Remove deprecated_throw_reason

[PATCH 0/4] Remove deprecated_throw_reason

[Andrew Burgess]

While working on something else I ran into deprecated_throw_reason,
it doesn't appear to be used that often so I thought I'd have a go
at removing it.

Thanks,
Andrew

[PATCH 1/4] Remove deprecated_throw_reason from internal_verror.

[Andrew Burgess]

Removes the use of deprecated_throw_reason from internal_verror.  The user
will now get an extra "Command aborted" error message in the case where gdb
hits an internal error, and the user decides not to quit.  This feels like
an improvement to me as it /might/ not be obvious that choosing to continue
the session will still drop you out of whatever command you were attempting
at the time.  What do you think?

OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-07-30  Andrew Burgess  <aburgess@broadcom.com>

	* utils.c (internal_verror): Replace use of deprecated_throw_reason
	with throw_error.

diff --git a/gdb/utils.c b/gdb/utils.c
index 01212ab..a4ce01c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -831,7 +831,7 @@ void
 internal_verror (const char *file, int line, const char *fmt, va_list ap)
 {
   internal_vproblem (&internal_error_problem, file, line, fmt, ap);
-  deprecated_throw_reason (RETURN_ERROR);
+  throw_error (GENERIC_ERROR, _("Command aborted"));
 }
 
 void

Re: [PATCH 1/4] Remove deprecated_throw_reason from internal_verror.

[Pedro Alves]

On 07/30/2013 05:10 PM, Andrew Burgess wrote:
> Removes the use of deprecated_throw_reason from internal_verror.  The user
> will now get an extra "Command aborted" error message in the case where gdb
> hits an internal error, and the user decides not to quit.  This feels like
> an improvement to me as it /might/ not be obvious that choosing to continue
> the session will still drop you out of whatever command you were attempting
> at the time.  What do you think?

...

> diff --git a/gdb/utils.c b/gdb/utils.c
> index 01212ab..a4ce01c 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -831,7 +831,7 @@ void
>  internal_verror (const char *file, int line, const char *fmt, va_list ap)
>  {
>    internal_vproblem (&internal_error_problem, file, line, fmt, ap);
> -  deprecated_throw_reason (RETURN_ERROR);
> +  throw_error (GENERIC_ERROR, _("Command aborted"));

That error can still be swallowed by a TRY_CATCH somewhere, and the
command might proceed.  I think we should throw a RETURN_QUIT instead,
which has exactly that semantic of cancelling the ongoing command.
IOW, I think this should call "fatal" instead.

-- 
Pedro Alves

Re: [PATCH 1/4] Remove deprecated_throw_reason from internal_verror.

[Andrew Burgess]

On 30/07/2013 8:08 PM, Pedro Alves wrote:
> On 07/30/2013 05:10 PM, Andrew Burgess wrote:
>> Removes the use of deprecated_throw_reason from internal_verror.  The user
>> will now get an extra "Command aborted" error message in the case where gdb
>> hits an internal error, and the user decides not to quit.  This feels like
>> an improvement to me as it /might/ not be obvious that choosing to continue
>> the session will still drop you out of whatever command you were attempting
>> at the time.  What do you think?
> 
> ...
> 
>> diff --git a/gdb/utils.c b/gdb/utils.c
>> index 01212ab..a4ce01c 100644
>> --- a/gdb/utils.c
>> +++ b/gdb/utils.c
>> @@ -831,7 +831,7 @@ void
>>  internal_verror (const char *file, int line, const char *fmt, va_list ap)
>>  {
>>    internal_vproblem (&internal_error_problem, file, line, fmt, ap);
>> -  deprecated_throw_reason (RETURN_ERROR);
>> +  throw_error (GENERIC_ERROR, _("Command aborted"));
> 
> That error can still be swallowed by a TRY_CATCH somewhere, and the
> command might proceed.  I think we should throw a RETURN_QUIT instead,
> which has exactly that semantic of cancelling the ongoing command.
> IOW, I think this should call "fatal" instead.

Committed with a call to fatal.

Thanks,
Andrew

[PATCH 2/4] Remove deprecated_throw_reason from mips_error.

[Andrew Burgess]

Removes use of deprecated_throw_reason from mips_error.  The user will now
see the extra "Remote debugging terminated" error message but that doesn't
feel too bad to me, what do you think?

OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-07-30  Andrew Burgess  <aburgess@broadcom.com>

	* remote-mips.c (mips_error): Replace use of deprecated_throw_reason
	with throw_error.

diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c
index 1619622..791aa8a 100644
--- a/gdb/remote-mips.c
+++ b/gdb/remote-mips.c
@@ -510,7 +510,7 @@ mips_error (char *string,...)
   if (!ptid_equal (inferior_ptid, null_ptid))
     target_mourn_inferior ();
 
-  deprecated_throw_reason (RETURN_ERROR);
+  throw_error (GENERIC_ERROR, "Remote debugging terminated");
 }
 
 /* putc_readable - print a character, displaying non-printable chars in

Re: [PATCH 2/4] Remove deprecated_throw_reason from mips_error.

[Pedro Alves]

On 07/30/2013 05:11 PM, Andrew Burgess wrote:
> Removes use of deprecated_throw_reason from mips_error.  The user will now
> see the extra "Remote debugging terminated" error message but that doesn't
> feel too bad to me, what do you think?
> 
> OK to apply?
> 
> Thanks,
> Andrew
> 
> gdb/ChangeLog
> 
> 2013-07-30  Andrew Burgess  <aburgess@broadcom.com>
> 
> 	* remote-mips.c (mips_error): Replace use of deprecated_throw_reason
> 	with throw_error.
> 
> diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c
> index 1619622..791aa8a 100644
> --- a/gdb/remote-mips.c
> +++ b/gdb/remote-mips.c
> @@ -510,7 +510,7 @@ mips_error (char *string,...)
>    if (!ptid_equal (inferior_ptid, null_ptid))
>      target_mourn_inferior ();
>  
> -  deprecated_throw_reason (RETURN_ERROR);
> +  throw_error (GENERIC_ERROR, "Remote debugging terminated");

Throw TARGET_CLOSE_ERROR instead.  Add i18n, and period at end
of sentence.  Okay with that change.

I'd suggest removing or merging the earlier printf_unfiltered with
the error message, they're a bit redundant, and having the text
in the error is better in that a frontend usually displays errors
in a special way (a messagebox or some such), while console prints
end up hidden in the console...

-- 
Pedro Alves

Re: [PATCH 2/4] Remove deprecated_throw_reason from mips_error.

[Andrew Burgess]

On 30/07/2013 8:14 PM, Pedro Alves wrote:
> On 07/30/2013 05:11 PM, Andrew Burgess wrote:
>>
>> diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c
>> index 1619622..791aa8a 100644
>> --- a/gdb/remote-mips.c
>> +++ b/gdb/remote-mips.c
>> @@ -510,7 +510,7 @@ mips_error (char *string,...)
>>    if (!ptid_equal (inferior_ptid, null_ptid))
>>      target_mourn_inferior ();
>>  
>> -  deprecated_throw_reason (RETURN_ERROR);
>> +  throw_error (GENERIC_ERROR, "Remote debugging terminated");
> 
> Throw TARGET_CLOSE_ERROR instead.  Add i18n, and period at end
> of sentence.  Okay with that change.
> 
> I'd suggest removing or merging the earlier printf_unfiltered with
> the error message, they're a bit redundant, and having the text
> in the error is better in that a frontend usually displays errors
> in a special way (a messagebox or some such), while console prints
> end up hidden in the console...

So, following the advice in your second paragraph I've got a new patch
below, given that it's totally different to the first I'm reposting for
a review before committing.

The only testing I've done of this code is to compile it
(--enable-targets=all), then add in a fake call to mips_error
and check that the output looks reasonable.

I've removed the use of error_pre_print given that all the output
is now passing through throw_verror, I assume that does the "correct"
thing, though interestingly I notice that error_pre_print, and
quit_pre_print are no longer used anywhere after this patch....

If this change seems too disruptive then I can always go back to
the original patch and just change the error code to TARGET_CLOSE_ERROR.

What do you think? OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-07-31  Andrew Burgess  <aburgess@broadcom.com>

	* remote-mips.c (mips_error): Replace use of deprecated_throw_reason
	with throw_verror.  Use the error message passed to mips_error as the
	error message for throw_verror.

diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c
index 1619622..df96997 100644
--- a/gdb/remote-mips.c
+++ b/gdb/remote-mips.c
@@ -488,17 +488,12 @@ static void ATTRIBUTE_NORETURN
 mips_error (char *string,...)
 {
   va_list args;
-
-  va_start (args, string);
+  char *fmt;
+  int status;
 
   target_terminal_ours ();
   wrap_here ("");		/* Force out any buffered output.  */
   gdb_flush (gdb_stdout);
-  if (error_pre_print)
-    fputs_filtered (error_pre_print, gdb_stderr);
-  vfprintf_filtered (gdb_stderr, string, args);
-  fprintf_filtered (gdb_stderr, "\n");
-  va_end (args);
   gdb_flush (gdb_stderr);
 
   /* Clean up in such a way that mips_close won't try to talk to the
@@ -506,11 +501,18 @@ mips_error (char *string,...)
      it).  */
   close_ports ();
 
-  printf_unfiltered ("Ending remote MIPS debugging.\n");
   if (!ptid_equal (inferior_ptid, null_ptid))
     target_mourn_inferior ();
 
-  deprecated_throw_reason (RETURN_ERROR);
+  status = asprintf (&fmt, "Ending remote MIPS debugging: %s", string);
+  if (fmt == NULL || status < 0)
+    fmt = string;
+  else
+    make_cleanup (xfree, fmt);
+
+  va_start (args, string);
+  throw_verror (TARGET_CLOSE_ERROR, fmt, args);
+  va_end (args);
 }
 
 /* putc_readable - print a character, displaying non-printable chars in

Re: [PATCH 2/4] Remove deprecated_throw_reason from mips_error.

[Pedro Alves]

On 07/31/2013 01:35 PM, Andrew Burgess wrote:
> On 30/07/2013 8:14 PM, Pedro Alves wrote:
>
>> I'd suggest removing or merging the earlier printf_unfiltered with
>> the error message, they're a bit redundant, and having the text
>> in the error is better in that a frontend usually displays errors
>> in a special way (a messagebox or some such), while console prints
>> end up hidden in the console...
> 
> So, following the advice in your second paragraph I've got a new patch
> below, given that it's totally different to the first I'm reposting for
> a review before committing.

Thanks.

> The only testing I've done of this code is to compile it
> (--enable-targets=all), then add in a fake call to mips_error
> and check that the output looks reasonable.

I think that's good enough.  One would hope that whoever still uses
this target tests it routinely.

> I've removed the use of error_pre_print given that all the output
> is now passing through throw_verror, I assume that does the "correct"
> thing, though interestingly I notice that error_pre_print, and
> quit_pre_print are no longer used anywhere after this patch....

Interesting.  Time to garbage collect them, then.

> -  deprecated_throw_reason (RETURN_ERROR);
> +  status = asprintf (&fmt, "Ending remote MIPS debugging: %s", string);

Can you add i18n/_(), while at it?

Don't use asprintf, use xstrprintf instead.  (The ARI would complain).
But, this is just concatenating the strings, so:

 fmt = concat (_("Ending remote MIPS debugging: "),
	       string, (char *) NULL);
 make_cleanup (xfree, fmt);

... is even simpler.  concat never returns NULL.

OK with that change.

-- 
Pedro Alves

Re: [PATCH 2/4] Remove deprecated_throw_reason from mips_error.

[Andrew Burgess]

On 31/07/2013 3:44 PM, Pedro Alves wrote:
> On 07/31/2013 01:35 PM, Andrew Burgess wrote:
>> On 30/07/2013 8:14 PM, Pedro Alves wrote:
>>
>>> I'd suggest removing or merging the earlier printf_unfiltered with
>>> the error message, they're a bit redundant, and having the text
>>> in the error is better in that a frontend usually displays errors
>>> in a special way (a messagebox or some such), while console prints
>>> end up hidden in the console...
>>
>> So, following the advice in your second paragraph I've got a new patch
>> below, given that it's totally different to the first I'm reposting for
>> a review before committing.
> 
> Thanks.
> 
>> The only testing I've done of this code is to compile it
>> (--enable-targets=all), then add in a fake call to mips_error
>> and check that the output looks reasonable.
> 
> I think that's good enough.  One would hope that whoever still uses
> this target tests it routinely.
> 
>> I've removed the use of error_pre_print given that all the output
>> is now passing through throw_verror, I assume that does the "correct"
>> thing, though interestingly I notice that error_pre_print, and
>> quit_pre_print are no longer used anywhere after this patch....
> 
> Interesting.  Time to garbage collect them, then.
> 
>> -  deprecated_throw_reason (RETURN_ERROR);
>> +  status = asprintf (&fmt, "Ending remote MIPS debugging: %s", string);
> 
> Can you add i18n/_(), while at it?
> 
> Don't use asprintf, use xstrprintf instead.  (The ARI would complain).
> But, this is just concatenating the strings, so:
> 
>  fmt = concat (_("Ending remote MIPS debugging: "),
> 	       string, (char *) NULL);
>  make_cleanup (xfree, fmt);
> 
> ... is even simpler.  concat never returns NULL.
> 
> OK with that change.

Committed with the changes you requested.

Thanks,
Andrew

[PATCH 3/4] Remove remaining uses of deprecated_throw_reason.

[Andrew Burgess]

This removes the final uses of deprecated_throw_reason.  In each case I've
replaced the use of deprecated_throw_reason with a call to quit.  The only
change this will have is that the user will now see a "Quit" message,
however this seems pretty appropriate to me, what do you think?

OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-07-30  Andrew Burgess  <aburgess@broadcom.com>

	* monitor.c (monitor_interrupt_query): Replace use of
	deprecated_throw_reason with quit.
	* nto-procfs.c (interrupt_query): Likewise.
	* remote-fileio.c (remote_fileio_sig_exit): Likewise.
	* remote-mips.c (mips_kill): Likewise.
	* remote.c (interrupt_query): Likewise.

diff --git a/gdb/monitor.c b/gdb/monitor.c
index beca4e4..0e47acc 100644
--- a/gdb/monitor.c
+++ b/gdb/monitor.c
@@ -1036,7 +1036,7 @@ monitor_interrupt_query (void)
 Give up (and stop debugging it)? ")))
     {
       target_mourn_inferior ();
-      deprecated_throw_reason (RETURN_QUIT);
+      quit ();
     }
 
   target_terminal_inferior ();
diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c
index 0c2d3a8..5109fac 100644
--- a/gdb/nto-procfs.c
+++ b/gdb/nto-procfs.c
@@ -694,7 +694,7 @@ interrupt_query (void)
 Give up (and stop debugging it)? ")))
     {
       target_mourn_inferior ();
-      deprecated_throw_reason (RETURN_QUIT);
+      quit ();
     }
 
   target_terminal_inferior ();
diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index cc39bdf..5b7ef1d 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -513,7 +513,7 @@ remote_fileio_sig_exit (void)
 static void
 async_remote_fileio_interrupt (gdb_client_data arg)
 {
-  deprecated_throw_reason (RETURN_QUIT);
+  quit ();
 }
 
 static void
diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c
index 1619622..791aa8a 100644
--- a/gdb/remote-mips.c
+++ b/gdb/remote-mips.c
@@ -2293,8 +2293,7 @@ Give up (and stop debugging it)? ")))
 
 	  printf_unfiltered ("Ending remote MIPS debugging.\n");
 	  target_mourn_inferior ();
-
-	  deprecated_throw_reason (RETURN_QUIT);
+	  quit ();
 	}
 
       target_terminal_inferior ();
diff --git a/gdb/remote.c b/gdb/remote.c
index 1d6ac90..7830e67 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5165,7 +5165,7 @@ interrupt_query (void)
   if (target_can_async_p ())
     {
       signal (SIGINT, handle_sigint);
-      deprecated_throw_reason (RETURN_QUIT);
+      quit ();
     }
   else
     {
@@ -5173,7 +5173,7 @@ interrupt_query (void)
 Give up (and stop debugging it)? ")))
 	{
 	  remote_unpush_target ();
-	  deprecated_throw_reason (RETURN_QUIT);
+	  quit ();
 	}
     }
 

Re: [PATCH 3/4] Remove remaining uses of deprecated_throw_reason.

[Pedro Alves]

On 07/30/2013 05:12 PM, Andrew Burgess wrote:
> This removes the final uses of deprecated_throw_reason.  In each case I've
> replaced the use of deprecated_throw_reason with a call to quit.  The only
> change this will have is that the user will now see a "Quit" message,
> however this seems pretty appropriate to me, what do you think?
> 
> OK to apply?

OK.


-- 
Pedro Alves

Re: [PATCH 3/4] Remove remaining uses of deprecated_throw_reason.

[Andrew Burgess]

On 30/07/2013 8:21 PM, Pedro Alves wrote:
> On 07/30/2013 05:12 PM, Andrew Burgess wrote:
>> This removes the final uses of deprecated_throw_reason.  In each case I've
>> replaced the use of deprecated_throw_reason with a call to quit.  The only
>> change this will have is that the user will now see a "Quit" message,
>> however this seems pretty appropriate to me, what do you think?
>>
>> OK to apply?
> 
> OK.

Committed ahead of patch 2/4 as they are independent.

Thanks,
Andrew

[PATCH 4/4] Remove deprecated_throw_reason.

[Andrew Burgess]

With no uses left, this removes deprecated_throw_reason.

OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-07-30  Andrew Burgess  <aburgess@broadcom.com>

	* exceptions.c (deprecated_throw_reason): Remove.
	* exceptions.h (deprecated_throw_reason): Remove.

diff --git a/gdb/exceptions.c b/gdb/exceptions.c
index 416d81d..c4c1e57 100644
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -234,28 +234,6 @@ throw_exception (struct gdb_exception exception)
   EXCEPTIONS_SIGLONGJMP (current_catcher->buf, exception.reason);
 }
 
-void
-deprecated_throw_reason (enum return_reason reason)
-{
-  struct gdb_exception exception;
-
-  memset (&exception, 0, sizeof exception);
-
-  exception.reason = reason;
-  switch (reason)
-    {
-    case RETURN_QUIT:
-      break;
-    case RETURN_ERROR:
-      exception.error = GENERIC_ERROR;
-      break;
-    default:
-      internal_error (__FILE__, __LINE__, _("bad switch"));
-    }
-  
-  throw_exception (exception);
-}
-
 static void
 print_flush (void)
 {
diff --git a/gdb/exceptions.h b/gdb/exceptions.h
index 19eacd3..bf41860 100644
--- a/gdb/exceptions.h
+++ b/gdb/exceptions.h
@@ -186,11 +186,6 @@ extern void throw_vfatal (const char *fmt, va_list ap)
 extern void throw_error (enum errors error, const char *fmt, ...)
      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (2, 3);
 
-/* Instead of deprecated_throw_reason, code should use
-   throw_exception.  */
-extern void deprecated_throw_reason (enum return_reason reason)
-     ATTRIBUTE_NORETURN;
-
 /* Call FUNC(UIOUT, FUNC_ARGS) but wrapped within an exception
    handler.  If an exception (enum return_reason) is thrown using
    throw_exception() than all cleanups installed since

Re: [PATCH 4/4] Remove deprecated_throw_reason.

[Pedro Alves]

On 07/30/2013 05:12 PM, Andrew Burgess wrote:
> With no uses left, this removes deprecated_throw_reason.
> 
> OK to apply?
> 
> Thanks,
> Andrew
> 
> gdb/ChangeLog
> 
> 2013-07-30  Andrew Burgess  <aburgess@broadcom.com>
> 
> 	* exceptions.c (deprecated_throw_reason): Remove.
> 	* exceptions.h (deprecated_throw_reason): Remove.

OK.

-- 
Pedro Alves

Re: [PATCH 4/4] Remove deprecated_throw_reason.

[Andrew Burgess]

On 30/07/2013 8:21 PM, Pedro Alves wrote:
> On 07/30/2013 05:12 PM, Andrew Burgess wrote:
>> With no uses left, this removes deprecated_throw_reason.
>>
>> OK to apply?
>>
>> Thanks,
>> Andrew
>>
>> gdb/ChangeLog
>>
>> 2013-07-30  Andrew Burgess  <aburgess@broadcom.com>
>>
>> 	* exceptions.c (deprecated_throw_reason): Remove.
>> 	* exceptions.h (deprecated_throw_reason): Remove.
> 
> OK.

Committed.

Thanks,
Andrew