[Windows GDBserver] Make GDBserver functional again on Windows

[Windows GDBserver] Make GDBserver functional again on Windows

[Joel Brobecker]

Hello,

I noticed a few weeks back that GDBserver on Windows was no longer
functional, so I started working on it as time allowed. With the
following patches, it's mostly functional, as in I only have few
regressions left. I wanted to have clean results before submitting,
but a recent patch showed that waiting could cause others to duplicate
something I had already done, so I'm posting now. The worse that can
happen is that I realize an earlier patch has a bug shown by the current
regressions, or that one of my patches is incomplete, but I think
it would be nothing compared to the risk of having work being duplicated.

    [PATCH 1/3] [gdbserver/win32] fatal "glob could not process pattern
    [PATCH 2/3] gdbserver/Windows: Fix "no program to debug" error
    [PATCH 3/3] gdbserver/Windows: crash during connection establishment

I haven't analyzed all the remaining failures yet, but most of them
seem to be related to the "kill" command not working. I will look
into that as soon as I have a moment (but it is looking like it may
be a week before I have the time).

I only quickly experimented with GDBserver 8.1, but I think it's
completely broken then as well :-(. Some of the fixes might be
applicable there. I will double-check -- also as soon as I have
a moment, and for sure before we release 8.1.1.

Thanks,
-- 
Joel

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

[Joel Brobecker]

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
+
   win32_tdesc = tdesc;
 }
 
-- 
2.1.4

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

[Pedro Alves]

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

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

[Joel Brobecker]

> > +#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.

-- 
Joel

PS: If you are surprised by me saying I can't test on LynxOS at
    the moment, it's because I don't have a C++ cross compiler
    for that platform. I'm still hoping that we might be able
    to build one, but it requires some work.

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

[Pedro Alves]

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

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]

Hi Pedro,

What do you think of the following approach? The idea is to make
sure we never forget to set the expedite_regs field when we initialize
the tdesc.

One part that could be up for discussion is how the constants for
x86 and x86-64 targets are being shared. The way it's done, right
now, each unit that includes x86-tdesc.h would end up with its own
static copy of the constants. I don't think it should be a problem
in practice, but if there is a better solution...

gdb/ChangeLog:

        * regformats/regdat.sh: Adjust script, following the addition
        of the new expedite_regs parameter to init_target_desc.

gdb/gdbserver/ChangeLog:

        * tdesc.h (init_target_desc) <expedite_regs>: New parameter.
        * tdesc.c (init_target_desc) <expedite_regs>: New parameter.
        Use it to set the expedite_regs field in the given tdesc.
        * x86-tdesc.h: New file.
        * linux-aarch64-tdesc.c (aarch64_linux_read_description):
        Adjust following the addition of the new expedite_regs parameter
        to init_target_desc.
        * linux-tic6x-low.c (tic6x_read_description): Likewise.
        * linux-x86-tdesc.c: #include "x86-tdesc.h".
        (i386_linux_read_description, amd64_linux_read_description):
        Adjust following the addition of the new expedite_regs parameter
        to init_target_desc.
        * lynx-i386-low.c: #include "x86-tdesc.h".
        (lynx_i386_arch_setup): Adjust following the addition of the new
        expedite_regs parameter to init_target_desc.
        * nto-x86-low.c: #include "x86-tdesc.h".
        (nto_x86_arch_setup): Adjust following the addition of the new
        expedite_regs parameter to init_target_desc.
        * win32-i386-low.c: #include "x86-tdesc.h".
        (i386_arch_setup): Adjust following the addition of the new
        expedite_regs parameter to init_target_desc.

