From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31222 invoked by alias); 22 Mar 2007 02:51:42 -0000 Received: (qmail 31208 invoked by uid 22791); 22 Mar 2007 02:51:39 -0000 X-Spam-Check-By: sourceware.org Received: from elrond.portugalmail.pt (HELO elrond.portugalmail.pt) (195.245.179.181) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 22 Mar 2007 02:51:34 +0000 Received: from localhost (localhost [127.0.0.1]) by elrond.portugalmail.pt (Postfix) with ESMTP id 9A3F83E5B0 for ; Thu, 22 Mar 2007 02:51:30 +0000 (WET) Received: from elrond.portugalmail.pt ([127.0.0.1]) by localhost (elrond.portugalmail.pt [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id RWcmn5RrN4Pv for ; Thu, 22 Mar 2007 02:51:30 +0000 (WET) Received: from [127.0.0.1] (88.210.65.121.rev.optimus.pt [88.210.65.121]) (Authenticated sender: pedro_alves@portugalmail.pt) by elrond.portugalmail.pt (Postfix) with ESMTP id B16F03CAD3 for ; Thu, 22 Mar 2007 02:51:24 +0000 (WET) Message-ID: <4601EF14.4080103@portugalmail.pt> Date: Thu, 22 Mar 2007 02:51:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; pt-BR; rv:1.8.0.10) Gecko/20070221 Thunderbird/1.5.0.10 Mnenhy/0.7.4.0 MIME-Version: 1.0 To: gdb-patches@sourceware.org Subject: Re: TUI + gdbserver broken? References: <45FDECB3.5000002@portugalmail.pt> <20070319021145.GA25872@caradoc.them.org> <45FEF7B0.9070209@portugalmail.pt> <20070319221430.GA24326@caradoc.them.org> <45FF1571.2080401@portugalmail.pt> <20070319234141.GA29137@caradoc.them.org> <45FF2D16.6030501@portugalmail.pt> <20070320025548.GA6238@caradoc.them.org> <4053daab0703200205r462e7501x771fa8a0d1a91ebf@mail.gmail.com> <46006BF5.8020907@portugalmail.pt> In-Reply-To: <46006BF5.8020907@portugalmail.pt> Content-Type: multipart/mixed; boundary="------------010406040803040705010501" X-Antivirus: avast! (VPS 000726-1, 21-03-2007), Outbound message X-Antivirus-Status: Clean X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2007-03/txt/msg00191.txt.bz2 This is a multi-part message in MIME format. --------------010406040803040705010501 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Content-length: 6094 You know what they say. "A patch per day, ..." :) I wish I new before about the set new-console 1 command. It makes debugging TUI on cygwin much easier and it would save me from holding the Guinness record of bad patches in a row. :) I've been trying to understand why doing as you suggested doesn't work, and what I see makes me believe that the current frame hook handling is not safe - that is, as I said before, I believe that calling: get_selected_frame -> select_frame -> deprecated_selected_frame_level_changed_hook inside a deprecated_selected_frame_level_changed_hook is a bad idea. I believe those hooks should be pure observers. Take a look at the stack trace that I get with the following pseudo-patch applied. static void tui_selected_frame_level_changed_hook (int level) { struct frame_info *fi; + if (level < 0) + return; fi = deprecated_safe_get_selected_frame (); remote_fetch_registers (regnum=8) at ../../gdb-server_search/src/gdb/remote.c:3688 #1 0x0047719e in regcache_raw_read (regcache=0x10042670, regnum=8, buf=0x22c440 "ð") at ../../gdb-server_search/src/gdb/regcache.c:510 #2 0x00477769 in regcache_cooked_read (regcache=0x10042670, regnum=8, buf=0x22c440 "ð") at ../../gdb-server_search/src/gdb/regcache.c:588 #3 0x00523575 in sentinel_frame_prev_register (next_frame=0x1005bb68, this_prologue_cache=0x1005bb6c, regnum=8, optimized=0x22c424, lvalp=0x22c418, addrp=0x22c420, realnum=0x22c41c, bufferp=0x22c440 "ð") at ../../gdb-server_search/src/gdb/sentinel-frame.c:69 #4 0x0047b794 in frame_register_unwind (frame=0x1005bb68, regnum=8, optimizedp=0x22c424, lvalp=0x22c418, addrp=0x22c420, realnump=0x22c41c, bufferp=0x22c440 "ð") at ../../gdb-server_search/src/gdb/frame.c:589 #5 0x0047ba84 in frame_unwind_register (frame=0x1005bb68, regnum=8, buf=0x22c440 "ð") at ../../gdb-server_search/src/gdb/frame.c:641 #6 0x0051db90 in i386_unwind_pc (gdbarch=0x101cdef8, next_frame=0x1005bb68) at ../../gdb-server_search/src/gdb/i386-tdep.c:915 #7 0x0045bfa5 in gdbarch_unwind_pc (gdbarch=0x101cdef8, next_frame=0x1005bb68) at ../../gdb-server_search/src/gdb/gdbarch.c:3071 #8 0x005235cb in sentinel_frame_prev_pc (next_frame=0x1005bb68, this_prologue_cache=0x1005bb6c) at ../../gdb-server_search/src/gdb/sentinel-frame.c:89 #9 0x0047b37f in frame_pc_unwind (this_frame=0x1005bb68) at ../../gdb-server_search/src/gdb/frame.c:438 #10 0x0047ced7 in frame_unwind_address_in_block (next_frame=0x1005bb68, this_type=NORMAL_FRAME) at ../../gdb-server_search/src/gdb/frame.c:1502 #11 0x004de9ca in dwarf2_frame_sniffer (next_frame=0x1005bb68) at ../../gdb-server_search/src/gdb/dwarf2-frame.c:1210 #12 0x004b1b6f in frame_unwind_find_by_frame (next_frame=0x1005bb68, this_cache=0x1005bbbc) at ../../gdb-server_search/src/gdb/frame-unwind.c:98 #13 0x0047d1c7 in get_frame_type (frame=0x1005bbb8) at ../../gdb-server_search/src/gdb/frame.c:1634 #14 0x0047cf20 in get_frame_address_in_block (this_frame=0x1005bbb8) at ../../gdb-server_search/src/gdb/frame.c:1529 #15 0x0047c36a in select_frame (fi=0x1005bbb8) at ../../gdb-server_search/src/gdb/frame.c:1016 #16 0x0047c2ab in get_selected_frame (message=0x0) at ../../gdb-server_search/src/gdb/frame.c:965 #17 0x0047c325 in deprecated_safe_get_selected_frame () at ../../gdb-server_search/src/gdb/frame.c:981 #18 0x004a3098 in tui_registers_changed_hook () at ../../gdb-server_search/src/gdb/tui/tui-hooks.c:135 #19 0x00477008 in registers_changed () at ../../gdb-server_search/src/gdb/regcache.c:469 #20 0x00425af2 in wait_for_inferior () at ../../gdb-server_search/src/gdb/infrun.c:993 #21 0x00425a08 in start_remote (from_tty=1) at ../../gdb-server_search/src/gdb/infrun.c:855 #22 0x005133bb in remote_start_remote (uiout=0x10069220, from_tty_p=0x22c764) at ../../gdb-server_search/src/gdb/remote.c:2109 #23 0x00412d5c in catch_exception (uiout=0x10069220, func=0x513329 , func_args=0x22c764, mask=6) at ../../gdb-server_search/src/gdb/exceptions.c:469 #24 0x00513c98 in remote_open_1 (name=0x10033bc8 ":9999", from_tty=1, target=0x6b8df0, extended_p=0, async_p=0) at ../../gdb-server_search/src/gdb/remote.c:2563 #25 0x005133ed in remote_open (name=0x10033bc8 ":9999", from_tty=1) at ../../gdb-server_search/src/gdb/remote.c:2118 #26 0x0042ca1b in do_cfunc (c=0x10042728, args=0x10033bc8 ":9999", from_tty=1) By bad luck, we ended up calling remote_fetch_registers after calling putpkt ("?") in remote_start_remote, which calls set_thread (*), followed by a fetch registers put/recv pkt. By the time we get to remote_wait, waiting for the resume reply, there is nothing left waiting to be processed, so we just sit there in an infinite loop waiting for a packet that never comes. (*) - It takes a few more millis than in the normal unpatched case. Probably a timeout? Now, why doesn't this happen without the patch ( current cvs ) ? Pure luck. Without that patch, luckily there is a tui_selected_frame_level_changed_hook called during gdbarch_update, before registers_changed (#19) is called, which has the side effect of calling target_fetch_registers before "?" is sent. By the time we reach #19 there is already a frame selected in the cache. This makes get_selected_frame (#16) return immediately without calling select_frame. This then means target_fetch_registers is not called again, and nothing is wrongly injected in the stream like in the patched version. As you can see, changing the selected_frame state from inside a selected_frame_changed hook is dangerous. I maintain my view that the hooks should be pure observers, or that either deprecated_safe_get_selected_frame should be taught some more to know if the target is running, or instead a patch like the first one I sent (the one that called target_mark_running/target_mark_exited) should be installed. If you agree that the former is the way to go, the attached patch works for me. Maybe we should also copy (not move) the 'if (selected == NULL) -> select (current)' mapping to select_frame. Cheers, Pedro Alves --------------010406040803040705010501 Content-Type: text/plain; name="tui_frame5.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="tui_frame5.diff" Content-length: 1798 --- gdb/frame.c | 6 ++++++ gdb/tui/tui-hooks.c | 11 ++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) Index: src/gdb/frame.c =================================================================== --- src.orig/gdb/frame.c 2007-03-20 20:00:04.000000000 +0000 +++ src/gdb/frame.c 2007-03-22 02:33:54.000000000 +0000 @@ -946,6 +946,12 @@ get_current_frame (void) static struct frame_info *selected_frame; +int +selected_frame_is_valid (void) +{ + return (selected_frame != NULL); +} + /* Return the selected frame. Always non-NULL (unless there isn't an inferior sufficient for creating a frame) in which case an error is thrown. */ Index: src/gdb/tui/tui-hooks.c =================================================================== --- src.orig/gdb/tui/tui-hooks.c 2007-03-21 22:48:08.000000000 +0000 +++ src/gdb/tui/tui-hooks.c 2007-03-22 02:46:56.000000000 +0000 @@ -127,12 +127,18 @@ tui_query_hook (const char * msg, va_lis /* Prevent recursion of deprecated_registers_changed_hook(). */ static int tui_refreshing_registers = 0; +/* Move to frame.h if patch is approved. */ +extern int selected_frame_is_valid (void); + static void tui_registers_changed_hook (void) { struct frame_info *fi; - fi = get_selected_frame (NULL); + if (!selected_frame_is_valid ()) + return; + + fi = deprecated_safe_get_selected_frame (); if (tui_refreshing_registers == 0) { tui_refreshing_registers = 1; @@ -230,6 +236,9 @@ tui_selected_frame_level_changed_hook (i { struct frame_info *fi; + if (!selected_frame_is_valid ()) + return; + fi = deprecated_safe_get_selected_frame (); /* Ensure that symbols for this frame are read in. Also, determine the source language of this frame, and switch to it if desired. */ --------------010406040803040705010501--