[v3,00/29] Windows code sharing + bug fix

Message ID 20200313190855.28662-1-tromey@adacore.com
Headers show
Series
  • Windows code sharing + bug fix
Related show

Message

Tom Tromey March 13, 2020, 7:08 p.m.
This is v3 of the series to share a lot of the Windows code between
gdb and gdbserver.  It also fixes a bug that a customer reported; in
fact this fix was the origin of this series.  See patch #11 for
details on the bug.

I think this addresses all the review comments.  It's somewhat hard to
be sure since they were done in gerrit, and extracting comments from
that is a pain.  Also I think I had sent v2 before I updated my
scripts to preserve the gerrit change ID, so it is in gerrit twice.
Anyway, the reviews happened around Nov 2019, so you can find some in
the mailing list archives.

I tested this largely by hand.  I also ran it through the AdaCore test
suite, where it did ok.  (There were some issues, but I think they are
largely unrelated; some things like gdb printing paths that the test
suite didn't really recognize.)

I've also updated this to handle the WOW64 stuff, but only in a
minimal way -- I didn't try to make this work in gdbserver.

Let me know what you think.

Tom

Comments

Tom Tromey April 8, 2020, 8:33 p.m. | #1
>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:


Tom> This is v3 of the series to share a lot of the Windows code between
Tom> gdb and gdbserver.  It also fixes a bug that a customer reported; in
Tom> fact this fix was the origin of this series.  See patch #11 for
Tom> details on the bug.

Tom> I think this addresses all the review comments.  It's somewhat hard to
Tom> be sure since they were done in gerrit, and extracting comments from
Tom> that is a pain.  Also I think I had sent v2 before I updated my
Tom> scripts to preserve the gerrit change ID, so it is in gerrit twice.
Tom> Anyway, the reviews happened around Nov 2019, so you can find some in
Tom> the mailing list archives.

Tom> I tested this largely by hand.  I also ran it through the AdaCore test
Tom> suite, where it did ok.  (There were some issues, but I think they are
Tom> largely unrelated; some things like gdb printing paths that the test
Tom> suite didn't really recognize.)

Tom> I've also updated this to handle the WOW64 stuff, but only in a
Tom> minimal way -- I didn't try to make this work in gdbserver.

I'm checking this in now.
Let me know if there are any issues.

Tom
Carl Love via Gdb-patches April 8, 2020, 10:17 p.m. | #2
Am Mittwoch, 8. April 2020, 22:33:18 MESZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> >>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

>

> Tom> This is v3 of the series to share a lot of the Windows code between

> Tom> gdb and gdbserver.  It also fixes a bug that a customer reported; in

> Tom> fact this fix was the origin of this series.  See patch #11 for

> Tom> details on the bug.

>

> Tom> I think this addresses all the review comments.  It's somewhat hard to

> Tom> be sure since they were done in gerrit, and extracting comments from

> Tom> that is a pain.  Also I think I had sent v2 before I updated my

> Tom> scripts to preserve the gerrit change ID, so it is in gerrit twice.

> Tom> Anyway, the reviews happened around Nov 2019, so you can find some in

> Tom> the mailing list archives.

>

> Tom> I tested this largely by hand.  I also ran it through the AdaCore test

> Tom> suite, where it did ok.  (There were some issues, but I think they are

> Tom> largely unrelated; some things like gdb printing paths that the test

> Tom> suite didn't really recognize.)

>

> Tom> I've also updated this to handle the WOW64 stuff, but only in a

> Tom> minimal way -- I didn't try to make this work in gdbserver.

>

> I'm checking this in now.

> Let me know if there are any issues.


I seems I kinda missed to actually use Wow64SuspendThread when I implemented
the WOW64 stuff.
I never noticed any problems, and SuspendThread actually seems to work fine
for WOW64 processes, but that might just be luck (or depend on the Windows
version).

You moved the SuspendThread stuff now into nat/windows-nat.c, so that
complicates the situation a bit (but it's also the reason I noticed).

Maybe I should just try to implement the WOW64 stuff for gdbserver as well, and
then try to move more shared stuff into nat/windows-nat.c?


Hannes
Tom Tromey April 9, 2020, 2:49 a.m. | #3
>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:


Hannes> I seems I kinda missed to actually use Wow64SuspendThread when I implemented
Hannes> the WOW64 stuff.

Could you double-check to make sure I didn't mess it up when rebasing
this series over your patches?

Hannes> Maybe I should just try to implement the WOW64 stuff for gdbserver as well, and
Hannes> then try to move more shared stuff into nat/windows-nat.c?

It would be nice to try to keep the code more in sync.
They still aren't identical but they do share quite a bit more now.
So, unless it's unusually hard, I would say yes.

Tom
Jon Turney April 9, 2020, 2:57 p.m. | #4
On 09/04/2020 03:49, Tom Tromey wrote:
>>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

> 

> Hannes> I seems I kinda missed to actually use Wow64SuspendThread when I implemented

> Hannes> the WOW64 stuff.

> 

> Could you double-check to make sure I didn't mess it up when rebasing

> this series over your patches?

> 

> Hannes> Maybe I should just try to implement the WOW64 stuff for gdbserver as well, and

> Hannes> then try to move more shared stuff into nat/windows-nat.c?

> 

> It would be nice to try to keep the code more in sync.

> They still aren't identical but they do share quite a bit more now.

> So, unless it's unusually hard, I would say yes.


I wonder if SuspendThread() is actually needed at all, since it doesn't 
make a huge amount of sense for the debugee to still be running when 
WaitForDebugEvent() returns.
Carl Love via Gdb-patches April 9, 2020, 3:08 p.m. | #5
Am Donnerstag, 9. April 2020, 16:57:38 MESZ hat Jon Turney <jon.turney@dronecode.org.uk> Folgendes geschrieben:

> On 09/04/2020 03:49, Tom Tromey wrote:

>

> >>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

> >

> > Hannes> I seems I kinda missed to actually use Wow64SuspendThread when I implemented

> > Hannes> the WOW64 stuff.

> >

> > Could you double-check to make sure I didn't mess it up when rebasing

> > this series over your patches?

> >

> > Hannes> Maybe I should just try to implement the WOW64 stuff for gdbserver as well, and

> > Hannes> then try to move more shared stuff into nat/windows-nat.c?

> >

> > It would be nice to try to keep the code more in sync.

> > They still aren't identical but they do share quite a bit more now.

> > So, unless it's unusually hard, I would say yes.

>

>

> I wonder if SuspendThread() is actually needed at all, since it doesn't

> make a huge amount of sense for the debugee to still be running when

> WaitForDebugEvent() returns.


I took me quite a while to figure out a situation where SuspendThread() was
actually called.

And I can get there if I start the target process in a new console, and then
press ctrl-c in there so I get the DBG_CONTROL_C exception.
And then do something with threads, either "thread 1" or "info threads".

The backtrace then looks like this:
Thread 1 hit Breakpoint 1, 0x0000000077182a60 in SuspendThread () from C:\Windows\system32\kernel32.dll
(gdb) bt
#0  0x0000000077182a60 in SuspendThread () from C:\Windows\system32\kernel32.dll
#1  0x00000000005b9d32 in windows_nat::windows_thread_info::suspend (this=this@entry=0x11c79100) at C:/src/repos/binutils-gdb.git/gdb/nat/windows-nat.c:63
#2  0x00000000006c7e09 in windows_nat::thread_rec (ptid=..., disposition=disposition@entry=windows_nat::INVALIDATE_CONTEXT)
    at C:/src/repos/binutils-gdb.git/gdb/windows-nat.c:406
#3  0x00000000006c7e5e in windows_nat_target::fetch_registers (this=<optimized out>, regcache=0x11d053f0, r=8)
    at C:/src/repos/binutils-gdb.git/gdb/regcache.h:383
#4  0x000000000066ab3c in target_fetch_registers (regcache=regcache@entry=0x11d053f0, regno=regno@entry=8) at C:/src/repos/binutils-gdb.git/gdb/target.h:1326
#5  0x00000000005ec6ca in regcache::raw_update (regnum=8, this=0x11d053f0) at C:/src/repos/binutils-gdb.git/gdb/regcache.c:499
#6  regcache::raw_update (this=0x11d053f0, regnum=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/regcache.c:488
#7  0x00000000005ed51e in readable_regcache::raw_read (this=0x11d053f0, regnum=8, buf=0x11d33e70 "") at C:/src/repos/binutils-gdb.git/gdb/regcache.c:513
#8  0x00000000005ee1bf in readable_regcache::cooked_read_value (this=0x11d053f0, regnum=8) at C:/src/repos/binutils-gdb.git/gdb/regcache.c:663
#9  0x000000000061af87 in sentinel_frame_prev_register (this_frame=<optimized out>, this_prologue_cache=<optimized out>, regnum=<optimized out>)
    at C:/src/repos/binutils-gdb.git/gdb/sentinel-frame.c:53
#10 0x0000000000527546 in frame_unwind_register_value (next_frame=next_frame@entry=0x11837cf0, regnum=regnum@entry=8)
    at C:/src/repos/binutils-gdb.git/gdb/frame.c:1229
#11 0x000000000052780d in frame_register_unwind (next_frame=next_frame@entry=0x11837cf0, regnum=regnum@entry=8, optimizedp=optimizedp@entry=0xfd5f4b8,
    unavailablep=unavailablep@entry=0xfd5f4bc, lvalp=lvalp@entry=0xfd5f4c4, addrp=addrp@entry=0xfd5f4c8, realnump=realnump@entry=0xfd5f4c0,
    bufferp=bufferp@entry=0xfd5f508 "@˜+\017") at C:/src/repos/binutils-gdb.git/gdb/frame.c:1132
#12 0x0000000000527b99 in frame_unwind_register (next_frame=next_frame@entry=0x11837cf0, regnum=8, buf=buf@entry=0xfd5f508 "@˜+\017")
    at C:/src/repos/binutils-gdb.git/gdb/frame.c:1188
#13 0x000000000054d9d7 in i386_unwind_pc (gdbarch=0x118ae400, next_frame=0x11837cf0) at C:/src/repos/binutils-gdb.git/gdb/i386-tdep.c:1971
#14 0x0000000000526eb0 in frame_unwind_pc (this_frame=0x11837cf0) at C:/src/repos/binutils-gdb.git/gdb/frame.c:928
#15 0x0000000000527010 in get_frame_pc (frame=0x11837dc0) at C:/src/repos/binutils-gdb.git/gdb/frame.c:2399
#16 get_frame_address_in_block (this_frame=0x11837dc0) at C:/src/repos/binutils-gdb.git/gdb/frame.c:2429
#17 0x00000000005270bf in get_frame_address_in_block_if_available (this_frame=<optimized out>, pc=pc@entry=0xfd5f628)
    at C:/src/repos/binutils-gdb.git/gdb/frame.c:2492
#18 0x0000000000527240 in select_frame (fi=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/frame.c:1738
#19 0x0000000000528627 in select_frame (fi=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/frame.c:1727
#20 get_selected_frame (message=message@entry=0x0) at C:/src/repos/binutils-gdb.git/gdb/frame.c:1680
#21 0x00000000006712a5 in print_thread_info_1 (uiout=0x118b6f00, requested_threads=0x0, global_ids=0, pid=-1, show_global_ids=0)
    at C:/src/repos/binutils-gdb.git/gdb/thread.c:1193
#22 0x0000000000671ed7 in info_threads_command (arg=<optimized out>, from_tty=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/thread.c:1284
#23 0x0000000000480d22 in cmd_func (cmd=0xf4, args=0x2 <error: Cannot access memory at address 0x2>, from_tty=7976)
    at C:/src/repos/binutils-gdb.git/gdb/cli/cli-decode.c:1952
#24 0x0000000000674638 in execute_command (p=<optimized out>, p@entry=0x11c68790 "info threads", from_tty=1) at C:/src/repos/binutils-gdb.git/gdb/top.c:655
#25 0x0000000000516054 in command_handler (command=0x11c68790 "info threads") at C:/src/repos/binutils-gdb.git/gdb/event-top.c:587
#26 0x0000000000516f02 in command_line_handler (rl=...) at C:/src/repos/binutils-gdb.git/gdb/event-top.c:772
#27 0x0000000000516833 in gdb_rl_callback_handler (rl=0x11805f60 "info threads")
    at c:/msys64/mingw64/x86_64-w64-mingw32/include/c++/9.3.0/bits/unique_ptr.h:153
#28 0x00000000006e60fc in rl_callback_read_char () at C:/src/repos/binutils-gdb.git/readline/readline/callback.c:281
#29 0x00000000006e633f in rl_callback_read_char () at C:/src/repos/binutils-gdb.git/readline/readline/callback.c:222
#30 0x0000000000515bae in gdb_rl_callback_read_char_wrapper_noexcept () at C:/src/repos/binutils-gdb.git/gdb/event-top.c:176
#31 0x00000000005166e4 in gdb_rl_callback_read_char_wrapper (client_data=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/event-top.c:192
#32 0x00000000005159f2 in stdin_event_handler (error=<optimized out>, client_data=0x1180b720) at C:/src/repos/binutils-gdb.git/gdb/event-top.c:515
#33 0x0000000000514a10 in handle_file_event (ready_mask=2, file_ptr=0x11ce4ee0) at C:/src/repos/binutils-gdb.git/gdb/event-loop.c:701
#34 gdb_wait_for_event (block=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/event-loop.c:852
#35 gdb_wait_for_event (block=<optimized out>) at C:/src/repos/binutils-gdb.git/gdb/event-loop.c:714
#36 0x0000000000514bee in gdb_do_one_event () at C:/src/repos/binutils-gdb.git/gdb/event-loop.c:313
#37 0x0000000000514cde in gdb_do_one_event () at C:/src/repos/binutils-gdb.git/gdb/event-loop.c:337
#38 start_event_loop () at C:/src/repos/binutils-gdb.git/gdb/event-loop.c:337
#39 0x0000000000593ce2 in captured_command_loop () at C:/src/repos/binutils-gdb.git/gdb/main.c:360
#40 0x0000000000595b05 in captured_main (data=0xfd5fdc0) at C:/src/repos/binutils-gdb.git/gdb/main.c:1198
#41 gdb_main (args=args@entry=0xfd5fe20) at C:/src/repos/binutils-gdb.git/gdb/main.c:1213
#42 0x000000000098adc7 in main (argc=3, argv=0x1348a0) at C:/src/repos/binutils-gdb.git/gdb/gdb.c:32


Hannes
Tom Tromey April 9, 2020, 5:54 p.m. | #6
>>>>> "Jon" == Jon Turney <jon.turney@dronecode.org.uk> writes:


Jon> I wonder if SuspendThread() is actually needed at all, since it
Jon> doesn't make a huge amount of sense for the debugee to still be
Jon> running when WaitForDebugEvent() returns.

I think when single-stepping we need to suspend the other threads.
Other than that, I tend to agree.

IIRC I didn't try to generally clean up thread suspension in this
series.  I extended it a little, that's all.

Tom