[3/3] gdbserver/Windows: crash during connection establishment phase
Commit Message
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.
Comments
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
> > 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.
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
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.
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
@@ -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;
@@ -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;
@@ -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;
}
@@ -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;
}
@@ -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;
}
@@ -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 *
@@ -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. */
@@ -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;
}
new file mode 100755
@@ -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 */
@@ -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