Closed Bug 1059942 Opened 6 years ago Closed 6 years ago

Make "build.sh buildsymbols" command read public symbols

Categories

(Firefox OS Graveyard :: General, defect)

All
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: wcosta, Assigned: wcosta)

References

Details

(Whiteboard: [c=devtools p= s= u=])

Attachments

(1 file, 8 obsolete files)

buildsymbols command uses the breakpad dump_syms tool, which only looks at dynamic symbols table if there is no debug info in the object.

Some libraries have some functions implemented in assembly modules and do not export debug info for them (aka bionic), so dump_syms currently skip these functions. There is a patch [1] that adds an option to dump_syms read public symbols table even when debug info is found.

This patch should be integrated into gecko tree and scripts updated accordingly.

[1] https://breakpad.appspot.com/9684002/
Summary: Make buid.sh buildsymbols command read public symbols → Make "buid.sh buildsymbols" command read public symbols
See Also: → 1022928
Whiteboard: [c=devtools p= s= u=]
As command line options were added, upstream implemented the DumpOptions
class to pass command line arguments across dump_syms layers.

r=ted
By default, dump_syms does not look at public symbols if the object file
has debug info. This may cause it to skip some symbols that do not have
.debug_info section. Notably we see this at bionic routines implemented
in assembly. Details can be found at [1].

We apply patch [2] to enable dump_syms read public symbols even when
DWARF debug info is found. We also change symbolstore script
accordingly.

r=ted

[1] http://tinyurl.com/k3pxw5l

[2] https://breakpad.appspot.com/9684002/
As command line options were added, upstream implemented the DumpOptions
class to pass command line arguments across dump_syms layers.

r=ted
By default, dump_syms does not look at public symbols if the object file
has debug info. This may cause it to skip some symbols that do not have
.debug_info section. Notably we see this at bionic routines implemented
in assembly. Details can be found at [1].

We apply patch [2] to enable dump_syms read public symbols even when
DWARF debug info is found. We also change symbolstore script
accordingly.

r=ted

[1] http://tinyurl.com/k3pxw5l

[2] https://breakpad.appspot.com/9684002/
As command line options were added, upstream implemented the DumpOptions
class to pass command line arguments across dump_syms layers.
By default, dump_syms does not look at public symbols if the object file
has debug info. This may cause it to skip some symbols that do not have
.debug_info section. Notably we see this at bionic routines implemented
in assembly. Details can be found at [1].

We apply patch [2] to enable dump_syms read public symbols even when
DWARF debug info is found. We also change symbolstore script
accordingly.

[1] http://tinyurl.com/k3pxw5l

[2] https://breakpad.appspot.com/9684002/
Attachment #8480952 - Attachment is obsolete: true
Attachment #8480951 - Attachment is obsolete: true
Attachment #8480949 - Attachment is obsolete: true
Attachment #8480945 - Attachment is obsolete: true
Attachment #8480939 - Flags: review?(ted)
Attachment #8480942 - Flags: review?(ted)
Jed: I think you were interested in this at one point? (I know you also wanted a similar thing, where we could read both CFI and .exidx tables).
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Jed: I think you were interested in this at one point? (I know you also
> wanted a similar thing, where we could read both CFI and .exidx tables).

Yes, I was — I briefly looked into merging ELF symbol names with DWARF subprogram names while working on bug 942290 (the one with merging mixed exidx/CFI unwind info), but I deferred it because it needed more extensive changes to breakpad and wasn't as critical, and it looks like I never filed a followup bug for it.
Summary: Make "buid.sh buildsymbols" command read public symbols → Make "build.sh buildsymbols" command read public symbols
Attachment #8480939 - Attachment is obsolete: true
Attachment #8480939 - Flags: review?(ted)
Attachment #8480942 - Attachment is obsolete: true
Attachment #8480942 - Flags: review?(ted)
Currently, dump_syms does not look at public symbols if the object file
has debug info. This may cause it to skip some symbols that do not have
.debug_info section. Notably we see this at bionic routines implemented
in assembly. Details can be found at [1].

We apply patch [2] to enable dump_syms read public symbols even when
DWARF debug info is found.

[1] http://tinyurl.com/k3pxw5l

[2] https://breakpad.appspot.com/9684002/
Patch updated according to upstream review [1]

[1] https://breakpad.appspot.com/9684002/
Attachment #8481302 - Flags: review?(ted)
Comment on attachment 8481302 [details] [diff] [review]
Make dump_syms to parse public symbols; r=ted

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

This looks pretty sensible. We need to make sure it doesn't break any of Breakpad's unit tests. You can try this yourself by checking Breakpad out from SVN, applyig your patch, and running "./configure && make check"

https://code.google.com/p/google-breakpad/source/checkout

::: toolkit/crashreporter/google-breakpad/src/common/module.cc
@@ +109,5 @@
> +  func.name = ext->name;
> +  func.address = ext->address;
> +
> +  // Since parsing debug section and public info are not necessarily
> +  // mutually exclusive, check if we already have read the symbol

nit: Breakpad's comments should all be written in third person.
Attachment #8481302 - Flags: review?(ted) → review+
Attachment #8481302 - Attachment is obsolete: true
Currently, dump_syms does not look at public symbols if the object file
has debug info. This may cause it to skip some symbols that do not have
.debug_info section. Notably we see this at bionic routines implemented
in assembly. Details can be found at [1].

