From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26743 invoked by alias); 17 May 2006 16:02:33 -0000 Received: (qmail 26735 invoked by uid 22791); 17 May 2006 16:02:31 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Wed, 17 May 2006 16:02:27 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1FgOTc-0001pu-OK; Wed, 17 May 2006 12:02:20 -0400 Date: Wed, 17 May 2006 17:59:00 -0000 From: Daniel Jacobowitz To: Mark Kettenis Cc: sjackman@gmail.com, gdb-patches@sourceware.org Subject: Re: Fix a crash when stepping and unwinding fails Message-ID: <20060517160220.GD4118@nevyn.them.org> Mail-Followup-To: Mark Kettenis , sjackman@gmail.com, gdb-patches@sourceware.org References: <20060220220331.GA29363@nevyn.them.org> <200602212015.k1LKFGrj005090@elgar.sibelius.xs4all.nl> <20060221202833.GA30161@nevyn.them.org> <200602212050.k1LKowmP012208@elgar.sibelius.xs4all.nl> <20060221205748.GA31483@nevyn.them.org> <200602212134.k1LLY3Sq028067@elgar.sibelius.xs4all.nl> <20060221215216.GA440@nevyn.them.org> <200602222154.k1MLsOxa008001@elgar.sibelius.xs4all.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200602222154.k1MLsOxa008001@elgar.sibelius.xs4all.nl> User-Agent: Mutt/1.5.11+cvs20060403 X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-05/txt/msg00380.txt.bz2 [GDB 6.5 note: I have seen this bug reported roughly five times now. I think we need to fix it before the release.] On Wed, Feb 22, 2006 at 10:54:24PM +0100, Mark Kettenis wrote: > > I don't think it would help me though. Perhaps the real problem is the > > use of null_frame_id for both the outermost frame and completely > > unknown frames. It would be nice if we could tell here: > > > > if (frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id)) > > > > that frame_unwind_id has returned something completely invalid instead > > of the outermost frame > > Indeed. > > > One way to do that in our current representation would be to check that > > the frame ID for the current frame is not null_ptid. > > Yes, I think it makes sense to punt trying to insert a step-resume > breakpoint earlier than you do in your patch. First, an apology for confusing the issue. The frame_id_eq line posted there is actually OK for this specific problem: frame_id_eq where either argument is null_frame_id will always return false. It's actually the _next_ call to insert_step_resume_breakpoint_at_frame that blows up. Of course this means stepping in the next-to-outermost frame doesn't work quite right. That will be harder to fix, and see my post to gdb@ [no responses yet...] about the underlying issue of the outermost frame ID representation. This is a smaller problem than the segfault. A related issue I noticed while working on this; there is an unlikely potential segfault if the undebuggable function being stepped over is named "main". frame_unwind_id, called in the line quoted above, will successfully unwind past main; but get_prev_frame will refuse to. Thus we will pass NULL to insert_step_resume_breakpoint_at_frame. Accordingly, I think the best fix for now is a little code duplication. I've split the "set a breakpoint at the current frame" operation, used for signals, from the "set a breakpoint at the return address" operation, used for stepping over functions. I've also clarified and updated some comments. What do you think of this version? If you'd like to reproduce this, here's a recipe that works on AMD64; you'll need to adjust the 0x10 to the return column if you want to try i386, IIRC 0x8. I don't want to put this in the testsuite because of the nasty hack with .rodata1, which serves to make sure stop_func_name is NULL. Otherwise, GDB will incorrectly associate it with some earlier symbol. This will become easier to test if we teach GDB about symbol sizes and to not "extend" previous symbols :-) a.s === .globl main main: xorl %ebp, %ebp call foo ret foo: .cfi_startproc .cfi_escape 0x7, 0x10 xorl %ebp, %ebp call bar ret .cfi_endproc b.s === .section ".rodata1", "ax" .globl bar bar: .cfi_startproc .cfi_escape 0x7, 0x10 ret .cfi_endproc drow@caradoc:~% gcc -c b.s b.s: Assembler messages: b.s:1: Warning: setting incorrect section attributes for .rodata1 drow@caradoc:~% gcc -g -o a a.s b.o drow@caradoc:~% strip -N bar a drow@caradoc:~% gdb ./a GNU gdb 6.4.50.20060511-debian Copyright (C) 2006 Free Software Foundation, Inc. GDB is free software, covered by the GNU General Public License, and you are welcome to change it and/or distribute copies of it under certain conditions. Type "show copying" to see the conditions. There is absolutely no warranty for GDB. Type "show warranty" for details. This GDB was configured as "x86_64-linux-gnu"...Using host libthread_db library "/lib/libthread_db.so.1". (gdb) b foo Breakpoint 1 at 0x400430: file a.s, line 10. (gdb) r Starting program: /home/drow/a Breakpoint 1, foo () at a.s:10 10 xorl %ebp, %ebp Current language: auto; currently asm (gdb) s foo () at a.s:11 11 call bar (gdb) zsh: segmentation fault (core dumped) gdb ./a -- Daniel Jacobowitz CodeSourcery 2006-05-17 Daniel Jacobowitz * infrun.c (insert_step_resume_breakpoint_at_caller): New function, based on insert_step_resume_breakpoint_at_frame. (handle_inferior_event): Update comments. Use insert_step_resume_breakpoint_at_caller. (insert_step_resume_breakpoint_at_frame): Revise comments. Index: infrun.c =================================================================== RCS file: /cvs/src/src/gdb/infrun.c,v retrieving revision 1.210 diff -u -p -r1.210 infrun.c --- infrun.c 30 Mar 2006 16:37:13 -0000 1.210 +++ infrun.c 17 May 2006 15:57:54 -0000 @@ -943,6 +943,7 @@ void handle_inferior_event (struct execu static void step_into_function (struct execution_control_state *ecs); static void insert_step_resume_breakpoint_at_frame (struct frame_info *step_frame); +static void insert_step_resume_breakpoint_at_caller (struct frame_info *); static void insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal, struct frame_id sr_id); static void stop_stepping (struct execution_control_state *ecs); @@ -2367,9 +2368,13 @@ process_event_stop_test: return; } + /* Check for subroutine calls. + + NOTE: frame_id_eq will never report two invalid frame IDs as + being equal, so to get into this block, both the current and + previous frame must have valid frame IDs. */ if (frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id)) { - /* It's a subroutine call. */ CORE_ADDR real_stop_pc; if (debug_infrun) @@ -2396,7 +2401,7 @@ process_event_stop_test: /* We're doing a "next", set a breakpoint at callee's return address (the address at which the caller will resume). */ - insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ())); + insert_step_resume_breakpoint_at_caller (get_current_frame ()); keep_going (ecs); return; } @@ -2459,7 +2464,7 @@ process_event_stop_test: /* Set a breakpoint at callee's return address (the address at which the caller will resume). */ - insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ())); + insert_step_resume_breakpoint_at_caller (get_current_frame ()); keep_going (ecs); return; } @@ -2513,8 +2518,11 @@ process_event_stop_test: and no line number corresponding to the address where the inferior stopped). Since we want to skip this kind of code, we keep going until the inferior returns from this - function. */ - if (step_stop_if_no_debug) + function - unless the user has asked us not to (via + set step-mode) or we no longer know how to get back + to the call site. */ + if (step_stop_if_no_debug + || !frame_id_p (frame_unwind_id (get_current_frame ()))) { /* If we have no line number and the step-stop-if-no-debug is set, we stop the step so that the user has a chance to @@ -2528,7 +2536,7 @@ process_event_stop_test: { /* Set a breakpoint at callee's return address (the address at which the caller will resume). */ - insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ())); + insert_step_resume_breakpoint_at_caller (get_current_frame ()); keep_going (ecs); return; } @@ -2735,20 +2743,13 @@ insert_step_resume_breakpoint_at_sal (st if (breakpoints_inserted) insert_breakpoints (); } - + /* Insert a "step resume breakpoint" at RETURN_FRAME.pc. This is used - to skip a function (next, skip-no-debug) or signal. It's assumed - that the function/signal handler being skipped eventually returns - to the breakpoint inserted at RETURN_FRAME.pc. - - For the skip-function case, the function may have been reached by - either single stepping a call / return / signal-return instruction, - or by hitting a breakpoint. In all cases, the RETURN_FRAME belongs - to the skip-function's caller. - - For the signals case, this is called with the interrupted - function's frame. The signal handler, when it returns, will resume - the interrupted function at RETURN_FRAME.pc. */ + to skip a potential signal handler. + + This is called with the interrupted function's frame. The signal + handler, when it returns, will resume the interrupted function at + RETURN_FRAME.pc. */ static void insert_step_resume_breakpoint_at_frame (struct frame_info *return_frame) @@ -2763,6 +2764,38 @@ insert_step_resume_breakpoint_at_frame ( insert_step_resume_breakpoint_at_sal (sr_sal, get_frame_id (return_frame)); } +/* Similar to insert_step_resume_breakpoint_at_frame, except + but a breakpoint at the previous frame's PC. This is used to + skip a function after stepping into it (for "next" or if the called + function has no debugging information). + + The current function has almost always been reached by single + stepping a call or return instruction. NEXT_FRAME belongs to the + current function, and the breakpoint will be set at the caller's + resume address. + + This is a separate function rather than reusing + insert_step_resume_breakpoint_at_frame in order to avoid + get_prev_frame, which may stop prematurely (see the implementation + of frame_unwind_id for an example). */ + +static void +insert_step_resume_breakpoint_at_caller (struct frame_info *next_frame) +{ + struct symtab_and_line sr_sal; + + /* We shouldn't have gotten here if we don't know where the call site + is. */ + gdb_assert (frame_id_p (frame_unwind_id (next_frame))); + + init_sal (&sr_sal); /* initialize to zeros */ + + sr_sal.pc = ADDR_BITS_REMOVE (frame_pc_unwind (next_frame)); + sr_sal.section = find_pc_overlay (sr_sal.pc); + + insert_step_resume_breakpoint_at_sal (sr_sal, frame_unwind_id (next_frame)); +} + static void stop_stepping (struct execution_control_state *ecs) {