[RFA] print error message if (auto) disassembly failed

[RFA] print error message if (auto) disassembly failed

[Joel Brobecker]

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

I noticed this by accident after we introduced the new feature where
the debugger prints the assembly of the next line each time we stop.
Basically, I loaded the wrong core file and got:

        (gdb) core core
        warning: core file may not match specified executable file.
        Core was generated by `./crash'.
        Program terminated with signal 6, Aborted.
        #0  0x00007f8befcc8307 in ?? ()
 !!!->  0x00007f8befcc8307:     (gdb)

As you can see, GDB tried to print the instruction at the address
where the core file says the program stopped, but instead printed
nothing. This has two consequences:

  1. The user does not really know what the problem was;
  2. A testsuite driver that relies on seeing the prompt being placed
     at the beginning of the line would wait indefinitely for the prompt
     to come back.

I propose to change the behavior to print the error message:

        (gdb) core core
        warning: core file may not match specified executable file.
        Core was generated by `./crash'.
        Program terminated with signal 6, Aborted.
        #0  0x00007f8befcc8307 in ?? ()
        0x00007f8befcc8307:     Cannot access memory at address 0x7f8befcc8307
        (gdb) 

2009-04-16  Joel Brobecker  <brobecker@adacore.com>

        * stack.c (do_gdb_disassembly): Print the exception message if an
        error was thrown while trying to perform the disassembly.

Tested on x86_64-linux. Any objections?

(FWIW: I did not realize that GDB would now print the current assembly
 instruction by default when the change came in. I don't think that's
 a useful default based on my own usage, but no big deal)

-- 
Joel

[-- Attachment #2: disass.diff --]
[-- Type: text/x-diff, Size: 822 bytes --]

commit 056d77e1b180988399b18ea5d109813dbd934da8
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Tue Apr 14 15:23:25 2009 -0700

        * stack.c (do_gdb_disassembly): Print the exception message if an
        error was thrown while trying to perform the disassembly.

diff --git a/gdb/stack.c b/gdb/stack.c
index dfe3900..956aaa0 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -507,6 +507,10 @@ do_gdb_disassembly (int how_many, CORE_ADDR low, CORE_ADDR high)
     {
       gdb_disassembly_stub (&args);
     }
+  /* If an exception was thrown while doing the disassembly, print
+     the error message, to give the user a clue of what happened.  */
+  if (exception.reason == RETURN_ERROR)
+    exception_print (gdb_stderr, exception);
 }
 
 /* Print information about frame FRAME.  The output is format according

Re: [RFA] print error message if (auto) disassembly failed

[Eli Zaretskii]

> Date: Thu, 16 Apr 2009 10:39:18 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> I propose to change the behavior to print the error message:
> 
>         (gdb) core core
>         warning: core file may not match specified executable file.
>         Core was generated by `./crash'.
>         Program terminated with signal 6, Aborted.
>         #0  0x00007f8befcc8307 in ?? ()
>         0x00007f8befcc8307:     Cannot access memory at address 0x7f8befcc8307
>         (gdb) 

How about

        #0  0x00007f8befcc8307 in ?? ()
        0x00007f8befcc8307:     (bad)

instead?  This is consistent with what "display/i $pc" displays if $pc
has a bad value, such as if you somehow land in the middle of an
instruction.

Re: [RFA] print error message if (auto) disassembly failed

[Joel Brobecker]

> How about
> 
>         #0  0x00007f8befcc8307 in ?? ()
>         0x00007f8befcc8307:     (bad)
> 
> instead?  This is consistent with what "display/i $pc" displays if $pc
> has a bad value, such as if you somehow land in the middle of an
> instruction.

The problem is that this is not actually a "bad" instruction in this case,
but an invalid address. So I think that the "Cannot read memory at ..."
error message is more accurate.

-- 
Joel

Re: [RFA] print error message if (auto) disassembly failed

[Eli Zaretskii]

> Date: Thu, 16 Apr 2009 11:01:46 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > How about
> > 
> >         #0  0x00007f8befcc8307 in ?? ()
> >         0x00007f8befcc8307:     (bad)
> > 
> > instead?  This is consistent with what "display/i $pc" displays if $pc
> > has a bad value, such as if you somehow land in the middle of an
> > instruction.
> 
> The problem is that this is not actually a "bad" instruction in this case,
> but an invalid address.

But an invalid address can sometimes (accidentally) be readable,
right?  What will we print then?

Re: [RFA] print error message if (auto) disassembly failed

[Joel Brobecker]

> But an invalid address can sometimes (accidentally) be readable,
> right?  What will we print then?

It looks like this "(bad)" is printed by opcode, not GDB.
So we should get your "(bad)" output if the address is valid
but points to an invalid instruction.

-- 
Joel

Re: [RFA] print error message if (auto) disassembly failed

[Eli Zaretskii]

> Date: Thu, 16 Apr 2009 12:06:54 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > But an invalid address can sometimes (accidentally) be readable,
> > right?  What will we print then?
> 
> It looks like this "(bad)" is printed by opcode, not GDB.
> So we should get your "(bad)" output if the address is valid
> but points to an invalid instruction.

OK, but your suggested message text is too long, I think.  Something
shorter, like "(address unreadable)" is better, IMO.

Re: [RFA] print error message if (auto) disassembly failed

[Joel Brobecker]

> OK, but your suggested message text is too long, I think.  Something
> shorter, like "(address unreadable)" is better, IMO.

I just printing the error message that has been "thrown". I don't see
any way of printing what you want without parsing that error message.
Do you see any?

-- 
Joel

Re: [RFA] print error message if (auto) disassembly failed

[Pedro Alves]

On Thursday 16 April 2009 22:54:26, Joel Brobecker wrote:
> > OK, but your suggested message text is too long, I think.  Something
> > shorter, like "(address unreadable)" is better, IMO.
> 
> I just printing the error message that has been "thrown". I don't see
> any way of printing what you want without parsing that error message.
> Do you see any?

I haven't been following the thread, so sorry if this overshoots
too far away, but, anyway, notice that there's now a MEMORY_ERROR error
class in exceptions.h.  If you have access to the error exception object,
you can check on ex.error.  See breakpoint.c:fetch_watchpoint_value for
a MEMORY_ERROR filtering usage.

-- 
Pedro Alves

Re: [RFA] print error message if (auto) disassembly failed

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> (FWIW: I did not realize that GDB would now print the current assembly
Joel>  instruction by default when the change came in. I don't think that's
Joel>  a useful default based on my own usage, but no big deal)

The default is "auto", which means:

   If AUTO (which is the default), or there's no line info to determine
   the source line of the next instruction, display disassembly of next
   instruction instead.  */