We apply patch [2] to enable dump_syms read public symbols even when
DWARF debug info is found.

[1] http://tinyurl.com/k3pxw5l

[2] https://breakpad.appspot.com/9684002/
Attachment #8485184 - Flags: review?(ted)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> Comment on attachment 8481302 [details] [diff] [review]
> Make dump_syms to parse public symbols; r=ted
> 
> Review of attachment 8481302 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty sensible. We need to make sure it doesn't break any of
> Breakpad's unit tests. You can try this yourself by checking Breakpad out
> from SVN, applyig your patch, and running "./configure && make check"
> 
> https://code.google.com/p/google-breakpad/source/checkout
> 

Ok, done, no regressions.

> ::: toolkit/crashreporter/google-breakpad/src/common/module.cc
> @@ +109,5 @@
> > +  func.name = ext->name;
> > +  func.address = ext->address;
> > +
> > +  // Since parsing debug section and public info are not necessarily
> > +  // mutually exclusive, check if we already have read the symbol
> 
> nit: Breakpad's comments should all be written in third person.

Fixed.
(In reply to Wander Lairson Costa [:wcosta] from comment #12)
> Created attachment 8485184 [details] [diff] [review]
> Make dump_syms to parse public symbols; r=ted
> 
> Currently, dump_syms does not look at public symbols if the object file
> has debug info. This may cause it to skip some symbols that do not have
> .debug_info section. Notably we see this at bionic routines implemented
> in assembly. Details can be found at [1].
> 
> We apply patch [2] to enable dump_syms read public symbols even when
> DWARF debug info is found.
> 
> [1] http://tinyurl.com/k3pxw5l
> 
> [2] https://breakpad.appspot.com/9684002/

The patch here is the same as the one in the appspot review, correct? I'll land it upstream and you can land it in the Mozilla tree.
Comment on attachment 8485184 [details] [diff] [review]
Make dump_syms to parse public symbols; r=ted

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

Gave this a review upstream:
https://breakpad.appspot.com/9684002/#msg3
Attachment #8485184 - Flags: review?(ted)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> (In reply to Wander Lairson Costa [:wcosta] from comment #12)
> > Created attachment 8485184 [details] [diff] [review]
> > Make dump_syms to parse public symbols; r=ted
> > 
> > Currently, dump_syms does not look at public symbols if the object file
> > has debug info. This may cause it to skip some symbols that do not have
> > .debug_info section. Notably we see this at bionic routines implemented
> > in assembly. Details can be found at [1].
> > 
> > We apply patch [2] to enable dump_syms read public symbols even when
> > DWARF debug info is found.
> > 
> > [1] http://tinyurl.com/k3pxw5l
> > 
> > [2] https://breakpad.appspot.com/9684002/
> 
> The patch here is the same as the one in the appspot review, correct?

Yes, it is.

> I'll land it upstream and you can land it in the Mozilla tree

But there some additional requested changes on upstream that need to applied here, right? I will update upstream and when everything is good there, I will apply the same changes here.
Yes, sorry, I realized I hadn't given it a thorough look after I wrote that. If you fix those last few things we can land it in both places.
Blocks: 1061796
Attachment #8485184 - Attachment is obsolete: true
Currently, dump_syms does not look at public symbols if the object file
has debug info. This may cause it to skip some symbols that do not have
.debug_info section. Notably we see this at bionic routines implemented
in assembly. Details can be found at [1].

Apply patch [2] to enable dump_syms read public symbols even when
DWARF debug info is found.

[1] http://tinyurl.com/k3pxw5l

[2] https://breakpad.appspot.com/9684002/
Attachment #8502814 - Flags: review?(ted)
I applied changes based on upstream review.
Comment on attachment 8502814 [details] [diff] [review]
Make dump_syms to parse public symbols. r=ted

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

This looks good, thanks! This is the same exact patch as what's on breakpad.appspot.com, right?
Attachment #8502814 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #20)
> Comment on attachment 8502814 [details] [diff] [review]
> Make dump_syms to parse public symbols. r=ted
> 
> Review of attachment 8502814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, thanks! This is the same exact patch as what's on
> breakpad.appspot.com, right?

Yep! Thanks for reviewing :)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Bugs aren't resolved until they land on mozilla-central. Please don't manually resolve unless it's landed on m-c and wasn't done so then.
Flags: needinfo?(wcosta)
Target Milestone: --- → 2.1 S8 (7Nov)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> Bugs aren't resolved until they land on mozilla-central. Please don't
> manually resolve unless it's landed on m-c and wasn't done so then.

Ops, my bad, sorry, I didn't know that.
Flags: needinfo?(wcosta)
Landed this upstream:
https://code.google.com/p/google-breakpad/source/detail?r=1400

Can you close the issue on breakpad.appspot.com please?
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #27)
> Landed this upstream:
> https://code.google.com/p/google-breakpad/source/detail?r=1400
> 
> Can you close the issue on breakpad.appspot.com please?

Ok, closed. Thank you :)
You need to log in before you can comment on or make changes to this bug.