This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH v4 1/3] Add support for recording xsave x86 instruction


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]