| Message ID | 1353518931.17833.181.camel@otta |
|---|---|
| State | New |
| Headers | show |
From: Peter Bergner <bergner@vnet.ibm.com> Date: Wed, 21 Nov 2012 11:28:51 -0600 > On Tue, 2012-11-20 at 23:19 -0500, David Miller wrote: >> The address violation detection seems to work properly and the only >> thing that seems to be left are some backtrace/unwind issues. These >> are perhaps similar to the unwind bits that the powerpc folks ran >> into. > > David, does the following patch (will have some fuzz since I removed > one ppc only hunk from the patch) fix your backtrace issue? I'll note > you'll have to add "|| defined(__sparc__)" to the #if ... or as > it's probably going to turn out, just replace the whole thing > with a "#if !defined(__i386__) && !defined(__x86_64__)". This patch works well but I have some unrelated sanitizer sparc issues to resolve before the testcase will pass properly. Feel free to submit this with the __sparc__ cpp test added, or the !x86 variant, at your discretion.
From: David Miller <davem@davemloft.net> Date: Wed, 21 Nov 2012 12:54:17 -0500 (EST) > From: Peter Bergner <bergner@vnet.ibm.com> > Date: Wed, 21 Nov 2012 11:28:51 -0600 > >> On Tue, 2012-11-20 at 23:19 -0500, David Miller wrote: >>> The address violation detection seems to work properly and the only >>> thing that seems to be left are some backtrace/unwind issues. These >>> are perhaps similar to the unwind bits that the powerpc folks ran >>> into. >> >> David, does the following patch (will have some fuzz since I removed >> one ppc only hunk from the patch) fix your backtrace issue? I'll note >> you'll have to add "|| defined(__sparc__)" to the #if ... or as >> it's probably going to turn out, just replace the whole thing >> with a "#if !defined(__i386__) && !defined(__x86_64__)". > > This patch works well but I have some unrelated sanitizer sparc > issues to resolve before the testcase will pass properly. > > Feel free to submit this with the __sparc__ cpp test added, or > the !x86 variant, at your discretion. Actually I looked more closely at this, and the trigger is hit one stack frame too late on sparc. The BP computed in the memcmp() interceptor is the frame pointer %fp, but on sparc that's the CFA of the caller, main() in the case of the memcmp-1.c testcase. So only main() appears in the backtrace. It might be easier to implement this by comparing the PC instead. And it also occurs to me that we probably need to be using __builtin_extract_return_addr() when recording the PC at the error trigger point.
On Wed, Nov 21, 2012 at 03:27:16PM -0500, David Miller wrote: > Actually I looked more closely at this, and the trigger is hit one > stack frame too late on sparc. Are you testing with -fno-builtin-memcmp? Without it the check is done directly in main... Jakub
From: Jakub Jelinek <jakub@redhat.com> Date: Wed, 21 Nov 2012 21:30:37 +0100 > On Wed, Nov 21, 2012 at 03:27:16PM -0500, David Miller wrote: >> Actually I looked more closely at this, and the trigger is hit one >> stack frame too late on sparc. > > Are you testing with -fno-builtin-memcmp? Without it the check is done > directly in main... I made sure that the error triggered in the ASAN memcmp interceptor, please read the paragraph after the one you are quoting.
On Wed, 2012-11-21 at 15:27 -0500, David Miller wrote: > Actually I looked more closely at this, and the trigger is hit one > stack frame too late on sparc. > > The BP computed in the memcmp() interceptor is the frame pointer > %fp, but on sparc that's the CFA of the caller, main() in the > case of the memcmp-1.c testcase. > > So only main() appears in the backtrace. > > It might be easier to implement this by comparing the PC instead. > > And it also occurs to me that we probably need to be using > __builtin_extract_return_addr() when recording the PC at the > error trigger point. If you have a suggested change/patch that does that, let me know and I can try it out on powerpc to make sure it works for us too. Peter
From: Peter Bergner <bergner@vnet.ibm.com> Date: Wed, 21 Nov 2012 17:46:57 -0600 > If you have a suggested change/patch that does that, let me know > and I can try it out on powerpc to make sure it works for us too. I will try to do so, but I am pretty sure at this point that it will end up being some time early next week.
On 2012-11-21 11:28, Peter Bergner wrote:
> + if (Unwind_GetBP(ctx) == p->bp) {
I've mentioned multiple times that BP is unusable on most RISC.
You need to be looking at SP.
r~
Index: libsanitizer/asan/asan_linux.cc =================================================================== --- libsanitizer/asan/asan_linux.cc (revision 193678) +++ libsanitizer/asan/asan_linux.cc (working copy) @@ -134,11 +141,27 @@ #endif } +uptr Unwind_GetBP(struct _Unwind_Context *ctx) { + return _Unwind_GetCFA(ctx); +} + +struct Unwind_Trace_Info { + StackTrace *stack; + uptr bp; +}; + _Unwind_Reason_Code Unwind_Trace(struct _Unwind_Context *ctx, void *param) { - StackTrace *b = (StackTrace*)param; + Unwind_Trace_Info *p = (Unwind_Trace_Info *)param; + StackTrace *b = p->stack; + uptr pc = Unwind_GetIP(ctx); + if (Unwind_GetBP(ctx) == p->bp) { + // We just encountered the frame pointer we want to start + // our backtrace with, so empty the backtrace before adding + // this frame to the backtrace. + b->size = 0; + } CHECK(b->size < b->max_size); - uptr pc = Unwind_GetIP(ctx); b->trace[b->size++] = pc; if (b->size == b->max_size) return UNWIND_STOP; return UNWIND_CONTINUE; @@ -149,8 +172,11 @@ stack->trace[0] = pc; if ((max_s) > 1) { stack->max_size = max_s; -#ifdef __arm__ - _Unwind_Backtrace(Unwind_Trace, stack); +#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__) + Unwind_Trace_Info param; + param.stack = stack; + param.bp = bp; + _Unwind_Backtrace(Unwind_Trace, ¶m); #else if (!asan_inited) return; if (AsanThread *t = asanThreadRegistry().GetCurrent())