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

[Pedro Alves <palves@redhat.com>]

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