[7/8] Initialise target descrption after skipping extra traps for --wrapper

  Nowadays, when --wrapper is used, GDBserver skips extra traps/stops
in the wrapper program, and stops at the first instruction of the
program to be debugged.  However, GDBserver created target description
in the first stop of inferior, and the executable of the inferior
is the wrapper program rather than the program to be debugged.  In
this way, the target description can be wrong if the architectures
of wrapper program and program to be debugged are different.  This
is shown by some fails in gdb.server/wrapper.exp on buildbot.

We are testing i686-linux GDB (Fedora-i686) on an x86_64-linux box
(fedora-x86-64-4) in buildbot, such configuration causes fails in
gdb.server/wrapper.exp like this:

spawn /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/../../gdb/gdbserver/gdbserver --once --wrapper env TEST=1 -- :2346 /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/outputs/gdb.server/wrapper/wrapper
Process /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/outputs/gdb.server/wrapper/wrapper created; pid = 8795
Can't debug 64-bit process with 32-bit GDBserver
Exiting
target remote localhost:2346
localhost:2346: Connection timed out.
(gdb) FAIL: gdb.server/wrapper.exp: setting breakpoint at marker

See https://sourceware.org/ml/gdb-testers/2015-q3/msg01541.html

In this case, program to be debugged ("wrapper") is 32-bit but wrapper
program ("/usr/bin/env") is 64-bit, so GDBserver gets the 64-bit
target description instead of 32-bit.

The root cause of this problem is that GDBserver creates target
description too early, and the rationale of fix could be creating
target description once the GDBserver skips extra traps and inferior
stops at the first instruction of the program we want to debug.  IOW,
when GDBserver skips extra traps, the inferior's tdesc is NULL, and
mywait and its callees shouldn't use inferior's tdesc, so in this
patch, we do the shortcut if proc->tdesc == NULL, see changes in
linux_resume_one_lwp_throw and need_step_over_p.

In linux_low_filter_event, if target description isn't initialised and
GDBserver attached the process, we create target description immediately,
because GDBserver don't have to skip extra traps for attach, IOW, it
makes no sense to use --attach and --wrapper together.  Otherwise, the
process is launched by GDBserver, we keep the status pending, and return.

After GDBserver skipped extra traps in start_inferior, we call a
target_ops hook arch_setup to initialise target description there.

gdb/gdbserver:

2015-07-16  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_arch_setup): New function.
	(linux_low_filter_event): If proc->tdesc is NULL and
	proc->attached is true, call the_low_target.arch_setup.
	Otherwise, keep status pending, and return.
	(linux_resume_one_lwp_throw): if proc->tdesc is NULL
	resume the inferior.
	(need_step_over_p): Return 0 if proc->tdesc is NULL.
	(linux_target_ops): Install arch_setup.
	* server.c (start_inferior): Call the_target->arch_setup.
	* target.h (struct target_ops) <arch_setup>: New field.
	(target_arch_setup): New marco.
	* lynx-low.c (lynx_target_ops): Update.
	* nto-low.c (nto_target_ops): Update.
	* spu-low.c (spu_target_ops): Update.
	* win32-low.c (win32_target_ops): Update.
---
 gdb/gdbserver/linux-low.c | 74 ++++++++++++++++++++++++++++++++++++++++-------
 gdb/gdbserver/lynx-low.c  |  1 +
 gdb/gdbserver/nto-low.c   |  1 +
 gdb/gdbserver/server.c    |  3 ++
 gdb/gdbserver/spu-low.c   |  1 +
 gdb/gdbserver/target.h    | 10 +++++++
 gdb/gdbserver/win32-low.c |  1 +
 7 files changed, 80 insertions(+), 11 deletions(-)
  

