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

feat: http-symbol-supplier configurability #647

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Gankra
Copy link
Collaborator

@Gankra Gankra commented Aug 2, 2022

  • Add a bunch of options to http symbol supplier so it's easier to turn on/off the various symbol lookup modes / features.
  • Add an expandable options struct and a new constructor function for using the options struct
  • Add a new use_minidump_paths mode to run dump_syms on the codefile and debugfile paths in the minidump, enabling local symbolication on the same machine that crashed/built
  • Expose some of those options as minidump-stackwalk flags

@Gankra
Copy link
Collaborator Author

Gankra commented Aug 3, 2022

Some experimental notes on --use-minidump-paths=true:

  • If I use this in minidump-pipeline, it works!
  • If I use this in mozcrash.py for a local test crash, it works!

Copy link
Contributor

@mstange mstange left a comment

Choose a reason for hiding this comment

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

This all looks good to me.

The only thing that jumped out to me was the invocation of dump_syms, which, on Windows, might be called with both the code file and the debug file. I'm not exactly sure how dump_syms deals with having both because it also has some autodetection to find the pdb for an exe and the exe for a pdb. It might dump both files twice and then merge the result. In any case, if this doesn't work the way we want it to, it should be fixed in dump_syms and not here - passing both seems like a fine API.

@Gankra Gankra marked this pull request as ready for review August 12, 2022 16:15
@Gankra
Copy link
Collaborator Author

Gankra commented Aug 12, 2022

In the current impl if we get two files we only use the "better" one (determined by the caller's ordering of the inputs). In this impl we pass the debugfile second, meaning it's preferred. In the event that the codefile and debugfile are in separate directories (cargo install --path ./) this will result in us only using the debugfile, even if the codefile's path resolves. This is definitely suboptimal, but is a pre-existing artifact of the old dump_syms interface where it would freak out if you gave it multiple paths (even if you specified type...). I don't know to what extent these hacks can be removed.

@Gankra
Copy link
Collaborator Author

Gankra commented Aug 12, 2022

TODO:

  • clean up all the nonsense around trying to make the cache/tmp optional. this feels like a failed experiment in the current form, but if we change dump_syms handling to work completely in-memory, this might be possible (and possibly desirable to avoid caching for the --use-minidump-paths feature?)

  • clean up the new minidump-stackwalk flags. the docs are currently copy-pasted from elsewhere, and should be reformatted.

  • optimize...? we currently do a lot of writing of intermediate results to the disk to make the code more uniform and debuggable, but this can have a ton of overhead for the native case

  • reflect on what order this mode should be used in. intuitively to me "last" feels right, because anything from a symbol server is more likely to have all the info.

@Gankra Gankra mentioned this pull request Aug 12, 2022
16 tasks
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.

None yet

2 participants