Last Comment Bug 711491 - Add glibc backtrace feature in profiling builds for the SPS Profiler
: Add glibc backtrace feature in profiling builds for the SPS Profiler
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
: 709381 (view as bug list)
Depends on:
Blocks: 713227
  Show dependency treegraph
 
Reported: 2011-12-16 08:41 PST by Benoit Girard (:BenWa)
Modified: 2012-01-05 12:22 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A series of patches that gets us most of the way there (32.96 KB, patch)
2011-12-16 17:19 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
The same thing as a single patch (27.81 KB, patch)
2011-12-16 17:29 PST, Jeff Muizelaar [:jrmuizel]
bgirard: review-
Details | Diff | Review
Order the patch series in the right order (32.96 KB, patch)
2011-12-16 17:37 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
Address the review comments. (25.25 KB, patch)
2011-12-20 11:33 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
The right one (30.50 KB, patch)
2011-12-20 11:45 PST, Jeff Muizelaar [:jrmuizel]
bgirard: review-
Details | Diff | Review

Description Benoit Girard (:BenWa) 2011-12-16 08:41:36 PST

    
Comment 1 Jeff Muizelaar [:jrmuizel] 2011-12-16 17:19:25 PST
Created attachment 582437 [details] [diff] [review]
A series of patches that gets us most of the way there

I'm attaching all 10 patches as a single one, cause that's a lot easier than attaching them individually. Hopefully, it's still reasonably easy to review.
Comment 2 Jeff Muizelaar [:jrmuizel] 2011-12-16 17:24:42 PST
*** Bug 709381 has been marked as a duplicate of this bug. ***
Comment 3 Jeff Muizelaar [:jrmuizel] 2011-12-16 17:29:34 PST
Created attachment 582441 [details] [diff] [review]
The same thing as a single patch
Comment 4 Jeff Muizelaar [:jrmuizel] 2011-12-16 17:37:11 PST
Created attachment 582447 [details] [diff] [review]
Order the patch series in the right order
Comment 5 Benoit Girard (:BenWa) 2011-12-16 19:47:54 PST
Comment on attachment 582441 [details] [diff] [review]
The same thing as a single patch

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

This looks good, just a few minor edits. I'll land programmatic event tracing soon. Once you land this I'll work on the StackWalk64 part.

(If you use NaN more it's a splinter bug)

::: tools/profiler/sps/TableTicker.cpp
@@ +312,5 @@
>  }
>  
> +#ifdef USE_BACKTRACE
> +static
> +void doBacktrace(Profile &aProfile)

This shouldn't go in TableTicker.cpp. Perhaps roll it into platform-* or add backtrace.h + backtrace-*.cpp.

@@ +367,5 @@
> +    marker = mStack->getMarker(i++);
> +  }
> +  mStack->mQueueClearMarker = true;
> +
> +#ifdef USE_BACKTRACE

We discussed allowing both of these to be toggle-able individually. Would be nice to have this in for this patch, but no mandatory.

@@ -353,3 +392,3 @@
> >      char tagBuff[1024];
> > -    MapInfo& maps = profile->getMap();
> > +    SharedLibraryInfo& shlibInfo = profile->getSharedLibraryInfo();
> > -    unsigned long pc = (unsigned long)mLeafAddress;
> > +    Address pc = (Address)mTagData;

We need to fix this casting hack. We should use a const char* to store address. Maybe use an union?

@@ +497,3 @@
>  
> +  char *rtn = (char*)malloc( (profile.Length()+1) * sizeof(char) );
> +  strcpy(rtn, profile.Buffer());

We're returning a new char*, can you add a free to nsProfiler.cpp.

::: tools/profiler/sps/shared-libraries-win32.cc
@@ +19,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2011
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *

Contributor?

::: tools/profiler/sps/shared-libraries.h
@@ +1,1 @@
> +#include <string.h>

License header?

I think we may want to use uintptr instead of unsigned long.

