Implement a network daemon + networked implementation of SymbolSupplier and SourceLineResolver

Attached is a first cut at a NetworkSourceLineServer as well as a
NetworkSourceLineResolver. The Server contains a SymbolSupplier and
SourceLineResolver, and listens for requests via UDP from clients. The
NetworkSourceLineResolver class implements both the SymbolSupplier and
SourceLineResolverInterface, proxying requests to the server via UDP. The
primary benefit here is that once the server has loaded a particular symbol
file, it can simply keep it in memory for quick access.

A simple test with a Firefox minidump + the associated symbols shows that on my
machine, the current minidump_stackwalk takes about 1.5 seconds, a first run of
minidump_stackwalk using NetworkSourceLineResolver takes about 6.5 seconds, and
following runs take about 0.35 seconds (when the server already has all symbols
in memory).

The patch needs a once-over for style and other nits, and also has three major
caveats right now:
1) No unit tests for added functionality--I'll write them before we get to
landing, I just didn't get around to it yet.
2) No way to evict loaded modules/symbol files. This means that the server will
grow in memory size until it runs out. I think fixing this won't be too hard,
just need to add a LRU cache of symbol files, then add a method to
BasicSourceLineResolver to let it remove a loaded module.
3) The server is single-threaded, which means that multiple clients hitting the
same server could cause delays, mostly when the server is forced to load symbol
files. Adding a bunch of locking doesn't sound very fun, and probably doesn't
solve the problem much anyway, since you'd just be waiting on a lock instead of
waiting on the server. I'm not sure how much of a problem this would be in
practice, it bears testing.

There's also a hack in this patch that's just for testing, I switched
minidump_stackwalk to use NetworkSourceLineResolver.

[Mark Mentovai]

This is a big change that I likely won't have time to review fully today.

I looked through the interfaces and they all look very reasonable.  There are a
few coding-style things I spotted - some of the indents look funny and there
were a few lines where you blew past 80 characters.

The protocol looks mostly all right, although I would prefer length-encoded to
whitespace or |-separated fields, although if you had a good way to escape the
field delimiters that would be OK too.

[Ted Mielczarek]

Yeah, sorry, this isn't actually review-ready yet, I just wanted to get it up
here to give you guys a heads-up and make sure that there were no architectural
changes needed. I'll give it a once over with the style guide and cleanup some
of my leftover TODOs before I ask you to review.

The protocol was just the simplest thing I could get working. I used ASCII just
because it was dead simple to test as I went along. I could definitely change
that if you'd prefer (and just encode integer values in binary instead of
formatting them out in ascii). Should be simple enough to do once I've written
unit tests.

[Ted Mielczarek]

Updated patch

[Ted Mielczarek]

On 2010/03/02 15:59:40, Ted Mielczarek wrote:
> Updated patch

Just some notes on what I did since the last patch, since it's been a while:
* implemented a binarystream class that supports serializing and deserializing
integers of known size as well as strings, use that for constructing network
packets
* pulled out the networking code into a UDPNetwork class which implements a
NetworkInterface for easier unit testing
* wrote a bunch of unit tests
* made NetworkSourceLineServer track the number of lines of symbols loaded as a
crude metric for memory usage, added the ability to unload modules to
BasicSourcelineResolver, unload symbols when over a specified limit. This should
make this usable in a production environment without exhausting memory.

There's probably more, it's been a while and I've forgotten all the details.

This also probably needs merging with Jim's patch on issue 53010, but I can deal
with that.

[Mark Mentovai]

This is headed in a nice direction.  It certainly will require some merge work
with Jim's big CFI patch.

http://breakpad.appspot.com/36001/diff/2001/2025
File src/google_breakpad/processor/network_source_line_resolver.h (right):

http://breakpad.appspot.com/36001/diff/2001/2025#newcode45
Line 45: #include <sys/socket.h>
C system headers should precede C++ library headers.

http://breakpad.appspot.com/36001/diff/2001/2025#newcode62
Line 62: // The server and port to connect to, and the
Be mindful of the style guide in here - specifically, the indent.

http://breakpad.appspot.com/36001/diff/2001/2025#newcode66
Line 66: int wait_time = 5000);
hmm, default arguments...style sez "no".

It also says that when you've got a variable that holds something measurable,
like time, that the name reflect the units that it's measured in.  wait_time is
what here, seconds?  thousandths of minutes?  I don't know!

http://breakpad.appspot.com/36001/diff/2001/2025#newcode128
Line 128: typedef std::map<u_int64_t, linked_ptr<WindowsFrameInfo> >
WindowsFrameCache;
80

