[patch] print a more useful error message for "gdb core"

[patch] print a more useful error message for "gdb core"

[Doug Evans]

Hi.

If the user accidentally does "gdb core" gdb currently prints:

"/foo/bar": not in executable format: File format not recognized

This patch prints a more useful error message for this particular case:

"/foo/bar" is a core file.
Please specify an executable to debug.

I will check this in in a few days if there are no objections.

2010-01-20  Doug Evans  <dje@google.com>

	* exec.c (exec_file_attach): Print a more useful error message if the
	user did "gdb core".

Index: exec.c
===================================================================
RCS file: /cvs/src/src/gdb/exec.c,v
retrieving revision 1.95
diff -u -p -r1.95 exec.c
--- exec.c	19 Jan 2010 09:47:47 -0000	1.95
+++ exec.c	20 Jan 2010 23:50:10 -0000
@@ -253,6 +253,13 @@ exec_file_attach (char *filename, int fr
       scratch_pathname = xstrdup (scratch_pathname);
       cleanups = make_cleanup (xfree, scratch_pathname);
 
+      /* If the user accidentally did "gdb core", print a more useful
+	 error message.  */
+      if (bfd_check_format (exec_bfd, bfd_core))
+	error (_("\"%s\" is a core file.\n"
+		 "Please specify an executable to debug."),
+		 scratch_pathname);
+
       if (!bfd_check_format (exec_bfd, bfd_object))
 	{
 	  /* Make sure to close exec_bfd, or else "run" might try to use

Re: [patch] print a more useful error message for "gdb core"

[Doug Evans]

On Wed, Jan 20, 2010 at 3:56 PM, Doug Evans <dje@google.com> wrote:
> Hi.
>
> If the user accidentally does "gdb core" gdb currently prints:
>
> "/foo/bar": not in executable format: File format not recognized
>
> This patch prints a more useful error message for this particular case:
>
> "/foo/bar" is a core file.
> Please specify an executable to debug.
>
> I will check this in in a few days if there are no objections.

As noted in irc, the joys of working between multiple trees.
The previous patch didn't call exec_close.

Here's an updated patch.

2010-01-20  Doug Evans  <dje@google.com>

        * exec.c (exec_file_attach): Print a more useful error message if the
        user did "gdb core".

Index: exec.c
===================================================================
RCS file: /cvs/src/src/gdb/exec.c,v
retrieving revision 1.95
diff -u -p -r1.95 exec.c
--- exec.c      19 Jan 2010 09:47:47 -0000      1.95
+++ exec.c      21 Jan 2010 00:10:17 -0000
@@ -253,6 +253,18 @@ exec_file_attach (char *filename, int fr
       scratch_pathname = xstrdup (scratch_pathname);
       cleanups = make_cleanup (xfree, scratch_pathname);

+      /* If the user accidentally did "gdb core", print a useful
+        error message.  */
+      if (bfd_check_format (exec_bfd, bfd_core))
+       {
+         /* Make sure to close exec_bfd, or else "run" might try to use
+            it.  */
+         exec_close ();
+         error (_("\"%s\" is a core file.\n"
+                  "Please specify an executable to debug."),
+                scratch_pathname);
+       }
+
       if (!bfd_check_format (exec_bfd, bfd_object))
        {
          /* Make sure to close exec_bfd, or else "run" might try to use

Re: [patch] print a more useful error message for "gdb core"

[Hui Zhu]

Hello,

If you don't mind, I suggest change bfd_check_format to
bfd_check_format_matches like
http://sourceware.org/ml/gdb-patches/2010-01/msg00468.html

It will give user more help when he got bfd_error_file_ambiguously_recognized.

Thanks,
Hui

On Thu, Jan 21, 2010 at 08:12, Doug Evans <dje@google.com> wrote:
> On Wed, Jan 20, 2010 at 3:56 PM, Doug Evans <dje@google.com> wrote:
>> Hi.
>>
>> If the user accidentally does "gdb core" gdb currently prints:
>>
>> "/foo/bar": not in executable format: File format not recognized
>>
>> This patch prints a more useful error message for this particular case:
>>
>> "/foo/bar" is a core file.
>> Please specify an executable to debug.
>>
>> I will check this in in a few days if there are no objections.
>
> As noted in irc, the joys of working between multiple trees.
> The previous patch didn't call exec_close.
>
> Here's an updated patch.
>
> 2010-01-20  Doug Evans  <dje@google.com>
>
>        * exec.c (exec_file_attach): Print a more useful error message if the
>        user did "gdb core".
>
> Index: exec.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/exec.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 exec.c
> --- exec.c      19 Jan 2010 09:47:47 -0000      1.95
> +++ exec.c      21 Jan 2010 00:10:17 -0000
> @@ -253,6 +253,18 @@ exec_file_attach (char *filename, int fr
>       scratch_pathname = xstrdup (scratch_pathname);
>       cleanups = make_cleanup (xfree, scratch_pathname);
>
> +      /* If the user accidentally did "gdb core", print a useful
> +        error message.  */
> +      if (bfd_check_format (exec_bfd, bfd_core))
> +       {
> +         /* Make sure to close exec_bfd, or else "run" might try to use
> +            it.  */
> +         exec_close ();
> +         error (_("\"%s\" is a core file.\n"
> +                  "Please specify an executable to debug."),
> +                scratch_pathname);
> +       }
> +
>       if (!bfd_check_format (exec_bfd, bfd_object))
>        {
>          /* Make sure to close exec_bfd, or else "run" might try to use
>

Re: [patch] print a more useful error message for "gdb core"

[Jan Kratochvil]

On Thu, 21 Jan 2010 01:12:49 +0100, Doug Evans wrote:
> On Wed, Jan 20, 2010 at 3:56 PM, Doug Evans <dje@google.com> wrote:
> > If the user accidentally does "gdb core" gdb currently prints:
> >
> > "/foo/bar": not in executable format: File format not recognized
> >
> > This patch prints a more useful error message for this particular case:
> >
> > "/foo/bar" is a core file.
> > Please specify an executable to debug.

I would prefer to try automatically loading it as a core file ("gdb -c core")
and to print a warning instead of error.  Fedora GDB is patched for build-id
(still not submitted for FSF) so that it can find the executable in such case.

But maybe such change should go in only with the build-id patch itself.


Regards,
Jan

Re: [patch] print a more useful error message for "gdb core"

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I would prefer to try automatically loading it as a core file ("gdb
Jan> -c core") and to print a warning instead of error.  Fedora GDB is
Jan> patched for build-id (still not submitted for FSF) so that it can
Jan> find the executable in such case.

I agree, but I don't think this should block Doug's patch.
It seems to me that something like Doug's patch will be needed anyway,
if the build-id is not available.

Tom

Re: [patch] print a more useful error message for "gdb core"

[Doug Evans]

On Thu, Jan 21, 2010 at 5:58 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Thu, 21 Jan 2010 01:12:49 +0100, Doug Evans wrote:
>> On Wed, Jan 20, 2010 at 3:56 PM, Doug Evans <dje@google.com> wrote:
>> > If the user accidentally does "gdb core" gdb currently prints:
>> >
>> > "/foo/bar": not in executable format: File format not recognized
>> >
>> > This patch prints a more useful error message for this particular case:
>> >
>> > "/foo/bar" is a core file.
>> > Please specify an executable to debug.
>
> I would prefer to try automatically loading it as a core file ("gdb -c core")
> and to print a warning instead of error.  Fedora GDB is patched for build-id
> (still not submitted for FSF) so that it can find the executable in such case.

One could do that alright.

Not an exhaustive list, but if we go down the path of converting "gdb
corefile" to "gdb -c corefile", then we also need to think about "file
corefile" being converted to "core corefile" [or "target core
corefile", "core" is apparently deprecated in favor of "target core"]
and "target exec corefile" -> "target core corefile".  Presumably
"file corefile" (and "target exec corefile") would discard the
currently selected executable.  But maybe not.  Will that be confusing
for users?  I don't know.

Whatever you want is fine with me.

Re: [patch] print a more useful error message for "gdb core"

[Jan Kratochvil]

On Thu, 21 Jan 2010 18:17:15 +0100, Doug Evans wrote:
> Not an exhaustive list, but if we go down the path of converting "gdb
> corefile" to "gdb -c corefile", then we also need to think about "file
> corefile" being converted to "core corefile" [or "target core
> corefile", "core" is apparently deprecated in favor of "target core"]
> and "target exec corefile" -> "target core corefile".  Presumably
> "file corefile" (and "target exec corefile") would discard the
> currently selected executable.  But maybe not.  Will that be confusing
> for users?  I don't know.

While thinking about it overriding some GDB _commands_ was not my intention.

There is a general assumption if I have a shell COMMAND and some FILE I can do
$ COMMAND FILE
and COMMAND will appropriately load the FILE.

FSF GDB currently needs to specify also the executable file for core files
which already inhibits this intuitive expectation.  OTOH with the build-id
locating patch which could allow such intuitive start  notneeding the
executable file.  Still it currently did not work due to the required "-c":
$ COMMAND -c COREFILE

Entering "file", "core-file" or "attach" commands is already explicit enough
so that it IMO should do what the command name says without any
autodetections.  The second command line argument
(captured_main->pid_or_core_arg) is also autodetected (for PID or CORE) but
neither "attach" accepts a core file nor "core-file" accepts a PID.


The patch makes sense only with the build-id patchset so this is not submit
for FSF GDB inclusion yet.  I am fine with your patch (+/- Hui Zhu's pending
bfd_check_format_matches) as the patch below is its natural extension.


Sorry for the delay,
Jan


2010-01-25  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* exceptions.h (enum errors <IS_CORE_ERROR>): New.
	* exec.c: Include exceptions.h.
	(exec_file_attach <bfd_core>): Call throw_error (IS_CORE_ERROR, ...).
	* main.c (exec_or_core_file_attach): New.
	(captured_main <optind < argc>): Set also corearg.
	(captured_main <strcmp (execarg, symarg) == 0>): New variable func.
	Call exec_or_core_file_attach if COREARG matches EXECARG.  Call
	symbol_file_add_main only if CORE_BFD remained NULL.

Http://sourceware.org/ml/gdb-patches/2010-01/msg00517.html
2010-01-20  Doug Evans  <dje@google.com>

	* exec.c (exec_file_attach): Print a more useful error message if the
	user did "gdb core".

--- a/gdb/exceptions.h
+++ b/gdb/exceptions.h
@@ -78,6 +78,9 @@ enum errors {
   /* Feature is not supported in this copy of GDB.  */
   UNSUPPORTED_ERROR,
 
+  /* Attempt to load a core file as executable.  */
+  IS_CORE_ERROR,
+
   /* Add more errors here.  */
   NR_ERRORS
 };
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -34,6 +34,7 @@
 #include "arch-utils.h"
 #include "gdbthread.h"
 #include "progspace.h"
+#include "exceptions.h"
 
 #include <fcntl.h>
 #include "readline/readline.h"
@@ -253,6 +254,19 @@ exec_file_attach (char *filename, int from_tty)
       scratch_pathname = xstrdup (scratch_pathname);
       cleanups = make_cleanup (xfree, scratch_pathname);
 
+      /* If the user accidentally did "gdb core", print a useful
+	 error message.  */
+      if (bfd_check_format (exec_bfd, bfd_core))
+       {
+	 /* Make sure to close exec_bfd, or else "run" might try to use
+	    it.  */
+	 exec_close ();
+	 throw_error (IS_CORE_ERROR,
+		      _("\"%s\" is a core file.\n"
+			"Please specify an executable to debug."),
+		      scratch_pathname);
+       }
+
       if (!bfd_check_format (exec_bfd, bfd_object))
 	{
 	  /* Make sure to close exec_bfd, or else "run" might try to use
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -240,6 +240,36 @@ captured_command_loop (void *data)
   return 1;
 }
 
+/* Call exec_file_attach.  If it detected FILENAME is a core file call
+   core_file_command.  Print the original exec_file_attach error only if
+   core_file_command failed to find a matching executable.  */
+
+static void
+exec_or_core_file_attach (char *filename, int from_tty)
+{
+  volatile struct gdb_exception e;
+
+  gdb_assert (exec_bfd == NULL);
+
+  TRY_CATCH (e, RETURN_MASK_ALL)
+    {
+      exec_file_attach (filename, from_tty);
+    }
+  if (e.reason < 0)
+    {
+      if (e.error == IS_CORE_ERROR)
+	{
+	  core_file_command (filename, from_tty);
+
+	  /* Iff the core file found its executable suppress the error message
+	     from exec_file_attach.  */
+	  if (exec_bfd != NULL)
+	    return;
+	}
+      throw_exception (e);
+    }
+}
+
 static int
 captured_main (void *data)
 {
@@ -668,6 +698,7 @@ extern int gdbtk_test (char *);
 	{
 	  symarg = argv[optind];
 	  execarg = argv[optind];
+	  corearg = argv[optind];
 	  optind++;
 	}
 
@@ -800,10 +831,25 @@ Excess command line arguments ignored. (%s%s)\n"),
       && symarg != NULL
       && strcmp (execarg, symarg) == 0)
     {
+      catch_command_errors_ftype *func;
+
+      /* Call exec_or_core_file_attach only if the file was specified as
+	 a command line argument (and not an a command line option).  */
+      if (corearg != NULL && strcmp (corearg, execarg) == 0)
+	{
+	  func = exec_or_core_file_attach;
+	  corearg = NULL;
+	}
+      else
+	func = exec_file_attach;
+
       /* The exec file and the symbol-file are the same.  If we can't
-         open it, better only print one error message.
-         catch_command_errors returns non-zero on success! */
-      if (catch_command_errors (exec_file_attach, execarg, !batch, RETURN_MASK_ALL))
+	 open it, better only print one error message.
+	 catch_command_errors returns non-zero on success!
+	 Do not load EXECARG as a symbol file if it has been already processed
+	 as a core file.  */
+      if (catch_command_errors (func, execarg, !batch, RETURN_MASK_ALL)
+	  && core_bfd == NULL)
 	catch_command_errors (symbol_file_add_main, symarg, !batch, RETURN_MASK_ALL);
     }
   else

Re: [patch] print a more useful error message for "gdb core"

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]

Hi Doug,

On Thu, 21 Jan 2010 00:56:05 +0100, Doug Evans wrote:
> --- exec.c	19 Jan 2010 09:47:47 -0000	1.95
> +++ exec.c	20 Jan 2010 23:50:10 -0000
> @@ -253,6 +253,13 @@ exec_file_attach (char *filename, int fr
>        scratch_pathname = xstrdup (scratch_pathname);
>        cleanups = make_cleanup (xfree, scratch_pathname);
>  
> +      /* If the user accidentally did "gdb core", print a more useful
> +	 error message.  */
> +      if (bfd_check_format (exec_bfd, bfd_core))
> +	error (_("\"%s\" is a core file.\n"
> +		 "Please specify an executable to debug."),
> +		 scratch_pathname);
> +
>        if (!bfd_check_format (exec_bfd, bfd_object))
>  	{
>  	  /* Make sure to close exec_bfd, or else "run" might try to use


this patch has a regression with --enable-targets=all with i386-unknown-linux
gnu native host as at least the attached i386 .so file gets identified as
"trad-core" (and it gets no longer identified as bfd_object afterwards).

One should try the bfd_core variant only if bfd_object has already failed.


Regards,
Jan

[-- Attachment #2: trad-core.so.gz --]
[-- Type: application/x-gzip, Size: 2067 bytes --]

Re: [patch] print a more useful error message for "gdb core"

[Doug Evans]

On Thu, Jan 28, 2010 at 1:37 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Hi Doug,
>
> On Thu, 21 Jan 2010 00:56:05 +0100, Doug Evans wrote:
>> --- exec.c    19 Jan 2010 09:47:47 -0000      1.95
>> +++ exec.c    20 Jan 2010 23:50:10 -0000
>> @@ -253,6 +253,13 @@ exec_file_attach (char *filename, int fr
>>        scratch_pathname = xstrdup (scratch_pathname);
>>        cleanups = make_cleanup (xfree, scratch_pathname);
>>
>> +      /* If the user accidentally did "gdb core", print a more useful
>> +      error message.  */
>> +      if (bfd_check_format (exec_bfd, bfd_core))
>> +     error (_("\"%s\" is a core file.\n"
>> +              "Please specify an executable to debug."),
>> +              scratch_pathname);
>> +
>>        if (!bfd_check_format (exec_bfd, bfd_object))
>>       {
>>         /* Make sure to close exec_bfd, or else "run" might try to use
>
>
> this patch has a regression with --enable-targets=all with i386-unknown-linux
> gnu native host as at least the attached i386 .so file gets identified as
> "trad-core" (and it gets no longer identified as bfd_object afterwards).
>
> One should try the bfd_core variant only if bfd_object has already failed.

Thanks.  You've got an extension in progress right. I don't mind just
waiting for that.