Tested on x86_64-linux, with and without gdbserver. Tested on
x86-windows as well (using GDBserver with AdaCore's gdb-testsuite).
Same results as before.

-- 
Joel

[-- Attachment #2: 0001-gdbserver-Windows-crash-during-connection-establishm.patch --]
[-- Type: text/x-diff, Size: 10432 bytes --]

From 76d0a2d702956bd7aa950b1ab4ce3b9dc6727961 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 1 May 2018 17:52:15 -0400
Subject: [PATCH] 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 patches fixes the issues by adding a parameter to init_target_desc,
in order to make sure that we always provide the list of registers when
we initialize a target description.

gdb/ChangeLog:

        * regformats/regdat.sh: Adjust script, following the addition
        of the new expedite_regs parameter to init_target_desc.

gdb/gdbserver/ChangeLog:

        * tdesc.h (init_target_desc) <expedite_regs>: New parameter.
        * tdesc.c (init_target_desc) <expedite_regs>: New parameter.
        Use it to set the expedite_regs field in the given tdesc.
        * x86-tdesc.h: New file.
        * linux-aarch64-tdesc.c (aarch64_linux_read_description):
        Adjust following the addition of the new expedite_regs parameter
        to init_target_desc.
        * linux-tic6x-low.c (tic6x_read_description): Likewise.
        * linux-x86-tdesc.c: #include "x86-tdesc.h".
        (i386_linux_read_description, amd64_linux_read_description):
        Adjust following the addition of the new expedite_regs parameter
        to init_target_desc.
        * lynx-i386-low.c: #include "x86-tdesc.h".
        (lynx_i386_arch_setup): Adjust following the addition of the new
        expedite_regs parameter to init_target_desc.
        * nto-x86-low.c: #include "x86-tdesc.h".
        (nto_x86_arch_setup): Adjust following the addition of the new
        expedite_regs parameter to init_target_desc.
        * win32-i386-low.c: #include "x86-tdesc.h".
        (i386_arch_setup): Adjust following the addition of the new
        expedite_regs parameter to init_target_desc.
---
 gdb/gdbserver/linux-aarch64-tdesc.c |  6 +-----
 gdb/gdbserver/linux-tic6x-low.c     |  4 +---
 gdb/gdbserver/linux-x86-tdesc.c     | 15 +++------------
 gdb/gdbserver/lynx-i386-low.c       |  3 ++-
 gdb/gdbserver/nto-x86-low.c         |  3 ++-
 gdb/gdbserver/tdesc.c               |  7 ++++++-
 gdb/gdbserver/tdesc.h               |  6 ++++--
 gdb/gdbserver/win32-i386-low.c      |  5 ++++-
 gdb/gdbserver/x86-tdesc.h           | 26 ++++++++++++++++++++++++++
 gdb/regformats/regdat.sh            |  3 +--
 10 files changed, 50 insertions(+), 28 deletions(-)
 create mode 100755 gdb/gdbserver/x86-tdesc.h

diff --git a/gdb/gdbserver/linux-aarch64-tdesc.c b/gdb/gdbserver/linux-aarch64-tdesc.c
index 9f7b9e5..f433038 100644
--- a/gdb/gdbserver/linux-aarch64-tdesc.c
+++ b/gdb/gdbserver/linux-aarch64-tdesc.c
@@ -34,12 +34,8 @@ aarch64_linux_read_description ()
     {
       *tdesc = aarch64_create_target_description ();
 
-      init_target_desc (*tdesc);
-
-#ifndef IN_PROCESS_AGENT
       static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
-      (*tdesc)->expedite_regs = expedite_regs_aarch64;
-#endif
+      init_target_desc (*tdesc, expedite_regs_aarch64);
     }
 
   return *tdesc;
diff --git a/gdb/gdbserver/linux-tic6x-low.c b/gdb/gdbserver/linux-tic6x-low.c
index e1df59f..d90bbcf 100644
--- a/gdb/gdbserver/linux-tic6x-low.c
+++ b/gdb/gdbserver/linux-tic6x-low.c
@@ -199,10 +199,8 @@ tic6x_read_description (enum c6x_feature feature)
   if (*tdesc == NULL)
     {
       *tdesc = tic6x_create_target_description (feature);
-      init_target_desc (*tdesc);
-
       static const char *expedite_regs[] = { "A15", "PC", NULL };
-      (*tdesc)->expedite_regs = expedite_regs;
+      init_target_desc (*tdesc, expedite_regs);
     }
 
   return *tdesc;
diff --git a/gdb/gdbserver/linux-x86-tdesc.c b/gdb/gdbserver/linux-x86-tdesc.c
index 47b8476..358659b 100644
--- a/gdb/gdbserver/linux-x86-tdesc.c
+++ b/gdb/gdbserver/linux-x86-tdesc.c
@@ -25,6 +25,7 @@
 #ifdef __x86_64__
 #include "arch/amd64.h"
 #endif
+#include "x86-tdesc.h"
 
 /* Return the right x86_linux_tdesc index for a given XCR0.  Return
    X86_TDESC_LAST if can't find a match.  */
@@ -88,12 +89,7 @@ i386_linux_read_description (uint64_t xcr0)
     {
       *tdesc = i386_create_target_description (xcr0, true);
 
-      init_target_desc (*tdesc);
-
-#ifndef IN_PROCESS_AGENT
-      static const char *expedite_regs_i386[] = { "ebp", "esp", "eip", NULL };
-      (*tdesc)->expedite_regs = expedite_regs_i386;
-#endif
+      init_target_desc (*tdesc, i386_expedite_regs);
     }
 
   return *tdesc;;
@@ -124,12 +120,7 @@ amd64_linux_read_description (uint64_t xcr0, bool is_x32)
     {
       *tdesc = amd64_create_target_description (xcr0, is_x32, true);
 
-      init_target_desc (*tdesc);
-
-#ifndef IN_PROCESS_AGENT
-      static const char *expedite_regs_amd64[] = { "rbp", "rsp", "rip", NULL };
-      (*tdesc)->expedite_regs = expedite_regs_amd64;
-#endif
+      init_target_desc (*tdesc, amd64_expedite_regs);
     }
   return *tdesc;
 }
diff --git a/gdb/gdbserver/lynx-i386-low.c b/gdb/gdbserver/lynx-i386-low.c
index c7b4fe4..37c0dc9 100644
--- a/gdb/gdbserver/lynx-i386-low.c
+++ b/gdb/gdbserver/lynx-i386-low.c
@@ -21,6 +21,7 @@
 #include <sys/ptrace.h>
 #include "x86-xstate.h"
 #include "arch/i386.h"
+#include "x86-tdesc.h"
 
 /* The following two typedefs are defined in a .h file which is not
    in the standard include path (/sys/include/family/x86/ucontext.h),
@@ -296,7 +297,7 @@ lynx_i386_arch_setup (void)
   struct target_desc *tdesc
     = i386_create_target_description (X86_XSTATE_SSE_MASK, false);
 
-  init_target_desc (tdesc);
+  init_target_desc (tdesc, i386_expedite_regs);
 
   lynx_tdesc = tdesc;
 }
diff --git a/gdb/gdbserver/nto-x86-low.c b/gdb/gdbserver/nto-x86-low.c
index b15c208..81c3c6a 100644
--- a/gdb/gdbserver/nto-x86-low.c
+++ b/gdb/gdbserver/nto-x86-low.c
@@ -25,6 +25,7 @@
 #include <x86/context.h>
 #include "x86-xstate.h"
 #include "arch/i386.h"
+#include "x86-tdesc.h"
 
 const unsigned char x86_breakpoint[] = { 0xCC };
 #define x86_breakpoint_len 1
@@ -90,7 +91,7 @@ nto_x86_arch_setup (void)
   struct target_desc *tdesc
     = i386_create_target_description (X86_XSTATE_SSE_MASK, false);
 
-  init_target_desc (tdesc);
+  init_target_desc (tdesc, i386_expedite_regs);
 
   nto_tdesc = tdesc;
 }
diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
index 92524b3..d035083 100644
--- a/gdb/gdbserver/tdesc.c
+++ b/gdb/gdbserver/tdesc.c
@@ -60,7 +60,8 @@ void target_desc::accept (tdesc_element_visitor &v) const
 }
 
 void
-init_target_desc (struct target_desc *tdesc)
+init_target_desc (struct target_desc *tdesc,
+		  const char **expedite_regs)
 {
   int offset = 0;
 
@@ -86,6 +87,10 @@ init_target_desc (struct target_desc *tdesc)
   /* Make sure PBUFSIZ is large enough to hold a full register
      packet.  */
   gdb_assert (2 * tdesc->registers_size + 32 <= PBUFSIZ);
+
+#ifndef IN_PROCESS_AGENT
+  tdesc->expedite_regs = expedite_regs;
+#endif
 }
 
 struct target_desc *
diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
index 61a3e4e..989c128 100644
--- a/gdb/gdbserver/tdesc.h
+++ b/gdb/gdbserver/tdesc.h
@@ -82,9 +82,11 @@ public:
 void copy_target_description (struct target_desc *dest,
 			      const struct target_desc *src);
 
-/* Initialize TDESC.  */
+/* Initialize TDESC, and then set its expedite_regs field to
+   EXPEDITE_REGS.  */
 
-void init_target_desc (struct target_desc *tdesc);
+void init_target_desc (struct target_desc *tdesc,
+		       const char **expedite_regs);
 
 /* Return the current inferior's target description.  Never returns
    NULL.  */
diff --git a/gdb/gdbserver/win32-i386-low.c b/gdb/gdbserver/win32-i386-low.c
index a242f72..16fe2c8 100644
--- a/gdb/gdbserver/win32-i386-low.c
+++ b/gdb/gdbserver/win32-i386-low.c
@@ -24,6 +24,7 @@
 #endif
 #include "arch/i386.h"
 #include "tdesc.h"
+#include "x86-tdesc.h"
 
 #ifndef CONTEXT_EXTENDED_REGISTERS
 #define CONTEXT_EXTENDED_REGISTERS 0
@@ -436,11 +437,13 @@ i386_arch_setup (void)
 #ifdef __x86_64__
   tdesc = amd64_create_target_description (X86_XSTATE_SSE_MASK, false,
 						 false);