http://breakpad.appspot.com/36001/diff/2001/2025#newcode156
Line 156: #endif  // GOOGLE_BREAKPAD_PROCESSOR_NETWORK_SOURCE_LINE_RESOLVER_H__
One trailing underscore on new files.

http://breakpad.appspot.com/36001/diff/2001/2007
File src/processor/basic_source_line_resolver.cc (right):

http://breakpad.appspot.com/36001/diff/2001/2007#newcode518
Line 518: if (!Tokenize(file_line, " \r\n", 2, &tokens)) {
These repeated " \r\n" things should be a constant.

http://breakpad.appspot.com/36001/diff/2001/2016
File src/processor/binarystream.h (right):

http://breakpad.appspot.com/36001/diff/2001/2016#newcode48
Line 48: explicit binarystream(ios_base::openmode which =
ios_base::out|ios_base::in)
more indentation (won't mention it again).

http://breakpad.appspot.com/36001/diff/2001/2016#newcode86
Line 86: } // namespace google_breakpad
(At least) two spaces between the last live character on a line and the //. 
Throughout.

http://breakpad.appspot.com/36001/diff/2001/2016#newcode88
Line 88: #endif // __GOOGLE_BREAKPAD_BINARYSTREAM_H_
Use the right name for this.  Leading __ is not within our control.

http://breakpad.appspot.com/36001/diff/2001/2011
File src/processor/network_interface.h (right):

http://breakpad.appspot.com/36001/diff/2001/2011#newcode41
Line 41: virtual ~NetworkInterface() {};
; not helpful.  :)

http://breakpad.appspot.com/36001/diff/2001/2011#newcode47
Line 47: // Send length bytes of data to the current address.
Blank line before, and similar for all of the other methods.  It keeps things
more readable when your eye has vertical whitespace to tab to.

