Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 3 May 2013 11:51:41 +0100
From:      Andrew Turner <andrew@fubar.geek.nz>
To:        Ian Lepore <ian@FreeBSD.org>
Cc:        freebsd-arm <freebsd-arm@FreeBSD.org>
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>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 01 May 2013 14:17:32 -0600
Ian Lepore <ian@FreeBSD.org> 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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130503115141.6c9cd15a>