Remove PDB code and use symbolic_debuginfo for all platforms #433

mstange · 2022-07-29T21:58:15Z

Fixes #406.
Fixes #280.

This PR adds inline support for Windows.
It also removes 1500 lines of code at the same time.

I've ported the following features from the Windows code to the cross-platform code:

Some Windows path handling (used in the source file code)
Appending a dummy symbol based on the last known symbol
Adopting the FUNC name from the PUBLIC at the same address if the function name doesn't have parameter information
Decoding parameter size from "C decorated" symbol names, and assigning it to the function or public at that address
Generating synthetic functions from the PE object's exception info, if no PDB is available
Skipping public symbols for string and number constants
Already landed: setting the "multiple" flag if multiple publics or functions have the same address (PR Make is_multiple work properly in elf.rs. #416)

I've also upstreamed the following changes to symbolic:

Emit function parameters, by using a TypeFormatter (Use pdb_addr2line::TypeFormatter. getsentry/symbolic#426)
Include functions from SeparatedCode symbols (Add support for SeparatedCode symbols. getsentry/symbolic#622)
Include symbols which are not marked as functions, as long as they're from an executable section (fix(debuginfo): include more pdb symbols getsentry/symbolic#641)
Allow detecting C decorated functions by not stripping underscores (fix(debuginfo): don't strip leading underscores from PDB symbols getsentry/symbolic#642)

I deleted the following code without replacement:

Computing FUNC parameter size for 32 bit Windows based on argument type information. Now FUNC entries on 32 bit Windows will only have a non-zero parameter size if they can get it from the "C decorated" name of a public symbol at that same address. Otherwise the parameter size will be zero, and stack walking needs to obtain it from the STACK WIN entries. rust-minidump already prefers the size from the STACK WIN entries anyway, because the FUNC parameter size was found to be incorrect in some cases, see FUNC and WIN STACK entries disagree on parameter size #284. Furthermore, all 32 bit dll / exe files which I tested already generated STACK WIN entries for all of their PDB's FUNC addresses.
Getting "has code" information from section contributions. This was used to include more public symbols, but it didn't seem necessary. The symbolic PDB code doesn't look at section contributions at all, only at the section headers themselves. Checking the section header's "characteristics" seems to work fine and includes all symbols we care about.
Detecting split functions based on Block symbols with disjoint address ranges.
Splitting line information based on any encountered disjoint address ranges.
Splitting line information based on the PDB's address map. For these last three, I haven't found a PDB for which this was necessary.

I've done a lot of comparisons of before/after .sym files.

The remaining differences I've found all fall in the following categories:

Extra leading underscores. I think in the past we were sometimes stripping leading underscores twice, mostly by accident.
Different (better) demangling, thanks to fix(demangle): tweak MSVC demangling flags getsentry/symbolic#640
Some functions use different names because of a different arbitrary choice between multiple functions at the same address.
Fewer FILE records, because the cross-platform code omits unused FILE records.
Different file IDs in line records, as a result of the different FILE records.
Sometimes fewer line records because duplicate consecutive lines have been merged into one.
ntdll has more PUBLIC symbols now. I'm not quite sure why but I appreciate it.

This parsese the PE header twice, but that's ok.

…ID earlier.

This eliminates the custom PDB parsing and relies on symbolic's PDB parsing instead.

@8

- Pass on a symbol name to an argument-less function name. - Detect the parameter size from the @8 at the end of the symbol name.

…tants.

Even if the symbol name is not MSVC-mangled, it's often a better choice for the function name. In the example I was looking at, the function name started with an underscore, and taking the name from the symbol stripped that underscore, which looked better. We don't strip underscores from function names normally, because function names are already expected to be clean.

They're not Linux-specific.

Remove lots of template parameters and traits because everything now has the same type. Make multi-file path use auto detection on the file type just like in the single-file path.

Otherwise we lose the size of the placeholder function, because publics don't have a size.

This is no longer needed because all file types now end up as ObjectInfo which can be merged with each other.

gabrielesvelto

LGTM, thanks for this excellent work 👏

gabrielesvelto · 2022-08-01T14:19:58Z

Cargo.toml

@@ -42,7 +41,7 @@ serde = "1.0"
 serde_json = "1.0"
 sha2 = "0.10"
 simplelog = { version = "0.12", optional = true, features = ["local-offset"] }
-symbolic = { version = "9", features = ["demangle", "cfi"] }
+symbolic = { git = "https://github.com/getsentry/symbolic", rev = "f0dc99bb400da20fed8a7a42d71d8206ffeee536", features = ["demangle", "cfi"] }


This is fine but let's file a bug to turn this into a version when the fix gets a proper release

Filed #438.

This restores the error handling from elf.rs from before mozilla#433 . That PR changed it to an unwrap because the PDB code was using unwrap. old elf.rs: https://github.com/mozilla/dump_syms/blob/de7cc440533b794f85ca4e1f10e26d918330140d/src/linux/elf.rs#L464-L466 old pdb.rs: https://github.com/mozilla/dump_syms/blob/de7cc440533b794f85ca4e1f10e26d918330140d/src/windows/pdb.rs#L254 We encounter this error for libmozavutil.so on certain Linux debug builds, see https://bugzilla.mozilla.org/show_bug.cgi?id=1783899#c6 .

mstange added 28 commits July 29, 2022 16:50

Remove some unnecessary Option wrapping and unwrapping.

c03f156

Move the debug ID check into the caller of set_pe.

87f97a5

Move the set_pe further outside.

3f938fe

This parsese the PE header twice, but that's ok.

Create the PDB outside of PDBInfo::new so that we can find its debug …

62f4850

…ID earlier.

Compute debug ID before creating PDBInfo.

18bdf13

Remove PDBInfo::set_pe.

9d2b8c7

Make ElfInfo more generic and use it for Windows.

3d2ea7d

This eliminates the custom PDB parsing and relies on symbolic's PDB parsing instead.

Implement Windows C undecorating in elf.rs.

2070899

- Pass on a symbol name to an argument-less function name. - Detect the parameter size from the @8 at the end of the symbol name.

Implement skipping of Windows symbols which are string or number cons…

f1dc4f6

…tants.

Add dummy symbol, Windows only.

f0524f0

Implement exception data based placeholder symbols.

2160e7a

Remove commented-out call to append_dummy_symbol.

50f0d97

Remove commented-out calls for PE symbols.

a85fd4d

Move this function out of PEInfo.

48f271c

Move all files out of the linux directory.

6932147

They're not Linux-specific.

Move this function out of MachoInfo.

9d9c5af

Split Platform and Collector out of elf.rs.

4775bac

Extract ElfInfo::new into linux.rs.

2e3f477

Rename ElfInfo to ObjectInfo and radically simplify.

968d3f6

Remove lots of template parameters and traits because everything now has the same type. Make multi-file path use auto detection on the file type just like in the single-file path.

Move Mac code out of subdirectory.

f87a147

Collect placeholder functions before publics.

d5f7737

Otherwise we lose the size of the placeholder function, because publics don't have a size.

Re-generate all test references.

b94156a

Rename ElfSymbol to Symbol.

05c6790

Remove LineFinalizer trait.

245d237

Make demangle_str non-pub.

556e732

Don't require type when dumping multiple files.

952b248

This is no longer needed because all file types now end up as ObjectInfo which can be merged with each other.

Remove FileType enum.

4bcc2e5

mstange requested a review from gabrielesvelto July 29, 2022 21:58

mstange self-assigned this Jul 29, 2022

gabrielesvelto approved these changes Aug 2, 2022

View reviewed changes

mstange mentioned this pull request Aug 2, 2022

Once symbolic 9.1 is released, update Cargo.toml #438

Closed

mstange merged commit 325cf2c into mozilla:main Aug 2, 2022

This was referenced Aug 10, 2022

Mixed paths emitted for PDBs with Linux paths #444

Closed

If CFI writing fails, print an error and continue. #447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove PDB code and use symbolic_debuginfo for all platforms #433

Remove PDB code and use symbolic_debuginfo for all platforms #433

mstange commented Jul 29, 2022 •

edited

gabrielesvelto left a comment

gabrielesvelto Aug 1, 2022

mstange Aug 2, 2022

Remove PDB code and use symbolic_debuginfo for all platforms #433

Remove PDB code and use symbolic_debuginfo for all platforms #433

Conversation

mstange commented Jul 29, 2022 • edited

gabrielesvelto left a comment

Choose a reason for hiding this comment

gabrielesvelto Aug 1, 2022

Choose a reason for hiding this comment

mstange Aug 2, 2022

Choose a reason for hiding this comment

mstange commented Jul 29, 2022 •

edited