Our own logging library to avoid 3rd party dependencies + added some logging to the mac handler

[mochalatte]

Hi Everyone
after lots of playing around with google log, I found that
a) it's support on Tiger, and Liinux on ARM support is lacking.  Unfortunately
it's a very big hammer when it comes to logging libraries, in that it has a lot
of logic for stackwalking, dumping thread states, etc., which means it's
dependency on OS/architecture might make Breakpad more difficult to incorporate
as a 3rd-party library.  I did contact the owner of google log and he's
receptive to making it lighter weight but it's not something we should wait on.
b) it doesn't support custom log line formats, which would not make the logs
parseable inside emacs as a compilation buffer.  I hacked some elisp together to
parse it's output, but it turns out Emacs' next-error function for parsing
compilation errors is not as simple as you might think and I didn't want to go
down that rat's nest.
c) Google log supports some very nice options for turning on/off verbosity from
the command line and that's something I've added to this fork of the Chrome
logging library.

I did look at a few others but in some cases it asn't possible to reference them
as an SVN external and in others they were heavyweight for other reasons(e.g.
logging to the Windows event log)

This will let us get rid of the pre-built libraries.  After this change I'll
move the linux tools that currently depend on them over to this logging library.

Thanks,

Neal

[mochalatte]

THis change set also has a fix for Breakpad to set/restore signal handlers
around a fork() using pthread_atfork() handlers.  

On 2010/02/22 23:11:31, mochalatte wrote:
> Hi Everyone
> after lots of playing around with google log, I found that
> a) it's support on Tiger, and Liinux on ARM support is lacking.  Unfortunately
> it's a very big hammer when it comes to logging libraries, in that it has a
lot
> of logic for stackwalking, dumping thread states, etc., which means it's
> dependency on OS/architecture might make Breakpad more difficult to
incorporate
> as a 3rd-party library.  I did contact the owner of google log and he's
> receptive to making it lighter weight but it's not something we should wait
on.
> b) it doesn't support custom log line formats, which would not make the logs
> parseable inside emacs as a compilation buffer.  I hacked some elisp together
to
> parse it's output, but it turns out Emacs' next-error function for parsing
> compilation errors is not as simple as you might think and I didn't want to go
> down that rat's nest.
> c) Google log supports some very nice options for turning on/off verbosity
from
> the command line and that's something I've added to this fork of the Chrome
> logging library.
> 
> I did look at a few others but in some cases it asn't possible to reference
them
> as an SVN external and in others they were heavyweight for other reasons(e.g.
> logging to the Windows event log)
> 
> This will let us get rid of the pre-built libraries.  After this change I'll
> move the linux tools that currently depend on them over to this logging
library.
> 
> Thanks,
> 
> Neal

[Ted Mielczarek]

Neal,

Are you thinking about replacing the logging in the processor code with this
framework as well? I'd love to get runtime verbosity control there, certainly.

[Ted Mielczarek]

I think you should probably get a second opinion on the mac framework changes
from someone more familiar with that code.

http://breakpad.appspot.com/63002/diff/1/11
File src/client/mac/Framework/Breakpad.mm (right):

http://breakpad.appspot.com/63002/diff/1/11#newcode128
Line 128: logging::DELETE_OLD_LOG_FILE);
This seems like a bad combination of options, logging to /tmp without locking
and with "delete old log file". Doesn't that basically ensure that if you run
two apps using this framework that they'll stomp each other's logs?

http://breakpad.appspot.com/63002/diff/1/6
File src/client/mac/crash_generation/Inspector.mm (right):

http://breakpad.appspot.com/63002/diff/1/6#newcode52
Line 52: #endif
You imported the logging header but didn't actually change the code to use it
here.

http://breakpad.appspot.com/63002/diff/1/7
File src/client/mac/handler/exception_handler.cc (left):

http://breakpad.appspot.com/63002/diff/1/7#oldcode87
Line 87: EXC_MASK_BAD_INSTRUCTION | EXC_MASK_ARITHMETIC | EXC_MASK_BREAKPOINT;
Did you really mean to remove this? Seems like you need a better explanation
than slipping it into a logging patch!

http://breakpad.appspot.com/63002/diff/1/7
File src/client/mac/handler/exception_handler.cc (right):

http://breakpad.appspot.com/63002/diff/1/7#newcode491
Line 491: self->UninstallHandler(false);
You have a lot of changes here, and I must admit I don't know the code well
enough to fully understand what you're doing.

http://breakpad.appspot.com/63002/diff/1/7#newcode556
Line 556: case 1:
Aren't there numeric constants for these somewhere?

Also, we do already have some code that does this in the processor:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/mi...

Do you think that's worth trying to share?

http://breakpad.appspot.com/63002/diff/1/7#newcode816
Line 816: LOG(INFO) << "Memory unprotect operation is a no-op in this build!";
Is this really necessary?

