(not currently active) Ted Mielczarek Reporter
	Description • 6 years ago

calixte noticed this crash report where we have symbols for libxul, but some functions in the stack (which are in libxul) do not have source line info:
https://crash-stats.mozilla.com/report/index/3fd24d17-435e-48e9-a104-6867b0180221

I grabbed the symbol file ( https://symbols.mozilla.org/libxul.so/EB11AE825A557DFD85AAA77A1F667E870/libxul.so.sym ) and looked at it, and we don't have FUNC records for these functions, only PUBLIC records:

$ rg nsNativeThemeGTK::DrawWidgetBackground /tmp/symbols/libxul.so/EB11AE825A557DFD85AAA77A1F667E870/libxul.so.sym 
5200908:PUBLIC 1d681cc 0 nsNativeThemeGTK::DrawWidgetBackground(gfxContext*, nsIFrame*, unsigned char, nsRect const&, nsRect const&) [clone .cold.190]
5315796:PUBLIC 36a8c10 0 nsNativeThemeGTK::DrawWidgetBackground(gfxContext*, nsIFrame*, unsigned char, nsRect const&, nsRect const&)
5315797:PUBLIC 36a94e0 0 non-virtual thunk to nsNativeThemeGTK::DrawWidgetBackground(gfxContext*, nsIFrame*, unsigned char, nsRect const&, nsRect const&)

I'll take a look at the original DWARF and see what it looks like ( https://symbols.mozilla.org/libxul.so/EB11AE825A557DFD85AAA77A1F667E870/libxul.so.dbg.gz )

	(not currently active) Ted Mielczarek Reporter
	Comment 1 • 6 years ago

With the DWARF, addr2line seems to do OK. Looking at the Raw Dump tab shows:
 { "frame": 0, "function": "nsNativeThemeGTK::DrawWidgetBackground(gfxContext*, nsIFrame*, unsigned char, nsRect const&, nsRect const&)", "function_offset": "0x3c5", "module": "libxul.so", "module_offset": "0x36a8fd5", ... }


Passing that module_offset to addr2line gives reasonable output including source info:
$ addr2line -f -C -e ./libxul.so.dbg 0x36a8fd5
nsNativeThemeGTK::DrawWidgetBackground(gfxContext*, nsIFrame*, unsigned char, nsRect const&, nsRect const&)
/builds/worker/workspace/build/src/widget/gtk/nsNativeThemeGTK.cpp:1148

This must be a bug in dump_syms.

	(not currently active) Ted Mielczarek Reporter
	Comment 2 • 6 years ago

I did some debugging, and the reason dump_syms is not generating FUNC records is because the DW_TAG_subprogram entries in the DWARF do not have DW_AT_{low,high}_pc attributes:

< 2><0x00114f07>      DW_TAG_subprogram
                        DW_AT_external              yes(1)
                        DW_AT_name                  DrawWidgetBackground
                        DW_AT_decl_file             0x00000001 /builds/worker/workspace/build/src/wid
get/gtk/nsNativeThemeGTK.cpp
                        DW_AT_decl_line             0x00000439
                        DW_AT_linkage_name          _ZN16nsNativeThemeGTK20DrawWidgetBackgroundEP10gfxContextP8nsIFramehRK6nsRectS6_
                        DW_AT_type                  <0x00077f08>
                        DW_AT_virtuality            DW_VIRTUALITY_virtual
                        DW_AT_vtable_elem_location  len 0x0002: 1008: DW_OP_constu 8
                        DW_AT_containing_type       <0x00114dd1>
                        DW_AT_accessibility         DW_ACCESS_public
                        DW_AT_declaration           yes(1)
                        DW_AT_object_pointer        <0x00114f29>
                        DW_AT_sibling               <0x00114f48>
< 3><0x00114f29>        DW_TAG_formal_parameter
                          DW_AT_type                  <0x00115337>
                          DW_AT_artificial            yes(1)
< 3><0x00114f2e>        DW_TAG_formal_parameter
                          DW_AT_type                  <0x0009871b>
< 3><0x00114f33>        DW_TAG_formal_parameter
                          DW_AT_type                  <0x000a8675>
< 3><0x00114f38>        DW_TAG_formal_parameter
                          DW_AT_type                  <0x0001a077>
< 3><0x00114f3d>        DW_TAG_formal_parameter
                          DW_AT_type                  <0x00092207>
< 3><0x00114f42>        DW_TAG_formal_parameter
                          DW_AT_type                  <0x00092207>

The dump_syms code checks that it got valid low and high pc values and skips the function if it didn't:
https://dxr.mozilla.org/mozilla-central/rev/3f474e48db7f7b0f1c2c090f812290a8d9a21650/toolkit/crashreporter/google-breakpad/src/common/dwarf_cu_to_module.cc#540

	(not currently active) Ted Mielczarek Reporter
	Comment 3 • 6 years ago

Oh, that was a bit of a red herring. That's the DIE for the method declaration (hence `DW_AT_declaration`) and later on is another `DW_TAG_subprogram` that references it:

< 1><0x00129948>    DW_TAG_subprogram
                      DW_AT_specification         <0x00114f07>
                      DW_AT_object_pointer        <0x00129960>
                      DW_AT_ranges                0x0202fd90
                ranges: 3 at .debug_ranges offset 33750416 (0x0202fd90) (48 bytes)
                        [ 0] range entry    0x036a8c10 0x036a94dd
                        [ 1] range entry    0x01d681cc 0x01d687ab
                        [ 2] range end      0x00000000 0x00000000
                      DW_AT_frame_base            len 0x0001: 9c: DW_OP_call_frame_cfa
                      DW_AT_object_pointer        <0x00129960>
                      DW_AT_GNU_all_call_sites    yes(1)
                      DW_AT_sibling               <0x0012b555>

This one still doesn't have {low,high}_pc, instead it uses `DW_AT_ranges`, which the dump_syms code does not support, so that's why these functions get left out.

	(not currently active) Ted Mielczarek Reporter
	Comment 4 • 6 years ago

I filed an issue in Breakpad's issue tracker for this as well:
https://bugs.chromium.org/p/google-breakpad/issues/detail?id=754

	Kim Moir [:kmoir] ET
	Comment 5 • 6 years ago

Anthony do you have anyone on your team that can take ownership of this bug?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 6 • 6 years ago

Taking this, will try to work on it this week.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 7 • 6 years ago

I've been poking at the code for some time now but I haven't found a way to feed the .debug_ranges info into the DwarfCUToModule function handler. Either I'm missing something or it doesn't seem that the class was designed for accessing data indirectly within the DWARF data. Maybe I haven't figured out yet how the whole thing works.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 8 • 6 years ago

Quick update: Breakpad probably should win a prize for the most obscure implementation of a visitor pattern ever. Anyway I managed to wrap my head around how parsing sections and attributes work and I'm actually making progress.

As it turns out implementing DW_AT_range involves more than meets the eye. First of all one need to check if the compilation unit has a DW_AT_low_pc attribute and if it does consider that as the base address used for calculations involve DW_AT_range addresses. That's in addition with base address selectors which can be found in a range list and which override the existing base address. In addition to this DW_AT_range may also appear as an attribute in compilation units and not just functions. In the former case it includes all the code ranges of the compilation unit; so for the DWARF code to be valid one should check if all the functions address ranges fall within those. To make things more interesting it is possible to find a DW_AT_high_pc used in a compilation unit instead of a function. When found it has the same meaning as DW_AT_ranges, i.e. its presence means that all the code in the compilation unit must fall within DW_AT_low_pc and DW_AT_high_pc.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 9 • 6 years ago

I'm steadily making progress and I've got almost everything working with the important exception of the line-to-function mapping logic. Switching from functions covering a single range to functions with multiple ranges requires significant changes and I'm still working on them.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 10 • 6 years ago

Changing the line-to-function mapping logic is hard. The biggest issue is that it assumes that functions cover a contiguous range and cannot overlap. Functions that use DW_AT_ranges breaks this assumption; for example functions where the compiler has done hot/cold code placement may have interwined parts in the hot code. So, either I rewrite the logic entirely or I roll a hack that splits functions with DW_AT_ranges into multiple functions even within Breakpad's logic. I wouldn't want to do it because it would be a terrible kludge but if there's no other way...

	Gabriele Svelto [:gsvelto] Assignee
	Comment 11 • 6 years ago

I've finally got a working WIP of this patch and I tested it on the debug information provided in comment 0. The number of unique FUNC entries rises from ~190k to ~210k but the actual code coverage is a lot more than that. The output .sym file size grows from 135M to 168M and that should give a hint of how many more address ranges we're covering.

There's still some work to do though: I haven't got all the warnings working correctly yet, I haven't written some additional unit-tests and finally the code is currently ugly as sin.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 12 • 6 years ago

I've finished writing the patch for supporting the DW_AT_ranges attribute in upstream breakpad, it's undergoing code review here:

https://chromium-review.googlesource.com/c/breakpad/breakpad/+/1124721

Once it's merged into master we'll pull breakpad again; I'll use this bug to track integrating that change into mozilla-central.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 13 • 6 years ago

My patch has been merged to upstream yesterday, I wanted to cherry-pick it but it doesn't apply cleanly anymore, meh :-| I'll have to update breakpad as a whole again. The upside is that we'll get AArch64 support too which we wanted for bug 1480915.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 14 • 6 years ago

I filed bug 769 on Breakpad's issue tracker to fix the issue:

https://bugs.chromium.org/p/google-breakpad/issues/detail?id=769

	Gabriele Svelto [:gsvelto] Assignee
	Comment 15 • 6 years ago

This adds support for the DW_AT_ranges attribute when dumping out symbols and
adds basic support for AArch64 (64-bit ARM) on Windows in the minidump
processor.

	(not currently active) Ted Mielczarek Reporter
	Comment 16 • 6 years ago

Comment on attachment 9001394 [details]
Bug 1440282 - Update breakpad to revision 1459e5df74dd03b7d3d473e6d271413d7aa98a88

Ted Mielczarek [:ted] [:ted.mielczarek] has approved the revision.

	Pulsebot
	Comment 17 • 6 years ago

Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fbe65340170c
Update breakpad to revision 1459e5df74dd03b7d3d473e6d271413d7aa98a88 r=ted

	Daniel Varga [:dvarga]
	Comment 18 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fbe65340170c