[patch] More suggestive displaced-stepping memory error message

[patch] More suggestive displaced-stepping memory error message

[Jan Kratochvil]

Hi,

with
	gdbserver ld-linux-x86-64.so.2 /bin/prog
and
	gdb /bin/prog
in non-stop mode during ld.so initialization happens an error as
displaced-stepping buffer at /bin/prog's _start is not yet available.

Also I think in single-threaded mode displaced-stepping is not useful.
But I understand turning off displaced-stepping if inferior has only one
thread would be too complicating all the troubleshooting.

With all the races and errors I would definitely find this message helpful.
It should at least state the error occured for the displaced-stepping buffer,
not just generic
	Cannot access memory at address %s

No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu.
I may check it in on my own without the "you may want .*" part.


Thanks,
Jan


gdb/
2012-04-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Provide more specific displaced-stepping memory error message.
	* infrun.c (displaced_step_prepare): New variable status.  Call
	target_read_memory instead of read_memory, provide more specific
	error message.

--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1296,6 +1296,7 @@ displaced_step_prepare (ptid_t ptid)
   ULONGEST len;
   struct displaced_step_closure *closure;
   struct displaced_step_inferior_state *displaced;
+  int status;
 
   /* We should never reach this function if the architecture does not
      support displaced stepping.  */
@@ -1356,7 +1357,13 @@ displaced_step_prepare (ptid_t ptid)
   displaced->step_saved_copy = xmalloc (len);
   ignore_cleanups = make_cleanup (free_current_contents,
 				  &displaced->step_saved_copy);
