From owner-freebsd-arm@FreeBSD.ORG Fri May 3 11:12:07 2013 Return-Path: Delivered-To: freebsd-arm@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id C7339A2B; Fri, 3 May 2013 11:12:07 +0000 (UTC) (envelope-from andrew@fubar.geek.nz) Received: from lon1-post-3.mail.demon.net (lon1-post-3.mail.demon.net [195.173.77.150]) by mx1.freebsd.org (Postfix) with ESMTP id 96D0217B4; Fri, 3 May 2013 11:12:07 +0000 (UTC) Received: from bostonbath-adsl.demon.co.uk ([80.176.134.48] helo=bender) by lon1-post-3.mail.demon.net with esmtp (Exim 4.69) id 1UYDaX-0002Cv-ea; Fri, 03 May 2013 10:51:49 +0000 Date: Fri, 3 May 2013 11:51:41 +0100 From: Andrew Turner To: Ian Lepore Subject: Re: A fix for the clang + eabi + kdb backtrace endless loop Message-ID: <20130503115141.6c9cd15a@bender> In-Reply-To: <1367439452.1180.92.camel@revolution.hippie.lan> References: <1367439452.1180.92.camel@revolution.hippie.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: freebsd-arm X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Porting FreeBSD to the StrongARM Processor List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 May 2013 11:12:07 -0000 On Wed, 01 May 2013 14:17:32 -0600 Ian Lepore wrote: > The attached patch fixes the problem where a kdb backtrace loops > endlessly on an eabi kernel. > > I'm no expert on this stuff, so while this fixes the problem for me, > I'm not sure it's right, especially the STOP_UNWINDING in > exception_exit (which handles a test case of breaking into the > debugger with the keyboard and typing 'bt'). I'm not going to commit > this until it's been reviewed by someone who actually knows what > they're doing. :) STOP_UNWINDING tells binutils to generate the required unwind op-code to tell the kernel's unwinder it needs to stop in this function. There is a bug with the unwinder where it should tell the user it is in the function, however it doesn't currently. See below for more comments. > Index: sys/arm/arm/db_trace.c > =================================================================== > --- sys/arm/arm/db_trace.c (revision 248960) > +++ sys/arm/arm/db_trace.c (working copy) > @@ -354,7 +354,7 @@ > index = db_find_index(state->start_pc); > > if (index->insn == EXIDX_CANTUNWIND) { > - db_printf("Unable to unwind\n"); > + db_printf("Unable to unwind further\n"); > break; > } else if (index->insn & (1 << 31)) { > /* The data is within the instruction */ > @@ -412,6 +412,23 @@ > } > } > db_printf("\n"); > + > + /* Stop if we've unwound back to the thread entry > point. */ > + if (state->registers[FP] == 0) { > + break; > + } This appears to be wrong, FP is not used with EABI, as such it is valid for it to be 0 at this point in the code. > + > + /* > + * Stop if the unwind function didn't change the PC, > to avoid > + * getting stuck in this loop forever. If this > happens, it's an > + * indication that the unwind information is > incorrect somehow > + * for the function named in the last frame printed > before you > + * see this message. > + */ > + if (state->start_pc == state->registers[PC]) { > + db_printf("Unwind failure (PC didn't > change)\n"); I'm not sure about this change, it may be hit in a recursive function. We should also check if sp changes. It is still possible to write code that defeats this check but this should be good enough (tm). Whether there should be recursive functions in the kernel is a different question. > + break; > + } > } > } > #endif > Index: sys/arm/arm/exception.S > =================================================================== > --- sys/arm/arm/exception.S (revision 248960) > +++ sys/arm/arm/exception.S (working copy) > @@ -196,15 +196,19 @@ > * Interrupts are disabled at suitable points to avoid ASTs > * being posted between testing and exit to user mode. > * > - * This function uses PULLFRAMEFROMSVCANDEXIT and > - * DO_AST > - * only be called if the exception handler used PUSHFRAMEINSVC > + * This function uses PULLFRAMEFROMSVCANDEXIT and DO_AST and can > + * only be called if the exception handler used PUSHFRAMEINSVC. > * > + * For EABI, don't try to unwind any further than this, because the > unwind > + * info generated here is a single INSN_FINISH instruction. Nothing > gets > + * unwound (no registers change) so the unwind would loop here > forever. > */ > > -exception_exit: > +ASENTRY_NP(exception_exit) > + STOP_UNWINDING > DO_AST > PULLFRAMEFROMSVCANDEXIT > +END(exception_exit) This looks good for now. We may be able to add the required unwind directives [1] to the PULLFRAMEFROMSVCANDEXIT macro at a later date. The directives will need to be handled in a similar way to STOP_UNWINDING as we should only add them in the EABI and debug case. Andrew [1] http://sourceware.org/binutils/docs/as/ARM-Directives.html