Bug 17487 - state->dr_control_mirror == 0 failed assertion in gdbserver on Windows XP
Summary: state->dr_control_mirror == 0 failed assertion in gdbserver on Windows XP
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: server (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-15 14:23 UTC by Joel Brobecker
Modified: 2014-10-15 20:00 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joel Brobecker 2014-10-15 14:23:23 UTC
When using GDBserver on Windows XP, GDBserver reports and assertion
failure after hitting a hardware watchpoint. The problem was reproduced
using the sources from gdb.ada/int_deref, but should probably reproduce
with any scenario involving hardware watchpoints.

In our scenario, we break on line 5, just before the increment, insert
a watchhpoint on it, and then continue:

    (gdb) b foo.adb:5
    Breakpoint 1 at 0x4017c2: file foo.adb, line 5.
    (gdb) cont
    Continuing.

    Breakpoint 1, foo () at foo.adb:5
    5          Pck.Watch := Pck.Watch + 1;
    (gdb) watch watch
    Hardware watchpoint 2: watch
    (gdb) c
    Continuing.
    Remote communication error.  Target disconnected.: Invalid argument.

The immediate cause for the communication error is easily explained,
gdbserver crashes due to a failed assertion:

    x86_remove_aligned_watchpoint: Assertion `state->dr_control_mirror == 0' failed.
Comment 1 cvs-commit@gcc.gnu.org 2014-10-15 14:29:05 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, gdb-7.8-branch has been updated
       via  a70c6d64c936f981640b8e3315ddadb141af7aad (commit)
      from  a39611f90c9a8ae50ab08c17e68af0490ab95352 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a70c6d64c936f981640b8e3315ddadb141af7aad

commit a70c6d64c936f981640b8e3315ddadb141af7aad
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Tue Oct 14 23:18:35 2014 +0200

    state->dr_control_mirror == 0 failed assertion in gdbserver on Windows XP
    
    When using GDBserver on Windows XP, GDBserver reports an assertion
    failure after hitting a hardware watchpoint. The problem was reproduced
    using the sources from gdb.ada/int_deref, but should probably reproduce
    with any scenario involving hardware watchpoints.
    
    In our scenario, we break on line 5, just before the increment, insert
    a watchhpoint on it, and then continue:
    
        (gdb) b foo.adb:5
        Breakpoint 1 at 0x4017c2: file foo.adb, line 5.
        (gdb) cont
        Continuing.
    
        Breakpoint 1, foo () at foo.adb:5
        5          Pck.Watch := Pck.Watch + 1;
        (gdb) watch watch
        Hardware watchpoint 2: watch
        (gdb) c
        Continuing.
        Remote communication error.  Target disconnected.: Invalid argument.
    
    The immediate cause for the communication error is easily explained,
    gdbserver crashes due to a failed assertion:
    
        x86_remove_aligned_watchpoint: Assertion `state->dr_control_mirror == 0' failed.
    
    The assertion occurs because debug_reg_state.dr_control_mirror gets
    overwritten by the value read from the inferior, when processing
    the watchpoint event in win32_wait: win32_wait finds that we stopped,
    calls get_thread_regcache which causes i386_get_thread_context to
    get called, which then...
    
      if (th->tid == current_event->dwThreadId)
        {
          /* Copy dr values from the current thread.  */
          struct x86_debug_reg_state *dr = &debug_reg_state;
          [...]
          dr->dr_control_mirror = th->context.Dr7;
        }
    
    Both should be identical, normally making this a no-op, but
    it turns out that bits 12-11-10 are documented as being fixed
    and equal to 001. Our handling of dr_control_mirror does not
    manage those bits, and leaves them as zeros instead. So, when
    we overwrite the value from the thread's DR7 register, we
    accidentally set bit 10, causing state->dr_control_mirror
    to be 0x400 after we've cleared everything internally.
    
    This patch fixes the issue by removing the statement setting
    state->dr_control_mirror to the thread's DR7 register value.
    
    gdb/gdbserver/ChangeLog:
    
            PR server/17487
            * win32-i386-low.c (i386_get_thread_context): Do not set
            dr->dr_control_mirror.

-----------------------------------------------------------------------

Summary of changes:
 gdb/gdbserver/ChangeLog        |    6 ++++++
 gdb/gdbserver/win32-i386-low.c |    1 -
 2 files changed, 6 insertions(+), 1 deletions(-)
Comment 2 cvs-commit@gcc.gnu.org 2014-10-15 19:00:43 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, master has been updated
       via  a2abc7de6804e7e9882a86375767b24a6c215f28 (commit)
      from  6979730b1b9378a143b1bea3d0ff7b96c7bf02c5 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a2abc7de6804e7e9882a86375767b24a6c215f28

commit a2abc7de6804e7e9882a86375767b24a6c215f28
Author: Pedro Alves <palves@redhat.com>
Date:   Wed Oct 15 19:55:50 2014 +0100

    gdbserver/win32: Rewrite debug registers handling
    
    Don't use debug_reg_state for both:
    
     * "intent" - what we want the debug registers to look like
    
     * "reality" - what/which were the contents of the DR registers when
       the event triggered
    
    Reserve it for the former only, like in the GNU/Linux port.
    
    Otherwise the core x86 debug registers code can get confused if the
    inferior itself changes the debug registers since GDB last set them.
    
    This is also a requirement for being able to set watchpoints while the
    target is running, if/when we get to it on Windows.  See the big
    comment in x86_dr_stopped_data_address.
    
    Seems to me this may also fixes propagating watchpoints to all threads
    -- continue_one_thread only calls win32_set_thread_context (what
    copies the DR registers to the thread), if something already fetched
    the thread's context before.  Something else may be masking this
    issue, I haven't checked.
    
    Smoke tested by running gdbserver under Wine, connecting to it from
    GNU/Linux, and checking that I could trigger a watchpoint as expected.
    
    Joel tested it on x86-windows using AdaCore's testsuite.
    
    gdb/gdbserver/
    2014-10-15  Pedro Alves  <palves@redhat.com>
    
    	PR server/17487
    	* win32-arm-low.c (arm_set_thread_context): Remove current_event
    	parameter.
    	(arm_set_thread_context): Delete.
    	(the_low_target): Adjust.
    	* win32-i386-low.c (debug_registers_changed)
    	(debug_registers_used): Delete.
    	(update_debug_registers_callback): New function.
    	(x86_dr_low_set_addr, x86_dr_low_set_control): Mark all threads as
    	needing to update their debug registers.
    	(win32_get_current_dr): New function.
    	(x86_dr_low_get_addr, x86_dr_low_get_control)
    	(x86_dr_low_get_status): Fetch the debug register from the thread
    	record's context.
    	(i386_initial_stuff): Adjust.
    	(i386_get_thread_context): Remove current_event parameter.  Don't
    	clear debug_registers_changed nor copy DR values to
    	debug_reg_state.
    	(i386_set_thread_context): Delete.
    	(i386_prepare_to_resume): New function.
    	(i386_thread_added): Mark the thread as needing to update irs
    	debug registers.
    	(the_low_target): Remove i386_set_thread_context and install
    	i386_prepare_to_resume.
    	* win32-low.c (win32_get_thread_context): Adjust.
    	(win32_set_thread_context): Use SetThreadContext
    	directly.
    	(win32_prepare_to_resume): New function.
    	(win32_require_context): New function, factored out from ...
    	(thread_rec): ... this.
    	(continue_one_thread): Call win32_prepare_to_resume on each thread
    	we're about to continue.
    	(win32_resume): Call win32_prepare_to_resume on the event thread.
    	* win32-low.h (struct win32_thread_info)
    	<debug_registers_changed>: New field.
    	(struct win32_target_ops): Change prototype of set_thread_context,
    	delete set_thread_context and add prepare_to_resume.
    	(win32_require_context): New declaration.

-----------------------------------------------------------------------

Summary of changes:
 gdb/gdbserver/ChangeLog        |   41 ++++++++++++
 gdb/gdbserver/win32-arm-low.c  |   10 +---
 gdb/gdbserver/win32-i386-low.c |  137 +++++++++++++++++++++------------------
 gdb/gdbserver/win32-low.c      |   73 ++++++++++++++-------
 gdb/gdbserver/win32-low.h      |   15 +++--
 5 files changed, 175 insertions(+), 101 deletions(-)
Comment 3 Joel Brobecker 2014-10-15 20:00:22 UTC
Fixed on the branch and HEAD.