Last Comment Bug 718025 - Add support for stacktraces on Windows to the built-in profiler
: Add support for stacktraces on Windows to the built-in profiler
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: :Ehsan Akhgari (out sick)
:
Mentors:
Depends on:
Blocks: 713227
  Show dependency treegraph
 
Reported: 2012-01-13 12:24 PST by :Ehsan Akhgari (out sick)
Modified: 2012-01-17 17:52 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (4.61 KB, patch)
2012-01-13 12:25 PST, :Ehsan Akhgari (out sick)
jmuizelaar: review-
Details | Diff | Review
Patch (v2) (4.83 KB, patch)
2012-01-16 13:16 PST, :Ehsan Akhgari (out sick)
jmuizelaar: review+
Details | Diff | Review
Patch (v2) which actually compiles (4.83 KB, patch)
2012-01-17 08:47 PST, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review

Description :Ehsan Akhgari (out sick) 2012-01-13 12:24:24 PST
I have a patch which enables us to walk the stack on Windows.  The symbolication part is missing, so it's not quite useful yet, but stepping through the code, things seem to be working fine.
Comment 1 :Ehsan Akhgari (out sick) 2012-01-13 12:25:13 PST
Created attachment 588491 [details] [diff] [review]
Patch (v1)
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-01-16 12:45:19 PST
Comment on attachment 588491 [details] [diff] [review]
Patch (v1)

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

::: tools/profiler/sps/TableTicker.cpp
@@ +361,5 @@
>  }
>  #endif
>  
> +#ifdef USE_NS_STACKWALK
> +typedef nsTArray<void*> PCArray;

I don't think you can safely use an nsTArray when a thread is paused.
nsTArray will do mallocs which will deadlock if the stopped thread is inside malloc.

::: tools/profiler/sps/platform.h
@@ +204,5 @@
>    PlatformData* platform_data() { return data_; }
>  
> +#ifdef XP_WIN
> +  // xxxehsan sucky hack :(
> +  static uintptr_t GetThreadHandle(PlatformData*);

gross
Comment 3 :Ehsan Akhgari (out sick) 2012-01-16 13:16:55 PST
Created attachment 588984 [details] [diff] [review]
Patch (v2)

Good catch.  I wonder why I didn't get hangs while testing. :/
Comment 4 :Ehsan Akhgari (out sick) 2012-01-16 16:59:47 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/2273e0264d4a
Comment 5 Phil Ringnalda (:philor) 2012-01-16 18:35:38 PST
Backed you out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e40e72d6264d since I was pretty sure you'd rather have Windows continue to build ;)
Comment 6 :Ehsan Akhgari (out sick) 2012-01-16 18:40:50 PST
Ouch...  Some day I will learn how to program.  Until that day comes, here's a link to the log for reference: https://tbpl.mozilla.org/php/getParsedLog.php?id=8595118&tree=Mozilla-Inbound
Comment 7 :Ehsan Akhgari (out sick) 2012-01-17 08:47:16 PST
Created attachment 589214 [details] [diff] [review]
Patch (v2) which actually compiles
Comment 8 :Ehsan Akhgari (out sick) 2012-01-17 10:14:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/419f8a0f6374
Comment 9 Joe Drew (not getting mail) 2012-01-17 17:52:03 PST
https://hg.mozilla.org/mozilla-central/rev/419f8a0f6374

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