+  const char **expedite_regs = amd64_expedite_regs;
 #else
   tdesc = i386_create_target_description (X86_XSTATE_SSE_MASK, false);
+  const char **expedite_regs = i386_expedite_regs;
 #endif
 
-  init_target_desc (tdesc);
+  init_target_desc (tdesc, expedite_regs);
 
   win32_tdesc = tdesc;
 }
diff --git a/gdb/gdbserver/x86-tdesc.h b/gdb/gdbserver/x86-tdesc.h
new file mode 100755
index 0000000..c1641b2
--- /dev/null
+++ b/gdb/gdbserver/x86-tdesc.h
@@ -0,0 +1,26 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef X86_TDESC_H
+
+/* The "expedite" registers for x86 targets.  */
+static const char *i386_expedite_regs[] = {"ebp", "esp", "eip", NULL};
+
+/* The "expedite" registers for x86_64 targets.  */
+static const char *amd64_expedite_regs[] = {"rbp", "rsp", "rip", NULL};
+
+#endif /* X86_TDESC_H */
diff --git a/gdb/regformats/regdat.sh b/gdb/regformats/regdat.sh
index 8f546fe..5a8564a 100755
--- a/gdb/regformats/regdat.sh
+++ b/gdb/regformats/regdat.sh
@@ -185,11 +185,10 @@ echo
 
 cat <<EOF
 #ifndef IN_PROCESS_AGENT
-  result->expedite_regs = expedite_regs_${name};
   result->xmltarget = xmltarget_${name};
 #endif
 
-  init_target_desc (result);
+  init_target_desc (result, expedite_regs_${name});
 
   tdesc_${name} = result;
 }
-- 
2.1.4

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

[Pedro Alves]

Hi Joel,

On 05/07/2018 10:47 PM, Joel Brobecker wrote:

> What do you think of the following approach? The idea is to make
> sure we never forget to set the expedite_regs field when we initialize
> the tdesc.

Thanks, this looks good to me.

> On Windows, starting a new process with GDBserver seeems to work,

"seeems" -> "seems"

Thanks,
Pedro Alves

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

[Joel Brobecker]

> > What do you think of the following approach? The idea is to make
> > sure we never forget to set the expedite_regs field when we initialize
> > the tdesc.
> 
> Thanks, this looks good to me.

Thanks, Pedro. Does it mean the first two look OK to you as well,
or should I wait for specific review. Given that GDBserver appears
to have been broken for quite a while, I don't think we are in any
kind of hurry... Unless we decide to backport to 8.1.1?

