* [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers
2018-06-25 18:55 x86_64-windows GDB crash due to fs_base/gs_base registers Joel Brobecker
2018-06-25 18:55 ` [PATCH 1/2] Minor reorganization of fetch_registers/store_registers in windows-nat.c Joel Brobecker
@ 2018-06-25 18:55 ` Joel Brobecker
2018-06-26 16:00 ` Pedro Alves
1 sibling, 1 reply; 10+ messages in thread
From: Joel Brobecker @ 2018-06-25 18:55 UTC (permalink / raw)
To: gdb-patches
GDB is currently crashing anytime we try to access the fs_base/gs_base
registers, either to read them, or to write them. This can be observed
under various scenarios:
- Explicit reference to those registers (eg: print $fs_base) --
probably relatively rare;
- Calling a function in the inferior, with the crash happening
because we are trying to read those registers in order to save
their value ahead of making the function call;
- Just a plain "info registers";
The crash was introduced by the following commit:
| commit 48aeef91c248291dd03583798904612426b1f40a
| Date: Mon Jun 26 18:14:43 2017 -0700
| Subject: Include the fs_base and gs_base registers in amd64 target descriptions.
The Windows-nat implementation was unfortunately not prepared to deal
with those new registers. In particular, the way it fetches registers
is done by using a table where the index is the register number, and
the value at that index is the offset in the area in the thread's CONTEXT
data where the corresponding register value is stored.
For instance, in amd64-windows-nat.c, we can find the mappings static
array containing the following 57 elements in it:
#define context_offset(x) (offsetof (CONTEXT, x))
static const int mappings[] =
{
context_offset (Rax),
[...]
context_offset (FloatSave.MxCsr)
};
That array is then used by windows_fetch_one_register via:
char *context_offset = ((char *) &th->context) + mappings[r];
The problem is that fs_base's register number is 172, which is
well past the end of the mappings array (57 elements in total).
We end up getting an undefined offset, which happens to be so large
that it then causes the address where we try to read the register
value (a little bit later) to be invalid, thus crashing GDB with
a SEGV.
This patch changes the approach taken for determining the offset
of a given register. Instead of the target-specific part of
window-nat (i.e. i386-windows-nat.c and amd64-windows-nat.c)
registering a table of offsets to windows-nat.c, they now
register functions that, given a register number, return an
offset. That way, we can more explicitly match register numbers
to specific offsets, which is a much less fragile, because
it doesn't depend on a register numbering which might change.
It is also resilient to the addition of new registers in the
fugure.
And regarding fs_base and gs_base themselves, it appears managing
those registers might not be supported; could not find a field
in CONTEXT that would correspond to those registers. So this patch
treats those two registers as not available. Eg:
(gdb) info registers
[...]
fs_base <unavailable>
gs_base <unavailable>
New AMD64_*_REGNUM and I386_*_REGNUM enumerates are being added
as well. We could have done without them, but they make the code
clearer to read.
gdb/ChangeLog:
* amd64-tdep.h (enum amd64_regnum): Add AMD64_ST2_REGNUM,
AMD64_ST3_REGNUM, AMD64_ST4_REGNUM, AMD64_ST5_REGNUM,
AMD64_ST6_REGNUM, AMD64_ST7_REGNUM, AMD64_FISEG_REGNUM,
AMD64_FIOFF_REGNUM, AMD64_FOSEG_REGNUM, AMD64_FOOFF_REGNUM,
AMD64_FOP_REGNUM, AMD64_XMM2_REGNUM, AMD64_XMM3_REGNUM,
AMD64_XMM4_REGNUM, AMD64_XMM5_REGNUM, AMD64_XMM6_REGNUM,
AMD64_XMM7_REGNUM, AMD64_XMM8_REGNUM, AMD64_XMM9_REGNUM,
AMD64_XMM10_REGNUM, AMD64_XMM11_REGNUM, AMD64_XMM12_REGNUM,
AMD64_XMM13_REGNUM, AMD64_XMM14_REGNUM, AMD64_XMM15_REGNUM.
* amd64-windows-nat.c (mappings): Delete.
(amd64_windows_context_register_offset): New function.
(_initialize_amd64_windows_nat): Replace call to
windows_set_context_register_offsets by call to
windows_set_context_register_offset.
* i386-tdep.h (enum i386_regnum): Add I386_ST1_REGNUM,
I386_ST2_REGNUM, I386_ST3_REGNUM, I386_ST4_REGNUM, I386_ST5_REGNUM,
I386_ST6_REGNUM, I386_ST7_REGNUM, I386_FCTRL_REGNUM,
I386_FSTAT_REGNUM, I386_FTAG_REGNUM, I386_FISEG_REGNUM,
I386_FIOFF_REGNUM, I386_FOSEG_REGNUM, I386_FOOFF_REGNUM,
I386_FOP_REGNUM, I386_XMM0_REGNUM, I386_XMM1_REGNUM,
I386_XMM2_REGNUM, I386_XMM3_REGNUM, I386_XMM4_REGNUM,
I386_XMM5_REGNUM, I386_XMM6_REGNUM, I386_XMM7_REGNUM.
* i386-windows-nat.c (mappings): Delete.
(i386_windows_context_register_offset): New function.
(_initialize_i386_windows_nat): Replace call to
windows_set_context_register_offsets by call to
windows_set_context_register_offset.
* windows-nat.c (mappings): Delete.
(context_register_offset): New static global.
(windows_set_context_register_offsets): Renamed to
windows_set_context_register_offset. Update.
(windows_fetch_one_register): Use context_register_offset
instead of mappings to compute the register location.
Add handling of the case where the register is not supported.
Rename context_offset into reg_buf.
(windows_store_one_register): Likewise.
* windows-nat.h (context_register_offset_ftype): New typedef.
(windows_set_context_register_offsets): Rename to
windows_set_context_register_offset, and change paramter
to "context_register_offset_ftype *" instead of "int".
No test added, as this issue is detected by gdb.base/reggroups.exp.
Any testcase that makes an inferior function call would also detect
this issue.
Tested on x86-windows and x86_64-windows using AdaCore's testsuite.
---
gdb/amd64-tdep.h | 25 +++++++++
gdb/amd64-windows-nat.c | 142 +++++++++++++++++++++++++++---------------------
gdb/i386-tdep.h | 23 ++++++++
gdb/i386-windows-nat.c | 110 +++++++++++++++++++++----------------
gdb/windows-nat.c | 46 ++++++++++------
gdb/windows-nat.h | 9 ++-
6 files changed, 226 insertions(+), 129 deletions(-)
diff --git a/gdb/amd64-tdep.h b/gdb/amd64-tdep.h
index 7d3791a..9c1b650 100644
--- a/gdb/amd64-tdep.h
+++ b/gdb/amd64-tdep.h
@@ -57,11 +57,36 @@ enum amd64_regnum
AMD64_GS_REGNUM, /* %gs */
AMD64_ST0_REGNUM = 24, /* %st0 */
AMD64_ST1_REGNUM, /* %st1 */
+ AMD64_ST2_REGNUM, /* %st2 */
+ AMD64_ST3_REGNUM, /* %st3 */
+ AMD64_ST4_REGNUM, /* %st4 */
+ AMD64_ST5_REGNUM, /* %st5 */
+ AMD64_ST6_REGNUM, /* %st6 */
+ AMD64_ST7_REGNUM, /* %st7 */
AMD64_FCTRL_REGNUM = AMD64_ST0_REGNUM + 8,
AMD64_FSTAT_REGNUM = AMD64_ST0_REGNUM + 9,
AMD64_FTAG_REGNUM = AMD64_ST0_REGNUM + 10,
+ AMD64_FISEG_REGNUM,
+ AMD64_FIOFF_REGNUM,
+ AMD64_FOSEG_REGNUM,
+ AMD64_FOOFF_REGNUM,
+ AMD64_FOP_REGNUM,
AMD64_XMM0_REGNUM = 40, /* %xmm0 */
AMD64_XMM1_REGNUM, /* %xmm1 */
+ AMD64_XMM2_REGNUM, /* %xmm2 */
+ AMD64_XMM3_REGNUM, /* %xmm3 */
+ AMD64_XMM4_REGNUM, /* %xmm4 */
+ AMD64_XMM5_REGNUM, /* %xmm5 */
+ AMD64_XMM6_REGNUM, /* %xmm6 */
+ AMD64_XMM7_REGNUM, /* %xmm7 */
+ AMD64_XMM8_REGNUM, /* %xmm8 */
+ AMD64_XMM9_REGNUM, /* %xmm9 */
+ AMD64_XMM10_REGNUM, /* %xmm10 */
+ AMD64_XMM11_REGNUM, /* %xmm11 */
+ AMD64_XMM12_REGNUM, /* %xmm12 */
+ AMD64_XMM13_REGNUM, /* %xmm13 */
+ AMD64_XMM14_REGNUM, /* %xmm14 */
+ AMD64_XMM15_REGNUM, /* %xmm15 */
AMD64_MXCSR_REGNUM = AMD64_XMM0_REGNUM + 16,
AMD64_YMM0H_REGNUM, /* %ymm0h */
AMD64_YMM15H_REGNUM = AMD64_YMM0H_REGNUM + 15,
diff --git a/gdb/amd64-windows-nat.c b/gdb/amd64-windows-nat.c
index 0c7dbed..61aed8d 100644
--- a/gdb/amd64-windows-nat.c
+++ b/gdb/amd64-windows-nat.c
@@ -22,70 +22,86 @@
#include <windows.h>
-#define context_offset(x) (offsetof (CONTEXT, x))
-static const int mappings[] =
+/* See context_register_offset_ftype. */
+
+static int
+amd64_windows_context_register_offset (int regnum)
{
- context_offset (Rax),
- context_offset (Rbx),
- context_offset (Rcx),
- context_offset (Rdx),
- context_offset (Rsi),
- context_offset (Rdi),
- context_offset (Rbp),
- context_offset (Rsp),
- context_offset (R8),
- context_offset (R9),
- context_offset (R10),
- context_offset (R11),
- context_offset (R12),
- context_offset (R13),
- context_offset (R14),
- context_offset (R15),
- context_offset (Rip),
- context_offset (EFlags),
- context_offset (SegCs),
- context_offset (SegSs),
- context_offset (SegDs),
- context_offset (SegEs),
- context_offset (SegFs),
- context_offset (SegGs),
- context_offset (FloatSave.FloatRegisters[0]),
- context_offset (FloatSave.FloatRegisters[1]),
- context_offset (FloatSave.FloatRegisters[2]),
- context_offset (FloatSave.FloatRegisters[3]),
- context_offset (FloatSave.FloatRegisters[4]),
- context_offset (FloatSave.FloatRegisters[5]),
- context_offset (FloatSave.FloatRegisters[6]),
- context_offset (FloatSave.FloatRegisters[7]),
- context_offset (FloatSave.ControlWord),
- context_offset (FloatSave.StatusWord),
- context_offset (FloatSave.TagWord),
- context_offset (FloatSave.ErrorSelector),
- context_offset (FloatSave.ErrorOffset),
- context_offset (FloatSave.DataSelector),
- context_offset (FloatSave.DataOffset),
- context_offset (FloatSave.ErrorSelector)
- /* XMM0-7 */ ,
- context_offset (Xmm0),
- context_offset (Xmm1),
- context_offset (Xmm2),
- context_offset (Xmm3),
- context_offset (Xmm4),
- context_offset (Xmm5),
- context_offset (Xmm6),
- context_offset (Xmm7),
- context_offset (Xmm8),
- context_offset (Xmm9),
- context_offset (Xmm10),
- context_offset (Xmm11),
- context_offset (Xmm12),
- context_offset (Xmm13),
- context_offset (Xmm14),
- context_offset (Xmm15),
- /* MXCSR */
- context_offset (FloatSave.MxCsr)
-};
+#define context_offset(x) (offsetof (CONTEXT, x))
+ switch (regnum)
+ {
+ case AMD64_RAX_REGNUM: return context_offset (Rax);
+ case AMD64_RBX_REGNUM: return context_offset (Rbx);
+ case AMD64_RCX_REGNUM: return context_offset (Rcx);
+ case AMD64_RDX_REGNUM: return context_offset (Rdx);
+ case AMD64_RSI_REGNUM: return context_offset (Rsi);
+ case AMD64_RDI_REGNUM: return context_offset (Rdi);
+ case AMD64_RBP_REGNUM: return context_offset (Rbp);
+ case AMD64_RSP_REGNUM: return context_offset (Rsp);
+ case AMD64_R8_REGNUM: return context_offset (R8);
+ case AMD64_R9_REGNUM: return context_offset (R9);
+ case AMD64_R10_REGNUM: return context_offset (R10);
+ case AMD64_R11_REGNUM: return context_offset (R11);
+ case AMD64_R12_REGNUM: return context_offset (R12);
+ case AMD64_R13_REGNUM: return context_offset (R13);
+ case AMD64_R14_REGNUM: return context_offset (R14);
+ case AMD64_R15_REGNUM: return context_offset (R15);
+ case AMD64_RIP_REGNUM: return context_offset (Rip);
+ case AMD64_EFLAGS_REGNUM: return context_offset (EFlags);
+ case AMD64_CS_REGNUM: return context_offset (SegCs);
+ case AMD64_SS_REGNUM: return context_offset (SegSs);
+ case AMD64_DS_REGNUM: return context_offset (SegDs);
+ case AMD64_ES_REGNUM: return context_offset (SegEs);
+ case AMD64_FS_REGNUM: return context_offset (SegFs);
+ case AMD64_GS_REGNUM: return context_offset (SegGs);
+ case AMD64_ST0_REGNUM:
+ return context_offset (FloatSave.FloatRegisters[0]);
+ case AMD64_ST1_REGNUM:
+ return context_offset (FloatSave.FloatRegisters[1]);
+ case AMD64_ST2_REGNUM:
+ return context_offset (FloatSave.FloatRegisters[2]);
+ case AMD64_ST3_REGNUM:
+ return context_offset (FloatSave.FloatRegisters[3]);
+ case AMD64_ST4_REGNUM:
+ return context_offset (FloatSave.FloatRegisters[4]);
+ case AMD64_ST5_REGNUM:
+ return context_offset (FloatSave.FloatRegisters[5]);
+ case AMD64_ST6_REGNUM:
+ return context_offset (FloatSave.FloatRegisters[6]);
+ case AMD64_ST7_REGNUM:
+ return context_offset (FloatSave.FloatRegisters[7]);
+ case AMD64_FCTRL_REGNUM: return context_offset (FloatSave.ControlWord);
+ case AMD64_FSTAT_REGNUM: return context_offset (FloatSave.StatusWord);
+ case AMD64_FTAG_REGNUM: return context_offset (FloatSave.TagWord);
+ case AMD64_FISEG_REGNUM: return context_offset (FloatSave.ErrorSelector);
+ case AMD64_FIOFF_REGNUM: return context_offset (FloatSave.ErrorOffset);
+ case AMD64_FOSEG_REGNUM: return context_offset (FloatSave.DataSelector);
+ case AMD64_FOOFF_REGNUM: return context_offset (FloatSave.DataOffset);
+ case AMD64_FOP_REGNUM: return context_offset (FloatSave.ErrorSelector);
+ case AMD64_XMM0_REGNUM: return context_offset (Xmm0);
+ case AMD64_XMM1_REGNUM: return context_offset (Xmm1);
+ case AMD64_XMM2_REGNUM: return context_offset (Xmm2);
+ case AMD64_XMM3_REGNUM: return context_offset (Xmm3);
+ case AMD64_XMM4_REGNUM: return context_offset (Xmm4);
+ case AMD64_XMM5_REGNUM: return context_offset (Xmm5);
+ case AMD64_XMM6_REGNUM: return context_offset (Xmm6);
+ case AMD64_XMM7_REGNUM: return context_offset (Xmm7);
+ case AMD64_XMM8_REGNUM: return context_offset (Xmm8);
+ case AMD64_XMM9_REGNUM: return context_offset (Xmm9);
+ case AMD64_XMM10_REGNUM: return context_offset (Xmm10);
+ case AMD64_XMM11_REGNUM: return context_offset (Xmm11);
+ case AMD64_XMM12_REGNUM: return context_offset (Xmm12);
+ case AMD64_XMM13_REGNUM: return context_offset (Xmm13);
+ case AMD64_XMM14_REGNUM: return context_offset (Xmm14);
+ case AMD64_XMM15_REGNUM: return context_offset (Xmm15);
+ case AMD64_MXCSR_REGNUM: return context_offset (FloatSave.MxCsr);
+ case AMD64_FSBASE_REGNUM: return -1 /* Not in CONTEXT struct. */;
+ case AMD64_GSBASE_REGNUM: return -1 /* Not in CONTEXT struct. */;
+ }
#undef context_offset
+
+ return -1;
+}
/* segment_register_p_ftype implementation for amd64. */
@@ -98,7 +114,7 @@ amd64_windows_segment_register_p (int regnum)
void
_initialize_amd64_windows_nat (void)
{
- windows_set_context_register_offsets (mappings);
+ windows_set_context_register_offset (amd64_windows_context_register_offset);
windows_set_segment_register_p (amd64_windows_segment_register_p);
x86_set_debug_register_length (8);
}
diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h
index 81a93f1..8cfa935 100644
--- a/gdb/i386-tdep.h
+++ b/gdb/i386-tdep.h
@@ -285,6 +285,29 @@ enum i386_regnum
I386_FS_REGNUM, /* %fs */
I386_GS_REGNUM, /* %gs */
I386_ST0_REGNUM, /* %st(0) */
+ I386_ST1_REGNUM, /* %st(1) */
+ I386_ST2_REGNUM, /* %st(2) */
+ I386_ST3_REGNUM, /* %st(3) */
+ I386_ST4_REGNUM, /* %st(4) */
+ I386_ST5_REGNUM, /* %st(5) */
+ I386_ST6_REGNUM, /* %st(6) */
+ I386_ST7_REGNUM, /* %st(7) */
+ I386_FCTRL_REGNUM,
+ I386_FSTAT_REGNUM,
+ I386_FTAG_REGNUM,
+ I386_FISEG_REGNUM,
+ I386_FIOFF_REGNUM,
+ I386_FOSEG_REGNUM,
+ I386_FOOFF_REGNUM,
+ I386_FOP_REGNUM,
+ I386_XMM0_REGNUM, /* %xmm0 */
+ I386_XMM1_REGNUM, /* %xmm1 */
+ I386_XMM2_REGNUM, /* %xmm2 */
+ I386_XMM3_REGNUM, /* %xmm3 */
+ I386_XMM4_REGNUM, /* %xmm4 */
+ I386_XMM5_REGNUM, /* %xmm5 */
+ I386_XMM6_REGNUM, /* %xmm6 */
+ I386_XMM7_REGNUM, /* %xmm7 */
I386_MXCSR_REGNUM = 40, /* %mxcsr */
I386_YMM0H_REGNUM, /* %ymm0h */
I386_YMM7H_REGNUM = I386_YMM0H_REGNUM + 7,
diff --git a/gdb/i386-windows-nat.c b/gdb/i386-windows-nat.c
index 9efe184..1f65774 100644
--- a/gdb/i386-windows-nat.c
+++ b/gdb/i386-windows-nat.c
@@ -22,55 +22,71 @@
#include <windows.h>
-#define context_offset(x) ((int)&(((CONTEXT *)NULL)->x))
-static const int mappings[] =
+/* See context_register_offset_ftype. */
+
+static int
+i386_windows_context_register_offset (int regnum)
{
- context_offset (Eax),
- context_offset (Ecx),
- context_offset (Edx),
- context_offset (Ebx),
- context_offset (Esp),
- context_offset (Ebp),
- context_offset (Esi),
- context_offset (Edi),
- context_offset (Eip),
- context_offset (EFlags),
- context_offset (SegCs),
- context_offset (SegSs),
- context_offset (SegDs),
- context_offset (SegEs),
- context_offset (SegFs),
- context_offset (SegGs),
- context_offset (FloatSave.RegisterArea[0 * 10]),
- context_offset (FloatSave.RegisterArea[1 * 10]),
- context_offset (FloatSave.RegisterArea[2 * 10]),
- context_offset (FloatSave.RegisterArea[3 * 10]),
- context_offset (FloatSave.RegisterArea[4 * 10]),
- context_offset (FloatSave.RegisterArea[5 * 10]),
- context_offset (FloatSave.RegisterArea[6 * 10]),
- context_offset (FloatSave.RegisterArea[7 * 10]),
- context_offset (FloatSave.ControlWord),
- context_offset (FloatSave.StatusWord),
- context_offset (FloatSave.TagWord),
- context_offset (FloatSave.ErrorSelector),
- context_offset (FloatSave.ErrorOffset),
- context_offset (FloatSave.DataSelector),
- context_offset (FloatSave.DataOffset),
- context_offset (FloatSave.ErrorSelector)
- /* XMM0-7 */ ,
- context_offset (ExtendedRegisters[10*16]),
- context_offset (ExtendedRegisters[11*16]),
- context_offset (ExtendedRegisters[12*16]),
- context_offset (ExtendedRegisters[13*16]),
- context_offset (ExtendedRegisters[14*16]),
- context_offset (ExtendedRegisters[15*16]),
- context_offset (ExtendedRegisters[16*16]),
- context_offset (ExtendedRegisters[17*16]),
- /* MXCSR */
- context_offset (ExtendedRegisters[24])
-};
+#define context_offset(x) ((int)&(((CONTEXT *)NULL)->x))
+ switch (regnum)
+ {
+ case I386_EAX_REGNUM: return context_offset (Eax);
+ case I386_ECX_REGNUM: return context_offset (Ecx);
+ case I386_EDX_REGNUM: return context_offset (Edx);
+ case I386_EBX_REGNUM: return context_offset (Ebx);
+ case I386_ESP_REGNUM: return context_offset (Esp);
+ case I386_EBP_REGNUM: return context_offset (Ebp);
+ case I386_ESI_REGNUM: return context_offset (Esi);
+ case I386_EDI_REGNUM: return context_offset (Edi);
+ case I386_EIP_REGNUM: return context_offset (Eip);
+ case I386_EFLAGS_REGNUM: return context_offset (EFlags);
+ case I386_CS_REGNUM: return context_offset (SegCs);
+ case I386_SS_REGNUM: return context_offset (SegSs);
+ case I386_DS_REGNUM: return context_offset (SegDs);
+ case I386_ES_REGNUM: return context_offset (SegEs);
+ case I386_FS_REGNUM: return context_offset (SegFs);
+ case I386_GS_REGNUM: return context_offset (SegGs);
+ case I386_ST0_REGNUM:
+ return context_offset (FloatSave.RegisterArea[0 * 10]);
+ case I386_ST1_REGNUM:
+ return context_offset (FloatSave.RegisterArea[1 * 10]);
+ case I386_ST2_REGNUM:
+ return context_offset (FloatSave.RegisterArea[2 * 10]);
+ case I386_ST3_REGNUM:
+ return context_offset (FloatSave.RegisterArea[3 * 10]);
+ case I386_ST4_REGNUM:
+ return context_offset (FloatSave.RegisterArea[4 * 10]);
+ case I386_ST5_REGNUM:
+ return context_offset (FloatSave.RegisterArea[5 * 10]);
+ case I386_ST6_REGNUM:
+ return context_offset (FloatSave.RegisterArea[6 * 10]);
+ case I386_ST7_REGNUM:
+ return context_offset (FloatSave.RegisterArea[7 * 10]);
+ case I386_FCTRL_REGNUM: return context_offset (FloatSave.ControlWord);
+ case I386_FSTAT_REGNUM: return context_offset (FloatSave.StatusWord);
+ case I386_FTAG_REGNUM: return context_offset (FloatSave.TagWord);
+ case I386_FISEG_REGNUM: return context_offset (FloatSave.ErrorSelector);
+ case I386_FIOFF_REGNUM: return context_offset (FloatSave.ErrorOffset);
+ case I386_FOSEG_REGNUM: return context_offset (FloatSave.DataSelector);
+ case I386_FOOFF_REGNUM: return context_offset (FloatSave.DataOffset);
+ case I386_FOP_REGNUM: return context_offset (FloatSave.ErrorSelector);
+ /* XMM0-7 */
+ case I386_XMM0_REGNUM: return context_offset (ExtendedRegisters[10*16]);
+ case I386_XMM1_REGNUM: return context_offset (ExtendedRegisters[11*16]);
+ case I386_XMM2_REGNUM: return context_offset (ExtendedRegisters[12*16]);
+ case I386_XMM3_REGNUM: return context_offset (ExtendedRegisters[13*16]);
+ case I386_XMM4_REGNUM: return context_offset (ExtendedRegisters[14*16]);
+ case I386_XMM5_REGNUM: return context_offset (ExtendedRegisters[15*16]);
+ case I386_XMM6_REGNUM: return context_offset (ExtendedRegisters[16*16]);
+ case I386_XMM7_REGNUM: return context_offset (ExtendedRegisters[17*16]);
+ /* MXCSR */
+ case I386_MXCSR_REGNUM: return context_offset (ExtendedRegisters[24]);
+ }
#undef context_offset
+ return -1;
+}
+
/* segment_register_p_ftype implementation for x86. */
static int
@@ -82,7 +98,7 @@ i386_windows_segment_register_p (int regnum)
void
_initialize_i386_windows_nat (void)
{
- windows_set_context_register_offsets (mappings);
+ windows_set_context_register_offset (i386_windows_context_register_offset);
windows_set_segment_register_p (i386_windows_segment_register_p);
x86_set_debug_register_length (4);
}
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index c51ef8f..3d4da60 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -254,15 +254,9 @@ static int debug_memory = 0; /* show target memory accesses */
static int debug_exceptions = 0; /* show target exceptions */
static int useshell = 0; /* use shell for subprocesses */
-/* This vector maps GDB's idea of a register's number into an offset
- in the windows exception context vector.
-
- It also contains the bit mask needed to load the register in question.
-
- The contents of this table can only be computed by the units
- that provide CPU-specific support for Windows native debugging.
- These units should set the table by calling
- windows_set_context_register_offsets.
+/* A function that returns the offset of a given register inside
+ a thread's CONTEXT structure. Should be set via a call to
+ windows_set_context_register_offset.
One day we could read a reg, we could inspect the context we
already have loaded, if it doesn't have the bit set that we need,
@@ -272,7 +266,7 @@ static int useshell = 0; /* use shell for subprocesses */
the other regs of the group, and then we copy the info in and set
out bit. */
-static const int *mappings;
+static context_register_offset_ftype *context_register_offset;
/* The function to use in order to determine whether a register is
a segment register or not. */
@@ -354,9 +348,9 @@ static windows_nat_target the_windows_nat_target;
See the description of MAPPINGS for more details. */
void
-windows_set_context_register_offsets (const int *offsets)
+windows_set_context_register_offset (context_register_offset_ftype *fun)
{
- mappings = offsets;
+ context_register_offset = fun;
}
/* See windows-nat.h. */
@@ -545,18 +539,27 @@ windows_fetch_one_register (struct regcache *regcache,
gdb_assert (r >= 0);
gdb_assert (!th->reload_context);
- char *context_offset = ((char *) &th->context) + mappings[r];
struct gdbarch *gdbarch = regcache->arch ();
struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+ int reg_offset = context_register_offset (r);
+
+ if (reg_offset < 0)
+ {
+ /* Unsupported register, nothing we can do, so just return.
+ GDB will show it as unavailable. */
+ return;
+ }
+
+ char *reg_buf = ((char *) &th->context) + reg_offset;
if (r == I387_FISEG_REGNUM (tdep))
{
- long l = *((long *) context_offset) & 0xffff;
+ long l = *((long *) reg_buf) & 0xffff;
regcache->raw_supply (r, (char *) &l);
}
else if (r == I387_FOP_REGNUM (tdep))
{
- long l = (*((long *) context_offset) >> 16) & ((1 << 11) - 1);
+ long l = (*((long *) reg_buf) >> 16) & ((1 << 11) - 1);
regcache->raw_supply (r, (char *) &l);
}
else if (segment_register_p (r))
@@ -564,11 +567,11 @@ windows_fetch_one_register (struct regcache *regcache,
/* GDB treats segment registers as 32bit registers, but they are
in fact only 16 bits long. Make sure we do not read extra
bits from our source buffer. */
- long l = *((long *) context_offset) & 0xffff;
+ long l = *((long *) reg_buf) & 0xffff;
regcache->raw_supply (r, (char *) &l);
}
else
- regcache->raw_supply (r, context_offset);
+ regcache->raw_supply (r, reg_buf);
}
void
@@ -629,7 +632,14 @@ windows_store_one_register (const struct regcache *regcache,
{
gdb_assert (r >= 0);
- regcache->raw_collect (r, ((char *) &th->context) + mappings[r]);
+ int reg_offset = context_register_offset (r);
+ if (reg_offset < 0)
+ {
+ /* Unsupported register, nothing we can do, so just return. */
+ return;
+ }
+
+ regcache->raw_collect (r, ((char *) &th->context) + reg_offset);
}
/* Store a new register value into the context of the thread tied to
diff --git a/gdb/windows-nat.h b/gdb/windows-nat.h
index a6a7f18..f4c9716 100644
--- a/gdb/windows-nat.h
+++ b/gdb/windows-nat.h
@@ -18,7 +18,14 @@
#ifndef WINDOWS_NAT_H
#define WINDOWS_NAT_H
-extern void windows_set_context_register_offsets (const int *offsets);
+/* A pointer to a function which, given a register number, returns
+ its offset into a thread's CONTEXT structure.
+
+ Returns a negative value if the register is not supported. */
+typedef int (context_register_offset_ftype) (int regnum);
+
+extern void windows_set_context_register_offset
+ (context_register_offset_ftype *fun);
/* A pointer to a function that should return non-zero iff REGNUM
corresponds to one of the segment registers. */
--
2.1.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* x86_64-windows GDB crash due to fs_base/gs_base registers
@ 2018-06-25 18:55 Joel Brobecker
2018-06-25 18:55 ` [PATCH 1/2] Minor reorganization of fetch_registers/store_registers in windows-nat.c Joel Brobecker
2018-06-25 18:55 ` [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers Joel Brobecker
0 siblings, 2 replies; 10+ messages in thread
From: Joel Brobecker @ 2018-06-25 18:55 UTC (permalink / raw)
To: gdb-patches
Hello,
Here are two patches aimed at fixing a crash in GDB when debugging
on native windows targets. The crash only occurs when trying to access
the fs_base/gs_base register, but is fairly easy to trigger, as any
function call will cause that. "info reg" will also trigger it.
When I saw this failure, it made me realize that I found the current
code to be quite fragile, and I wasn't surprised once I understood
the situation that it eventually lead to this crash. As a result,
I decided to implement the register fetch/store code a bit differently.
As such, I did the work in two steps, hence the two patches.
[PATCH 1/2] Minor reorganization of fetch_registers/store_registers
[PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers
Both patches were tested on x86-windows and x86_64-windows, using
AdaCore's testsuite.
Let me know what you think of the approach and choices taken in those
two patches.
Thanks!
--
Joel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] Minor reorganization of fetch_registers/store_registers in windows-nat.c
2018-06-25 18:55 x86_64-windows GDB crash due to fs_base/gs_base registers Joel Brobecker
@ 2018-06-25 18:55 ` Joel Brobecker
2018-06-26 16:03 ` Pedro Alves
2018-06-25 18:55 ` [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers Joel Brobecker
1 sibling, 1 reply; 10+ messages in thread
From: Joel Brobecker @ 2018-06-25 18:55 UTC (permalink / raw)
To: gdb-patches
This patch is a small reorganizational patch that splits
do_windows_fetch_inferior_registers into two parts:
(a) One part that first reloads the thread's context when needed,
and then decides based on the given register number whether
one register needs to be fetched or all of them.
This part is moved to windows_nat_target::fetch_registers.
(b) The rest of the code, which actually fetches the register value
and supplies it to the regcache.
A similar treatment is applied to do_windows_store_inferior_registers.
This change is preparation work for changing the way we calculate
the location of a given register in the thread context structure,
and should be a no op.
gdb/ChangeLog:
* windows-nat.c (do_windows_fetch_inferior_registers): Rename
to windows_fetch_one_register, and only handle the case of
fetching one register. Move the code that reloads the context
and iterates over all registers if R is negative to...
(windows_nat_target::fetch_registers): ... here.
(do_windows_store_inferior_registers): Rename to
windows_store_one_register, and only handle the case of storing
one register. Move the code that handles the case where r is
negative to...
(windows_nat_target::store_registers) ... here.
Tested on x86-windows and x86_64-windows using AdaCore's testsuite.
---
gdb/windows-nat.c | 115 ++++++++++++++++++++++++++++++------------------------
1 file changed, 65 insertions(+), 50 deletions(-)
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 620e25c..c51ef8f 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -528,14 +528,59 @@ windows_delete_thread (const ptid_t &ptid, DWORD exit_code)
}
}
+/* Fetches register number R from the given windows_thread_info,
+ and supplies its value to the given regcache.
+
+ This function assumes that R is either null or positive. A failed
+ assertion is raised if that is not true.
+
+ This function assumes that TH->RELOAD_CONTEXT is not set, meaning
+ that the windows_thread_info has an up-to-date context. A failed
+ assertion is raised if that assumption is violated. */
+
static void
-do_windows_fetch_inferior_registers (struct regcache *regcache,
- windows_thread_info *th, int r)
+windows_fetch_one_register (struct regcache *regcache,
+ windows_thread_info *th, int r)
{
+ gdb_assert (r >= 0);
+ gdb_assert (!th->reload_context);
+
char *context_offset = ((char *) &th->context) + mappings[r];
struct gdbarch *gdbarch = regcache->arch ();
struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
- long l;
+
+ if (r == I387_FISEG_REGNUM (tdep))
+ {
+ long l = *((long *) context_offset) & 0xffff;
+ regcache->raw_supply (r, (char *) &l);
+ }
+ else if (r == I387_FOP_REGNUM (tdep))
+ {
+ long l = (*((long *) context_offset) >> 16) & ((1 << 11) - 1);
+ regcache->raw_supply (r, (char *) &l);
+ }
+ else if (segment_register_p (r))
+ {
+ /* GDB treats segment registers as 32bit registers, but they are
+ in fact only 16 bits long. Make sure we do not read extra
+ bits from our source buffer. */
+ long l = *((long *) context_offset) & 0xffff;
+ regcache->raw_supply (r, (char *) &l);
+ }
+ else
+ regcache->raw_supply (r, context_offset);
+}
+
+void
+windows_nat_target::fetch_registers (struct regcache *regcache, int r)
+{
+ DWORD pid = ptid_get_tid (regcache->ptid ());
+ windows_thread_info *th = thread_rec (pid, TRUE);
+
+ /* Check if TH exists. Windows sometimes uses a non-existent
+ thread id in its events. */
+ if (th == NULL)
+ return;
if (th->reload_context)
{
@@ -571,56 +616,20 @@ do_windows_fetch_inferior_registers (struct regcache *regcache,
th->reload_context = 0;
}
- if (r == I387_FISEG_REGNUM (tdep))
- {
- l = *((long *) context_offset) & 0xffff;
- regcache->raw_supply (r, (char *) &l);
- }
- else if (r == I387_FOP_REGNUM (tdep))
- {
- l = (*((long *) context_offset) >> 16) & ((1 << 11) - 1);
- regcache->raw_supply (r, (char *) &l);
- }
- else if (segment_register_p (r))
- {
- /* GDB treats segment registers as 32bit registers, but they are
- in fact only 16 bits long. Make sure we do not read extra
- bits from our source buffer. */
- l = *((long *) context_offset) & 0xffff;
- regcache->raw_supply (r, (char *) &l);
- }
- else if (r >= 0)
- regcache->raw_supply (r, context_offset);
+ if (r < 0)
+ for (r = 0; r < gdbarch_num_regs (regcache->arch()); r++)
+ windows_fetch_one_register (regcache, th, r);
else
- {
- for (r = 0; r < gdbarch_num_regs (gdbarch); r++)
- do_windows_fetch_inferior_registers (regcache, th, r);
- }
-}
-
-void
-windows_nat_target::fetch_registers (struct regcache *regcache, int r)
-{
- DWORD pid = ptid_get_tid (regcache->ptid ());
- windows_thread_info *th = thread_rec (pid, TRUE);
-
- /* Check if TH exists. Windows sometimes uses a non-existent
- thread id in its events. */
- if (th != NULL)
- do_windows_fetch_inferior_registers (regcache, th, r);
+ windows_fetch_one_register (regcache, th, r);
}
static void
-do_windows_store_inferior_registers (const struct regcache *regcache,
- windows_thread_info *th, int r)
+windows_store_one_register (const struct regcache *regcache,
+ windows_thread_info *th, int r)
{
- if (r >= 0)
- regcache->raw_collect (r, ((char *) &th->context) + mappings[r]);
- else
- {
- for (r = 0; r < gdbarch_num_regs (regcache->arch ()); r++)
- do_windows_store_inferior_registers (regcache, th, r);
- }
+ gdb_assert (r >= 0);
+
+ regcache->raw_collect (r, ((char *) &th->context) + mappings[r]);
}
/* Store a new register value into the context of the thread tied to
@@ -634,8 +643,14 @@ windows_nat_target::store_registers (struct regcache *regcache, int r)
/* Check if TH exists. Windows sometimes uses a non-existent
thread id in its events. */
- if (th != NULL)
- do_windows_store_inferior_registers (regcache, th, r);
+ if (th == NULL)
+ return;
+
+ if (r < 0)
+ for (r = 0; r < gdbarch_num_regs (regcache->arch ()); r++)
+ windows_store_one_register (regcache, th, r);
+ else
+ windows_store_one_register (regcache, th, r);
}
/* Encapsulate the information required in a call to
--
2.1.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers
2018-06-25 18:55 ` [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers Joel Brobecker
@ 2018-06-26 16:00 ` Pedro Alves
2018-06-26 21:53 ` Joel Brobecker
0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2018-06-26 16:00 UTC (permalink / raw)
To: Joel Brobecker, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 19438 bytes --]
Hi Joel,
I spent a bit playing with this today. See comments below.
On 06/25/2018 07:55 PM, Joel Brobecker wrote:
> GDB is currently crashing anytime we try to access the fs_base/gs_base
> registers, either to read them, or to write them. This can be observed
> under various scenarios:
> - Explicit reference to those registers (eg: print $fs_base) --
> probably relatively rare;
> - Calling a function in the inferior, with the crash happening
> because we are trying to read those registers in order to save
> their value ahead of making the function call;
> - Just a plain "info registers";
>
> The crash was introduced by the following commit:
>
> | commit 48aeef91c248291dd03583798904612426b1f40a
> | Date: Mon Jun 26 18:14:43 2017 -0700
> | Subject: Include the fs_base and gs_base registers in amd64 target descriptions.
>
> The Windows-nat implementation was unfortunately not prepared to deal
> with those new registers. In particular, the way it fetches registers
> is done by using a table where the index is the register number, and
> the value at that index is the offset in the area in the thread's CONTEXT
> data where the corresponding register value is stored.
>
> For instance, in amd64-windows-nat.c, we can find the mappings static
> array containing the following 57 elements in it:
>
> #define context_offset(x) (offsetof (CONTEXT, x))
> static const int mappings[] =
> {
> context_offset (Rax),
> [...]
> context_offset (FloatSave.MxCsr)
> };
>
> That array is then used by windows_fetch_one_register via:
>
> char *context_offset = ((char *) &th->context) + mappings[r];
>
> The problem is that fs_base's register number is 172, which is
> well past the end of the mappings array (57 elements in total).
> We end up getting an undefined offset, which happens to be so large
> that it then causes the address where we try to read the register
> value (a little bit later) to be invalid, thus crashing GDB with
> a SEGV.
>
> This patch changes the approach taken for determining the offset
> of a given register. Instead of the target-specific part of
> window-nat (i.e. i386-windows-nat.c and amd64-windows-nat.c)
> registering a table of offsets to windows-nat.c, they now
> register functions that, given a register number, return an
> offset. That way, we can more explicitly match register numbers
> to specific offsets, which is a much less fragile, because
> it doesn't depend on a register numbering which might change.
The gdb register numbering cannot be reordered, because it would break
remote debugging when the remote target does not supply an explicit
xml target description. The only thing that can be done is add
new registers at the end.
So to fix this, we could also make windows_nat_target::{fetch,store}_registers
check whether the requested register is within the mappings array. We could
do that by simply changing the 'mappings' global from a plain pointer to a
gdb::array_view for example (which stores the array's size). I suspect the
current table based approach generates a little better code (the compiler may
end up generating a table anyway from the switch/case in your patch, but it's
not garanteed), though it shouldn't really matter in practice.
But see below.
> It is also resilient to the addition of new registers in the
> fugure.
"fugure" -> "future"
>
> And regarding fs_base and gs_base themselves, it appears managing
> those registers might not be supported; could not find a field
> in CONTEXT that would correspond to those registers. So this patch
> treats those two registers as not available. Eg:
>
> (gdb) info registers
> [...]
> fs_base <unavailable>
> gs_base <unavailable>
>
Since Windows doesn't expose these to userspace, I'm thinking that it
doesn't make much sense to show the registers at all? The (xml) target
features are designed exactly such that if you don't have some
optional feature (like org.gnu.gdb.i386.segments), then the corresponding
registers don't exist at all in gdb.
The attached patch implements that. It makes amd64_windows_init_abi
create a target description without the org.gnu.gdb.i386.segments
feature. Seems to work here, smoke testing with cross gdb targetting Windows and
doing "maint print remote-registers", and also, connecting that gdb to a Windows
gdbserver running under wine.
Before / after diff:
(gdb) info registers
rax 0x0 0
rbx 0x7b8127f0 2072061936
rcx 0x0 0
rdx 0x0 0
rsi 0x7b8127f0 2072061936
rdi 0x7b8127f0 2072061936
rbp 0x354ffd0 0x354ffd0
rsp 0x354fdc8 0x354fdc8
r8 0x354fcf0 55901424
r9 0x8 8
r10 0x8 8
r11 0x246 582
r12 0x0 0
r13 0x0 0
r14 0x0 0
r15 0x0 0
rip 0x7bcafbb5 0x7bcafbb5
eflags 0x202 [ IF ]
cs 0x33 51
ss 0x202002b 33685547
ds 0x0 0
es 0x0 0
fs 0x0 0
gs 0x2b0000 2818048
- fs_base <unavailable>
- gs_base <unavailable>
(gdb)
> diff --git a/gdb/amd64-windows-nat.c b/gdb/amd64-windows-nat.c
> index 0c7dbed..61aed8d 100644
> --- a/gdb/amd64-windows-nat.c
> +++ b/gdb/amd64-windows-nat.c
> @@ -22,70 +22,86 @@
>
> #include <windows.h>
>
> -#define context_offset(x) (offsetof (CONTEXT, x))
> -static const int mappings[] =
> +/* See context_register_offset_ftype. */
> +
> +static int
> +amd64_windows_context_register_offset (int regnum)
> {
> - context_offset (Rax),
> - context_offset (Rbx),
> - context_offset (Rcx),
> - context_offset (Rdx),
> - context_offset (Rsi),
> - context_offset (Rdi),
> - context_offset (Rbp),
> - context_offset (Rsp),
> - context_offset (R8),
> - context_offset (R9),
> - context_offset (R10),
> - context_offset (R11),
> - context_offset (R12),
> - context_offset (R13),
> - context_offset (R14),
> - context_offset (R15),
> - context_offset (Rip),
> - context_offset (EFlags),
> - context_offset (SegCs),
> - context_offset (SegSs),
> - context_offset (SegDs),
> - context_offset (SegEs),
> - context_offset (SegFs),
> - context_offset (SegGs),
> - context_offset (FloatSave.FloatRegisters[0]),
> - context_offset (FloatSave.FloatRegisters[1]),
> - context_offset (FloatSave.FloatRegisters[2]),
> - context_offset (FloatSave.FloatRegisters[3]),
> - context_offset (FloatSave.FloatRegisters[4]),
> - context_offset (FloatSave.FloatRegisters[5]),
> - context_offset (FloatSave.FloatRegisters[6]),
> - context_offset (FloatSave.FloatRegisters[7]),
> - context_offset (FloatSave.ControlWord),
> - context_offset (FloatSave.StatusWord),
> - context_offset (FloatSave.TagWord),
> - context_offset (FloatSave.ErrorSelector),
> - context_offset (FloatSave.ErrorOffset),
> - context_offset (FloatSave.DataSelector),
> - context_offset (FloatSave.DataOffset),
> - context_offset (FloatSave.ErrorSelector)
> - /* XMM0-7 */ ,
> - context_offset (Xmm0),
> - context_offset (Xmm1),
> - context_offset (Xmm2),
> - context_offset (Xmm3),
> - context_offset (Xmm4),
> - context_offset (Xmm5),
> - context_offset (Xmm6),
> - context_offset (Xmm7),
> - context_offset (Xmm8),
> - context_offset (Xmm9),
> - context_offset (Xmm10),
> - context_offset (Xmm11),
> - context_offset (Xmm12),
> - context_offset (Xmm13),
> - context_offset (Xmm14),
> - context_offset (Xmm15),
> - /* MXCSR */
> - context_offset (FloatSave.MxCsr)
> -};
> +#define context_offset(x) (offsetof (CONTEXT, x))
> + switch (regnum)
> + {
> + case AMD64_RAX_REGNUM: return context_offset (Rax);
> + case AMD64_RBX_REGNUM: return context_offset (Rbx);
> + case AMD64_RCX_REGNUM: return context_offset (Rcx);
> + case AMD64_RDX_REGNUM: return context_offset (Rdx);
> + case AMD64_RSI_REGNUM: return context_offset (Rsi);
> + case AMD64_RDI_REGNUM: return context_offset (Rdi);
> + case AMD64_RBP_REGNUM: return context_offset (Rbp);
> + case AMD64_RSP_REGNUM: return context_offset (Rsp);
> + case AMD64_R8_REGNUM: return context_offset (R8);
> + case AMD64_R9_REGNUM: return context_offset (R9);
> + case AMD64_R10_REGNUM: return context_offset (R10);
> + case AMD64_R11_REGNUM: return context_offset (R11);
> + case AMD64_R12_REGNUM: return context_offset (R12);
> + case AMD64_R13_REGNUM: return context_offset (R13);
> + case AMD64_R14_REGNUM: return context_offset (R14);
> + case AMD64_R15_REGNUM: return context_offset (R15);
> + case AMD64_RIP_REGNUM: return context_offset (Rip);
> + case AMD64_EFLAGS_REGNUM: return context_offset (EFlags);
> + case AMD64_CS_REGNUM: return context_offset (SegCs);
> + case AMD64_SS_REGNUM: return context_offset (SegSs);
> + case AMD64_DS_REGNUM: return context_offset (SegDs);
> + case AMD64_ES_REGNUM: return context_offset (SegEs);
> + case AMD64_FS_REGNUM: return context_offset (SegFs);
> + case AMD64_GS_REGNUM: return context_offset (SegGs);
> + case AMD64_ST0_REGNUM:
> + return context_offset (FloatSave.FloatRegisters[0]);
> + case AMD64_ST1_REGNUM:
> + return context_offset (FloatSave.FloatRegisters[1]);
> + case AMD64_ST2_REGNUM:
> + return context_offset (FloatSave.FloatRegisters[2]);
> + case AMD64_ST3_REGNUM:
> + return context_offset (FloatSave.FloatRegisters[3]);
> + case AMD64_ST4_REGNUM:
> + return context_offset (FloatSave.FloatRegisters[4]);
> + case AMD64_ST5_REGNUM:
> + return context_offset (FloatSave.FloatRegisters[5]);
> + case AMD64_ST6_REGNUM:
> + return context_offset (FloatSave.FloatRegisters[6]);
> + case AMD64_ST7_REGNUM:
> + return context_offset (FloatSave.FloatRegisters[7]);
> + case AMD64_FCTRL_REGNUM: return context_offset (FloatSave.ControlWord);
> + case AMD64_FSTAT_REGNUM: return context_offset (FloatSave.StatusWord);
> + case AMD64_FTAG_REGNUM: return context_offset (FloatSave.TagWord);
> + case AMD64_FISEG_REGNUM: return context_offset (FloatSave.ErrorSelector);
> + case AMD64_FIOFF_REGNUM: return context_offset (FloatSave.ErrorOffset);
> + case AMD64_FOSEG_REGNUM: return context_offset (FloatSave.DataSelector);
> + case AMD64_FOOFF_REGNUM: return context_offset (FloatSave.DataOffset);
> + case AMD64_FOP_REGNUM: return context_offset (FloatSave.ErrorSelector);
> + case AMD64_XMM0_REGNUM: return context_offset (Xmm0);
> + case AMD64_XMM1_REGNUM: return context_offset (Xmm1);
> + case AMD64_XMM2_REGNUM: return context_offset (Xmm2);
> + case AMD64_XMM3_REGNUM: return context_offset (Xmm3);
> + case AMD64_XMM4_REGNUM: return context_offset (Xmm4);
> + case AMD64_XMM5_REGNUM: return context_offset (Xmm5);
> + case AMD64_XMM6_REGNUM: return context_offset (Xmm6);
> + case AMD64_XMM7_REGNUM: return context_offset (Xmm7);
> + case AMD64_XMM8_REGNUM: return context_offset (Xmm8);
> + case AMD64_XMM9_REGNUM: return context_offset (Xmm9);
> + case AMD64_XMM10_REGNUM: return context_offset (Xmm10);
> + case AMD64_XMM11_REGNUM: return context_offset (Xmm11);
> + case AMD64_XMM12_REGNUM: return context_offset (Xmm12);
> + case AMD64_XMM13_REGNUM: return context_offset (Xmm13);
> + case AMD64_XMM14_REGNUM: return context_offset (Xmm14);
> + case AMD64_XMM15_REGNUM: return context_offset (Xmm15);
> + case AMD64_MXCSR_REGNUM: return context_offset (FloatSave.MxCsr);
I'd suggest writing the above like this:
#define CASE(GDB, CTX_REG) \
case GDB_REG: return offsetof (CONTEXT, CTX_REG)
switch (regnum)
{
CASE (AMD64_RAX_REGNUM, Rax);
CASE (AMD64_RBX_REGNUM, Rbx);
...
> + case AMD64_FSBASE_REGNUM: return -1 /* Not in CONTEXT struct. */;
> + case AMD64_GSBASE_REGNUM: return -1 /* Not in CONTEXT struct. */;
> + }
> #undef context_offset
> +
> + return -1;
> +}
>
> /* segment_register_p_ftype implementation for amd64. */
>
> @@ -98,7 +114,7 @@ amd64_windows_segment_register_p (int regnum)
> void
> _initialize_amd64_windows_nat (void)
> {
> - windows_set_context_register_offsets (mappings);
> + windows_set_context_register_offset (amd64_windows_context_register_offset);
> windows_set_segment_register_p (amd64_windows_segment_register_p);
> x86_set_debug_register_length (8);
> }
> diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h
> index 81a93f1..8cfa935 100644
> --- a/gdb/i386-tdep.h
> +++ b/gdb/i386-tdep.h
> @@ -285,6 +285,29 @@ enum i386_regnum
> I386_FS_REGNUM, /* %fs */
> I386_GS_REGNUM, /* %gs */
> I386_ST0_REGNUM, /* %st(0) */
> + I386_ST1_REGNUM, /* %st(1) */
> + I386_ST2_REGNUM, /* %st(2) */
> + I386_ST3_REGNUM, /* %st(3) */
> + I386_ST4_REGNUM, /* %st(4) */
> + I386_ST5_REGNUM, /* %st(5) */
> + I386_ST6_REGNUM, /* %st(6) */
> + I386_ST7_REGNUM, /* %st(7) */
> + I386_FCTRL_REGNUM,
> + I386_FSTAT_REGNUM,
> + I386_FTAG_REGNUM,
> + I386_FISEG_REGNUM,
> + I386_FIOFF_REGNUM,
> + I386_FOSEG_REGNUM,
> + I386_FOOFF_REGNUM,
> + I386_FOP_REGNUM,
> + I386_XMM0_REGNUM, /* %xmm0 */
> + I386_XMM1_REGNUM, /* %xmm1 */
> + I386_XMM2_REGNUM, /* %xmm2 */
> + I386_XMM3_REGNUM, /* %xmm3 */
> + I386_XMM4_REGNUM, /* %xmm4 */
> + I386_XMM5_REGNUM, /* %xmm5 */
> + I386_XMM6_REGNUM, /* %xmm6 */
> + I386_XMM7_REGNUM, /* %xmm7 */
> I386_MXCSR_REGNUM = 40, /* %mxcsr */
> I386_YMM0H_REGNUM, /* %ymm0h */
> I386_YMM7H_REGNUM = I386_YMM0H_REGNUM + 7,
> diff --git a/gdb/i386-windows-nat.c b/gdb/i386-windows-nat.c
> index 9efe184..1f65774 100644
> --- a/gdb/i386-windows-nat.c
> +++ b/gdb/i386-windows-nat.c
> @@ -22,55 +22,71 @@
>
> #include <windows.h>
>
> -#define context_offset(x) ((int)&(((CONTEXT *)NULL)->x))
> -static const int mappings[] =
> +/* See context_register_offset_ftype. */
> +
> +static int
> +i386_windows_context_register_offset (int regnum)
> {
> - context_offset (Eax),
> - context_offset (Ecx),
> - context_offset (Edx),
> - context_offset (Ebx),
> - context_offset (Esp),
> - context_offset (Ebp),
> - context_offset (Esi),
> - context_offset (Edi),
> - context_offset (Eip),
> - context_offset (EFlags),
> - context_offset (SegCs),
> - context_offset (SegSs),
> - context_offset (SegDs),
> - context_offset (SegEs),
> - context_offset (SegFs),
> - context_offset (SegGs),
> - context_offset (FloatSave.RegisterArea[0 * 10]),
> - context_offset (FloatSave.RegisterArea[1 * 10]),
> - context_offset (FloatSave.RegisterArea[2 * 10]),
> - context_offset (FloatSave.RegisterArea[3 * 10]),
> - context_offset (FloatSave.RegisterArea[4 * 10]),
> - context_offset (FloatSave.RegisterArea[5 * 10]),
> - context_offset (FloatSave.RegisterArea[6 * 10]),
> - context_offset (FloatSave.RegisterArea[7 * 10]),
> - context_offset (FloatSave.ControlWord),
> - context_offset (FloatSave.StatusWord),
> - context_offset (FloatSave.TagWord),
> - context_offset (FloatSave.ErrorSelector),
> - context_offset (FloatSave.ErrorOffset),
> - context_offset (FloatSave.DataSelector),
> - context_offset (FloatSave.DataOffset),
> - context_offset (FloatSave.ErrorSelector)
> - /* XMM0-7 */ ,
> - context_offset (ExtendedRegisters[10*16]),
> - context_offset (ExtendedRegisters[11*16]),
> - context_offset (ExtendedRegisters[12*16]),
> - context_offset (ExtendedRegisters[13*16]),
> - context_offset (ExtendedRegisters[14*16]),
> - context_offset (ExtendedRegisters[15*16]),
> - context_offset (ExtendedRegisters[16*16]),
> - context_offset (ExtendedRegisters[17*16]),
> - /* MXCSR */
> - context_offset (ExtendedRegisters[24])
> -};
> +#define context_offset(x) ((int)&(((CONTEXT *)NULL)->x))
> + switch (regnum)
> + {
> + case I386_EAX_REGNUM: return context_offset (Eax);
> + case I386_ECX_REGNUM: return context_offset (Ecx);
> + case I386_EDX_REGNUM: return context_offset (Edx);
> + case I386_EBX_REGNUM: return context_offset (Ebx);
> + case I386_ESP_REGNUM: return context_offset (Esp);
> + case I386_EBP_REGNUM: return context_offset (Ebp);
> + case I386_ESI_REGNUM: return context_offset (Esi);
> + case I386_EDI_REGNUM: return context_offset (Edi);
> + case I386_EIP_REGNUM: return context_offset (Eip);
> + case I386_EFLAGS_REGNUM: return context_offset (EFlags);
> + case I386_CS_REGNUM: return context_offset (SegCs);
> + case I386_SS_REGNUM: return context_offset (SegSs);
> + case I386_DS_REGNUM: return context_offset (SegDs);
> + case I386_ES_REGNUM: return context_offset (SegEs);
> + case I386_FS_REGNUM: return context_offset (SegFs);
> + case I386_GS_REGNUM: return context_offset (SegGs);
> + case I386_ST0_REGNUM:
> + return context_offset (FloatSave.RegisterArea[0 * 10]);
> + case I386_ST1_REGNUM:
> + return context_offset (FloatSave.RegisterArea[1 * 10]);
> + case I386_ST2_REGNUM:
> + return context_offset (FloatSave.RegisterArea[2 * 10]);
> + case I386_ST3_REGNUM:
> + return context_offset (FloatSave.RegisterArea[3 * 10]);
> + case I386_ST4_REGNUM:
> + return context_offset (FloatSave.RegisterArea[4 * 10]);
> + case I386_ST5_REGNUM:
> + return context_offset (FloatSave.RegisterArea[5 * 10]);
> + case I386_ST6_REGNUM:
> + return context_offset (FloatSave.RegisterArea[6 * 10]);
> + case I386_ST7_REGNUM:
> + return context_offset (FloatSave.RegisterArea[7 * 10]);
> + case I386_FCTRL_REGNUM: return context_offset (FloatSave.ControlWord);
> + case I386_FSTAT_REGNUM: return context_offset (FloatSave.StatusWord);
> + case I386_FTAG_REGNUM: return context_offset (FloatSave.TagWord);
> + case I386_FISEG_REGNUM: return context_offset (FloatSave.ErrorSelector);
> + case I386_FIOFF_REGNUM: return context_offset (FloatSave.ErrorOffset);
> + case I386_FOSEG_REGNUM: return context_offset (FloatSave.DataSelector);
> + case I386_FOOFF_REGNUM: return context_offset (FloatSave.DataOffset);
> + case I386_FOP_REGNUM: return context_offset (FloatSave.ErrorSelector);
> + /* XMM0-7 */
> + case I386_XMM0_REGNUM: return context_offset (ExtendedRegisters[10*16]);
> + case I386_XMM1_REGNUM: return context_offset (ExtendedRegisters[11*16]);
> + case I386_XMM2_REGNUM: return context_offset (ExtendedRegisters[12*16]);
> + case I386_XMM3_REGNUM: return context_offset (ExtendedRegisters[13*16]);
> + case I386_XMM4_REGNUM: return context_offset (ExtendedRegisters[14*16]);
> + case I386_XMM5_REGNUM: return context_offset (ExtendedRegisters[15*16]);
> + case I386_XMM6_REGNUM: return context_offset (ExtendedRegisters[16*16]);
> + case I386_XMM7_REGNUM: return context_offset (ExtendedRegisters[17*16]);
> + /* MXCSR */
> + case I386_MXCSR_REGNUM: return context_offset (ExtendedRegisters[24]);
Ditto.
Note this would create a bit of divergence with the gdbserver
implementation of the same functions.
Thanks,
Pedro Alves
[-- Attachment #2: 0001-windows.patch --]
[-- Type: text/x-patch, Size: 5793 bytes --]
From cebec834837380395b9ff1288533d65ffda80d74 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 26 Jun 2018 16:33:27 +0100
Subject: [PATCH] windows
---
gdb/amd64-linux-tdep.c | 3 ++-
gdb/amd64-tdep.c | 17 ++++++++++-------
gdb/amd64-tdep.h | 3 ++-
gdb/amd64-windows-tdep.c | 2 +-
gdb/arch/amd64.c | 6 ++++--
gdb/arch/amd64.h | 2 +-
gdb/gdbserver/win32-i386-low.c | 2 +-
7 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index ef9248d708d..7fe37d83f69 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1594,7 +1594,8 @@ amd64_linux_read_description (uint64_t xcr0_features_bit, bool is_x32)
}
if (*tdesc == NULL)
- *tdesc = amd64_create_target_description (xcr0_features_bit, is_x32, true);
+ *tdesc = amd64_create_target_description (xcr0_features_bit, is_x32,
+ true, true);
return *tdesc;
}
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 9f8f018dd15..190e086ebfe 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -3225,7 +3225,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
static void
amd64_none_init_abi (gdbarch_info info, gdbarch *arch)
{
- amd64_init_abi (info, arch, amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_init_abi (info, arch, amd64_target_description (X86_XSTATE_SSE_MASK,
+ true));
}
static struct type *
@@ -3266,25 +3267,27 @@ static void
amd64_x32_none_init_abi (gdbarch_info info, gdbarch *arch)
{
amd64_x32_init_abi (info, arch,
- amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_target_description (X86_XSTATE_SSE_MASK, true));
}
/* Return the target description for a specified XSAVE feature mask. */
const struct target_desc *
-amd64_target_description (uint64_t xcr0)
+amd64_target_description (uint64_t xcr0, bool segments)
{
static target_desc *amd64_tdescs \
- [2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/] = {};
+ [2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/][2/*segments*/] = {};
target_desc **tdesc;
tdesc = &amd64_tdescs[(xcr0 & X86_XSTATE_AVX) ? 1 : 0]
[(xcr0 & X86_XSTATE_MPX) ? 1 : 0]
[(xcr0 & X86_XSTATE_AVX512) ? 1 : 0]
- [(xcr0 & X86_XSTATE_PKRU) ? 1 : 0];
+ [(xcr0 & X86_XSTATE_PKRU) ? 1 : 0]
+ [segments ? 1 : 0];
if (*tdesc == NULL)
- *tdesc = amd64_create_target_description (xcr0, false, false);
+ *tdesc = amd64_create_target_description (xcr0, false, false,
+ segments);
return *tdesc;
}
@@ -3314,7 +3317,7 @@ _initialize_amd64_tdep (void)
for (auto &a : xml_masks)
{
- auto tdesc = amd64_target_description (a.mask);
+ auto tdesc = amd64_target_description (a.mask, true);
selftests::record_xml_tdesc (a.xml, tdesc);
}
diff --git a/gdb/amd64-tdep.h b/gdb/amd64-tdep.h
index 7d3791ad936..94e012632ff 100644
--- a/gdb/amd64-tdep.h
+++ b/gdb/amd64-tdep.h
@@ -106,7 +106,8 @@ extern void amd64_init_abi (struct gdbarch_info info,
extern void amd64_x32_init_abi (struct gdbarch_info info,
struct gdbarch *gdbarch,
const target_desc *default_tdesc);
-extern const struct target_desc *amd64_target_description (uint64_t xcr0);
+extern const struct target_desc *amd64_target_description (uint64_t xcr0,
+ bool segments);
/* Fill register REGNUM in REGCACHE with the appropriate
floating-point or SSE register value from *FXSAVE. If REGNUM is
diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 83a7f2f32ed..904875baccd 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -1226,7 +1226,7 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
frame_unwind_append_unwinder (gdbarch, &amd64_windows_frame_unwind);
amd64_init_abi (info, gdbarch,
- amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_target_description (X86_XSTATE_SSE_MASK, false));
windows_init_abi (info, gdbarch);
diff --git a/gdb/arch/amd64.c b/gdb/arch/amd64.c
index d31d8f1f75a..3a289df12a5 100644
--- a/gdb/arch/amd64.c
+++ b/gdb/arch/amd64.c
@@ -36,7 +36,8 @@
descriptions for Linux. */
target_desc *
-amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux)
+amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux,
+ bool segments)
{
target_desc *tdesc = allocate_target_description ();
@@ -57,7 +58,8 @@ amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux)
regnum = create_feature_i386_64bit_sse (tdesc, regnum);
if (is_linux)
regnum = create_feature_i386_64bit_linux (tdesc, regnum);
- regnum = create_feature_i386_64bit_segments (tdesc, regnum);
+ if (segments)
+ regnum = create_feature_i386_64bit_segments (tdesc, regnum);
if (xcr0 & X86_XSTATE_AVX)
regnum = create_feature_i386_64bit_avx (tdesc, regnum);
diff --git a/gdb/arch/amd64.h b/gdb/arch/amd64.h
index c0c4dc27efe..4a659657da1 100644
--- a/gdb/arch/amd64.h
+++ b/gdb/arch/amd64.h
@@ -19,4 +19,4 @@
#include <stdint.h>
target_desc *amd64_create_target_description (uint64_t xcr0, bool is_x32,
- bool is_linux);
+ bool is_linux, bool segments);
diff --git a/gdb/gdbserver/win32-i386-low.c b/gdb/gdbserver/win32-i386-low.c
index 16fe2c85b2a..62221688cbd 100644
--- a/gdb/gdbserver/win32-i386-low.c
+++ b/gdb/gdbserver/win32-i386-low.c
@@ -436,7 +436,7 @@ i386_arch_setup (void)
#ifdef __x86_64__
tdesc = amd64_create_target_description (X86_XSTATE_SSE_MASK, false,
- false);
+ false, false);
const char **expedite_regs = amd64_expedite_regs;
#else
tdesc = i386_create_target_description (X86_XSTATE_SSE_MASK, false);
--
2.14.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Minor reorganization of fetch_registers/store_registers in windows-nat.c
2018-06-25 18:55 ` [PATCH 1/2] Minor reorganization of fetch_registers/store_registers in windows-nat.c Joel Brobecker
@ 2018-06-26 16:03 ` Pedro Alves
2018-06-26 21:41 ` Joel Brobecker
0 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2018-06-26 16:03 UTC (permalink / raw)
To: Joel Brobecker, gdb-patches
On 06/25/2018 07:55 PM, Joel Brobecker wrote:
> This patch is a small reorganizational patch that splits
> do_windows_fetch_inferior_registers into two parts:
>
> (a) One part that first reloads the thread's context when needed,
> and then decides based on the given register number whether
> one register needs to be fetched or all of them.
>
> This part is moved to windows_nat_target::fetch_registers.
>
> (b) The rest of the code, which actually fetches the register value
> and supplies it to the regcache.
>
> A similar treatment is applied to do_windows_store_inferior_registers.
>
> This change is preparation work for changing the way we calculate
> the location of a given register in the thread context structure,
> and should be a no op.
>
I agree this looks clearer without the recursion.
LGTM. A couple nits on comments below.
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 620e25c..c51ef8f 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -528,14 +528,59 @@ windows_delete_thread (const ptid_t &ptid, DWORD exit_code)
> }
> }
>
> +/* Fetches register number R from the given windows_thread_info,
> + and supplies its value to the given regcache.
> +
> + This function assumes that R is either null or positive. A failed
> + assertion is raised if that is not true.
R is an integer, talking about "null" gave me pause. I'd suggest
saying either "zero" instead of null, or say "assumes non-negative".
> +
> + This function assumes that TH->RELOAD_CONTEXT is not set, meaning
> + that the windows_thread_info has an up-to-date context. A failed
> + assertion is raised if that assumption is violated. */
> +
> static void
> -do_windows_fetch_inferior_registers (struct regcache *regcache,
> - windows_thread_info *th, int r)
> +windows_fetch_one_register (struct regcache *regcache,
> + windows_thread_info *th, int r)
> {
>
> static void
> -do_windows_store_inferior_registers (const struct regcache *regcache,
> - windows_thread_info *th, int r)
> +windows_store_one_register (const struct regcache *regcache,
> + windows_thread_info *th, int r)
Add similar leading comment?
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Minor reorganization of fetch_registers/store_registers in windows-nat.c
2018-06-26 16:03 ` Pedro Alves
@ 2018-06-26 21:41 ` Joel Brobecker
0 siblings, 0 replies; 10+ messages in thread
From: Joel Brobecker @ 2018-06-26 21:41 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 859 bytes --]
> I agree this looks clearer without the recursion.
>
> LGTM. A couple nits on comments below.
Thanks, Pedro. Attached is the version I ended up pushing.
gdb/ChangeLog:
* windows-nat.c (do_windows_fetch_inferior_registers): Rename
to windows_fetch_one_register, and only handle the case of
fetching one register. Move the code that reloads the context
and iterates over all registers if R is negative to...
(windows_nat_target::fetch_registers): ... here.
(do_windows_store_inferior_registers): Rename to
windows_store_one_register, and only handle the case of storing
one register. Move the code that handles the case where r is
negative to...
(windows_nat_target::store_registers) ... here.
Tested on x86-windows and x86_64-windows using AdaCore's testsuite.
--
Joel
[-- Attachment #2: 0001-Minor-reorganization-of-fetch_registers-store_regist.patch --]
[-- Type: text/x-diff, Size: 6906 bytes --]
From 59b78c81d3bdd7cd5ad8e8eae8f940a6115f1209 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 26 Jun 2018 16:46:09 -0400
Subject: [PATCH] Minor reorganization of fetch_registers/store_registers in
windows-nat.c
This patch is a small reorganizational patch that splits
do_windows_fetch_inferior_registers into two parts:
(a) One part that first reloads the thread's context when needed,
and then decides based on the given register number whether
one register needs to be fetched or all of them.
This part is moved to windows_nat_target::fetch_registers.
(b) The rest of the code, which actually fetches the register value
and supplies it to the regcache.
A similar treatment is applied to do_windows_store_inferior_registers.
This change is preparation work for changing the way we calculate
the location of a given register in the thread context structure,
and should be a no op.
gdb/ChangeLog:
* windows-nat.c (do_windows_fetch_inferior_registers): Rename
to windows_fetch_one_register, and only handle the case of
fetching one register. Move the code that reloads the context
and iterates over all registers if R is negative to...
(windows_nat_target::fetch_registers): ... here.
(do_windows_store_inferior_registers): Rename to
windows_store_one_register, and only handle the case of storing
one register. Move the code that handles the case where r is
negative to...
(windows_nat_target::store_registers) ... here.
Tested on x86-windows and x86_64-windows using AdaCore's testsuite.
---
gdb/windows-nat.c | 117 ++++++++++++++++++++++++++++++++----------------------
1 file changed, 69 insertions(+), 48 deletions(-)
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 98d94a3..35ad865 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -509,14 +509,59 @@ windows_delete_thread (ptid_t ptid, DWORD exit_code)
}
}
+/* Fetches register number R from the given windows_thread_info,
+ and supplies its value to the given regcache.
+
+ This function assumes that R is non-negative. A failed assertion
+ is raised if that is not true.
+
+ This function assumes that TH->RELOAD_CONTEXT is not set, meaning
+ that the windows_thread_info has an up-to-date context. A failed
+ assertion is raised if that assumption is violated. */
+
static void
-do_windows_fetch_inferior_registers (struct regcache *regcache,
- windows_thread_info *th, int r)
+windows_fetch_one_register (struct regcache *regcache,
+ windows_thread_info *th, int r)
{
+ gdb_assert (r >= 0);
+ gdb_assert (!th->reload_context);
+
char *context_offset = ((char *) &th->context) + mappings[r];
struct gdbarch *gdbarch = regcache->arch ();
struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
- long l;
+
+ if (r == I387_FISEG_REGNUM (tdep))
+ {
+ long l = *((long *) context_offset) & 0xffff;
+ regcache->raw_supply (r, (char *) &l);
+ }
+ else if (r == I387_FOP_REGNUM (tdep))
+ {
+ long l = (*((long *) context_offset) >> 16) & ((1 << 11) - 1);
+ regcache->raw_supply (r, (char *) &l);
+ }
+ else if (segment_register_p (r))
+ {
+ /* GDB treats segment registers as 32bit registers, but they are
+ in fact only 16 bits long. Make sure we do not read extra
+ bits from our source buffer. */
+ long l = *((long *) context_offset) & 0xffff;
+ regcache->raw_supply (r, (char *) &l);
+ }
+ else
+ regcache->raw_supply (r, context_offset);
+}
+
+void
+windows_nat_target::fetch_registers (struct regcache *regcache, int r)
+{
+ DWORD pid = ptid_get_tid (regcache->ptid ());
+ windows_thread_info *th = thread_rec (pid, TRUE);
+
+ /* Check if TH exists. Windows sometimes uses a non-existent
+ thread id in its events. */
+ if (th == NULL)
+ return;
if (th->reload_context)
{
@@ -552,56 +597,26 @@ do_windows_fetch_inferior_registers (struct regcache *regcache,
th->reload_context = 0;
}
- if (r == I387_FISEG_REGNUM (tdep))
- {
- l = *((long *) context_offset) & 0xffff;
- regcache->raw_supply (r, (char *) &l);
- }
- else if (r == I387_FOP_REGNUM (tdep))
- {
- l = (*((long *) context_offset) >> 16) & ((1 << 11) - 1);
- regcache->raw_supply (r, (char *) &l);
- }
- else if (segment_register_p (r))
- {
- /* GDB treats segment registers as 32bit registers, but they are
- in fact only 16 bits long. Make sure we do not read extra
- bits from our source buffer. */
- l = *((long *) context_offset) & 0xffff;
- regcache->raw_supply (r, (char *) &l);
- }
- else if (r >= 0)
- regcache->raw_supply (r, context_offset);
+ if (r < 0)
+ for (r = 0; r < gdbarch_num_regs (regcache->arch()); r++)
+ windows_fetch_one_register (regcache, th, r);
else
- {
- for (r = 0; r < gdbarch_num_regs (gdbarch); r++)
- do_windows_fetch_inferior_registers (regcache, th, r);
- }
+ windows_fetch_one_register (regcache, th, r);
}
-void
-windows_nat_target::fetch_registers (struct regcache *regcache, int r)
-{
- DWORD pid = ptid_get_tid (regcache->ptid ());
- windows_thread_info *th = thread_rec (pid, TRUE);
+/* Collect the register number R from the given regcache, and store
+ its value into the corresponding area of the given thread's context.
- /* Check if TH exists. Windows sometimes uses a non-existent
- thread id in its events. */
- if (th != NULL)
- do_windows_fetch_inferior_registers (regcache, th, r);
-}
+ This function assumes that R is non-negative. A failed assertion
+ assertion is raised if that is not true. */
static void
-do_windows_store_inferior_registers (const struct regcache *regcache,
- windows_thread_info *th, int r)
+windows_store_one_register (const struct regcache *regcache,
+ windows_thread_info *th, int r)
{
- if (r >= 0)
- regcache->raw_collect (r, ((char *) &th->context) + mappings[r]);
- else
- {
- for (r = 0; r < gdbarch_num_regs (regcache->arch ()); r++)
- do_windows_store_inferior_registers (regcache, th, r);
- }
+ gdb_assert (r >= 0);
+
+ regcache->raw_collect (r, ((char *) &th->context) + mappings[r]);
}
/* Store a new register value into the context of the thread tied to
@@ -615,8 +630,14 @@ windows_nat_target::store_registers (struct regcache *regcache, int r)
/* Check if TH exists. Windows sometimes uses a non-existent
thread id in its events. */
- if (th != NULL)
- do_windows_store_inferior_registers (regcache, th, r);
+ if (th == NULL)
+ return;
+
+ if (r < 0)
+ for (r = 0; r < gdbarch_num_regs (regcache->arch ()); r++)
+ windows_store_one_register (regcache, th, r);
+ else
+ windows_store_one_register (regcache, th, r);
}
/* Encapsulate the information required in a call to
--
2.8.3
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers
2018-06-26 16:00 ` Pedro Alves
@ 2018-06-26 21:53 ` Joel Brobecker
2018-06-29 12:32 ` Pedro Alves
2018-06-29 12:32 ` Pedro Alves
0 siblings, 2 replies; 10+ messages in thread
From: Joel Brobecker @ 2018-06-26 21:53 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 2636 bytes --]
> I spent a bit playing with this today. See comments below.
Thank you!
> remote debugging when the remote target does not supply an explicit
> xml target description. The only thing that can be done is add
> new registers at the end.
>
> So to fix this, we could also make windows_nat_target::{fetch,store}_registers
> check whether the requested register is within the mappings array. We could
> do that by simply changing the 'mappings' global from a plain pointer to a
> gdb::array_view for example (which stores the array's size). I suspect the
> current table based approach generates a little better code (the compiler may
> end up generating a table anyway from the switch/case in your patch, but it's
> not garanteed), though it shouldn't really matter in practice.
>
> But see below.
I am OK with taking the bounded-buffer approach, as it would clearly
already be an improvement. I thought about something like that, but
using a C-oriented solution, which was not elegant, so discarded it
then. Your suggestion takes care of that aspect.
However, thinking further about this, we have this huge gap between
the majority of the registers, and the next ones. What if we wanted
one day to add more? We'd have to triple the size of the table and
keep it mostly empty. The counter point is that I tend to think that
this is probably unlikely.
> Since Windows doesn't expose these to userspace, I'm thinking that it
> doesn't make much sense to show the registers at all? The (xml) target
> features are designed exactly such that if you don't have some
> optional feature (like org.gnu.gdb.i386.segments), then the corresponding
> registers don't exist at all in gdb.
>
> The attached patch implements that.
Makes sense to me indeed.
I tested your patch on x86_64-windows, and didn't detect any issues
with it, so I suggest we push that (see attached patch with comments
added and initial revision log provided as well -- hopefully a modest
thank you for sending the patch showing how it's done).
... and then decide whether we want to reorganize a bit the way
we get the index of each register in the CONTEXT structure. I would
say that we do want to do something. Perhaps, the path of least
resistance is to just change the mappings structure from a C array
to a gdb::array_view as you suggested. I may have a preference for
the approach I took, but it is a large diff, and it's not clear
whether it's going to be beneficial in the long run...
You pick! ;-) I'll take care of your comments if you chose the first
patch. I'll send a new one if you prefere the gdb::array_view approach.
Thanks Pedro!
--
Joel
[-- Attachment #2: 0002-x86_64-windows-GDB-crash-due-to-fs_base-gs_base-regi.patch --]
[-- Type: text/x-diff, Size: 9189 bytes --]
From 12fceda7e7835925c92ef8185cb43b57fd468667 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 26 Jun 2018 16:33:27 +0100
Subject: [PATCH] x86_64-windows GDB crash due to fs_base/gs_base registers
GDB is currently crashing anytime we try to access the fs_base/gs_base
registers, either to read them, or to write them. This can be observed
under various scenarios:
- Explicit reference to those registers (eg: print $fs_base) --
probably relatively rare;
- Calling a function in the inferior, with the crash happening
because we are trying to read those registers in order to save
their value ahead of making the function call;
- Just a plain "info registers";
The crash was introduced by the following commit:
| commit 48aeef91c248291dd03583798904612426b1f40a
| Date: Mon Jun 26 18:14:43 2017 -0700
| Subject: Include the fs_base and gs_base registers in amd64 target descriptions.
The Windows-nat implementation was unfortunately not prepared to deal
with those new registers. In particular, the way it fetches registers
is done by using a table where the index is the register number, and
the value at that index is the offset in the area in the thread's CONTEXT
data where the corresponding register value is stored.
For instance, in amd64-windows-nat.c, we can find the mappings static
array containing the following 57 elements in it:
#define context_offset(x) (offsetof (CONTEXT, x))
static const int mappings[] =
{
context_offset (Rax),
[...]
context_offset (FloatSave.MxCsr)
};
That array is then used by windows_fetch_one_register via:
char *context_offset = ((char *) &th->context) + mappings[r];
The problem is that fs_base's register number is 172, which is
well past the end of the mappings array (57 elements in total).
We end up getting an undefined offset, which happens to be so large
that it then causes the address where we try to read the register
value (a little bit later) to be invalid, thus crashing GDB with
a SEGV.
This patch side-steps the issue entirely by removing support for
those registers in GDB on x86_64-windows, because a look at the
CONTEXT structure indicates no support for getting those registers.
A more comprehensive fix would patch the potential buffer overflow
of the mappings array, but this can be done as a separate commit.
gdb/ChangeLog:
* gdb/amd64-tdep.h (amd64_create_target_description): Add
"segments" parameter.
* gdb/amd64-tdep.c (amd64_none_init_abi, amd64_x32_none_init_abi)
(_initialize_amd64_tdep): Update call to
amd64_create_target_description.
(amd64_target_description): Add "segments" parameter. Adjust
the implementation to use it.
* gdb/amd64-linux-tdep.c (amd64_linux_read_description): Update
call to amd64_create_target_description.
* gdb/amd64-windows-tdep.c (amd64_windows_init_abi): Likewise.
* gdb/arch/amd64.h (amd64_create_target_description): Add
"segments" register.
* gdb/arch/amd64.c (amd64_create_target_description): Add
"segments" parameter. Call create_feature_i386_64bit_segments
only if SEGMENTS is true.
* gdb/gdbserver/win32-i386-low.c (i386_arch_setup): Update
call to amd64_create_target_description.
Tested on x86_64-windows using AdaCore's testsuite.
---
gdb/amd64-linux-tdep.c | 3 ++-
gdb/amd64-tdep.c | 17 ++++++++++-------
gdb/amd64-tdep.h | 3 ++-
gdb/amd64-windows-tdep.c | 2 +-
gdb/arch/amd64.c | 9 ++++++---
gdb/arch/amd64.h | 2 +-
gdb/gdbserver/win32-i386-low.c | 2 +-
7 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 521e32a..454dc93 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1594,7 +1594,8 @@ amd64_linux_read_description (uint64_t xcr0_features_bit, bool is_x32)
}
if (*tdesc == NULL)
- *tdesc = amd64_create_target_description (xcr0_features_bit, is_x32, true);
+ *tdesc = amd64_create_target_description (xcr0_features_bit, is_x32,
+ true, true);
return *tdesc;
}
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 9f8f018..190e086 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -3225,7 +3225,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
static void
amd64_none_init_abi (gdbarch_info info, gdbarch *arch)
{
- amd64_init_abi (info, arch, amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_init_abi (info, arch, amd64_target_description (X86_XSTATE_SSE_MASK,
+ true));
}
static struct type *
@@ -3266,25 +3267,27 @@ static void
amd64_x32_none_init_abi (gdbarch_info info, gdbarch *arch)
{
amd64_x32_init_abi (info, arch,
- amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_target_description (X86_XSTATE_SSE_MASK, true));
}
/* Return the target description for a specified XSAVE feature mask. */
const struct target_desc *
-amd64_target_description (uint64_t xcr0)
+amd64_target_description (uint64_t xcr0, bool segments)
{
static target_desc *amd64_tdescs \
- [2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/] = {};
+ [2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/][2/*segments*/] = {};
target_desc **tdesc;
tdesc = &amd64_tdescs[(xcr0 & X86_XSTATE_AVX) ? 1 : 0]
[(xcr0 & X86_XSTATE_MPX) ? 1 : 0]
[(xcr0 & X86_XSTATE_AVX512) ? 1 : 0]
- [(xcr0 & X86_XSTATE_PKRU) ? 1 : 0];
+ [(xcr0 & X86_XSTATE_PKRU) ? 1 : 0]
+ [segments ? 1 : 0];
if (*tdesc == NULL)
- *tdesc = amd64_create_target_description (xcr0, false, false);
+ *tdesc = amd64_create_target_description (xcr0, false, false,
+ segments);
return *tdesc;
}
@@ -3314,7 +3317,7 @@ _initialize_amd64_tdep (void)
for (auto &a : xml_masks)
{
- auto tdesc = amd64_target_description (a.mask);
+ auto tdesc = amd64_target_description (a.mask, true);
selftests::record_xml_tdesc (a.xml, tdesc);
}
diff --git a/gdb/amd64-tdep.h b/gdb/amd64-tdep.h
index 7d3791a..94e0126 100644
--- a/gdb/amd64-tdep.h
+++ b/gdb/amd64-tdep.h
@@ -106,7 +106,8 @@ extern void amd64_init_abi (struct gdbarch_info info,
extern void amd64_x32_init_abi (struct gdbarch_info info,
struct gdbarch *gdbarch,
const target_desc *default_tdesc);
-extern const struct target_desc *amd64_target_description (uint64_t xcr0);
+extern const struct target_desc *amd64_target_description (uint64_t xcr0,
+ bool segments);
/* Fill register REGNUM in REGCACHE with the appropriate
floating-point or SSE register value from *FXSAVE. If REGNUM is
diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 83a7f2f..904875b 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -1226,7 +1226,7 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
frame_unwind_append_unwinder (gdbarch, &amd64_windows_frame_unwind);
amd64_init_abi (info, gdbarch,
- amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_target_description (X86_XSTATE_SSE_MASK, false));
windows_init_abi (info, gdbarch);
diff --git a/gdb/arch/amd64.c b/gdb/arch/amd64.c
index d31d8f1..b562018 100644
--- a/gdb/arch/amd64.c
+++ b/gdb/arch/amd64.c
@@ -33,10 +33,12 @@
/* Create amd64 target descriptions according to XCR0. If IS_X32 is
true, create the x32 ones. If IS_LINUX is true, create target
- descriptions for Linux. */
+ descriptions for Linux. If SEGMENTS is true, the include the segment
+ registers. */
target_desc *
-amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux)
+amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux,
+ bool segments)
{
target_desc *tdesc = allocate_target_description ();
@@ -57,7 +59,8 @@ amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux)
regnum = create_feature_i386_64bit_sse (tdesc, regnum);
if (is_linux)
regnum = create_feature_i386_64bit_linux (tdesc, regnum);
- regnum = create_feature_i386_64bit_segments (tdesc, regnum);
+ if (segments)
+ regnum = create_feature_i386_64bit_segments (tdesc, regnum);
if (xcr0 & X86_XSTATE_AVX)
regnum = create_feature_i386_64bit_avx (tdesc, regnum);
diff --git a/gdb/arch/amd64.h b/gdb/arch/amd64.h
index c0c4dc2..4a65965 100644
--- a/gdb/arch/amd64.h
+++ b/gdb/arch/amd64.h
@@ -19,4 +19,4 @@
#include <stdint.h>
target_desc *amd64_create_target_description (uint64_t xcr0, bool is_x32,
- bool is_linux);
+ bool is_linux, bool segments);
diff --git a/gdb/gdbserver/win32-i386-low.c b/gdb/gdbserver/win32-i386-low.c
index 16fe2c8..6222168 100644
--- a/gdb/gdbserver/win32-i386-low.c
+++ b/gdb/gdbserver/win32-i386-low.c
@@ -436,7 +436,7 @@ i386_arch_setup (void)
#ifdef __x86_64__
tdesc = amd64_create_target_description (X86_XSTATE_SSE_MASK, false,
- false);
+ false, false);
const char **expedite_regs = amd64_expedite_regs;
#else
tdesc = i386_create_target_description (X86_XSTATE_SSE_MASK, false);
--
2.8.3
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers
2018-06-26 21:53 ` Joel Brobecker
@ 2018-06-29 12:32 ` Pedro Alves
2018-06-29 12:32 ` Pedro Alves
1 sibling, 0 replies; 10+ messages in thread
From: Pedro Alves @ 2018-06-29 12:32 UTC (permalink / raw)
To: Joel Brobecker; +Cc: gdb-patches
On 06/26/2018 10:53 PM, Joel Brobecker wrote:
> ... and then decide whether we want to reorganize a bit the way
> we get the index of each register in the CONTEXT structure. I would
> say that we do want to do something. Perhaps, the path of least
> resistance is to just change the mappings structure from a C array
> to a gdb::array_view as you suggested. I may have a preference for
> the approach I took, but it is a large diff, and it's not clear
> whether it's going to be beneficial in the long run...
>
> You pick! ;-) I'll take care of your comments if you chose the first
> patch. I'll send a new one if you prefere the gdb::array_view approach.
In all honesty, I'd just leave it alone as is, this sort of array is
used for other targets not just Windows (e.g., amd64fbsd64_r_reg_offset,
amd64_linux_gregset32_reg_offset). If we went for safety, we likely
wouldn't have noticed the unnecessary <unavailable> registers.
On the other hand, if we're going for safely, I'm fine with your
version too, I don't really mind your way vs array_view. Up to you.
I do think that it would be nice if gdbserver is changed in the
same way to avoid further divergence (until some brave soul
spends time merging gdb's and gdbserver's windows-nat.c and
win32-low.c, probably the easiest backends to merge).
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers
2018-06-26 21:53 ` Joel Brobecker
2018-06-29 12:32 ` Pedro Alves
@ 2018-06-29 12:32 ` Pedro Alves
2018-06-29 22:08 ` Joel Brobecker
1 sibling, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2018-06-29 12:32 UTC (permalink / raw)
To: Joel Brobecker; +Cc: gdb-patches
Thanks for writing up the commit log and ChangeLog. The commit looks
fine to me, but you should add your name there somewhere too.
One comment on a comment below.
> diff --git a/gdb/arch/amd64.c b/gdb/arch/amd64.c
> index d31d8f1..b562018 100644
> --- a/gdb/arch/amd64.c
> +++ b/gdb/arch/amd64.c
> @@ -33,10 +33,12 @@
>
> /* Create amd64 target descriptions according to XCR0. If IS_X32 is
> true, create the x32 ones. If IS_LINUX is true, create target
> - descriptions for Linux. */
> + descriptions for Linux. If SEGMENTS is true, the include the segment
> + registers. */
"the include" -> "then include"
I think it'd be clearer to say:
include the "org.gnu.gdb.i386.segments" feature registers.
because "segment registers" along makes one think of cs,ds,ss...,
but those are part of the core register set.
>
> target_desc *
> -amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux)
> +amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux,
> + bool segments)
> {
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers
2018-06-29 12:32 ` Pedro Alves
@ 2018-06-29 22:08 ` Joel Brobecker
0 siblings, 0 replies; 10+ messages in thread
From: Joel Brobecker @ 2018-06-29 22:08 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]
> Thanks for writing up the commit log and ChangeLog. The commit looks
> fine to me, but you should add your name there somewhere too.
I ended up putting my name as the tester. That way, if someone sees
the revision log and wonders what kind of testing was done, he can
contact me. The rest was entirely you, so left you as the sole author.
> One comment on a comment below.
>
> > diff --git a/gdb/arch/amd64.c b/gdb/arch/amd64.c
> > index d31d8f1..b562018 100644
> > --- a/gdb/arch/amd64.c
> > +++ b/gdb/arch/amd64.c
> > @@ -33,10 +33,12 @@
> >
> > /* Create amd64 target descriptions according to XCR0. If IS_X32 is
> > true, create the x32 ones. If IS_LINUX is true, create target
> > - descriptions for Linux. */
> > + descriptions for Linux. If SEGMENTS is true, the include the segment
> > + registers. */
>
> "the include" -> "then include"
>
> I think it'd be clearer to say:
>
> include the "org.gnu.gdb.i386.segments" feature registers.
>
> because "segment registers" along makes one think of cs,ds,ss...,
> but those are part of the core register set.
Makes sense. I made the change you suggested, and corrected the typo
as well, and then pushed the attached patch to master.
gdb/ChangeLog:
* gdb/amd64-tdep.h (amd64_create_target_description): Add
"segments" parameter.
* gdb/amd64-tdep.c (amd64_none_init_abi, amd64_x32_none_init_abi)
(_initialize_amd64_tdep): Update call to
amd64_create_target_description.
(amd64_target_description): Add "segments" parameter. Adjust
the implementation to use it.
* gdb/amd64-linux-tdep.c (amd64_linux_read_description): Update
call to amd64_create_target_description.
* gdb/amd64-windows-tdep.c (amd64_windows_init_abi): Likewise.
* gdb/arch/amd64.h (amd64_create_target_description): Add
"segments" register.
* gdb/arch/amd64.c (amd64_create_target_description): Add
"segments" parameter. Call create_feature_i386_64bit_segments
only if SEGMENTS is true.
* gdb/gdbserver/win32-i386-low.c (i386_arch_setup): Update
call to amd64_create_target_description.
Thanks a lot for your help!
--
Joel
[-- Attachment #2: 0002-x86_64-windows-GDB-crash-due-to-fs_base-gs_base-regi.patch --]
[-- Type: text/x-diff, Size: 10508 bytes --]
From de52b9607d2623f18b7a7dbee3e1123d8d63f5da Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 26 Jun 2018 16:33:27 +0100
Subject: [PATCH] x86_64-windows GDB crash due to fs_base/gs_base registers
GDB is currently crashing anytime we try to access the fs_base/gs_base
registers, either to read them, or to write them. This can be observed
under various scenarios:
- Explicit reference to those registers (eg: print $fs_base) --
probably relatively rare;
- Calling a function in the inferior, with the crash happening
because we are trying to read those registers in order to save
their value ahead of making the function call;
- Just a plain "info registers";
The crash was introduced by the following commit:
| commit 48aeef91c248291dd03583798904612426b1f40a
| Date: Mon Jun 26 18:14:43 2017 -0700
| Subject: Include the fs_base and gs_base registers in amd64 target descriptions.
The Windows-nat implementation was unfortunately not prepared to deal
with those new registers. In particular, the way it fetches registers
is done by using a table where the index is the register number, and
the value at that index is the offset in the area in the thread's CONTEXT
data where the corresponding register value is stored.
For instance, in amd64-windows-nat.c, we can find the mappings static
array containing the following 57 elements in it:
#define context_offset(x) (offsetof (CONTEXT, x))
static const int mappings[] =
{
context_offset (Rax),
[...]
context_offset (FloatSave.MxCsr)
};
That array is then used by windows_fetch_one_register via:
char *context_offset = ((char *) &th->context) + mappings[r];
The problem is that fs_base's register number is 172, which is
well past the end of the mappings array (57 elements in total).
We end up getting an undefined offset, which happens to be so large
that it then causes the address where we try to read the register
value (a little bit later) to be invalid, thus crashing GDB with
a SEGV.
This patch side-steps the issue entirely by removing support for
those registers in GDB on x86_64-windows, because a look at the
CONTEXT structure indicates no support for getting those registers.
A more comprehensive fix would patch the potential buffer overflow
of the mappings array, but this can be done as a separate commit.
gdb/ChangeLog:
* gdb/amd64-tdep.h (amd64_create_target_description): Add
"segments" parameter.
* gdb/amd64-tdep.c (amd64_none_init_abi, amd64_x32_none_init_abi)
(_initialize_amd64_tdep): Update call to
amd64_create_target_description.
(amd64_target_description): Add "segments" parameter. Adjust
the implementation to use it.
* gdb/amd64-linux-tdep.c (amd64_linux_read_description): Update
call to amd64_create_target_description.
* gdb/amd64-windows-tdep.c (amd64_windows_init_abi): Likewise.
* gdb/arch/amd64.h (amd64_create_target_description): Add
"segments" register.
* gdb/arch/amd64.c (amd64_create_target_description): Add
"segments" parameter. Call create_feature_i386_64bit_segments
only if SEGMENTS is true.
* gdb/gdbserver/win32-i386-low.c (i386_arch_setup): Update
call to amd64_create_target_description.
Tested on x86_64-windows using AdaCore's testsuite (by Joel Brobecker
<brobecker at adacore dot com>).
---
gdb/ChangeLog | 20 ++++++++++++++++++++
gdb/amd64-linux-tdep.c | 3 ++-
gdb/amd64-tdep.c | 17 ++++++++++-------
gdb/amd64-tdep.h | 3 ++-
gdb/amd64-windows-tdep.c | 2 +-
gdb/arch/amd64.c | 9 ++++++---
gdb/arch/amd64.h | 2 +-
gdb/gdbserver/win32-i386-low.c | 2 +-
8 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0cbccf5f8a..c47c111466 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,23 @@
+2018-06-29 Pedro Alves <palves@redhat.com>
+
+ * gdb/amd64-tdep.h (amd64_create_target_description): Add
+ "segments" parameter.
+ * gdb/amd64-tdep.c (amd64_none_init_abi, amd64_x32_none_init_abi)
+ (_initialize_amd64_tdep): Update call to
+ amd64_create_target_description.
+ (amd64_target_description): Add "segments" parameter. Adjust
+ the implementation to use it.
+ * gdb/amd64-linux-tdep.c (amd64_linux_read_description): Update
+ call to amd64_create_target_description.
+ * gdb/amd64-windows-tdep.c (amd64_windows_init_abi): Likewise.
+ * gdb/arch/amd64.h (amd64_create_target_description): Add
+ "segments" register.
+ * gdb/arch/amd64.c (amd64_create_target_description): Add
+ "segments" parameter. Call create_feature_i386_64bit_segments
+ only if SEGMENTS is true.
+ * gdb/gdbserver/win32-i386-low.c (i386_arch_setup): Update
+ call to amd64_create_target_description.
+
2018-06-29 Pedro Alves <palves@redhat.com>
* thread.c (thread_target_id_str): New, factored out from ...
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index ef9248d708..7fe37d83f6 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1594,7 +1594,8 @@ amd64_linux_read_description (uint64_t xcr0_features_bit, bool is_x32)
}
if (*tdesc == NULL)
- *tdesc = amd64_create_target_description (xcr0_features_bit, is_x32, true);
+ *tdesc = amd64_create_target_description (xcr0_features_bit, is_x32,
+ true, true);
return *tdesc;
}
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 9f8f018dd1..190e086ebf 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -3225,7 +3225,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
static void
amd64_none_init_abi (gdbarch_info info, gdbarch *arch)
{
- amd64_init_abi (info, arch, amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_init_abi (info, arch, amd64_target_description (X86_XSTATE_SSE_MASK,
+ true));
}
static struct type *
@@ -3266,25 +3267,27 @@ static void
amd64_x32_none_init_abi (gdbarch_info info, gdbarch *arch)
{
amd64_x32_init_abi (info, arch,
- amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_target_description (X86_XSTATE_SSE_MASK, true));
}
/* Return the target description for a specified XSAVE feature mask. */
const struct target_desc *
-amd64_target_description (uint64_t xcr0)
+amd64_target_description (uint64_t xcr0, bool segments)
{
static target_desc *amd64_tdescs \
- [2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/] = {};
+ [2/*AVX*/][2/*MPX*/][2/*AVX512*/][2/*PKRU*/][2/*segments*/] = {};
target_desc **tdesc;
tdesc = &amd64_tdescs[(xcr0 & X86_XSTATE_AVX) ? 1 : 0]
[(xcr0 & X86_XSTATE_MPX) ? 1 : 0]
[(xcr0 & X86_XSTATE_AVX512) ? 1 : 0]
- [(xcr0 & X86_XSTATE_PKRU) ? 1 : 0];
+ [(xcr0 & X86_XSTATE_PKRU) ? 1 : 0]
+ [segments ? 1 : 0];
if (*tdesc == NULL)
- *tdesc = amd64_create_target_description (xcr0, false, false);
+ *tdesc = amd64_create_target_description (xcr0, false, false,
+ segments);
return *tdesc;
}
@@ -3314,7 +3317,7 @@ _initialize_amd64_tdep (void)
for (auto &a : xml_masks)
{
- auto tdesc = amd64_target_description (a.mask);
+ auto tdesc = amd64_target_description (a.mask, true);
selftests::record_xml_tdesc (a.xml, tdesc);
}
diff --git a/gdb/amd64-tdep.h b/gdb/amd64-tdep.h
index 7d3791ad93..94e012632f 100644
--- a/gdb/amd64-tdep.h
+++ b/gdb/amd64-tdep.h
@@ -106,7 +106,8 @@ extern void amd64_init_abi (struct gdbarch_info info,
extern void amd64_x32_init_abi (struct gdbarch_info info,
struct gdbarch *gdbarch,
const target_desc *default_tdesc);
-extern const struct target_desc *amd64_target_description (uint64_t xcr0);
+extern const struct target_desc *amd64_target_description (uint64_t xcr0,
+ bool segments);
/* Fill register REGNUM in REGCACHE with the appropriate
floating-point or SSE register value from *FXSAVE. If REGNUM is
diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 83a7f2f32e..904875bacc 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -1226,7 +1226,7 @@ amd64_windows_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
frame_unwind_append_unwinder (gdbarch, &amd64_windows_frame_unwind);
amd64_init_abi (info, gdbarch,
- amd64_target_description (X86_XSTATE_SSE_MASK));
+ amd64_target_description (X86_XSTATE_SSE_MASK, false));
windows_init_abi (info, gdbarch);
diff --git a/gdb/arch/amd64.c b/gdb/arch/amd64.c
index d31d8f1f75..4ad17c3fdc 100644
--- a/gdb/arch/amd64.c
+++ b/gdb/arch/amd64.c
@@ -33,10 +33,12 @@
/* Create amd64 target descriptions according to XCR0. If IS_X32 is
true, create the x32 ones. If IS_LINUX is true, create target
- descriptions for Linux. */
+ descriptions for Linux. If SEGMENTS is true, then include
+ the "org.gnu.gdb.i386.segments" feature registers. */
target_desc *
-amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux)
+amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux,
+ bool segments)
{
target_desc *tdesc = allocate_target_description ();
@@ -57,7 +59,8 @@ amd64_create_target_description (uint64_t xcr0, bool is_x32, bool is_linux)
regnum = create_feature_i386_64bit_sse (tdesc, regnum);
if (is_linux)
regnum = create_feature_i386_64bit_linux (tdesc, regnum);
- regnum = create_feature_i386_64bit_segments (tdesc, regnum);
+ if (segments)
+ regnum = create_feature_i386_64bit_segments (tdesc, regnum);
if (xcr0 & X86_XSTATE_AVX)
regnum = create_feature_i386_64bit_avx (tdesc, regnum);
diff --git a/gdb/arch/amd64.h b/gdb/arch/amd64.h
index c0c4dc27ef..4a659657da 100644
--- a/gdb/arch/amd64.h
+++ b/gdb/arch/amd64.h
@@ -19,4 +19,4 @@
#include <stdint.h>
target_desc *amd64_create_target_description (uint64_t xcr0, bool is_x32,
- bool is_linux);
+ bool is_linux, bool segments);
diff --git a/gdb/gdbserver/win32-i386-low.c b/gdb/gdbserver/win32-i386-low.c
index 16fe2c85b2..62221688cb 100644
--- a/gdb/gdbserver/win32-i386-low.c
+++ b/gdb/gdbserver/win32-i386-low.c
@@ -436,7 +436,7 @@ i386_arch_setup (void)
#ifdef __x86_64__
tdesc = amd64_create_target_description (X86_XSTATE_SSE_MASK, false,
- false);
+ false, false);
const char **expedite_regs = amd64_expedite_regs;
#else
tdesc = i386_create_target_description (X86_XSTATE_SSE_MASK, false);
--
2.17.1
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-06-29 22:08 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-25 18:55 x86_64-windows GDB crash due to fs_base/gs_base registers Joel Brobecker
2018-06-25 18:55 ` [PATCH 1/2] Minor reorganization of fetch_registers/store_registers in windows-nat.c Joel Brobecker
2018-06-26 16:03 ` Pedro Alves
2018-06-26 21:41 ` Joel Brobecker
2018-06-25 18:55 ` [PATCH 2/2] x86_64-windows GDB crash due to fs_base/gs_base registers Joel Brobecker
2018-06-26 16:00 ` Pedro Alves
2018-06-26 21:53 ` Joel Brobecker
2018-06-29 12:32 ` Pedro Alves
2018-06-29 12:32 ` Pedro Alves
2018-06-29 22:08 ` Joel Brobecker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).