Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add glibc backtrace feature in profiling builds for the SPS Profiler

RESOLVED FIXED in mozilla12

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: BenWa, Assigned: jrmuizel)

Tracking

Trunk
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
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.
Attachment #582437 - Flags: review?(bgirard)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 709381
(Assignee)

Comment 3

6 years ago
Created attachment 582441 [details] [diff] [review]
The same thing as a single patch
Attachment #582441 - Flags: review?(bgirard)
(Assignee)

Comment 4

6 years ago
Created attachment 582447 [details] [diff] [review]
Order the patch series in the right order
Attachment #582447 - Flags: review?(bgirard)
(Reporter)

Comment 5

6 years ago
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?
Attachment #582441 - Flags: review?(bgirard) → review-
(Assignee)

Comment 6

6 years ago
Created attachment 583232 [details] [diff] [review]
Address the review comments.
Attachment #582437 - Attachment is obsolete: true
Attachment #582441 - Attachment is obsolete: true
Attachment #582447 - Attachment is obsolete: true
Attachment #582437 - Flags: review?(bgirard)
Attachment #582447 - Flags: review?(bgirard)
Attachment #583232 - Flags: review?(bgirard)
(Assignee)

Comment 7

6 years ago
Created attachment 583236 [details] [diff] [review]
The right one
Attachment #583236 - Flags: review?(bgirard)
(Reporter)

Comment 8

6 years ago
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.
Attachment #583236 - Flags: review?(bgirard) → review+
(Reporter)

Updated

6 years ago
Blocks: 713227
No longer depends on: 683229
(Reporter)

Comment 9

6 years ago
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.
Attachment #583236 - Flags: review+ → review-
(Reporter)

Comment 10

6 years ago
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 ()
FYI, the "operator=(const MapEntry& aEntry)" leaks if aEntry == this.
https://hg.mozilla.org/integration/mozilla-inbound/rev/31b68bd629ff#l1.5
(Assignee)

Updated

6 years ago
Assignee: nobody → jmuizelaar
Target Milestone: --- → mozilla11
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla11 → mozilla12
(Reporter)

Comment 13

6 years ago
(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.
(Reporter)

Comment 14

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #583232 - Flags: review?(bgirard)
You need to log in before you can comment on or make changes to this bug.