> > On Windows, starting a new process with GDBserver seeems to work,
> 
> "seeems" -> "seems"

Thanks. I think I will start running ispell on my revision logs.

For various reasons, I had to transition from a laptop keyboard
with nice scissor switches to super nice PC keyboard, but with
cherry-mx keys, so very very different. I seem to be making
those kinds of mistakes...

Fixed locally.

-- 
Joel

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

[Pedro Alves]

On 05/09/2018 10:57 PM, Joel Brobecker wrote:
>>> What do you think of the following approach? The idea is to make
>>> sure we never forget to set the expedite_regs field when we initialize
>>> the tdesc.
>>
>> Thanks, this looks good to me.
> 
> Thanks, Pedro. Does it mean the first two look OK to you as well,
> or should I wait for specific review. 

Let me take a look.  I think I have a question about patch #2.

> Given that GDBserver appears
> to have been broken for quite a while, I don't think we are in any
> kind of hurry... Unless we decide to backport to 8.1.1?

I think backporting would be good.

Thanks,
Pedro Alves

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

[Joel Brobecker]

gdb/ChangeLog:

        * regformats/regdat.sh: Adjust script, following the addition
        of the new expedite_regs parameter to init_target_desc.

gdb/gdbserver/ChangeLog:

        * tdesc.h (init_target_desc) <expedite_regs>: New parameter.
        * tdesc.c (init_target_desc) <expedite_regs>: New parameter.
        Use it to set the expedite_regs field in the given tdesc.
        * x86-tdesc.h: New file.
        * linux-aarch64-tdesc.c (aarch64_linux_read_description):
        Adjust following the addition of the new expedite_regs parameter
        to init_target_desc.
        * linux-tic6x-low.c (tic6x_read_description): Likewise.
        * linux-x86-tdesc.c: #include "x86-tdesc.h".
        (i386_linux_read_description, amd64_linux_read_description):
        Adjust following the addition of the new expedite_regs parameter
        to init_target_desc.
        * lynx-i386-low.c: #include "x86-tdesc.h".
        (lynx_i386_arch_setup): Adjust following the addition of the new
        expedite_regs parameter to init_target_desc.
        * nto-x86-low.c: #include "x86-tdesc.h".
        (nto_x86_arch_setup): Adjust following the addition of the new
        expedite_regs parameter to init_target_desc.
        * win32-i386-low.c: #include "x86-tdesc.h".
        (i386_arch_setup): Adjust following the addition of the new

> Thanks, this looks good to me.

Thanks Pedro. Pushed to master and gdb-8.1-branch, under PR cli/23158.

-- 
Joel

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

[Eli Zaretskii]

> 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.

[PATCH 1/3] [gdbserver/win32] fatal "glob could not process pattern '(null)'" error

[Joel Brobecker]

Trying to start GDBserver on Windows currently yields the following
error...

    $ gdbserver.exe --once :4444 simple_main.exe
    glob could not process pattern '(null)'.
    Exiting

... after which GDB terminates with a nonzero status.

