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

[Pedro Alves <palves at redhat dot com>]

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