From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3521 invoked by alias); 5 Jun 2012 19:17:01 -0000 Received: (qmail 3506 invoked by uid 22791); 5 Jun 2012 19:17:00 -0000 X-SWARE-Spam-Status: No, hits=-7.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 05 Jun 2012 19:16:38 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q55JGck2013214 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 5 Jun 2012 15:16:38 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q55JGbcR022853; Tue, 5 Jun 2012 15:16:37 -0400 Message-ID: <4FCE5B14.4030808@redhat.com> Date: Tue, 05 Jun 2012 19:17:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 MIME-Version: 1.0 To: Jan Kratochvil CC: gdb-patches@sourceware.org Subject: Re: [downstream patch FYI] workaround stale frame_info * (PR 13866) References: <20120404191416.GA29603@host2.jankratochvil.net> In-Reply-To: <20120404191416.GA29603@host2.jankratochvil.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2012-06/txt/msg00151.txt.bz2 On 04/04/2012 08:14 PM, Jan Kratochvil wrote: > Hi, > > I did not look at which commit caused this regression but apparently it was > introduced at least with multi-inferiors. > > I understand this fix is not right fix of the crash; but in most GDB cases one > does not use multi-inferior so why to regress single-inferior by it. > Some more simple solutions still fix the single-inferior mode but they > regressed the multi-inferior mode > gdb.threads/no-unwaited-for-left.exp > gdb.multi/base.exp > so I had to put there that sorting magic. > > With proper C++ sanity check of stale live frame_info references the testcase > would be simple without the "frame_garbage_collection" reproducer below. > It is also reproducible just with valgrind but regularly running the whole > testsuite under valgrind I did not find feasible. > > No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu. > I've used this frame validation patch, based on yours, which sprinkles validate_frame calls around frame.c to catch uses of already dead frame_info objects. It makes the "until" bug 100% reproducible (with an internal_error) without valgrind. I've then ran the whole testsuite with this on, and that didn't catch any other problem. You mention in PR13866 many situations with stale frame_info; did you have some other way to catch those, or was that just a hunch? --- gdb/frame.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- gdb/top.c | 5 ++++ 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index e012f2d..285b60d 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -129,6 +129,14 @@ struct frame_info enum unwind_stop_reason stop_reason; }; +#define DEAD_FRAME_LEVEL (-55) + +static void +validate_frame (struct frame_info *frame) +{ + gdb_assert (frame->level != DEAD_FRAME_LEVEL); +} + /* A frame stash used to speed up frame lookups. */ /* We currently only stash one frame at a time, as this seems to be @@ -324,6 +332,8 @@ get_frame_id (struct frame_info *fi) if (fi == NULL) return null_frame_id; + validate_frame (fi); + if (!fi->this_id.p) { if (frame_debug) @@ -621,6 +631,8 @@ frame_find_by_id (struct frame_id id) static int frame_unwind_pc_if_available (struct frame_info *this_frame, CORE_ADDR *pc) { + validate_frame (this_frame); + if (!this_frame->prev_pc.p) { if (gdbarch_unwind_pc_p (frame_unwind_arch (this_frame))) @@ -1522,12 +1534,30 @@ frame_observer_target_changed (struct target_ops *target) reinit_frame_cache (); } +typedef struct obstack obstack_s; +DEF_VEC_O (obstack_s); +static VEC (obstack_s) *frame_poison_vec; + +void frame_garbage_collection (void); +void +frame_garbage_collection (void) +{ + struct obstack *obstack_p; + int ix; + + for (ix = 0; VEC_iterate (obstack_s, frame_poison_vec, ix, obstack_p); ix++) + obstack_free (obstack_p, 0); + + VEC_free (obstack_s, frame_poison_vec); + frame_poison_vec = NULL; +} + /* Flush the entire frame cache. */ void reinit_frame_cache (void) { - struct frame_info *fi; + struct frame_info *fi, *fi_prev; /* Tear down all frame caches. */ for (fi = current_frame; fi != NULL; fi = fi->prev) @@ -1538,8 +1568,15 @@ reinit_frame_cache (void) fi->base->unwind->dealloc_cache (fi, fi->base_cache); } + for (fi = current_frame; fi != NULL; fi = fi_prev) + { + fi_prev = fi->prev; + memset (fi, 0, sizeof (*fi)); + fi->level = DEAD_FRAME_LEVEL; + } + VEC_safe_push (obstack_s, frame_poison_vec, &frame_cache_obstack); + /* Since we can't really be sure what the first object allocated was. */ - obstack_free (&frame_cache_obstack, 0); obstack_init (&frame_cache_obstack); if (current_frame != NULL) @@ -2070,6 +2107,8 @@ get_frame_address_in_block_if_available (struct frame_info *this_frame, { volatile struct gdb_exception ex; + validate_frame (this_frame); + TRY_CATCH (ex, RETURN_MASK_ERROR) { *pc = get_frame_address_in_block (this_frame); @@ -2089,6 +2128,8 @@ find_frame_sal (struct frame_info *frame, struct symtab_and_line *sal) int notcurrent; CORE_ADDR pc; + validate_frame (frame); + /* If the next frame represents an inlined function call, this frame's sal is the "call site" of that inlined function, which can not be inferred from get_frame_pc. */ @@ -2145,6 +2186,8 @@ find_frame_sal (struct frame_info *frame, struct symtab_and_line *sal) CORE_ADDR get_frame_base (struct frame_info *fi) { + validate_frame (fi); + return get_frame_id (fi).stack_addr; } @@ -2220,6 +2263,8 @@ frame_relative_level (struct frame_info *fi) enum frame_type get_frame_type (struct frame_info *frame) { + validate_frame (frame); + if (frame->unwind == NULL) /* Initialize the frame's unwinder because that's what provides the frame's type. */ @@ -2230,6 +2275,8 @@ get_frame_type (struct frame_info *frame) struct program_space * get_frame_program_space (struct frame_info *frame) { + validate_frame (frame); + return frame->pspace; } @@ -2238,6 +2285,8 @@ frame_unwind_program_space (struct frame_info *this_frame) { gdb_assert (this_frame); + validate_frame (this_frame); + /* This is really a placeholder to keep the API consistent --- we assume for now that we don't have frame chains crossing spaces. */ @@ -2247,6 +2296,8 @@ frame_unwind_program_space (struct frame_info *this_frame) struct address_space * get_frame_address_space (struct frame_info *frame) { + validate_frame (frame); + return frame->aspace; } @@ -2256,6 +2307,8 @@ void get_frame_memory (struct frame_info *this_frame, CORE_ADDR addr, gdb_byte *buf, int len) { + validate_frame (this_frame); + read_memory (addr, buf, len); } @@ -2266,6 +2319,8 @@ get_frame_memory_signed (struct frame_info *this_frame, CORE_ADDR addr, struct gdbarch *gdbarch = get_frame_arch (this_frame); enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); + validate_frame (this_frame); + return read_memory_integer (addr, len, byte_order); } @@ -2276,6 +2331,8 @@ get_frame_memory_unsigned (struct frame_info *this_frame, CORE_ADDR addr, struct gdbarch *gdbarch = get_frame_arch (this_frame); enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); + validate_frame (this_frame); + return read_memory_unsigned_integer (addr, len, byte_order); } @@ -2283,6 +2340,8 @@ int safe_frame_unwind_memory (struct frame_info *this_frame, CORE_ADDR addr, gdb_byte *buf, int len) { + validate_frame (this_frame); + /* NOTE: target_read_memory returns zero on success! */ return !target_read_memory (addr, buf, len); } @@ -2292,12 +2351,15 @@ safe_frame_unwind_memory (struct frame_info *this_frame, struct gdbarch * get_frame_arch (struct frame_info *this_frame) { + validate_frame (this_frame); return frame_unwind_arch (this_frame->next); } struct gdbarch * frame_unwind_arch (struct frame_info *next_frame) { + validate_frame (next_frame); + if (!next_frame->prev_arch.p) { struct gdbarch *arch; @@ -2326,6 +2388,8 @@ frame_unwind_arch (struct frame_info *next_frame) struct gdbarch * frame_unwind_caller_arch (struct frame_info *next_frame) { + validate_frame (next_frame); + return frame_unwind_arch (skip_inlined_frames (next_frame)); } @@ -2336,6 +2400,8 @@ get_frame_sp (struct frame_info *this_frame) { struct gdbarch *gdbarch = get_frame_arch (this_frame); + validate_frame (this_frame); + /* Normality - an architecture that provides a way of obtaining any frame inner-most address. */ if (gdbarch_unwind_sp_p (gdbarch)) @@ -2355,6 +2421,8 @@ get_frame_sp (struct frame_info *this_frame) enum unwind_stop_reason get_frame_unwind_stop_reason (struct frame_info *frame) { + validate_frame (frame); + /* If we haven't tried to unwind past this point yet, then assume that unwinding would succeed. */ if (frame->prev_p == 0) diff --git a/gdb/top.c b/gdb/top.c index 061ad48..d797aff 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -360,6 +360,11 @@ prepare_execute_command (void) if (non_stop) target_dcache_invalidate (); + { + extern void frame_garbage_collection (void); + frame_garbage_collection (); + } + return cleanup; }