But I agree with you that this is the wrong default.
I think the default should be "off".

Tom

Re: [RFA] print error message if (auto) disassembly failed

[Joel Brobecker]

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

> OK, but your suggested message text is too long, I think.  Something
> shorter, like "(address unreadable)" is better, IMO.

Frankly, I think this is overkill - even with the 64bit addresses
the line was still less than 70 characters long. But I'm not going
to argue. Here you go:

    (gdb) core core
    warning: core file may not match specified executable file.
    Core was generated by `./crash'.
    Program terminated with signal 6, Aborted.
    #0  0x00007fe6fe543307 in ?? ()
    0x00007fe6fe543307:     (cannot read memory)

2009-04-17  Joel Brobecker  <brobecker@adacore.com>

        * stack.c (do_gdb_disassembly): Print an error message if an error
        was thrown while trying to perform the disassembly.

I'm currently testing this patch on x86_64-linux, but I don't really
expect any problem.

-- 
Joel

[-- Attachment #2: stack.diff --]
[-- Type: text/x-diff, Size: 1128 bytes --]

commit de33df66fc64edf55aa81a7d08adccf911aa4fdf
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Tue Apr 14 15:23:25 2009 -0700

        * stack.c (do_gdb_disassembly): Print an error message if an error
        was thrown while trying to perform the disassembly.

diff --git a/gdb/stack.c b/gdb/stack.c
index dfe3900..bbeff2c 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -507,6 +507,21 @@ do_gdb_disassembly (int how_many, CORE_ADDR low, CORE_ADDR high)
     {
       gdb_disassembly_stub (&args);
     }
+  /* If an exception was thrown while doing the disassembly, print
+     the error message, to give the user a clue of what happened.  */
+  if (exception.reason == RETURN_ERROR)
+    {
+      if (exception.error == MEMORY_ERROR)
+	{
+	  /* For memory errors, prefer a short error message over
+	     the error message provided in the exception.  */
+	  gdb_flush (gdb_stdout);
+	  gdb_flush (gdb_stderr);
+	  fprintf_filtered (gdb_stderr, _("(cannot read memory)\n"));
+	}
+      else
+	exception_print (gdb_stderr, exception);
+    }
 }
 
 /* Print information about frame FRAME.  The output is format according

Re: [RFA] print error message if (auto) disassembly failed

[Joel Brobecker]

> The default is "auto", which means:
> 
>    If AUTO (which is the default), or there's no line info to determine
>    the source line of the next instruction, display disassembly of next
>    instruction instead.  */
> 
> But I agree with you that this is the wrong default.
> I think the default should be "off".

Would anyone object to changing the default to "off"?

-- 
Joel

Re: [RFA] print error message if (auto) disassembly failed

[Hui Zhu]

Hi Guys,

Sorry for late.

In before, when the user step into the program that don't have line
message,  they will lost the the help from debuger.  They will not
know what will happen in next except a pc address.

For now, this auto will auto output the asm code.  I think it will
make user experience (special for new user) better.
And I think  Joel's patch fix the bug.

So I suggest keep it default "auto".

Thanks,
Hui

On Fri, Apr 17, 2009 at 07:59, Joel Brobecker <brobecker@adacore.com> wrote:
>> The default is "auto", which means:
>>
>>    If AUTO (which is the default), or there's no line info to determine
>>    the source line of the next instruction, display disassembly of next
>>    instruction instead.  */
>>
>> But I agree with you that this is the wrong default.
>> I think the default should be "off".
>
> Would anyone object to changing the default to "off"?
>
> --
> Joel
>

Re: [RFA] print error message if (auto) disassembly failed

[Hui Zhu]

I try it in x86-linux with simple inferior and core file.
This patch works very well.

Thanks,
Hui

On Fri, Apr 17, 2009 at 07:57, Joel Brobecker <brobecker@adacore.com> wrote:
>> OK, but your suggested message text is too long, I think.  Something
>> shorter, like "(address unreadable)" is better, IMO.
>
> Frankly, I think this is overkill - even with the 64bit addresses
> the line was still less than 70 characters long. But I'm not going
> to argue. Here you go:
>
>    (gdb) core core
>    warning: core file may not match specified executable file.
>    Core was generated by `./crash'.
>    Program terminated with signal 6, Aborted.
>    #0  0x00007fe6fe543307 in ?? ()
>    0x00007fe6fe543307:     (cannot read memory)
>
> 2009-04-17  Joel Brobecker  <brobecker@adacore.com>
>
>        * stack.c (do_gdb_disassembly): Print an error message if an error
>        was thrown while trying to perform the disassembly.
>
> I'm currently testing this patch on x86_64-linux, but I don't really
> expect any problem.
>
> --
> Joel
>

