From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27174 invoked by alias); 29 May 2014 09:41:12 -0000 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 Received: (qmail 27163 invoked by uid 89); 29 May 2014 09:41:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 29 May 2014 09:41:10 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4T9f8Xm025091 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 29 May 2014 05:41:08 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s4T9f6cQ002849; Thu, 29 May 2014 05:41:07 -0400 Message-ID: <538700B2.50704@redhat.com> Date: Thu, 29 May 2014 09:41:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Andrew Burgess , gdb-patches@sourceware.org Subject: Re: [PATCH v3 4/4] Add a TRY_CATCH to get_prev_frame to better handle errors during unwind. References: <533EC5B7.6080600@broadcom.com> <1398855344-25278-1-git-send-email-aburgess@broadcom.com> <1398855344-25278-5-git-send-email-aburgess@broadcom.com> <53862B81.2070306@redhat.com> <538672C2.2080601@broadcom.com> In-Reply-To: <538672C2.2080601@broadcom.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-05/txt/msg00720.txt.bz2 On 05/29/2014 12:35 AM, Andrew Burgess wrote: > On 28/05/2014 7:31 PM, Pedro Alves wrote: >> On 04/30/2014 11:55 AM, Andrew Burgess wrote: >>> Here a new TRY_CATCH is added to the core of get_prev_frame, all uncaught >>> errors are turned into UNWIND_MISC_ERROR, and where possible the error >>> message associated with the error is stored as a frame specific stop reason >>> string. The reason string is held of the frame OBSTACK so it lives as long >>> as the frame does. >>> >>> There's a new function for getting the frame_stop_reason_string, this >>> replaces the now (thanks to patch #3) old frame_stop_reason_string >>> function, I know that reusing the name could confuse, but this function was >>> not widely used, so I hope that'll not be an issue. >> >> Sounds like we'll want to expose this to Python too. If you're not >> planning on doing that, could you file a PR once this goes in? > > I'm happy to add the new python (and even guile) functions, I figured > I'd post a follow up patch once these were merged ... if that's OK. That's great, thanks. > Commit message: > > Currently a MEMORY_ERROR raised during unwinding a frame will cause the > unwind to stop with an error message, for example: > > (gdb) bt > #0 breakpt () at amd64-invalid-stack-middle.c:27 > #1 0x00000000004008f0 in func5 () at amd64-invalid-stack-middle.c:32 > #2 0x0000000000400900 in func4 () at amd64-invalid-stack-middle.c:38 > #3 0x0000000000400910 in func3 () at amd64-invalid-stack-middle.c:44 > #4 0x0000000000400928 in func2 () at amd64-invalid-stack-middle.c:50 > Cannot access memory at address 0x2aaaaaab0000 > > However, frame #4 is marked as being the end of the stack unwind, so a > subsequent request for the backtrace looses the error message, such as: > > (gdb) bt > #0 breakpt () at amd64-invalid-stack-middle.c:27 > #1 0x00000000004008f0 in func5 () at amd64-invalid-stack-middle.c:32 > #2 0x0000000000400900 in func4 () at amd64-invalid-stack-middle.c:38 > #3 0x0000000000400910 in func3 () at amd64-invalid-stack-middle.c:44 > #4 0x0000000000400928 in func2 () at amd64-invalid-stack-middle.c:50 > > When fetching the backtrace, or requesting the stack depth using the MI > interface the situation is even worse, the first time a request is made > we encounter the memory error and so the MI returns an error instead of > the correct result, for example: > > (gdb) -stack-info-depth > ^error,msg="Cannot access memory at address 0x2aaaaaab0000" > > Or, > > (gdb) -stack-list-frames > ^error,msg="Cannot access memory at address 0x2aaaaaab0000" > > However, once one of these commands has been used gdb has, internally, > walked the stack and figured that out that frame #4 is the bottom of the > stack, so the second time an MI command is tried you'll get the "expected" > result: > > (gdb) -stack-info-depth > ^done,depth="5" > > Or, > > (gdb) -stack-list-frames > ^done,stack=[frame={level="0", .. snip lots .. }] Now here's an excellent description / commit message. Thanks! > > After this patch the MEMORY_ERROR encountered during the frame unwind is > attached to frame #4 as the stop reason, and is displayed in the CLI each > time the backtrace is requested. Please show examples of this in the commit log too. > In the MI, catching the error means that > the "expected" result is returned the first time the MI command is issued. > --- a/gdb/doc/python.texi > +++ b/gdb/doc/python.texi > @@ -3199,6 +3199,9 @@ stack corruption. > The frame unwinder did not find any saved PC, but we needed > one to unwind further. > > +@item gdb.FRAME_UNWIND_MEMORY_ERROR > +The frame unwinder caused an error while trying to access memory. > + > + /* The error needs to live as long as the frame does. */ > + stop_string = > + FRAME_OBSTACK_CALLOC (strlen (ex.message) + 1, char); #1 - Per GNU coding conventions, operators go on the next line, and '=' is an operator (assignment), so: stop_string = FRAME_OBSTACK_CALLOC (strlen (ex.message) + 1, char); #2 - sizeof (char) is 1 by definition, so no need for calloc, actually. And then that might fit on a single line: stop_string = FRAME_OBSTACK_ZALLOC (strlen (ex.message) + 1); > + strcpy (stop_string, ex.message); > + this_frame->stop_string = stop_string; But even better we could do this instead: size_t size; size = strlen (ex.message) + 1; this_frame->stop_string = FRAME_OBSTACK_ZALLOC (size); memcpy (this_frame->stop_string, ex.message, size); > + } > + prev_frame = NULL; > + } > + else > + throw_exception (ex); > + } > + > + return prev_frame; > +} > + > /* Construct a new "struct frame_info" and link it previous to > this_frame. */ > > @@ -2576,6 +2618,24 @@ unwind_frame_stop_reason_string (enum unwind_stop_reason reason) > } > } > > +/* Return a possibly frame specific string explaining why the unwind > + stopped here. Should only be called for frames that don't have a > + previous frame. If there's no specific reason stored for a frame then > + a generic reason string will be returned. */ > +const char *frame_stop_reason_string (struct frame_info *); Let's include an example to make it clearer: /* Return a possibly frame specific string explaining why the unwind stopped here. E.g., if unwinding tripped on a memory error, this will return the error description string, which includes the address that we failed to access. If there's no specific reason stored for a frame then a generic reason string will be returned. Should only be called for frames that don't have a previous frame. */ (I imagine that this "Should" will need to be addressed when we expose this to Python somehow. We wouldn't want failure to comply to that to cause an internal error.) > +/* Return a possibly frame specific string explaining why the unwind > + stopped at frame FI. Must only be called if there is no previous > + frame. */ Let's just remove this comment, thus avoiding maintaining the same description in two places. Note it was already divergent from the comment in the header. > + > +const char * > +frame_stop_reason_string (struct frame_info *fi) > +{ > + gdb_assert (fi->prev_p); > + gdb_assert (fi->prev == NULL); > + > + /* Return the specific string if we have one. */ > + if (fi->stop_string) > + return fi->stop_string; if (fi->stop_string != NULL) return fi->stop_string; Otherwise looks good. Thanks, Pedro Alves