On 07/20/2015 12:35 PM, Yao Qi wrote:
> Nowadays, when --wrapper is used, GDBserver skips extra traps/stops
> in the wrapper program, and stops at the first instruction of the
> program to be debugged.  However, GDBserver created target description
> in the first stop of inferior, and the executable of the inferior
> is the wrapper program rather than the program to be debugged.  In
> this way, the target description can be wrong if the architectures
> of wrapper program and program to be debugged are different.  This
> is shown by some fails in gdb.server/wrapper.exp on buildbot.
> 
> We are testing i686-linux GDB (Fedora-i686) on an x86_64-linux box
> (fedora-x86-64-4) in buildbot, such configuration causes fails in
> gdb.server/wrapper.exp like this:
> 
> spawn /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/../../gdb/gdbserver/gdbserver --once --wrapper env TEST=1 -- :2346 /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/outputs/gdb.server/wrapper/wrapper
> Process /home/gdb-buildbot-2/fedora-x86-64-4/fedora-i686/build/gdb/testsuite/outputs/gdb.server/wrapper/wrapper created; pid = 8795
> Can't debug 64-bit process with 32-bit GDBserver
> Exiting
> target remote localhost:2346
> localhost:2346: Connection timed out.
> (gdb) FAIL: gdb.server/wrapper.exp: setting breakpoint at marker
> 
> See https://sourceware.org/ml/gdb-testers/2015-q3/msg01541.html
> 
> In this case, program to be debugged ("wrapper") is 32-bit but wrapper
> program ("/usr/bin/env") is 64-bit, so GDBserver gets the 64-bit
> target description instead of 32-bit.
> 
> The root cause of this problem is that GDBserver creates target
> description too early, and the rationale of fix could be creating
> target description once the GDBserver skips extra traps and inferior
> stops at the first instruction of the program we want to debug.  IOW,
> when GDBserver skips extra traps, the inferior's tdesc is NULL, and
> mywait and its callees shouldn't use inferior's tdesc, so in this
> patch, we do the shortcut if proc->tdesc == NULL, see changes in
> linux_resume_one_lwp_throw and need_step_over_p.
> 
> In linux_low_filter_event, if target description isn't initialised and
> GDBserver attached the process, we create target description immediately,
> because GDBserver don't have to skip extra traps for attach, IOW, it
> makes no sense to use --attach and --wrapper together.  Otherwise, the
> process is launched by GDBserver, we keep the status pending, and return.
> 
> After GDBserver skipped extra traps in start_inferior, we call a
> target_ops hook arch_setup to initialise target description there.
> 

Great, thanks for doing this.  Looks generally good to me.  One
comment below.

> @@ -3651,6 +3671,31 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>    struct thread_info *thread = get_lwp_thread (lwp);
>    struct thread_info *saved_thread;
>    int fast_tp_collecting;
> +  struct process_info *proc = get_thread_process (thread);
> +
> +  if (proc->tdesc == NULL)
> +    {
> +      /* Target description isn't initialised because the program hasn't
> +	 stop at the first instruction.  It means GDBserver skips the

"hasn't stopped" ... "first instruction yet".

> +	 extra traps from the wrapper program (see option --wrapper),
> +	 and GDBserver just simply resumes the program without accessing
> +	 inferior registers, because target description is unavailable
> +	 at this point.  */
> +      errno = 0;
> +      lwp->stepping = step;
> +      ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (thread),
> +	      (PTRACE_TYPE_ARG3) 0,
> +	      /* Coerce to a uintptr_t first to avoid potential gcc warning
> +		 of coercing an 8 byte integer to a 4 byte pointer.  */
> +	      (PTRACE_TYPE_ARG4) (uintptr_t) signal);
> +
> +      if (errno)
> +	perror_with_name ("resuming thread");
> +
> +      lwp->stopped = 0;
> +      lwp->stop_reason = TARGET_STOPPED_BY_NO_REASON;
> +      return;

Instead of this whole code block, I think we should be able to skip
the bits in the function that require accessing registers.  E.g.,
this:

 /* Cancel actions that rely on GDB not changing the PC (e.g., the
     user used the "jump" command, or "set $pc = foo").  */
  if (lwp->stop_pc != get_pc (lwp))
    {
      /* Collecting 'while-stepping' actions doesn't make sense
	 anymore.  */
      release_while_stepping_state_list (thread);
    }

Could be guarded by:

  if (thread->while_stepping != NULL)

And this:

  if (the_low_target.get_pc != NULL)
    {
      struct regcache *regcache = get_thread_regcache (current_thread, 1);

      lwp->stop_pc = (*the_low_target.get_pc) (regcache);

      if (debug_threads)
	{
	  debug_printf ("  %s from pc 0x%lx\n", step ? "step" : "continue",
			(long) lwp->stop_pc);
	}
    }

could be guarded by:

  if (proc->tdesc == NULL)

Did you try that?

