Patchwork [v4,1/3] Add support for recording xsave x86 instruction

login
register
mail settings
Submitter Pierre Marsais
Date Oct. 6, 2018, 12:15 a.m.
Message ID <20181006001539.32414-1-pierre.marsais@lse.epita.fr>
Download mbox | patch
Permalink /patch/29660/
State New
Headers show

Comments

Pierre Marsais - Oct. 6, 2018, 12:15 a.m.
Latest version of glibc's ld.so use the xsave instruction in the
resolver. This breaks gdb record when calling shared libraries:

```
$ gcc -o fail -ggdb -x c - <<EOF
#include <stdlib.h>

int main() {
        exit(0);
}
EOF
$ gdb ./fail
Reading symbols from ./fail...done.
(gdb) b main
Breakpoint 1 at 0x113d: file <stdin>, line 4.
(gdb) r
Starting program: /tmp/fail

Breakpoint 1, main () at <stdin>:4
4       <stdin>: No such file or directory.
(gdb) record
(gdb) c
Continuing.
Process record does not support instruction 0xfae64 at address
0x7ffff7fe96dc.
```

In order to record xsave instructions, we record the from the beginning
of the XSAVE area to the end of the last state component in the Request
Feature Bitmap (whose value is XCR0 & EDX:EAX). At the moment we don't
account for non requested state component within the XSAVE area.

gdb/ChangeLog:
2018-09-21  Pierre Marsais <pierre.marsais@lse.epita.fr>

	* i386-tdep.c: (i386_process_record): Handle xsave instruction.

gdb/testsuite/ChangeLog:
2018-09-21  Pierre Marsais <pierre.marsais@lse.epita.fr>

	* gdb.reverse/i386-xsave-reverse.c: New file.
	* gdb.reverse/i386-xsave-reverse.exp: New file.
---
Changes in v4:
* Add support of recording xsavec instruction
* Fix decoding of 0x0fc7 instructions/rewrite it as a switch
* Patch 1 is still the same as v3

Changes in v3:
* Only save XSAVE SSE area when requested
* Do not save XMM8-XMM15 registers in XSAVE area for 32-bit mode
* Account for the hole between the legacy region and the XSAVE header
* Add a test without SSE in the Request Feature Bitmap
* Improve style of tests

Changes in v2:
* Address Markus review
* Do not support instruction when MOD == 3
* Query XSAVE area buffer size with cpuid in tests
* Use X86_XSTATE_SIZE macro to get XSAVE area size instead of cpuid

 gdb/i386-tdep.c                               |  45 ++++++++
 .../gdb.reverse/i386-xsave-reverse.c          |  53 +++++++++
 .../gdb.reverse/i386-xsave-reverse.exp        | 102 ++++++++++++++++++
 3 files changed, 200 insertions(+)
 create mode 100644 gdb/testsuite/gdb.reverse/i386-xsave-reverse.c
 create mode 100644 gdb/testsuite/gdb.reverse/i386-xsave-reverse.exp
Metzger, Markus T - Oct. 11, 2018, 11:33 a.m.
Hello Pierre,

> +            if (rfbm & ~X86_XSTATE_ALL_MASK) {
> +              printf_unfiltered (_("Process record does not support XSAVE "
> +                                   "instruction with RFBM=%" PRIx64 ".\n"),
> +                                   rfbm);

The indentation looks a bit off.


> +              opcode = (opcode << 8) | ir.modrm;
> +              goto no_support;
> +            }
> +
> +            uint64_t tmpu64;
> +            if (i386_record_lea_modrm_addr (&ir, &tmpu64))
> +              return -1;
> +
> +            uint64_t legacy_size = 160; /* x87 xsave size */
> +            if (rfbm & X86_XSTATE_SSE) {
> +              if (ir.regmap[X86_RECORD_R8_REGNUM])
> +                legacy_size = 416; /* 64-bit sse xsave size */
> +              else
> +                legacy_size = 288; /* 32-bit sse xsave size */
> +            }
> +            if (record_full_arch_list_add_mem (tmpu64, legacy_size))
> +              return -1;
> +
> +            uint64_t size = X86_XSTATE_SIZE (rfbm) - 512;
> +            if (record_full_arch_list_add_mem (tmpu64 + 512, size))
> +              return -1;

This stores the full xsave area header.  XSAVE only writes the first 8 bytes.

Also it turns out the macros are not correct, either.  Jan Beulich sent a
patch to fix XSAVE handling:
https://sourceware.org/ml/gdb-patches/2018-10/msg00261.html

We'd need to do a similar analysis here.


> +uint32_t get_xsave_buffer_size (void) {

Please use the GNU coding style also in tests.


> +# This test tests some i386 general instructions for reverse execution.
> +#
> +
> +if {![supports_reverse] || ![supports_process_record]} {
> +    return
> +}

You'd typically log them as unsupported.  Not sure if you need to check
supports_reverse, as well.



> +gdb_test "break $end_xsave_test" \
> +    "Breakpoint $decimal at .* line $end_xsave_test\." \
> +    "set breakpoint at end of xsave_test"
> +
> +set test "continue to end of xsave_test"
> +gdb_test_multiple "continue" $test {
> +    -re " end xsave_test .*\r\n$gdb_prompt $" {
> +        pass $test
> +    }
> +    -re " Illegal instruction.*\r\n$gdb_prompt $" {
> +        untested i386-xsave-reverse
> +        return -1
> +    }
> +}

Wouldn't it be easier to check for XSAVE support separately?


The tests don't really cover that record is not saving too much.
I think both tests would pass if we recorded the entire page.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Patch

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index a6994aaf12..a9fe290307 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -7385,6 +7385,51 @@  no_support_3dnow_data:
             return -1;
           break;
 
+        case 4: /* xsave */
+          {
+            if (ir.mod == 3) {
+              opcode = (opcode << 8) | ir.modrm;
+              goto no_support;
+            }
+
+            ULONGEST rfbm_eax, rfbm_edx;
+
+            regcache_raw_read_unsigned (regcache, I386_EAX_REGNUM, &rfbm_eax);
+            rfbm_eax &= (1ULL << 32) - 1;
+
+            regcache_raw_read_unsigned (regcache, I386_EDX_REGNUM, &rfbm_edx);
+            rfbm_edx &= (1ULL << 32) - 1;
+
+            uint64_t rfbm = tdep->xcr0 & (rfbm_eax | (rfbm_edx << 32));
+
+            if (rfbm & ~X86_XSTATE_ALL_MASK) {
+              printf_unfiltered (_("Process record does not support XSAVE "
+                                   "instruction with RFBM=%" PRIx64 ".\n"),
+                                   rfbm);
+              opcode = (opcode << 8) | ir.modrm;
+              goto no_support;
+            }
+
+            uint64_t tmpu64;
+            if (i386_record_lea_modrm_addr (&ir, &tmpu64))
+              return -1;
+
+            uint64_t legacy_size = 160; /* x87 xsave size */
+            if (rfbm & X86_XSTATE_SSE) {
+              if (ir.regmap[X86_RECORD_R8_REGNUM])
+                legacy_size = 416; /* 64-bit sse xsave size */
+              else
+                legacy_size = 288; /* 32-bit sse xsave size */
+            }
+            if (record_full_arch_list_add_mem (tmpu64, legacy_size))
+              return -1;
+
+            uint64_t size = X86_XSTATE_SIZE (rfbm) - 512;
+            if (record_full_arch_list_add_mem (tmpu64 + 512, size))
+              return -1;
+          }
+          break;
+
         case 5:    /* lfence */
         case 6:    /* mfence */
         case 7:    /* sfence clflush */
diff --git a/gdb/testsuite/gdb.reverse/i386-xsave-reverse.c b/gdb/testsuite/gdb.reverse/i386-xsave-reverse.c
new file mode 100644
index 0000000000..18453405a6
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/i386-xsave-reverse.c
@@ -0,0 +1,53 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Architecture tests for intel i386 platform.  */
+
+#include <cpuid.h>
+#include <stdint.h>
+#include <string.h>
+
+uint32_t get_xsave_buffer_size (void) {
+  uint32_t size, eax, ecx, edx;
+  __get_cpuid_count (0xd, 0, &eax, &size, &ecx, &edx);
+
+  return size;
+}
+
+void xsave_test (void) {
+  uint32_t xsave_buf_sze = get_xsave_buffer_size ();
+  char buf[xsave_buf_sze] __attribute__ ((aligned (64)));
+  memset (buf, 0, xsave_buf_sze);
+
+  asm ("xsave %0":"=m"(buf) : "a"(~0L), "d"(~0L));
+} /* end xsave_test */
+
+void xsave_no_sse_test (void) {
+  uint32_t xsave_buf_sze = get_xsave_buffer_size ();
+  char buf[xsave_buf_sze] __attribute__ ((aligned (64)));
+  memset (buf, 0, xsave_buf_sze);
+
+  asm ("xsave %0":"=m"(buf): "a"((1 << 1) ^ ~0L), "d"(~0L));
+} /* end xsave_no_sse_test */
+
+int
+main ()
+{
+  xsave_test ();
+  xsave_no_sse_test ();
+  return 0;	/* end of main */
+}
diff --git a/gdb/testsuite/gdb.reverse/i386-xsave-reverse.exp b/gdb/testsuite/gdb.reverse/i386-xsave-reverse.exp
new file mode 100644
index 0000000000..f3c0f93bfc
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/i386-xsave-reverse.exp
@@ -0,0 +1,102 @@ 
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+#
+# This test tests some i386 general instructions for reverse execution.
+#
+
+if {![supports_reverse] || ![supports_process_record]} {
+    return
+}
+
+if ![istarget "*86*-*linux*"] then {
+    verbose "Skipping i386 reverse tests."
+    return
+}
+
+standard_testfile
+
+# some targets have leading underscores on assembly symbols.
+set additional_flags [gdb_target_symbol_prefix_flags]
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
+         [list debug $additional_flags]]} {
+    return -1
+}
+
+set end_of_main          [gdb_get_line_number " end of main "]
+set end_xsave_test         [gdb_get_line_number " end xsave_test "]
+set end_xsave_no_sse_test         [gdb_get_line_number " end xsave_no_sse_test "]
+
+if ![runto main] then {
+    fail "run to main"
+    return
+}
+
+# Activate process record/replay
+gdb_test_no_output "record" "turn on process record"
+
+global hex
+global decimal
+
+#xsave_test
+
+gdb_test "break $end_xsave_test" \
+    "Breakpoint $decimal at .* line $end_xsave_test\." \
+    "set breakpoint at end of xsave_test"
+
+set test "continue to end of xsave_test"
+gdb_test_multiple "continue" $test {
+    -re " end xsave_test .*\r\n$gdb_prompt $" {
+        pass $test
+    }
+    -re " Illegal instruction.*\r\n$gdb_prompt $" {
+        untested i386-xsave-reverse
+        return -1
+    }
+}
+
+gdb_test "reverse-step" "xsave.*" "reverse-step to xsave"
+
+gdb_test "print buf" ".* = '\\\\000' <repeats .* times>" \
+    "verify xsave buffer after reverse xsave"
+
+#xsave_no_sse_test
+
+gdb_test "continue" \
+    " end xsave_test .*" \
+    "continue to end of xsave_test"
+
+gdb_test "break $end_xsave_no_sse_test" \
+    "Breakpoint $decimal at .* line $end_xsave_no_sse_test\." \
+    "set breakpoint at end of xsave_no_sse_test"
+
+set test "continue to end of xsave_no_sse_test"
+gdb_test_multiple "continue" $test {
+    -re " end xsave_no_sse_test .*\r\n$gdb_prompt $" {
+        pass $test
+    }
+    -re " Illegal instruction.*\r\n$gdb_prompt $" {
+        untested i386-xsave-no-sse-reverse
+        return -1
+    }
+}
+
+gdb_test "reverse-step" "xsave.*" "reverse-step to xsave"
+
+gdb_test "print buf" ".* = '\\\\000' <repeats .* times>" \
+    "verify xsave buffer after reverse xsave (no sse)"