Patchwork Sparc ASAN

login
register
mail settings
Submitter Peter Bergner
Date Nov. 21, 2012, 5:28 p.m.
Message ID <1353518931.17833.181.camel@otta>
Download mbox | patch
Permalink /patch/200815/
State New
Headers show

Comments

Peter Bergner - Nov. 21, 2012, 5:28 p.m.
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__)".

Peter
David Miller - Nov. 21, 2012, 5:54 p.m.
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.
David Miller - Nov. 21, 2012, 8:27 p.m.
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.
Jakub Jelinek - Nov. 21, 2012, 8:30 p.m.
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
David Miller - Nov. 21, 2012, 8:32 p.m.
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.
Peter Bergner - Nov. 21, 2012, 11:46 p.m.
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
David Miller - Nov. 22, 2012, 4:35 a.m.
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.
Richard Henderson - Dec. 3, 2012, 11:06 p.m.
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~

Patch

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, &param);
 #else
     if (!asan_inited) return;
     if (AsanThread *t = asanThreadRegistry().GetCurrent())