> +    }
>  
>    if (lwp->stopped == 0)
>      return;

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> Instead of this whole code block, I think we should be able to skip
> the bits in the function that require accessing registers.  E.g.,
> this:
>
>  /* Cancel actions that rely on GDB not changing the PC (e.g., the
>      user used the "jump" command, or "set $pc = foo").  */
>   if (lwp->stop_pc != get_pc (lwp))
>     {
>       /* Collecting 'while-stepping' actions doesn't make sense
> 	 anymore.  */
>       release_while_stepping_state_list (thread);
>     }
>
> Could be guarded by:
>
>   if (thread->while_stepping != NULL)
>
> And this:
>
>   if (the_low_target.get_pc != NULL)
>     {
>       struct regcache *regcache = get_thread_regcache (current_thread, 1);
>
>       lwp->stop_pc = (*the_low_target.get_pc) (regcache);
>
>       if (debug_threads)
> 	{
> 	  debug_printf ("  %s from pc 0x%lx\n", step ? "step" : "continue",
> 			(long) lwp->stop_pc);
> 	}
>     }
>
> could be guarded by:
>
>   if (proc->tdesc == NULL)
>
> Did you try that?

To make sure I understand you correctly, is the change below what you suggested?
If yes, I thought about this approach before, but I didn't try that
because I was worried that we should check every piece of code in
linux_resume_one_lwp_throw and its callees that don't access registers
when target description isn't initialised yet.  Especially for
the_low_target.prepare_to_resume, the implementation of this hook should
be aware of that target description may not be available.  Nowadays,
prepare_to_resume is only used to update HW debug registers, and
should be OK because GDBserver shouldn't update HW debug registers
before the inferior stops at the first instruction of the program.
However, in nat/x86-linux.c:lwp_debug_registers_changed, I saw such
comments

  struct arch_lwp_info *info = lwp_arch_private_info (lwp);

  /* NULL means either that this is the main thread still going
     through the shell, or that no watchpoint has been set yet.
     The debug registers are unchanged in either case.  */

I was wondering all the implementations of prepare_to_resume of
different targets should be aware that "thread still going through the
shell"?  I'll test this patch on targets other than x86 (such as arm and
aarch64) and see if it causes fails.
  

On 07/24/2015 12:11 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Instead of this whole code block, I think we should be able to skip
>> the bits in the function that require accessing registers.  E.g.,
>> this:
>>
>>  /* Cancel actions that rely on GDB not changing the PC (e.g., the
>>      user used the "jump" command, or "set $pc = foo").  */
>>   if (lwp->stop_pc != get_pc (lwp))
>>     {
>>       /* Collecting 'while-stepping' actions doesn't make sense
>> 	 anymore.  */
>>       release_while_stepping_state_list (thread);
>>     }
>>
>> Could be guarded by:
>>
>>   if (thread->while_stepping != NULL)
>>
>> And this:
>>
>>   if (the_low_target.get_pc != NULL)
>>     {
>>       struct regcache *regcache = get_thread_regcache (current_thread, 1);
>>
>>       lwp->stop_pc = (*the_low_target.get_pc) (regcache);
>>
>>       if (debug_threads)
>> 	{
>> 	  debug_printf ("  %s from pc 0x%lx\n", step ? "step" : "continue",
>> 			(long) lwp->stop_pc);
>> 	}
>>     }
>>
>> could be guarded by:
>>
>>   if (proc->tdesc == NULL)
>>
>> Did you try that?
> 
> To make sure I understand you correctly, is the change below what you suggested?

Yes.

> If yes, I thought about this approach before, but I didn't try that
> because I was worried that we should check every piece of code in
> linux_resume_one_lwp_throw and its callees that don't access registers
> when target description isn't initialised yet.  Especially for
> the_low_target.prepare_to_resume, the implementation of this hook should
> be aware of that target description may not be available.  Nowadays,
> prepare_to_resume is only used to update HW debug registers, and
> should be OK because GDBserver shouldn't update HW debug registers
> before the inferior stops at the first instruction of the program.
> However, in nat/x86-linux.c:lwp_debug_registers_changed, I saw such
> comments
> 
>   struct arch_lwp_info *info = lwp_arch_private_info (lwp);
> 
>   /* NULL means either that this is the main thread still going
>      through the shell, or that no watchpoint has been set yet.
>      The debug registers are unchanged in either case.  */
> 
> I was wondering all the implementations of prepare_to_resume of
> different targets should be aware that "thread still going through the
> shell"?  

Yes, I think they should.  It's what the GDB side used to do already
when that code was x86 gdb only (and hence that shell comment, which
gdbserver doesn't use yet), and then other archs followed suit.
"Going through the shell" is the exact same as going through
the exec wrapper -- we're not interested in debugging the shell,
and it may well have a different bitness/architecture of the target
program we want to debug.

In practice that hook's implementation should want to avoid work if some
flag is not set, to avoid unnecessary ptrace syscalls and thus ends up
not doing anything when going through the shell/exec-wrapper.

> I'll test this patch on targets other than x86 (such as arm and
> aarch64) and see if it causes fails.

Thanks,
Pedro Alves