Add an option to not handle DWARF inter-compilation unit references in Linux dump_syms.

Add an option to not handle DWARF inter-compilation unit references in Linux
dump_syms. 

This saves a lot of memory for dump_syms.

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1163

[Lei Zhang (chromium)]

Cary Coutant says there are very few known tools that actually use
DW_FORM_ref_addr. Given this, we can cut dump_syms's memory usage in almost half
by not keeping all the specifications data around.

[jimb]

On 2013/04/22 18:18:28, thestig wrote:
> Cary Coutant says there are very few known tools that actually use
> DW_FORM_ref_addr. Given this, we can cut dump_syms's memory usage in almost
half
> by not keeping all the specifications data around.

I like the 'options' struct, and I appreciate the style compliance cleanups. (I
recently got Emacs to show me trailing spaces...)

I think it would be better to make the flag a member of FileContext, and have a
method of FileContext, to be called after each compilation unit, that clears the
specifications map. That makes FileContext responsible for managing its internal
state (FilePrivate is really just an extension of FileContext), rather than
having the CUContext reach in and fiddle with things. DwarfCUToModule::Finish
should call that method of the FileContext.

The warning is nice; it seems to me you should still be able to print it with
the arrangement I suggest above. If you use WarningReporter, you can actually
write unit tests for the flag.

[Lei Zhang (chromium)]

On 2013/04/22 20:24:55, jimb wrote:
> On 2013/04/22 18:18:28, thestig wrote:
> > Cary Coutant says there are very few known tools that actually use
> > DW_FORM_ref_addr. Given this, we can cut dump_syms's memory usage in almost
> half
> > by not keeping all the specifications data around.
> 
> I like the 'options' struct, and I appreciate the style compliance cleanups.
(I
> recently got Emacs to show me trailing spaces...)
> 
> I think it would be better to make the flag a member of FileContext, and have
a
> method of FileContext, to be called after each compilation unit, that clears
the
> specifications map. That makes FileContext responsible for managing its
internal
> state (FilePrivate is really just an extension of FileContext), rather than
> having the CUContext reach in and fiddle with things. DwarfCUToModule::Finish
> should call that method of the FileContext.
> 
> The warning is nice; it seems to me you should still be able to print it with
> the arrangement I suggest above. If you use WarningReporter, you can actually
> write unit tests for the flag.

Ok, mostly moved everything over. Since FileContext is no longer a simple
struct, I turned it into a class. I'll add WarningReporter usage tomorrow and
hopefully some tests.

[Lei Zhang (chromium)]

Onward to patch set 3.

[jimb]

Looks great - thank you!

I had some minor suggestions; follow them if you agree.

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module.cc
File src/common/dwarf_cu_to_module.cc (right):

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module....
src/common/dwarf_cu_to_module.cc:135: module_(module),
The _arg ending was especially stupid because it was completely unnecessary...

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module....
src/common/dwarf_cu_to_module.cc:310: FileContext* file_context =
cu_context_->file_context;
Ambient style is 'FileContext *'.

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module....
src/common/dwarf_cu_to_module.cc:321: SpecificationByOffset* specifications =
Same here.

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module.h
File src/common/dwarf_cu_to_module.h (right):

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module....
src/common/dwarf_cu_to_module.h:120: // TODO(thestig) Convert to a scoped_ptr.
Do TODO comments in the code function well? In the files I'm familiar with, we
have TODO's dating to 2011, and it doesn't seem like they're capturing the most
important tasks; they would seem to be more impulsive: "I thought of this while
working in this area, and filing a bug is a pain."

[Lei Zhang (chromium)]

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module.cc
File src/common/dwarf_cu_to_module.cc (right):

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module....
src/common/dwarf_cu_to_module.cc:135: module_(module),
On 2013/04/24 04:49:03, jimb wrote:
> The _arg ending was especially stupid because it was completely unnecessary...

It might have helped avoid -Wshadow warnings. Not that we compile with that
flag.

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module....
src/common/dwarf_cu_to_module.cc:310: FileContext* file_context =
cu_context_->file_context;
On 2013/04/24 04:49:03, jimb wrote:
> Ambient style is 'FileContext *'.

Done.

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module....
src/common/dwarf_cu_to_module.cc:321: SpecificationByOffset* specifications =
On 2013/04/24 04:49:03, jimb wrote:
> Same here.

Done.

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module.h
File src/common/dwarf_cu_to_module.h (right):

https://breakpad.appspot.com/565002/diff/13008/src/common/dwarf_cu_to_module....
src/common/dwarf_cu_to_module.h:120: // TODO(thestig) Convert to a scoped_ptr.
On 2013/04/24 04:49:03, jimb wrote:
> Do TODO comments in the code function well? In the files I'm familiar with, we
> have TODO's dating to 2011, and it doesn't seem like they're capturing the
most
> important tasks; they would seem to be more impulsive: "I thought of this
while
> working in this area, and filing a bug is a pain."

Hard to say. I do plan to follow up on this right after this CL lands. Removed
and made a mental note instead.

[Lei Zhang (chromium)]

On 2013/04/24 04:49:03, jimb wrote:
> Looks great - thank you!

I'll take that as a l-g-t-m.

[Lei Zhang (chromium)]

Committed patchset #4 manually as r1163 (presubmit successful).

[jimb]

On 2013/04/24 21:18:29, thestig wrote:
> On 2013/04/24 04:49:03, jimb wrote:
> > Looks great - thank you!
> 
> I'll take that as a l-g-t-m.

Yes --- that was my intention.

[icodestuff]

Please fix dump_syms.mm.

https://breakpad.appspot.com/565002/diff/22001/src/common/dwarf_cu_to_module.h
File src/common/dwarf_cu_to_module.h (right):

https://breakpad.appspot.com/565002/diff/22001/src/common/dwarf_cu_to_module....
src/common/dwarf_cu_to_module.h:79: FileContext(const string &filename,
The change of constructor breaks dump_syms on OS X. dump_syms.mm:263 will no
longer compile

[Lei Zhang (chromium)]

On 2013/05/08 01:43:21, icodestuff wrote:
> Please fix dump_syms.mm.
> 
> https://breakpad.appspot.com/565002/diff/22001/src/common/dwarf_cu_to_module.h
> File src/common/dwarf_cu_to_module.h (right):
> 
>
https://breakpad.appspot.com/565002/diff/22001/src/common/dwarf_cu_to_module....
> src/common/dwarf_cu_to_module.h:79: FileContext(const string &filename,
> The change of constructor breaks dump_syms on OS X. dump_syms.mm:263 will no
> longer compile

Sure thing.

[Lei Zhang (chromium)]

Next CL for Mac: https://breakpad.appspot.com/592002/