Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Write STACK WIN records correctly #513

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

loewenheim
Copy link
Contributor

The structure of STACK WIN records is described in https://github.com/google/breakpad/blob/master/docs/symbol_files.md#stack-win-records. In particular:

  • The has_program_string flag should be
    • For FrameData (type 4) records: 1 if the record has a program string, 0 otherwise;
    • For FPO (type 0) records: always 0.
  • The last argument should be
    • For FrameData records: the program string if it exists, empty otherwise;
    • For FPO records: 1 if the frame pointer register is used as a general-purpose register, 0 otherwise.

However, this is not what the current code does; we instead unify has_program_string and uses_base_pointer into one flag. Kudos to rust-minidump for discarding the resulting invalid records and logging this fact.

@loewenheim loewenheim requested a review from a team February 24, 2022 17:19
@codecov-commenter
Copy link

Codecov Report

Merging #513 (12cb239) into master (7810fee) will decrease coverage by 0.00%.
The diff coverage is 28.57%.

❗ Current head 12cb239 differs from pull request most recent head 0fa67cd. Consider uploading reports for the commit 0fa67cd to get more accurate results

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
- Coverage   68.73%   68.73%   -0.01%     
==========================================
  Files          84       84              
  Lines       17500    17503       +3     
==========================================
+ Hits        12029    12031       +2     
- Misses       5471     5472       +1     

@Gankra
Copy link
Contributor

Gankra commented Feb 24, 2022

oh is this the reason we were hitting this: rust-minidump/rust-minidump#252 (comment)

Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches my understanding of the format

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, this is Breakpad's dump_syms implementation: https://github.com/google/breakpad/blob/04a9ffbe592378a988dd736f4d30dd524f01706a/src/common/windows/pdb_source_line_writer.cc#L691-L700

And here's also the correct writer when processing Breakpad syms directly:

if r.program_string.is_some() { "1" } else { "0" },
if let Some(ps) = r.program_string {
ps
} else if r.uses_base_pointer {
"1"
} else {
"0"
}

My bad :)

@loewenheim loewenheim merged commit c03080a into master Feb 25, 2022
@loewenheim loewenheim deleted the fix/write-stack-win-records branch February 25, 2022 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants