This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH v4 1/3] Add support for recording xsave x86 instruction
- From: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- To: Pierre Marsais <pierre dot marsais at lse dot epita dot fr>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Thu, 11 Oct 2018 11:33:30 +0000
- Subject: RE: [PATCH v4 1/3] Add support for recording xsave x86 instruction
- References: <20180921003827.1525-1-pierre.marsais@lse.epita.fr> <20181006001539.32414-1-pierre.marsais@lse.epita.fr>
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