[3/3] gdbserver/Windows: crash during connection establishment phase

  On Windows, starting a new process with GDBserver seeems to work,
in the sense that the program does get started, and GDBserver
confirms that it is listening for GDB to connect. However, as soon as
GDB establishes the connection with GDBserver, and starts discussing
with it, GDBserver crashes, with a SEGV.

This SEGV occurs in remote-utils.c::prepare_resume_reply...

  | regp = current_target_desc ()->expedite_regs;
  | [...]
  | while (*regp)

... because, in our case, REGP is NULL.

This patch fixes the problem for Windows targets based on Intel
(x86 and x86_64).

gdb/gdbserver/ChangeLog:

	* win32-i386-low.c (i386_arch_setup): set tdesc->expedite_regs.
---
 gdb/gdbserver/win32-i386-low.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
  

On 05/04/2018 07:30 PM, Joel Brobecker wrote:
> On Windows, starting a new process with GDBserver seeems to work,
> in the sense that the program does get started, and GDBserver
> confirms that it is listening for GDB to connect. However, as soon as
> GDB establishes the connection with GDBserver, and starts discussing
> with it, GDBserver crashes, with a SEGV.
> 
> This SEGV occurs in remote-utils.c::prepare_resume_reply...
> 
>   | regp = current_target_desc ()->expedite_regs;
>   | [...]
>   | while (*regp)
> 
> ... because, in our case, REGP is NULL.
> 
> This patch fixes the problem for Windows targets based on Intel
> (x86 and x86_64).
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* win32-i386-low.c (i386_arch_setup): set tdesc->expedite_regs.
> ---
>  gdb/gdbserver/win32-i386-low.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/gdb/gdbserver/win32-i386-low.c b/gdb/gdbserver/win32-i386-low.c
> index a242f72..fdb95ce 100644
> --- a/gdb/gdbserver/win32-i386-low.c
> +++ b/gdb/gdbserver/win32-i386-low.c
> @@ -442,6 +442,16 @@ i386_arch_setup (void)
>  
>    init_target_desc (tdesc);
>  
> +#ifndef IN_PROCESS_AGENT
> +#ifdef __x86_64__
> +  static const char *expedite_regs_amd64[] = { "rbp", "rsp", "rip", NULL };
> +  tdesc->expedite_regs = expedite_regs_amd64;
> +#else /* __x86_64__ */
> +  static const char *expedite_regs_i386[] = { "ebp", "esp", "eip", NULL };
> +  tdesc->expedite_regs = expedite_regs_i386;
> +#endif /* __x86_64__ */
> +#endif

Won't all x86 ports have the same problem?  I.e., don't
nto-x86-low.c:nto_x86_arch_setup and
lynx-i386-low.c:lynx_i386_arch_setup need the same treatment?
Should we put those arrays in some shared i386 file?

Thanks,
Pedro Alves
  

> > +#ifndef IN_PROCESS_AGENT
> > +#ifdef __x86_64__
> > +  static const char *expedite_regs_amd64[] = { "rbp", "rsp", "rip", NULL };
> > +  tdesc->expedite_regs = expedite_regs_amd64;
> > +#else /* __x86_64__ */
> > +  static const char *expedite_regs_i386[] = { "ebp", "esp", "eip", NULL };
> > +  tdesc->expedite_regs = expedite_regs_i386;
> > +#endif /* __x86_64__ */
> > +#endif
> 
> Won't all x86 ports have the same problem?  I.e., don't
> nto-x86-low.c:nto_x86_arch_setup and
> lynx-i386-low.c:lynx_i386_arch_setup need the same treatment?
> Should we put those arrays in some shared i386 file?

They probably would, but these are platforms I cannot test at
the moment, so I didn't want to take a risk and create a situation
for them. I'm quite happy to adjust the way you suggest.
  

On 05/04/2018 07:58 PM, Joel Brobecker wrote:
>>> +#ifndef IN_PROCESS_AGENT
>>> +#ifdef __x86_64__
>>> +  static const char *expedite_regs_amd64[] = { "rbp", "rsp", "rip", NULL };
>>> +  tdesc->expedite_regs = expedite_regs_amd64;
>>> +#else /* __x86_64__ */
>>> +  static const char *expedite_regs_i386[] = { "ebp", "esp", "eip", NULL };
>>> +  tdesc->expedite_regs = expedite_regs_i386;
>>> +#endif /* __x86_64__ */
>>> +#endif
>>
>> Won't all x86 ports have the same problem?  I.e., don't
>> nto-x86-low.c:nto_x86_arch_setup and
>> lynx-i386-low.c:lynx_i386_arch_setup need the same treatment?
>> Should we put those arrays in some shared i386 file?
> 
> They probably would, but these are platforms I cannot test at
> the moment, so I didn't want to take a risk and create a situation
> for them. I'm quite happy to adjust the way you suggest.

I think I'd take the risk anyway, even without testing.  (Just like
the original patch that broke them didn't get testing on !Linux
ports.)  They're already most probably broken, so we can't be
making it worse, IMO.

Thanks,
Pedro Alves
  

> From: Joel Brobecker <brobecker@adacore.com>
> Date: Fri,  4 May 2018 14:30:03 -0400
> 
> +#ifndef IN_PROCESS_AGENT
> +#ifdef __x86_64__
> +  static const char *expedite_regs_amd64[] = { "rbp", "rsp", "rip", NULL };
> +  tdesc->expedite_regs = expedite_regs_amd64;
> +#else /* __x86_64__ */

I believe the comment here should say /* !__x86_64__ */

> +  static const char *expedite_regs_i386[] = { "ebp", "esp", "eip", NULL };
> +  tdesc->expedite_regs = expedite_regs_i386;
> +#endif /* __x86_64__ */

Likewise here.

(Yes, nit-picking, I know.)

Thanks.