-  read_memory (copy, displaced->step_saved_copy, len);
+  status = target_read_memory (copy, displaced->step_saved_copy, len);
+  if (status != 0)
+    throw_error (MEMORY_ERROR,
+		 _("Error accessing memory address %s (%s) for "
+		   "displaced-stepping buffer, you may want to temporarily "
+		   "turn it off if there is only one inferior thread."),
+		 paddress (target_gdbarch, copy), safe_strerror (status));
   if (debug_displaced)
     {
       fprintf_unfiltered (gdb_stdlog, "displaced: saved %s: ",

Re: [patch] More suggestive displaced-stepping memory error message

[Yao Qi]

On 04/11/2012 03:19 AM, Jan Kratochvil wrote:
> -  read_memory (copy, displaced->step_saved_copy, len);
> +  status = target_read_memory (copy, displaced->step_saved_copy, len);
> +  if (status != 0)
> +    throw_error (MEMORY_ERROR,
> +		 _("Error accessing memory address %s (%s) for "
> +		   "displaced-stepping buffer, you may want to temporarily "
> +		   "turn it off if there is only one inferior thread."),

The error message is not clear to me.  Why "one inferior thread" matters
here?  If GDB is unable to do displaced-stepping due to unavailable
scratch pad, user has no choice except disable displaced-stepping for a
moment, and turn it on later.

> +		 paddress (target_gdbarch, copy), safe_strerror (status));
                           ^^^^^^^^^^^^^^  why not `gdbarch'?

Think a little more, is it a little over-strict and less user-friendly
in this case?  If I got such error message like "turn it off temporarily
and turn it on later", I can turn displaced-stepping off, but I don't
know when to turn it on again.  Probably, GDB can be smart enough to not
use displaced stepping when get error on accessing scratch pad.  So far,
displaced_step_prepare returns 1 for success and 0 for queued request.
We can add one more return value -1 for `unable to do displaced
stepping', so that caller can go non-displaced-stepping path.

-- 
Yao (齐尧)

Re: [patch] More suggestive displaced-stepping memory error message

[Jan Kratochvil]

On Wed, 11 Apr 2012 01:31:35 +0200, Yao Qi wrote:
> On 04/11/2012 03:19 AM, Jan Kratochvil wrote:
> > -  read_memory (copy, displaced->step_saved_copy, len);
> > +  status = target_read_memory (copy, displaced->step_saved_copy, len);
> > +  if (status != 0)
> > +    throw_error (MEMORY_ERROR,
> > +		 _("Error accessing memory address %s (%s) for "
> > +		   "displaced-stepping buffer, you may want to temporarily "
> > +		   "turn it off if there is only one inferior thread."),
> 
> The error message is not clear to me.  Why "one inferior thread" matters
> here?

Because with only one inferior thread you can "set displaced-stepping off"
without any disadvantages.  If you have more threads then sure
"set displaced-stepping off" can have performance side-effects.

As I expect this problem can happen (or at least happened for me) only during
very early inferior initialization then the user sees there is only one thread
and therefore the user can easily do "set displaced-stepping off".
But I was not sure if this suggestion makes too much sense so I am fine to put
there just the message explaining it happened with "displaced-stepping buffer",
user should figure out how to workaround it himself/herself.


> If GDB is unable to do displaced-stepping due to unavailable
> scratch pad, user has no choice except disable displaced-stepping for a
> moment, and turn it on later.
> 
> > +		 paddress (target_gdbarch, copy), safe_strerror (status));
>                            ^^^^^^^^^^^^^^  why not `gdbarch'?

copy-paste mistake, thanks.


> Probably, GDB can be smart enough to not
> use displaced stepping when get error on accessing scratch pad.

... and there is only one inferior thread. As if there are more threads it
would be bad to hide the displaced-stepping buffer problem from the user while
affecting performance / non-stop mode of the other threads.


> So far,
> displaced_step_prepare returns 1 for success and 0 for queued request.
> We can add one more return value -1 for `unable to do displaced
> stepping', so that caller can go non-displaced-stepping path.

We could discuss first more the cases when it can happen and it is not some
GDB bug.  So far I know only about the ld.so initialization.


As I see there are concerns about the message I will just check in this part,
it should be sufficient for troubleshooting.


Thanks,
Jan


gdb/
2012-04-11  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Provide more specific displaced-stepping memory error message.
	* infrun.c (displaced_step_prepare): New variable status.  Call
	target_read_memory instead of read_memory, provide more specific
	error message.

--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1296,6 +1296,7 @@ displaced_step_prepare (ptid_t ptid)
   ULONGEST len;
   struct displaced_step_closure *closure;
   struct displaced_step_inferior_state *displaced;
+  int status;
 
   /* We should never reach this function if the architecture does not
      support displaced stepping.  */
@@ -1356,7 +1357,12 @@ displaced_step_prepare (ptid_t ptid)
   displaced->step_saved_copy = xmalloc (len);
   ignore_cleanups = make_cleanup (free_current_contents,
 				  &displaced->step_saved_copy);
-  read_memory (copy, displaced->step_saved_copy, len);
+  status = target_read_memory (copy, displaced->step_saved_copy, len);
+  if (status != 0)
+    throw_error (MEMORY_ERROR,
+		 _("Error accessing memory address %s (%s) for "
+		   "displaced-stepping scratch space."),
+		 paddress (gdbarch, copy), safe_strerror (status));
   if (debug_displaced)
     {
       fprintf_unfiltered (gdb_stdlog, "displaced: saved %s: ",

Re: [patch] More suggestive displaced-stepping memory error message

[Yao Qi]

On 04/11/2012 04:41 PM, Jan Kratochvil wrote:
>> > So far,
>> > displaced_step_prepare returns 1 for success and 0 for queued request.
>> > We can add one more return value -1 for `unable to do displaced
>> > stepping', so that caller can go non-displaced-stepping path.
> We could discuss first more the cases when it can happen and it is not some
> GDB bug.  So far I know only about the ld.so initialization.
> 
> As I see there are concerns about the message I will just check in this part,
> it should be sufficient for troubleshooting.

Looks like it is a corner case, and throwing an error message is fine to me.

-- 
Yao (齐尧)

Re: [patch] More suggestive displaced-stepping memory error message

[Pedro Alves]

On 04/11/2012 09:41 AM, Jan Kratochvil wrote:

> As I see there are concerns about the message I will just check in this part,
> it should be sufficient for troubleshooting.


I like this one.  Thanks.

-- 
Pedro Alves

[commit] [patch] More suggestive displaced-stepping memory error message

[Jan Kratochvil]

On Wed, 11 Apr 2012 17:47:46 +0200, Pedro Alves wrote:
> On 04/11/2012 09:41 AM, Jan Kratochvil wrote:
> > As I see there are concerns about the message I will just check in this part,
> > it should be sufficient for troubleshooting.
> 
> I like this one.  Thanks.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2012-04/msg00068.html


Thanks,
Jan

Re: [patch] More suggestive displaced-stepping memory error message

[Pedro Alves]

On 04/11/2012 12:31 AM, Yao Qi wrote:

> Probably, GDB can be smart enough to not
> use displaced stepping when get error on accessing scratch pad.  So far,
> displaced_step_prepare returns 1 for success and 0 for queued request.
> We can add one more return value -1 for `unable to do displaced
> stepping', so that caller can go non-displaced-stepping path.


We can't just go non-displaced-stepping path in non-stop with the
current code because nothing knows to pause all running threads
in order to be able to lift the breakpoint and step over it without other
threads missing it (like gdbserver knows, with linux-low.c:start_step_over
and all that).  My patch #1 "all-stop on top of non-stop" patch of
the itsets series goes in that direction.

-- 
Pedro Alves