Added a switch to dump minidump modules in minidump_stackwalk.

Added a switch to dump minidump modules in minidump_stackwalk.

In order to figure out what symbols we need associated to a minidump,
it is useful to be able to dump all the modules the minidump contains.

R=thestig@chromium.org
BUG=CHROMIUM:563716

[Lei Zhang, 2016-01-29 20:27:37]

Do you really need this? I think the information you want can easily be
extracted from the -m output, unless you need the full paths to the modules.

minidump_stack foo.dmp | grep ^Module | cut -d '|' -f 2


The -m switch seems broken. Can you check on that?

https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
File src/processor/minidump_stackwalk.cc (right):

https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
src/processor/minidump_stackwalk.cc:101: else if (machine_readable) {
nit: merge with prev line

https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
src/processor/minidump_stackwalk.cc:111: fprintf(stderr, "usage: %s [-m|-s]
<minidump-file> [symbol-path ...]\n"
Update this to mention -b

https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
src/processor/minidump_stackwalk.cc:152: } if (strcmp(argv[1], "-b") == 0) {
else if?

https://codereview.chromium.org/1651593002/diff/1/src/processor/stackwalk_com...
File src/processor/stackwalk_common.cc (right):

https://codereview.chromium.org/1651593002/diff/1/src/processor/stackwalk_com...
src/processor/stackwalk_common.cc:930: const CodeModules *modules =
process_state.modules();
nit: CodeModules*, and line 933 too.

[David Yen, 2016-01-29 20:36:26]

Whoops, had a lot of formatting issues...

https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
File src/processor/minidump_stackwalk.cc (right):

https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
src/processor/minidump_stackwalk.cc:101: else if (machine_readable) {
On 2016/01/29 20:27:37, Lei Zhang wrote:
> nit: merge with prev line

Done.

https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
src/processor/minidump_stackwalk.cc:111: fprintf(stderr, "usage: %s [-m|-s]
<minidump-file> [symbol-path ...]\n"
On 2016/01/29 20:27:37, Lei Zhang wrote:
> Update this to mention -b

Done.

https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
src/processor/minidump_stackwalk.cc:152: } if (strcmp(argv[1], "-b") == 0) {
On 2016/01/29 20:27:37, Lei Zhang wrote:
> else if?

Done. I think this is what was breaking the other switches.

https://codereview.chromium.org/1651593002/diff/1/src/processor/stackwalk_com...
File src/processor/stackwalk_common.cc (right):

https://codereview.chromium.org/1651593002/diff/1/src/processor/stackwalk_com...
src/processor/stackwalk_common.cc:930: const CodeModules *modules =
process_state.modules();
On 2016/01/29 20:27:37, Lei Zhang wrote:
> nit: CodeModules*, and line 933 too.

I was following the coding style in the rest of the file, I was thinking it was
odd too... do you think I should still change it?

[David Yen, 2016-01-29 20:38:58]

On 2016/01/29 20:27:37, Lei Zhang wrote:
> Do you really need this? I think the information you want can easily be
> extracted from the -m output, unless you need the full paths to the modules.
> 
> minidump_stack foo.dmp | grep ^Module | cut -d '|' -f 2
> 
> 
> The -m switch seems broken. Can you check on that?
> 
>
https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
> File src/processor/minidump_stackwalk.cc (right):
> 
>
https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
> src/processor/minidump_stackwalk.cc:101: else if (machine_readable) {
> nit: merge with prev line
> 
>
https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
> src/processor/minidump_stackwalk.cc:111: fprintf(stderr, "usage: %s [-m|-s]
> <minidump-file> [symbol-path ...]\n"
> Update this to mention -b
> 
>
https://codereview.chromium.org/1651593002/diff/1/src/processor/minidump_stac...
> src/processor/minidump_stackwalk.cc:152: } if (strcmp(argv[1], "-b") == 0) {
> else if?
> 
>
https://codereview.chromium.org/1651593002/diff/1/src/processor/stackwalk_com...
> File src/processor/stackwalk_common.cc (right):
> 
>
https://codereview.chromium.org/1651593002/diff/1/src/processor/stackwalk_com...
> src/processor/stackwalk_common.cc:930: const CodeModules *modules =
> process_state.modules();
> nit: CodeModules*, and line 933 too.

Yes, the full path is what is needed. Modules are often located in strange
paths, for example on mac "Chromium Framework" is something like
/out/Release/Chromium.app/Version/{VERSION_NUM}/..., while debug builds use
dylib files directly in out/Debug. Also sometimes the files are even loaded
directly from the file cache if the vfs loaded it from there. With this patch I
can just list out all the modules, filter out the system ones, and then dump the
symbols for exactly the ones I need.

[Lei Zhang, 2016-01-29 21:31:57]

lgtm

https://codereview.chromium.org/1651593002/diff/1/src/processor/stackwalk_com...
File src/processor/stackwalk_common.cc (right):

https://codereview.chromium.org/1651593002/diff/1/src/processor/stackwalk_com...
src/processor/stackwalk_common.cc:930: const CodeModules *modules =
process_state.modules();
On 2016/01/29 20:36:26, David Yen wrote:
> On 2016/01/29 20:27:37, Lei Zhang wrote:
> > nit: CodeModules*, and line 933 too.
> 
> I was following the coding style in the rest of the file, I was thinking it
was
> odd too... do you think I should still change it?

Yes, please change it.

https://codereview.chromium.org/1651593002/diff/20001/src/processor/minidump_...
File src/processor/minidump_stackwalk.cc (right):

https://codereview.chromium.org/1651593002/diff/20001/src/processor/minidump_...
src/processor/minidump_stackwalk.cc:113: "    -b : Output contained binary
modules\n",
Can you mention the full path part?

[David Yen, 2016-01-29 21:35:43]

https://codereview.chromium.org/1651593002/diff/1/src/processor/stackwalk_com...
File src/processor/stackwalk_common.cc (right):

https://codereview.chromium.org/1651593002/diff/1/src/processor/stackwalk_com...
src/processor/stackwalk_common.cc:930: const CodeModules *modules =
process_state.modules();
On 2016/01/29 21:31:57, Lei Zhang wrote:
> On 2016/01/29 20:36:26, David Yen wrote:
> > On 2016/01/29 20:27:37, Lei Zhang wrote:
> > > nit: CodeModules*, and line 933 too.
> > 
> > I was following the coding style in the rest of the file, I was thinking it
> was
> > odd too... do you think I should still change it?
> 
> Yes, please change it.

Done.

https://codereview.chromium.org/1651593002/diff/20001/src/processor/minidump_...
File src/processor/minidump_stackwalk.cc (right):

https://codereview.chromium.org/1651593002/diff/20001/src/processor/minidump_...
src/processor/minidump_stackwalk.cc:113: "    -b : Output contained binary
modules\n",
On 2016/01/29 21:31:57, Lei Zhang wrote:
> Can you mention the full path part?

Done.

[Lei Zhang, 2016-01-29 21:59:35]

Description was changed from

==========
Added a switch to dump minidump modules in minidump_stackwalk.

In order to figure out what symbols we need associated to a minidump,
it is useful to be able to dump all the modules the minidump contains.

R=thestig@chromium.org
BUG=CHROMIUM:563716
==========

to

==========
Added a switch to dump minidump modules in minidump_stackwalk.

In order to figure out what symbols we need associated to a minidump,
it is useful to be able to dump all the modules the minidump contains.

R=thestig@chromium.org
BUG=CHROMIUM:563716
==========