Last Comment Bug 724079 - Use the mac profiler backend again
: Use the mac profiler backend again
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Jeff Muizelaar [:jrmuizel]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-03 12:56 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-02-13 08:53 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add an internal stackwalk API (4.81 KB, patch)
2012-02-03 12:57 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Bring the mac backend back (11.04 KB, patch)
2012-02-03 12:58 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Add an internal stackwalk API v2 (5.13 KB, text/plain)
2012-02-03 13:04 PST, Jeff Muizelaar [:jrmuizel]
no flags Details
Add an internal stackwalk API v3 (5.83 KB, patch)
2012-02-04 10:17 PST, Jeff Muizelaar [:jrmuizel]
ehsan: review+
Details | Diff | Splinter Review
Bring the mac backend back v2 (11.48 KB, patch)
2012-02-04 10:18 PST, Jeff Muizelaar [:jrmuizel]
ehsan: review-
Details | Diff | Splinter Review
Bring the mac backend back v3 (11.80 KB, patch)
2012-02-10 08:02 PST, Jeff Muizelaar [:jrmuizel]
ehsan: review+
Details | Diff | Splinter Review
Add an internal stackwalk API v4 (5.72 KB, patch)
2012-02-10 08:03 PST, Jeff Muizelaar [:jrmuizel]
ehsan: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-02-03 12:56:32 PST
I'm hoping this will fix up the crash problems we've been seeing.
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-02-03 12:57:45 PST
Created attachment 594277 [details] [diff] [review]
Add an internal stackwalk API

This will be used by the profiler to be able to unwind arbitrary threads.
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-02-03 12:58:29 PST
Created attachment 594278 [details] [diff] [review]
Bring the mac backend back
Comment 3 Jeff Muizelaar [:jrmuizel] 2012-02-03 13:04:00 PST
Created attachment 594279 [details]
Add an internal stackwalk API v2
Comment 4 Jeff Muizelaar [:jrmuizel] 2012-02-04 10:17:36 PST
Created attachment 594468 [details] [diff] [review]
Add an internal stackwalk API v3
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-02-04 10:18:46 PST
Created attachment 594469 [details] [diff] [review]
Bring the mac backend back v2

This patches actually work now.
Comment 6 Jeff Muizelaar [:jrmuizel] 2012-02-04 18:02:18 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> Created attachment 594469 [details] [diff] [review]
> Bring the mac backend back v2
> 
> This patches actually work now.

I had an unrefreshed change to make this use sample->fp instead of pc
Comment 7 :Ehsan Akhgari 2012-02-06 11:06:18 PST
Comment on attachment 594468 [details] [diff] [review]
Add an internal stackwalk API v3

Review of attachment 594468 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/StackWalk.h
@@ +12,5 @@
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Initial Developer of the Original Code is the Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2011

2012!

::: xpcom/base/nsStackWalk.cpp
@@ +1573,4 @@
>  
>  #else // not __sun-specific
>  
> +#if (defined(__i386) || defined(__ppc__) || defined(PPC) || defined(__amd64)) && defined(__GNUC__)

Please move the X86_OR_PPC definition back up here and change this to use that macro.
Comment 8 :Ehsan Akhgari 2012-02-06 11:17:05 PST
Comment on attachment 594469 [details] [diff] [review]
Bring the mac backend back v2

Review of attachment 594469 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/sps/TableTicker.cpp
@@ +497,4 @@
>    }
>  }
>  #endif
> +*/

This function will still be used on Linux right?  Not sure why you're removing it.  I'm pretty sure this breaks Linux builds!

@@ +501,2 @@
>  
> +//#ifdef USE_NS_STACKWALK

This will also break Linux!

@@ +529,5 @@
>      mozilla::ArrayLength(pc_array),
>      0
>    };
> +#ifdef XP_MACOSX
> +  nsresult rv = FramePointerStackWalk(StackWalkCallback, 1, &array, reinterpret_cast<void**>(pc));

How is the size of this buffer communicated to FramePointerStackWalk?  As far as I can tell, we're just hoping that we won't be more than 1000 frames deep in the stack, right?

@@ +542,4 @@
>      }
>    }
>  }
> +//#endif

See above.
Comment 9 Jeff Muizelaar [:jrmuizel] 2012-02-10 08:02:26 PST
Created attachment 596052 [details] [diff] [review]
Bring the mac backend back v3

Address review comments
Comment 10 Jeff Muizelaar [:jrmuizel] 2012-02-10 08:03:05 PST
Created attachment 596053 [details] [diff] [review]
Add an internal stackwalk API v4

Note You need to log in before you can comment on or make changes to this bug.