http://breakpad.appspot.com/63002/diff/1/10
File src/client/mac/handler/minidump_generator.cc (right):

http://breakpad.appspot.com/63002/diff/1/10#newcode81
Line 81: << mach_task_self() << ")";
I'd probably say "for current task" or "current process" or something.

http://breakpad.appspot.com/63002/diff/1/2
File src/common/logging.cc (right):

http://breakpad.appspot.com/63002/diff/1/2#newcode3
Line 3: // found in the LICENSE file.
I don't think we have a LICENSE file in Breakpad, FWIW.

http://breakpad.appspot.com/63002/diff/1/2#newcode7
Line 7: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode40
Line 40: #include <algorithm>
pedantry: sort includes alphabetically within their category (libc++,
system/libc, local headers)

http://breakpad.appspot.com/63002/diff/1/2#newcode65
Line 65: #endif
If you're not planning on sharing this file with any other projects, it probably
makes more sense to just #include breakpad_types.h.

http://breakpad.appspot.com/63002/diff/1/2#newcode110
Line 110: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode125
Line 125: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode154
Line 154: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode165
Line 165: #error platform not currently supported
I know you're not using this on Windows yet, but are you ok with leaving this as
a compile error there currently?

http://breakpad.appspot.com/63002/diff/1/2#newcode170
Line 170: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode178
Line 178: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode184
Line 184: #elif defined(OS_FREEBSD)
Doesn't look like you actually have this defined in logging.h.

http://breakpad.appspot.com/63002/diff/1/2#newcode191
Line 191: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode208
Line 208: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode216
Line 216: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode233
Line 233: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode237
Line 237: log_file_name = new std::wstring(module_name);
You should probably use the return value of GetModuleFileName as the second
parameter of the wstring constructor here, since on Windows XP the path won't be
null terminated if it's longer than MAX_PATH.

http://breakpad.appspot.com/63002/diff/1/2#newcode251
Line 251: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode287
Line 287: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode383
Line 383: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode424
Line 424: const char* last_slash = strrchr(file, '\\');
This isn't ifdef OS_WINDOWS, so why is it looking for backslashes?

http://breakpad.appspot.com/63002/diff/1/2#newcode468
Line 468: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode485
Line 485: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode499
Line 499: // logs.  See http://b/1343647
This seems very chromium-specific. Also, if you're going to leave it in, it'd be
nice to not leave a link to your internal bug tracker.

http://breakpad.appspot.com/63002/diff/1/2#newcode522
Line 522: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode534
Line 534: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode548
Line 548: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode580
Line 580: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode588
Line 588: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/2#newcode597
Line 597: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/3
File src/common/logging.h (right):

http://breakpad.appspot.com/63002/diff/1/3#newcode22
Line 22: #endif
Maybe I'm missing something, but this block doesn't seem to account for
compiling on Linux.

http://breakpad.appspot.com/63002/diff/1/3#newcode38
Line 38: // get processed and hang the dialog, or the application might go
crazy.
Doesn't look like you included this capability, so remove the comments.

http://breakpad.appspot.com/63002/diff/1/3#newcode55
Line 55: // identify which repetition is happening.
I think you've got more comments in here that refer to things you've removed.
This comment, for example, doesn't make sense in context. Can you give the
comments a once-over and make sure that they match the reality of what you kept
in place?

http://breakpad.appspot.com/63002/diff/1/3#newcode280
Line 280: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/3#newcode354
Line 354: (!defined(OS_WIN) && defined(NDEBUG) && defined(GOOGLE_CHROME_BUILD))
OS_WINDOWS, also, the rest of this kind of sucks. Can we just condition this on
DEBUG / NDEBUG if these chromium defines aren't set? I don't want to have to
stick a bunch of random defines in my build to build without the debug logging.

I see below that there are basically three states:
1) Debug logging completely disabled at compile time
2) Debug logging compiled in and enabled
3) Debug logging compiled in and disabled, but available to be enabled at
run-time.

Can we make it the default to do 1 & 2 in non-debug / debug builds,
respectively, and have some other goofy define to get 3?

http://breakpad.appspot.com/63002/diff/1/3#newcode371
Line 371: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/3#newcode441
Line 441: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/3#newcode513
Line 513: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/3#newcode675
Line 675: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/3#newcode713
Line 713: #if defined(OS_WIN)
OS_WINDOWS

http://breakpad.appspot.com/63002/diff/1/3#newcode723
Line 723: #if defined(OS_WIN)
OS_WINDOWS

[jimb]

Hi --- it would be awesome to get those prebuild libraries out of there.  Can we
get this patch revised and approved?

[zhurunz]

Hi there,
   Is this the CL trying to cut the deps on libglog.a?
   That will be great!
   Currently only x86 version of libglog.a is available.
   But I need build against x64 on linux as well.
   What is the status on this?

   Thanks a lot!
-Zhurun