http://breakpad.appspot.com/36001/diff/2001/2011#newcode59
Line 59: } // namespace google_breakpad
again (spaces before //)

http://breakpad.appspot.com/36001/diff/2001/2011#newcode60
Line 60: #endif // _GOOGLE_BREAKPAD_NETWORK_INTERFACE_H_
again (naming and spaces before //)

http://breakpad.appspot.com/36001/diff/2001/2005
File src/processor/network_source_line_protocol.h (right):

http://breakpad.appspot.com/36001/diff/2001/2005#newcode35
Line 35: // Requests are sent via UDP. All requests begin with a sequence number
Are we worried about datagram length restrictions?

http://breakpad.appspot.com/36001/diff/2001/2005#newcode36
Line 36: // that is prepended to the response. The sequence number is opaque
seq is allowed to be anything, right?  They don't actually need to be
sequential?

http://breakpad.appspot.com/36001/diff/2001/2005#newcode95
Line 95: // Lookup source line info for this module, loaded at this base
address,
As a verb, "look up" (two words).

http://breakpad.appspot.com/36001/diff/2001/2005#newcode126
Line 126: const u_int8_t OK    = 1;
Use enum for these.  You can have an enum group for each block.

http://breakpad.appspot.com/36001/diff/2001/2005#newcode145
Line 145: } // namespace source_line_protocol
spaces before //, and on the next line

http://breakpad.appspot.com/36001/diff/2001/2008
File src/processor/network_source_line_resolver.cc (right):

http://breakpad.appspot.com/36001/diff/2001/2008#newcode62
Line 62: {
curly belongs on previous (also at 73 and 79)

http://breakpad.appspot.com/36001/diff/2001/2008#newcode119
Line 119: message << HAS
This comes out pretty nicely.

[Ted Mielczarek]

Updated with mark's comments

[Ted Mielczarek]

On 2010/03/08 14:37:23, Ted Mielczarek wrote:
> Updated with mark's comments

Hopefully interdiff works here, if not I can provide a diff of the changes. I
scoured my patch for all instances of formatting details you commented on, and I
think I fixed them all.

I didn't make the enum change you suggested in the protocol header, for the
reasons I mentioned in a previous email (which I apparently didn't send from the
review app, so it doesn't show up here).

[Mark Mentovai]

I don't really have the resources to give the entire patch another read-through
now, but based on what I saw during the first few passes, I'm satisfied with
where this stands right now.

[Ted Mielczarek]

Merge to tip, add CFI support

[Ted Mielczarek]

On 2010/04/07 16:31:49, Ted Mielczarek wrote:
> Merge to tip, add CFI support

Jim, can you give the latest changes a once-over? I've added CFI support and
that involved touching CFIStackInfo a little bit.

I've also added a round-trip system test,
network_source_line_resolver_server_unittest.cc that uses both
NetworkSourceLineServer and NetworkSourceLineResolver and verifies that
processing a dump works correctly.

[jimb]

The CFIFrameInfo changes look good.  Looking at the others...

[jimb]

Hi, Ted.  The CFI-related stuff all LGreatTM.  Here are some comments I had
looking over the rest of the code.

http://breakpad.appspot.com/36001/diff/16001/17016
File src/processor/network_interface.h (right):

http://breakpad.appspot.com/36001/diff/16001/17016#newcode54
Line 54: virtual bool WaitToReceive(int timeout) = 0;
You could use -1 as "infinite timeout", and then get rid of the five-second
heartbeat in NetworkSourceLineServer::RunForever.

http://breakpad.appspot.com/36001/diff/16001/17017
File src/processor/network_source_line_protocol.h (right):

http://breakpad.appspot.com/36001/diff/16001/17017#newcode40
Line 40: // <foo:N>, which should be read as "foo, an N byte unsigned integer".
"in network byte order"

http://breakpad.appspot.com/36001/diff/16001/17018
File src/processor/network_source_line_resolver.cc (right):

http://breakpad.appspot.com/36001/diff/16001/17018#newcode128
Line 128: return false;
Google doesn't say anything about this, as far as I know, but it's apparently
Mozilla practice to always make the non-error path the main path through the
function, and write error checks with early returns.

http://breakpad.appspot.com/36001/diff/16001/17021
File src/processor/network_source_line_server.cc (right):

http://breakpad.appspot.com/36001/diff/16001/17021#newcode59
Line 59: using namespace source_line_protocol;
The Google C++ Style Guide forbids "using namespace" directives.

http://breakpad.appspot.com/36001/diff/16001/17021#newcode330
Line 330: // shared.
Would it cause trouble to use -1 here?  If clients ever do start caring about
the difference between FPO and FRAME_DATA records, it'd be nice to have them
fail immediately.

[Ted Mielczarek]

Thanks for the reviews. Anything I didn't respond to I probably just fixed.

http://breakpad.appspot.com/36001/diff/16001/17016
File src/processor/network_interface.h (right):

http://breakpad.appspot.com/36001/diff/16001/17016#newcode54
Line 54: virtual bool WaitToReceive(int timeout) = 0;
On 2010/04/07 19:51:35, jimb wrote:
> You could use -1 as "infinite timeout", and then get rid of the five-second
> heartbeat in NetworkSourceLineServer::RunForever.
> 

This isn't a bad idea, but I don't think it'd let me refactor RunForever like
that unless I also refactor RunOnce to loop until time is expired, since
WaitToReceive still has to return if data is available.

http://breakpad.appspot.com/36001/diff/16001/17021
File src/processor/network_source_line_server.cc (right):

http://breakpad.appspot.com/36001/diff/16001/17021#newcode59
Line 59: using namespace source_line_protocol;
On 2010/04/07 19:51:35, jimb wrote:
> The Google C++ Style Guide forbids "using namespace" directives.

That's kind of lame. I understand the point of the style guide, but I think it's
penalizing me here for trying to be nice and using a sub-namespace in my header.
Note that if I dropped the sub-namespace entirely I wouldn't be violating the
style guide despite having the exact same effect!

Anyway, I'll follow the letter of the law and use namespace aliasing.

http://breakpad.appspot.com/36001/diff/16001/17021#newcode330
Line 330: // shared.
On 2010/04/07 19:51:35, jimb wrote:
> Would it cause trouble to use -1 here?  If clients ever do start caring about
> the difference between FPO and FRAME_DATA records, it'd be nice to have them
> fail immediately.

The parsing code (in windows_frame_info.h) uses strtoull, which would fail on
that. It's storing the values in a u_int64_t, so changing it to non-unsigned
seems wrong.

[thakis]

https://breakpad.appspot.com/36001/diff/16001/src/processor/tokenize.cc
File src/processor/tokenize.cc (right):

https://breakpad.appspot.com/36001/diff/16001/src/processor/tokenize.cc#newco...
src/processor/tokenize.cc:60: if (!remaining > 0) {
This is supposed to read `if (remaining > 0)`, not `if (!remaining > 0)`, right?
clang's -Wlogical-not-parentheses warns about this with a recent clang, and from
looking at the code it looks like the warning is correct.