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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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: ",