This is because create_process in win32-low.c calls gdb_tilde_expand
with the result of a call to get_inferior_cwd without verifying that
the returned directory is not NULL:

    | static BOOL
    | create_process (const char *program, char *args,
    |                 DWORD flags, PROCESS_INFORMATION *pi)
    | {
    |   const char *inferior_cwd = get_inferior_cwd ();
    |   std::string expanded_infcwd = gdb_tilde_expand (inferior_cwd);

This patch avoids this by only calling gdb_tilde_expand when
INFERIOR_CWD is not NULL, which is similar to what is done on
GNU/Linux for instance.

gdb/gdbserver/ChangeLog:

	* win32-low.c (create_process): Only call gdb_tilde_expand if
	inferior_cwd is not NULL.
---
 gdb/gdbserver/win32-low.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 9f0c4e4..db5dd49 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -556,7 +556,6 @@ create_process (const char *program, char *args,
 		DWORD flags, PROCESS_INFORMATION *pi)
 {
   const char *inferior_cwd = get_inferior_cwd ();
-  std::string expanded_infcwd = gdb_tilde_expand (inferior_cwd);
   BOOL ret;
 
 #ifdef _WIN32_WCE
@@ -576,6 +575,7 @@ create_process (const char *program, char *args,
 
   if (inferior_cwd != NULL)
     {
+      std::string expanded_infcwd = gdb_tilde_expand (inferior_cwd);
       std::replace (expanded_infcwd.begin (), expanded_infcwd.end (),
 		    '/', '\\');
       wcwd = alloca ((expanded_infcwd.size () + 1) * sizeof (wchar_t));
@@ -607,7 +607,10 @@ Could not convert the expanded inferior cwd to wide-char."));
 			TRUE,     /* inherit handles */
 			flags,    /* start flags */
 			NULL,     /* environment */
-			expanded_infcwd.c_str (), /* current directory */
+			/* current directory */
+			(inferior_cwd == NULL
+			 ? NULL
+			 : gdb_tilde_expand (inferior_cwd).c_str()),
 			&si,      /* start info */
 			pi);      /* proc info */
 #endif
-- 
2.1.4

Re: [PATCH 1/3] [gdbserver/win32] fatal "glob could not process pattern '(null)'" error

[Pedro Alves]

On 05/04/2018 07:30 PM, Joel Brobecker wrote:
> Trying to start GDBserver on Windows currently yields the following
> error...
> 
>     $ gdbserver.exe --once :4444 simple_main.exe
>     glob could not process pattern '(null)'.
>     Exiting
> 
> ... after which GDB terminates with a nonzero status.
> 
> This is because create_process in win32-low.c calls gdb_tilde_expand
> with the result of a call to get_inferior_cwd without verifying that
> the returned directory is not NULL:
> 
>     | static BOOL
>     | create_process (const char *program, char *args,
>     |                 DWORD flags, PROCESS_INFORMATION *pi)
>     | {
>     |   const char *inferior_cwd = get_inferior_cwd ();
>     |   std::string expanded_infcwd = gdb_tilde_expand (inferior_cwd);
> 
> This patch avoids this by only calling gdb_tilde_expand when
> INFERIOR_CWD is not NULL, which is similar to what is done on
> GNU/Linux for instance.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* win32-low.c (create_process): Only call gdb_tilde_expand if
> 	inferior_cwd is not NULL.
LGTM.

Thanks,
Pedro Alves

Re: [PATCH 1/3] [gdbserver/win32] fatal "glob could not process pattern '(null)'" error

[Joel Brobecker]

> > gdb/gdbserver/ChangeLog:
> > 
> > 	* win32-low.c (create_process): Only call gdb_tilde_expand if
> > 	inferior_cwd is not NULL.
> LGTM.

Thanks Pedro. Pushed to master and gdb-8.1-branch, under PR cli/23158.

-- 
Joel

[PATCH 2/3] gdbserver/Windows: Fix "no program to debug" error

[Joel Brobecker]

Trying to start a program with GDBserver on Windows yields
the following error:

    $ gdbserver.exe --once :4444 simple_main.exe
    Killing process(es): 5008
    No program to debug
    Exiting

The error itself comes from the following code shortly after
create_inferior gets called (in server.c::main):

    /* Wait till we are at first instruction in program.  */
    create_inferior (program_path.get (), program_args);
    [...]

    if (last_status.kind == TARGET_WAITKIND_EXITED
        || last_status.kind == TARGET_WAITKIND_SIGNALLED)
      was_running = 0;
    else
      was_running = 1;

    if (!was_running && !multi_mode)
      error ("No program to debug");

What happens is that the "last_status" global starts initialized
as zeroes, which means last_status.kind == TARGET_WAITKIND_EXITED,
and we expect create_inferior to be waiting for the inferior to
start until reaching the SIGTRAP, and to set the "last_status"
global to match that last event we received.

I suspect this is an unintended side-effect of the following change...

    commit 2090129c36c7e582943b7d300968d19b46160d84
    Date:   Thu Dec 22 21:11:11 2016 -0500
    Subject: Share fork_inferior et al with gdbserver

... which removes some code in server.c that was responsible for
starting the inferior in a functin that was named start_inferior,
and looked like this:

   signal_pid = create_inferior (new_argv[0], &new_argv[0]);
   [...]
   /* Wait till we are at 1st instruction in program, return new pid
      (assuming success).  */
   last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);

The code has been transitioned to using fork_inferior, but sadly,
only for the targets that support it. On Windows, the calls to wait
setting "last_status" simply disappeared.

This patch adds it back in the Windows-specific implementation of
create_inferior.

gdb/gdbserver/ChangeLog:

	* win32-low.c (win32_create_inferior): Add call to my_wait
	setting last_status global.
---
 gdb/gdbserver/win32-low.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index db5dd49..7ed5fc5 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -704,6 +704,10 @@ win32_create_inferior (const char *program,
 
   do_initial_child_stuff (pi.hProcess, pi.dwProcessId, 0);
 
+  /* Wait till we are at 1st instruction in program, return new pid
+     (assuming success).  */
+  last_ptid = win32_wait (pid_to_ptid (current_process_id), &last_status, 0);
+
   return current_process_id;
 }
 
-- 
2.1.4

Re: [PATCH 2/3] gdbserver/Windows: Fix "no program to debug" error

[Pedro Alves]

On 05/04/2018 07:30 PM, Joel Brobecker wrote:
> Trying to start a program with GDBserver on Windows yields
> the following error:
> 
>     $ gdbserver.exe --once :4444 simple_main.exe
>     Killing process(es): 5008
>     No program to debug
>     Exiting
> 
> The error itself comes from the following code shortly after
> create_inferior gets called (in server.c::main):
> 
>     /* Wait till we are at first instruction in program.  */
>     create_inferior (program_path.get (), program_args);
>     [...]
> 
>     if (last_status.kind == TARGET_WAITKIND_EXITED
>         || last_status.kind == TARGET_WAITKIND_SIGNALLED)
>       was_running = 0;
>     else
>       was_running = 1;
> 
>     if (!was_running && !multi_mode)
>       error ("No program to debug");
> 
> What happens is that the "last_status" global starts initialized
> as zeroes, which means last_status.kind == TARGET_WAITKIND_EXITED,
> and we expect create_inferior to be waiting for the inferior to
> start until reaching the SIGTRAP, and to set the "last_status"
> global to match that last event we received.
> 
> I suspect this is an unintended side-effect of the following change...
> 
>     commit 2090129c36c7e582943b7d300968d19b46160d84
>     Date:   Thu Dec 22 21:11:11 2016 -0500
>     Subject: Share fork_inferior et al with gdbserver
> 
> ... which removes some code in server.c that was responsible for
> starting the inferior in a functin that was named start_inferior,
> and looked like this:
> 
>    signal_pid = create_inferior (new_argv[0], &new_argv[0]);
>    [...]
>    /* Wait till we are at 1st instruction in program, return new pid
>       (assuming success).  */
>    last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
> 
> The code has been transitioned to using fork_inferior, but sadly,
> only for the targets that support it. On Windows, the calls to wait
> setting "last_status" simply disappeared.
> 
> This patch adds it back in the Windows-specific implementation of
> create_inferior.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* win32-low.c (win32_create_inferior): Add call to my_wait
> 	setting last_status global.
> ---
>  gdb/gdbserver/win32-low.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
> index db5dd49..7ed5fc5 100644
> --- a/gdb/gdbserver/win32-low.c
> +++ b/gdb/gdbserver/win32-low.c
> @@ -704,6 +704,10 @@ win32_create_inferior (const char *program,
>  
>    do_initial_child_stuff (pi.hProcess, pi.dwProcessId, 0);
>  
> +  /* Wait till we are at 1st instruction in program, return new pid
> +     (assuming success).  */
> +  last_ptid = win32_wait (pid_to_ptid (current_process_id), &last_status, 0);
> +
>    return current_process_id;
>  }

Ah, I was confused about how this actually works, since
do_initial_child_stuff already waits until we're at the 1st
instruction of the program:

  /* Flush all currently pending debug events (thread and dll list) up
     to the initial breakpoint.  */
  while (1)
    {

but after staring at this for a bit, I remembered/realized the
loop leaves the status pending in the 'cached_status' global.

So the patch LGTM as is.

Thanks,
Pedro Alves

Re: [PATCH 2/3] gdbserver/Windows: Fix "no program to debug" error

[Joel Brobecker]

> > gdb/gdbserver/ChangeLog:
> > 
> > 	* win32-low.c (win32_create_inferior): Add call to my_wait
> > 	setting last_status global.
[...]
> So the patch LGTM as is.

Thanks Pedro. Pushed to master and gdb-8.1-branch, under PR cli/23158.

-- 
Joel

Re: [Windows GDBserver] Make GDBserver functional again on Windows

[Sergio Durigan Junior]

On Friday, May 04 2018, Joel Brobecker wrote:

> Hello,

Hi Joel,

> I noticed a few weeks back that GDBserver on Windows was no longer
> functional, so I started working on it as time allowed. With the
> following patches, it's mostly functional, as in I only have few
> regressions left. I wanted to have clean results before submitting,
> but a recent patch showed that waiting could cause others to duplicate
> something I had already done, so I'm posting now. The worse that can
> happen is that I realize an earlier patch has a bug shown by the current
> regressions, or that one of my patches is incomplete, but I think
> it would be nothing compared to the risk of having work being duplicated.
>
>     [PATCH 1/3] [gdbserver/win32] fatal "glob could not process pattern
>     [PATCH 2/3] gdbserver/Windows: Fix "no program to debug" error

Thanks for the patches.  The first two in the series fix issues
introduced by some modifications I've made, and I apologize for that.

I looked at them, and FWIW, they look good to me.  Thanks for taking
care of this.

> I haven't analyzed all the remaining failures yet, but most of them
> seem to be related to the "kill" command not working. I will look
> into that as soon as I have a moment (but it is looking like it may
> be a week before I have the time).

I wonder if the failures have something to do with the recent
modifications I've made on 'set print inferior-events' and the messages
printed when the inferior is killed.  Anyway, something to keep in mind.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [Windows GDBserver] Make GDBserver functional again on Windows

[Joel Brobecker]

> > I noticed a few weeks back that GDBserver on Windows was no longer
> > functional, so I started working on it as time allowed. With the
> > following patches, it's mostly functional, as in I only have few
> > regressions left. I wanted to have clean results before submitting,
> > but a recent patch showed that waiting could cause others to duplicate
> > something I had already done, so I'm posting now. The worse that can
> > happen is that I realize an earlier patch has a bug shown by the current
> > regressions, or that one of my patches is incomplete, but I think
> > it would be nothing compared to the risk of having work being duplicated.
> >
> >     [PATCH 1/3] [gdbserver/win32] fatal "glob could not process pattern
> >     [PATCH 2/3] gdbserver/Windows: Fix "no program to debug" error
> 
> Thanks for the patches.  The first two in the series fix issues
> introduced by some modifications I've made, and I apologize for that.

Thanks for having looked at the patches. No need to apologize for
breaking stuff -- it's just something that is so easy to do that
all of us do it despite our best efforts.

> > I haven't analyzed all the remaining failures yet, but most of them
> > seem to be related to the "kill" command not working. I will look
> > into that as soon as I have a moment (but it is looking like it may
> > be a week before I have the time).
> 
> I wonder if the failures have something to do with the recent
> modifications I've made on 'set print inferior-events' and the messages
> printed when the inferior is killed.  Anyway, something to keep in mind.

It's unfortunately really broken. killing fails with an error
(that I don't remember).

-- 
Joel

Re: [Windows GDBserver] Make GDBserver functional again on Windows

[Joel Brobecker]

> I only quickly experimented with GDBserver 8.1, but I think it's
> completely broken then as well :-(. Some of the fixes might be
> applicable there. I will double-check -- also as soon as I have
> a moment, and for sure before we release 8.1.1.

Thanks to Pedro for the reviews.

FTR, I verified that GDBserver 8.1 has the same issues as well,
so I applied the patches there, and the results on the branch
are back to nominal. In other words, the issues with the "kill"
command I saw on the master branch are not present in the 8.1 branch :).

-- 
Joel