::: xpcom/ds/StringBuilder.h
@@ +44,5 @@
> +   This does a doubling allocation when out of capacity.
> +
> +   This does not use nsTArray because it starts growing
> +   by multiples of page size after it is the size of one
> +   page. We wan't keep doubling in size so that

want to*

@@ +55,5 @@
> + */
> +
> +namespace mozilla {
> +
> +class StringBuilder

Maybe this should get it's own cpp file since we're providing it for others? Although most methods are relatively small.

@@ +68,5 @@
> +
> +    void Append(const char *s) {
> +        size_t newLength = strlen(s);
> +
> +        EnsureCapacity(mLength + newLength);

+ 1 for null terminator?
Comment 6 Jeff Muizelaar [:jrmuizel] 2011-12-20 11:33:39 PST
Created attachment 583232 [details] [diff] [review]
Address the review comments.
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-12-20 11:45:12 PST
Created attachment 583236 [details] [diff] [review]
The right one
Comment 8 Benoit Girard (:BenWa) 2011-12-20 11:58:56 PST
Comment on attachment 583236 [details] [diff] [review]
The right one

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

r+ with the printf fixed for 32-bit.

::: tools/profiler/sps/TableTicker.cpp
@@ +425,4 @@
>        if (pc > e.GetStart() && pc < e.GetEnd()) {
>          if (e.GetName()) {
>            found = true;
> +          snprintf(tagBuff, 1024, "l-%900s@%llu\n", e.GetName(), pc - e.GetStart());

These should be %p

@@ +431,5 @@
>          }
>        }
>      }
>      if (!found) {
> +      snprintf(tagBuff, 1024, "l-???@%llu\n", pc);

%p

@@ +498,4 @@
>  
> +  char *rtn = (char*)malloc( (profile.Length()+1) * sizeof(char) );
> +  printf("%s", profile.Buffer());
> +  strcpy(rtn, profile.Buffer());

This should really have been strdup() from the start, although this code is correct.
Comment 9 Benoit Girard (:BenWa) 2011-12-23 12:24:54 PST
Comment on attachment 583236 [details] [diff] [review]
The right one

I've discussed this with Rafael and Ehsan and we decided that we should rather extend nsStackWalk.h to fit our needs rather then duplicate the code.

It would be nice to split your profile saving code and land that right away.
Comment 10 Benoit Girard (:BenWa) 2011-12-26 14:32:55 PST
I've tried using nsStackWalk.h on mac x64. It works for ~ a few seconds before I crash on:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xffffffffffffffdf
0x00007fff89da5ed5 in libunwind::UnwindCursor<libunwind::LocalAddressSpace, libunwind::Registers_x86_64>::step ()
(gdb) where
#0  0x00007fff89da5ed5 in libunwind::UnwindCursor<libunwind::LocalAddressSpace, libunwind::Registers_x86_64>::step ()
#1  0x00007fff89ea15a8 in _Unwind_Backtrace ()
Comment 11 Mats Palmgren (:mats) 2011-12-29 14:24:33 PST
FYI, the "operator=(const MapEntry& aEntry)" leaks if aEntry == this.
https://hg.mozilla.org/integration/mozilla-inbound/rev/31b68bd629ff#l1.5
Comment 13 Benoit Girard (:BenWa) 2011-12-30 05:09:01 PST
(In reply to Mats Palmgren [:mats] from comment #11)
> FYI, the "operator=(const MapEntry& aEntry)" leaks if aEntry == this.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/31b68bd629ff#l1.5

Thanks Mats, I'll file a follow up bug for this.
Comment 14 Benoit Girard (:BenWa) 2012-01-05 12:13:18 PST
(In reply to Mats Palmgren [:mats] from comment #11)
> FYI, the "operator=(const MapEntry& aEntry)" leaks if aEntry == this.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/31b68bd629ff#l1.5

Filed as bug 715618 with a patch. Thanks.

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