1061800 - [b2g] Gecko Profiler should store the breakpad binary IDs for symbolication

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Benoit Girard (:BenWa)]

It appears we properly archive the breakpad symbol data on symbolapi.mozilla.org . This symbol requires a the matching breakpad binary IDs/key to fetch these symbols. This hash should be unique for each build of each library. The profiler correctly computes and store the IDs for the windows library but is missing the implementation on other platforms.

If we implement this and match the breakpad ids well be able to symbolicate builds where the breakpad data is sent to symbolapi.mozilla.org.

Comment 2

[Wander Lairson Costa]

Attachment: Add breakpad ids to profiler in Linux. r=BenWa

By default, breakpad uses the build-id section for ids, if it isn't
found, it uses the .text section. On the later case, the stripped
libraries will have different ids from their non-stripped counterparts.

See Bug 1062459 for details.

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 8486580 [details] [diff] [review]
Add breakpad ids to profiler in Linux. r=BenWa

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

We should document in the code why we're removing - and appending 0.

Comment 4

[Wander Lairson Costa]

Attachment: Add breakpad ids to profiler in Linux. r=BenWa

By default, breakpad uses the build-id section for ids, if it isn't
found, it uses the .text section. On the later case, the stripped
libraries will have different ids from their non-stripped counterparts.

See Bug 1062459 for details.

Comment 5

[Wander Lairson Costa]

(In reply to Benoit Girard (:BenWa) from comment #3)
> Comment on attachment 8486580 [details] [diff] [review]
> Add breakpad ids to profiler in Linux. r=BenWa
> 
> Review of attachment 8486580 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should document in the code why we're removing - and appending 0.

Ok, done.

Comment 6

[Carsten Book [:Tomcat]]

Hi, could you provide a try link for this changes? thanks!

Comment 7

[Wander Lairson Costa]

(In reply to Carsten Book [:Tomcat] from comment #6)
> Hi, could you provide a try link for this changes? thanks!

Here it is: https://tbpl.mozilla.org/?tree=Try&rev=9732a93e78c2

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8486727 [details] [diff] [review]
Add breakpad ids to profiler in Linux. r=BenWa

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

::: tools/profiler/shared-libraries-linux.cc
@@ +35,5 @@
> +    // ConvertIdentifierToString converts the identifier to a string with
> +    // some dashes (don't ask me why), but we need it raw, so remove the dashes.
> +    char *id_end = remove(id_str, id_str + strlen(id_str), '-');
> +    // Also add an extra "0" by the end.  google-breakpad does it for
> +    // consistency with PDB files so we need to do too.

FWIW, if you wanted to patch Breakpad to make ConvertIdentifierToString do the right thing here I think we could take that. Seems silly that all the callers wind up stripping out the dashes and appending zero.

Comment 9

[Wander Lairson Costa]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> Comment on attachment 8486727 [details] [diff] [review]
> Add breakpad ids to profiler in Linux. r=BenWa
> 
> Review of attachment 8486727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/profiler/shared-libraries-linux.cc
> @@ +35,5 @@
> > +    // ConvertIdentifierToString converts the identifier to a string with
> > +    // some dashes (don't ask me why), but we need it raw, so remove the dashes.
> > +    char *id_end = remove(id_str, id_str + strlen(id_str), '-');
> > +    // Also add an extra "0" by the end.  google-breakpad does it for
> > +    // consistency with PDB files so we need to do too.
> 
> FWIW, if you wanted to patch Breakpad to make ConvertIdentifierToString do
> the right thing here I think we could take that. Seems silly that all the
> callers wind up stripping out the dashes and appending zero.

I take it after this patch lands.

Comment 10

[Wander Lairson Costa]

(In reply to Carsten Book [:Tomcat] from comment #6)
> Hi, could you provide a try link for this changes? thanks!

Hi, is there anything else pending to land this patch?

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/83804f7688e7

Comment 12

[Wander Lairson Costa]

(In reply to Carsten Book [:Tomcat] from comment #11)
> https://hg.mozilla.org/integration/b2g-inbound/rev/83804f7688e7

Thanks :)

Comment 13

[Carsten Book [:Tomcat]]

(In reply to Wander Lairson Costa [:wcosta] from comment #12)
> (In reply to Carsten Book [:Tomcat] from comment #11)
> > https://hg.mozilla.org/integration/b2g-inbound/rev/83804f7688e7
> 
> Thanks :)

np :) bascially when we something is missing for check-in needed bugs (like missing try run or patch does not apply cleanly) we remove the checkin-needed keyword like in this case, so it was fallen under the radar when you posted the link to the try run :)

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/83804f7688e7