711491 - Add glibc backtrace feature in profiling builds for the SPS Profiler

Status: RESOLVED FIXED

(Core :: General, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: 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 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: The same thing as a single patch

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Order the patch series in the right order

Comment 5

[Benoit Girard (:BenWa)]

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]]

Attachment: Address the review comments.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: The right one

Comment 8

[Benoit Girard (:BenWa)]

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)]

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)]

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 (inactive)]

FYI, the "operator=(const MapEntry& aEntry)" leaks if aEntry == this.
https://hg.mozilla.org/integration/mozilla-inbound/rev/31b68bd629ff#l1.5

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/9cfa99d3807f
https://hg.mozilla.org/mozilla-central/rev/31b68bd629ff
https://hg.mozilla.org/mozilla-central/rev/7c2796bdba55
https://hg.mozilla.org/mozilla-central/rev/464e76c406e3
https://hg.mozilla.org/mozilla-central/rev/d09abc8c361d
https://hg.mozilla.org/mozilla-central/rev/2d2fbfd61980
https://hg.mozilla.org/mozilla-central/rev/b22cb97847a7
https://hg.mozilla.org/mozilla-central/rev/c0f11bea9cff
https://hg.mozilla.org/mozilla-central/rev/8f6dbb41479e
https://hg.mozilla.org/mozilla-central/rev/87fe97c8123b

Comment 13

[Benoit Girard (:BenWa)]

(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)]

(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.