Re: [RFA] print error message if (auto) disassembly failed

[Eli Zaretskii]

> Date: Thu, 16 Apr 2009 14:54:26 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > OK, but your suggested message text is too long, I think.  Something
> > shorter, like "(address unreadable)" is better, IMO.
> 
> I just printing the error message that has been "thrown". I don't see
> any way of printing what you want without parsing that error message.
> Do you see any?

No, I guess not.  Too bad that we are at the mercy of some code that
has no idea of the context.

Re: [RFA] print error message if (auto) disassembly failed

[Eli Zaretskii]

> Date: Thu, 16 Apr 2009 16:57:35 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > OK, but your suggested message text is too long, I think.  Something
> > shorter, like "(address unreadable)" is better, IMO.
> 
> Frankly, I think this is overkill - even with the 64bit addresses
> the line was still less than 70 characters long. But I'm not going
> to argue. Here you go:
> 
>     (gdb) core core
>     warning: core file may not match specified executable file.
>     Core was generated by `./crash'.
>     Program terminated with signal 6, Aborted.
>     #0  0x00007fe6fe543307 in ?? ()
>     0x00007fe6fe543307:     (cannot read memory)

Fine with me, although "cannot read memory" is a bit more general,
English-wise, than the actual cause, which is that this specific
address is unreadable.

Re: [RFA] print error message if (auto) disassembly failed

[Eli Zaretskii]

> Date: Thu, 16 Apr 2009 16:59:44 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > The default is "auto", which means:
> > 
> >    If AUTO (which is the default), or there's no line info to determine
> >    the source line of the next instruction, display disassembly of next
> >    instruction instead.  */
> > 
> > But I agree with you that this is the wrong default.
> > I think the default should be "off".
> 
> Would anyone object to changing the default to "off"?

My vote is to set it to "off", since this is additional functionality
that evidently not everyone likes.

Re: [RFA] print error message if (auto) disassembly failed

[Eli Zaretskii]

> Date: Fri, 17 Apr 2009 12:23:48 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: tromey@redhat.com, teawater@gmail.com, gdb-patches@sourceware.org
> 
> > Date: Thu, 16 Apr 2009 16:59:44 -0700
> > From: Joel Brobecker <brobecker@adacore.com>
> > Cc: gdb-patches@sourceware.org
> > 
> > > The default is "auto", which means:
> > > 
> > >    If AUTO (which is the default), or there's no line info to determine
> > >    the source line of the next instruction, display disassembly of next
> > >    instruction instead.  */
> > > 
> > > But I agree with you that this is the wrong default.
> > > I think the default should be "off".
> > 
> > Would anyone object to changing the default to "off"?
> 
> My vote is to set it to "off", since this is additional functionality
> that evidently not everyone likes.

Btw, the documentation of "auto" is misleading: it makes it sound like
it should _always_ display the disassembly of the next instruction.
But in fact, it only does so when you step over code that doesn't have
line info information or whose source file is unavailable.  So how
about the following patch?  (If it is accepted, I will fix the manual
as well.)

2009-04-17  Eli Zaretskii  <eliz@gnu.org>

	* stack.c (_initialize_stack) <disassemble-next-line>: Doc fix.

--- gdb/stack.c~	2009-04-02 04:05:29.000000000 +0300
+++ gdb/stack.c	2009-04-17 13:00:05.940500000 +0300
@@ -2145,13 +2145,16 @@
 
   add_setshow_auto_boolean_cmd ("disassemble-next-line", class_stack,
 			        &disassemble_next_line, _("\
-Set whether to disassemble next source line when execution stops."), _("\
-Show whether to disassemble next source line when execution stops."), _("\
-If ON, GDB will display disassembly of the next source line when\n\
-execution of the program being debugged stops.\n\
-If AUTO (which is the default), or there's no line info to determine\n\
-the source line of the next instruction, display disassembly of next\n\
-instruction instead."),
+Set whether to disassemble next source line or insn when execution stops."), _("\
+Show whether to disassemble next source line or insn when execution stops."), _("\
+If ON, GDB will display disassembly of the next source line, in addition\n\
+to displaying the source line itself.  If the next source line cannot\n\
+be displayed (e.g., source is unavailable or there's no line info), GDB\n\
+will display disassembly of next instruction instead of showing the\n\
+source line.\n\
+If AUTO (which is the default), display disassembly of next\n\
+instruction only if the source line cannot be displayed.\n\
+If OFF, never display the disassembly of the next source line."),
 			        NULL,
 			        show_disassemble_next_line,
 			        &setlist, &showlist);

Re: [RFA] print error message if (auto) disassembly failed

[Joel Brobecker]

> >     (gdb) core core
> >     warning: core file may not match specified executable file.
> >     Core was generated by `./crash'.
> >     Program terminated with signal 6, Aborted.
> >     #0  0x00007fe6fe543307 in ?? ()
> >     0x00007fe6fe543307:     (cannot read memory)
> 
> Fine with me, although "cannot read memory" is a bit more general,
> English-wise, than the actual cause, which is that this specific
> address is unreadable.

