From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13344 invoked by alias); 19 Aug 2006 16:11:50 -0000 Received: (qmail 13334 invoked by uid 22791); 19 Aug 2006 16:11:48 -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; Sat, 19 Aug 2006 16:11:41 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1GETQB-0006m1-Bw for gdb-patches@sourceware.org; Sat, 19 Aug 2006 12:11:39 -0400 Date: Sun, 20 Aug 2006 13:58:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sourceware.org Subject: [rfc, frame] Always check for unsaved PC Message-ID: <20060819161139.GC25238@nevyn.them.org> Mail-Followup-To: gdb-patches@sourceware.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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-08/txt/msg00134.txt.bz2 This patch, combined with my previous two frame patches for today, fixes the infinite backtrace Olav Zarges reported on gdb@. Infinite backtraces are a funny phenomenon, which I've seen on a lot of platforms in the time I've been working on GDB. Consider a function prologue which creates a stack frame, but does not store the PC into the frame - for instance, because the compiler knows it will never return. Then it calls some other function, clobbering the return register. A hapless prologue unwinder may report "the stack frame size is 200 bytes, and the saved PC still lives in LR" not realizing that LR's been clobbered. If there were no stack frame, this would trigger the duplicate frame ID check, but there is; the function remains the same but the CFA moves continually down, and the frame repeats endlessly. Here's a simple way to detect this. Suppose we have two normal frames in a row. Unwind PC_REGNUM from both of them. Two functions having the same return address isn't a problem. But two functions sharing a stack slot for the return address is a big problem; that never happens normally, since the first one would pop it. So this indicates that the older frame didn't actually save it. This isn't a perfect check, since it relies on PC_REGNUM, as opposed to gdbarch_unwind_pc and frame_unwind's prev_pc, but it's still safe; if the PC_REGNUM does not map onto a real and saved register, then it won't end up with lval_memory. There's one wart in this patch. frame_register_unwind doesn't tell us where a register was ultimately stored; it recurses for the register value, but fills in *optimizedp, *lvalp, *realnump, *addrp for where _this frame_ saved it. Which is reasonable and useful behavior. So, we need to iterate ourselves in order to find the final location. There's an associated FIXME; I discovered that three targets don't do it this way. That doesn't break this patch, but it's inconsistent, and does a bit of extra computation. So if there's general agreement that this patch is a good idea, I'll go through and fix them, and try to document more clearly that you aren't supposed to do it that way. Then I can remove the FIXME. Performance-wise: for frames past the current frame, this causes us to do an additional two register unwinds. That's not too expensive, and we're usually backtracing anyway by that point. I couldn't measure a performance impact. I don't think we really cache this information, but if this code ever becomes a performance issue somehow, that's how it should be fixed: per-frame unwound register caching. Any thoughts on this patch? -- Daniel Jacobowitz CodeSourcery 2006-08-19 Daniel Jacobowitz * frame.c (frame_register_unwind_location): New function. (get_prev_frame_1): Check for UNWIND_NO_SAVED_PC. (frame_stop_reason_string): Handle UNWIND_NO_SAVED_PC. * frame.h (enum unwind_stop_reason): Add UNWIND_NO_SAVED_PC. --- gdb/frame.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ gdb/frame.h | 4 +++ 2 files changed, 71 insertions(+) Index: src/gdb/frame.c =================================================================== --- src.orig/gdb/frame.c 2006-08-19 11:33:33.000000000 -0400 +++ src/gdb/frame.c 2006-08-19 11:38:43.000000000 -0400 @@ -1023,6 +1023,34 @@ reinit_frame_cache (void) } } +/* Find where a register is saved (in memory or another register). + The result of frame_register_unwind is just where it is saved + relative to this particular frame. + + FIXME: alpha, m32c, and h8300 actually do the transitive operation + themselves. */ + +static void +frame_register_unwind_location (struct frame_info *this_frame, int regnum, + int *optimizedp, enum lval_type *lvalp, + CORE_ADDR *addrp, int *realnump) +{ + while (this_frame->level >= 0) + { + frame_register_unwind (this_frame, regnum, optimizedp, lvalp, + addrp, realnump, NULL); + + if (*optimizedp) + break; + + if (*lvalp != lval_register) + break; + + regnum = *realnump; + this_frame = get_next_frame (this_frame); + } +} + /* Return a "struct frame_info" corresponding to the frame that called THIS_FRAME. Returns NULL if there is no such frame. @@ -1077,6 +1105,42 @@ get_prev_frame_1 (struct frame_info *thi return NULL; } + /* Check that this and the next frame do not unwind the PC register + to the same memory location. If they do, then even though they + have different frame IDs, the new frame will be bogus; two + functions can't share a register save slot for the PC. This can + happen when the prologue analyzer finds a stack adjustment, but + no PC save. This check does assume that the "PC register" is + roughly a traditional PC, even if the gdbarch_unwind_pc method + frobs it. */ + if (this_frame->level > 0 + && get_frame_type (this_frame) == NORMAL_FRAME + && get_frame_type (this_frame->next) == NORMAL_FRAME) + { + int optimized, realnum; + enum lval_type lval, nlval; + CORE_ADDR addr, naddr; + + frame_register_unwind_location (this_frame, PC_REGNUM, &optimized, + &lval, &addr, &realnum); + frame_register_unwind_location (get_next_frame (this_frame), PC_REGNUM, + &optimized, &nlval, &naddr, &realnum); + + if (lval == lval_memory && lval == nlval && addr == naddr) + { + if (frame_debug) + { + fprintf_unfiltered (gdb_stdlog, "-> "); + fprint_frame (gdb_stdlog, NULL); + fprintf_unfiltered (gdb_stdlog, " // no saved PC }\n"); + } + + this_frame->stop_reason = UNWIND_NO_SAVED_PC; + this_frame->prev = NULL; + return NULL; + } + } + /* Allocate the new frame but do not wire it in to the frame chain. Some (bad) code in INIT_FRAME_EXTRA_INFO tries to look along frame->next to pull some fancy tricks (of course such code is, by @@ -1623,6 +1687,9 @@ frame_stop_reason_string (enum unwind_st case UNWIND_SAME_ID: return _("previous frame identical to this frame (corrupt stack?)"); + case UNWIND_NO_SAVED_PC: + return _("frame did not save the PC"); + case UNWIND_NO_REASON: case UNWIND_FIRST_ERROR: default: Index: src/gdb/frame.h =================================================================== --- src.orig/gdb/frame.h 2006-08-19 11:29:33.000000000 -0400 +++ src/gdb/frame.h 2006-08-19 11:35:41.000000000 -0400 @@ -428,6 +428,10 @@ enum unwind_stop_reason this is a sign of unwinder failure. It could also indicate stack corruption. */ UNWIND_SAME_ID, + + /* The frame unwinder didn't find any saved PC, but we needed + one to unwind further. */ + UNWIND_NO_SAVED_PC, }; /* Return the reason why we can't unwind past this frame. */