bug 1118446 - Implement HTTP symbol fetching in stackwalker #2550
Conversation
|
r? @rhelmer @twobraids Not sure who wants to review this, it's a fair bit of C++. Maybe @bsmedberg ? |
…es it fails to fetch
This changeset adds two new commandline arguments to the stackwalker: --symbols-url: to specify a base URL for fetching symbol files --symbols-cache: to specify a directory in which to save fetched symbol files If either argument is provided they must both be provided. If neither is provided the stackwalker will simply use any local paths listed after the minidump argument as it currently does. If both a symbols URL and local paths are provided the stackwalker will search for symbols in the following order: 1) In the symbols cache directory 2) In each local path provided in order 3) By constructing a URL from the symbols URL and the symbol filename and GETting it, storing it under the symbols cache directory.
|
This lgtm, @twobraids want to take a look over the C++ ? |
| } | ||
| for (auto d = dirs.rbegin(); d != dirs.rend(); d++) { | ||
| if (mkdir(d->c_str(), 0755) != 0) { | ||
| BPLOG(ERROR) << "Error creating " << dir << ": " << errno; |
There was a problem hiding this comment.
feeling a need to at least say something about this code: it might be nice in logging the problem here to say what subdir actually failed by indicating where in the iterator we were when mkdir failed.
and a question prompted by my rusty C++ skills, the iterator in the for loop, is it just a size_t or is an object of some sort? If it is an object, is its copy constructor being invoked by use of the post increment rather than pre increment in the loop? or is that not an issue in modern C++?
There was a problem hiding this comment.
That's a good point, I should log the exact dir that couldn't be created.
rbegin() returns a reverse iterator, so it's an object. The postincrement operator on that just mutates its internal state, iterator traversal should be plenty fast in modern C++.
There was a problem hiding this comment.
actually speed isn't my concern, it's mainly a refresher in my own understanding of C++.
Twenty years ago doing code review a Merrill Lynch, demonstrated a "gotcha" to a skeptical audience about the use preincrement vs postincrement for iterators that encapsulated cursors in relational databases. Using post increment actually invoked the copy constructor, spawning a new cursor and throwing it away on each iteration. Is that sort of unintended side effect something that we still have to watch out for, or does the compiler optimize that away these days?
There was a problem hiding this comment.
This is still true, although I suspect (but haven't verified) that the compiler can optimize it away. I'll fix it anyway as I'm using preincrement everywhere else (probably from the Google style guide for that very reason).
bug 1118446 - Implement HTTP symbol fetching in stackwalker
There's better documentation in luser@cc2849e for what I actually implemented here.
This allows using the stackwalker in the current mode unless we specify the new options, and we should be able to transition to the S3 storage incrementally.