I did not line "address unreadable" because we weren't reading
an address, we were reading the memory at that address, shown
on the left hand side.  That's why I proposed this instead, which
in my opinion is more accurate.

-- 
Joel

Re: [RFA] print error message if (auto) disassembly failed

[Joel Brobecker]

> In before, when the user step into the program that don't have line
> message,  they will lost the the help from debuger.  They will not
> know what will happen in next except a pc address.

IMO, the vast majority of gdb users will not care, or even understand 
what the instruction means. Most of the time, the only reason I would
personally land in code that doesn't have any debugging info is when
attaching to a program, loading a core file, or pressing control-c.
In such cases, the first thing I usually do is "break ...; continue".

I have very very very occasionally done stepi's that got me inside
such code, but in that case, I use a special user-defined command
that does the stepi and then prints the instruction.

> For now, this auto will auto output the asm code.  I think it will
> make user experience (special for new user) better.
> And I think  Joel's patch fix the bug.

It's not just about the bug, actually - I wasn't suggesting that
we change the default as a way to make it go away. Rather, I think
that this default setting will be useful to only very few people,
while at the same time creating the equivalent of noise for the
rest of the population. This is why I maintain my suggestion of
changing the default to "off".

-- 
Joel

Re: [RFA] print error message if (auto) disassembly failed

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> IMO, the vast majority of gdb users will not care, or even understand 
Joel> what the instruction means.

That's my belief as well.

Tom

Re: [RFA] print error message if (auto) disassembly failed

[Hui Zhu]

Agree with you.
I will post a patch for it.

Thanks,
Hui

On Fri, Apr 17, 2009 at 23:57, Joel Brobecker <brobecker@adacore.com> wrote:
>> In before, when the user step into the program that don't have line
>> message,  they will lost the the help from debuger.  They will not
>> know what will happen in next except a pc address.
>
> IMO, the vast majority of gdb users will not care, or even understand
> what the instruction means. Most of the time, the only reason I would
> personally land in code that doesn't have any debugging info is when
> attaching to a program, loading a core file, or pressing control-c.
> In such cases, the first thing I usually do is "break ...; continue".
>
> I have very very very occasionally done stepi's that got me inside
> such code, but in that case, I use a special user-defined command
> that does the stepi and then prints the instruction.
>
>> For now, this auto will auto output the asm code.  I think it will
>> make user experience (special for new user) better.
>> And I think  Joel's patch fix the bug.
>
> It's not just about the bug, actually - I wasn't suggesting that
> we change the default as a way to make it go away. Rather, I think
> that this default setting will be useful to only very few people,
> while at the same time creating the equivalent of noise for the
> rest of the population. This is why I maintain my suggestion of
> changing the default to "off".
>
> --
> Joel
>

Re: [RFA] print error message if (auto) disassembly failed

[Daniel Jacobowitz]

On Thu, Apr 16, 2009 at 04:57:35PM -0700, Joel Brobecker wrote:
> 2009-04-17  Joel Brobecker  <brobecker@adacore.com>
> 
>         * stack.c (do_gdb_disassembly): Print an error message if an error
>         was thrown while trying to perform the disassembly.
> 
> I'm currently testing this patch on x86_64-linux, but I don't really
> expect any problem.

My two cents: this should really match whatever x/i does.  Is that
code now shared, or does it still have the old (IMO perfectly fine)
error message?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] print error message if (auto) disassembly failed

[Joel Brobecker]

> > 2009-04-17  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * stack.c (do_gdb_disassembly): Print an error message if an error
> >         was thrown while trying to perform the disassembly.
> > 
> > I'm currently testing this patch on x86_64-linux, but I don't really
> > expect any problem.
> 
> My two cents: this should really match whatever x/i does.  Is that
> code now shared, or does it still have the old (IMO perfectly fine)
> error message?

The code is not shared. They both call gdbarch_print_insn to print
the instruction but from different code paths. In the "x/i" case,
I think that the top-level command exception handler ends up printing
the exception message.  In the case at hand, do_gdb_disassembly traps
the exception. What my first patch does is just print the exception
message to give the user a clue of what's going on.  What the second
patch adds is a bit of extra code to handle memory errors differently,
in order to print the shorter error message.

Since we can decide on the particular error message we want for
memory errors is only midly related to the issue of printing something,
I suggest I commit my first patch, and let Eli promote the idea of
his shorter error message. I agree to write and test the change,
but I don't want to spend time discussing something that I think
is only cosmetic.

-- 
Joel

Re: [RFA] print error message if (auto) disassembly failed

[Eli Zaretskii]

> Date: Fri, 17 Apr 2009 15:16:10 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> I suggest I commit my first patch, and let Eli promote the idea of
> his shorter error message.

Thanks for the offer, but I don't think I will have enough resources
to promote that idea.  I expressed my opinion; if it is not shared by
others, I don't think this is an issue that is serious enough to start
a dedicated discussion.

Thanks.

Re: [RFA] print error message if (auto) disassembly failed

[Joel Brobecker]

> 2009-04-16  Joel Brobecker  <brobecker@adacore.com>
> 
>         * stack.c (do_gdb_disassembly): Print the exception message if an
>         error was thrown while trying to perform the disassembly.

I checked this one in.

-- 
Joel