Print backtraces on FATAL log messages in debug mode.

Print backtraces on FATAL log messages in debug mode.

This provides basic support for looking up backtrace information on GNU libc systems and in Windows.  The code is only enabled for FATAL log messages in debug mode.  In a release build, it is unlikely that symbols will be available making the backtrace less useful.

[awong, 2009-04-08 03:39:34]

Hey guys,

This is draft one of the CL I'm thinking of sending out.

The CL still has some small issues (eg., lint mistake, accidentally checked in
.sln file, and currently won't link in windows since I need to modify base's
library dependencies and don't know how to do that in an "official" manner).

However, if one of you might be able to take a look at it and see if it even
seems sane, that'd be awesome.

Thanks,
Albert

[scherkus (not reviewing), 2009-04-08 18:05:39]

yeah mostly lint-type stuff

looking good though!

http://codereview.chromium.org/62140/diff/1/2
File base/base.gyp (right):

http://codereview.chromium.org/62140/diff/1/2#newcode529
Line 529: 'debug_util_unittest.cc',
alphabetize filenames, so it should go after data_pack_unittest

don't know why gfx is at the top though haha

http://codereview.chromium.org/62140/diff/1/3
File base/debug_util.h (right):

http://codereview.chromium.org/62140/diff/1/3#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump the copyright year to 2009

http://codereview.chromium.org/62140/diff/1/4
File base/debug_util_posix.cc (right):

http://codereview.chromium.org/62140/diff/1/4#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump the copyright year to 2009

http://codereview.chromium.org/62140/diff/1/4#newcode136
Line 136: for (int i = 0; i < trace_.size(); ++i) {
is vector size_type an int or a size_t?  doesn't matter, just as long as it
doesn't generate a warning (we generally treat warnings as errors)

http://codereview.chromium.org/62140/diff/1/5
File base/debug_util_unittest.cc (right):

http://codereview.chromium.org/62140/diff/1/5#newcode2
Line 2: // Author: ajwong@google.com (Albert Wong)
we use a different copyright header

refer to http://dev.chromium.org/developers/coding-style#TOC-File-headers

http://codereview.chromium.org/62140/diff/1/6
File base/debug_util_win.cc (right):

http://codereview.chromium.org/62140/diff/1/6#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump copyright year

http://codereview.chromium.org/62140/diff/1/6#newcode6
Line 6: #include <dbghelp.h>
alphabetize includes unless windows.h must be included first (wouldn't surprise
me... it's usually the exception to the rule)

http://codereview.chromium.org/62140/diff/1/6#newcode112
Line 112: memcpy(&trace_[0], callers, sizeof(void*) * count);
using sizeof(varname) is preferred, so I guess it would be sizeof(&callers)..
not a huge deal

http://codereview.chromium.org/62140/diff/1/6#newcode121
Line 121: // check fail.
you can check out base/lock.h

http://codereview.chromium.org/62140/diff/1/6#newcode147
Line 147: DWORD64 sym_displacement = 0;
lots of windows code here!  would recommend having cpu look at this file when
you send it out

http://codereview.chromium.org/62140/diff/1/6#newcode161
Line 161: BOOL has_line = SymGetLineFromAddr64(process, frame,
&line_displacement, &line);
line over 80 chars, wrap arguments

http://codereview.chromium.org/62140/diff/1/7
File base/logging.cc (right):

http://codereview.chromium.org/62140/diff/1/7#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
copyright year

[awong, 2009-04-08 21:28:11]

Thanks for the review Andrew!

I fixed the lint issues, and then tried to fix the concurrent access issues.

Will upload a new patch in a bit, and then send it out for review to CPU and
whoever owns the debug_util/logging files.

http://codereview.chromium.org/62140/diff/1/2
File base/base.gyp (right):

http://codereview.chromium.org/62140/diff/1/2#newcode529
Line 529: 'debug_util_unittest.cc',
On 2009/04/08 18:05:40, scherkus wrote:
> alphabetize filenames, so it should go after data_pack_unittest
> 
> don't know why gfx is at the top though haha

fixed...e before i...apparently I don't know my alphabet.

http://codereview.chromium.org/62140/diff/1/3
File base/debug_util.h (right):

http://codereview.chromium.org/62140/diff/1/3#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/04/08 18:05:40, scherkus wrote:
> bump the copyright year to 2009

Done.

http://codereview.chromium.org/62140/diff/1/4
File base/debug_util_posix.cc (right):

http://codereview.chromium.org/62140/diff/1/4#newcode136
Line 136: for (int i = 0; i < trace_.size(); ++i) {
On 2009/04/08 18:05:40, scherkus wrote:
> is vector size_type an int or a size_t?  doesn't matter, just as long as it
> doesn't generate a warning (we generally treat warnings as errors)

Changed to match everyone else.  I don't think it generates a warning because it
is an assign, and C/C++ doesn't warn on coercion from unsigned to signed....only
on comparison (and only in C++.

Stupid huh?  Lots of security bugs...

http://codereview.chromium.org/62140/diff/1/5
File base/debug_util_unittest.cc (right):

http://codereview.chromium.org/62140/diff/1/5#newcode2
Line 2: // Author: ajwong@google.com (Albert Wong)
On 2009/04/08 18:05:40, scherkus wrote:
> we use a different copyright header
> 
> refer to http://dev.chromium.org/developers/coding-style#TOC-File-headers

Done.

http://codereview.chromium.org/62140/diff/1/6
File base/debug_util_win.cc (right):

http://codereview.chromium.org/62140/diff/1/6#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/04/08 18:05:40, scherkus wrote:
> bump copyright year

Done.

http://codereview.chromium.org/62140/diff/1/6#newcode6
Line 6: #include <dbghelp.h>
On 2009/04/08 18:05:40, scherkus wrote:
> alphabetize includes unless windows.h must be included first (wouldn't
surprise
> me... it's usually the exception to the rule)

reordered.

http://codereview.chromium.org/62140/diff/1/6#newcode112
Line 112: memcpy(&trace_[0], callers, sizeof(void*) * count);
On 2009/04/08 18:05:40, scherkus wrote:
> using sizeof(varname) is preferred, so I guess it would be sizeof(&callers)..
> not a huge deal

did sizeof(callers[0]).

http://codereview.chromium.org/62140/diff/1/6#newcode121
Line 121: // check fail.
On 2009/04/08 18:05:40, scherkus wrote:
> you can check out base/lock.h

Thanks. :) Refactored to use that.

http://codereview.chromium.org/62140/diff/1/6#newcode147
Line 147: DWORD64 sym_displacement = 0;
On 2009/04/08 18:05:40, scherkus wrote:
> lots of windows code here!  would recommend having cpu look at this file when
> you send it out

Sounds good.  I'm only 80% confident in my having gotten this right.

http://codereview.chromium.org/62140/diff/1/6#newcode161
Line 161: BOOL has_line = SymGetLineFromAddr64(process, frame,
&line_displacement, &line);
On 2009/04/08 18:05:40, scherkus wrote:
> line over 80 chars, wrap arguments

fixed.

http://codereview.chromium.org/62140/diff/1/7
File base/logging.cc (right):

http://codereview.chromium.org/62140/diff/1/7#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/04/08 18:05:40, scherkus wrote:
> copyright year

Done.

[awong, 2009-04-08 22:14:06]

Hi Adam,

I thought I'd add the backtrace printing logic into LOG(FATAL) on debug builds
(was annoyed when a check popped up without giving me any info on why it
happened).

I also filled out the windows side of the backtrace logic.  Most of these code
paths seemed to be last touched by you.

Please let me know if there's anyone else that should be on the review.  The
windows code probably needs extra checking since I'm not a win32 expert, and am
not entirely sure it's done correctly.

Thanks,
Albert

[agl, 2009-04-08 22:22:00]

I can't comment on the Windows code, but the Linux code looks good with one nit,
below.

http://codereview.chromium.org/62140/diff/1014/31
File build/common.gypi (right):

http://codereview.chromium.org/62140/diff/1014/31#newcode176
Line 176: '-rdynamic',  # Allows backtrace to resolve symbols.
This should only be enabled for Debug builds, not all builds.

[awong, 2009-04-08 23:05:23]

[ +cpu@ ]

Thanks Adam,

I'll move the -rdynamic flag into the Debug section.  will submit a new patch
once I get the mac version passing the base_unittests (I think it needs
-rdynamic too)

Carlos, would you mind taking a look at the windows code in debug_util_win.cc
and letting me know if it's okay?

-Albert

[awong, 2009-04-09 01:41:11]

rdynamic moved.  Mac unittest ifdef-ed.  Apparently there is no rdynamic
equivalent in osX currently.  Lame.

On 2009/04/08 23:05:23, awong wrote:
> [ +cpu@ ]
> 
> Thanks Adam,
> 
> I'll move the -rdynamic flag into the Debug section.  will submit a new patch
> once I get the mac version passing the base_unittests (I think it needs
> -rdynamic too)
> 
> Carlos, would you mind taking a look at the windows code in debug_util_win.cc
> and letting me know if it's okay?
> 
> -Albert

[cpu_(ooo_6.6-7.5), 2009-04-09 21:06:32]

http://codereview.chromium.org/62140/diff/33/1022
File base/debug_util_win.cc (right):

http://codereview.chromium.org/62140/diff/33/1022#newcode14
Line 14: #pragma comment(lib, "dbghelp.lib")
don't forget to make sure dbghelp is being delay loaded. We don't want to get a
startup perf hit.

http://codereview.chromium.org/62140/diff/33/1022#newcode83
Line 83: AutoLock lock(lock_);
what happens if the symbols are not available?
what happens if the internet is slow or broken?

Is it going to go fetch the symbols from MS symbol server?

http://codereview.chromium.org/62140/diff/33/1022#newcode93
Line 93: init_error_ = ERROR_SUCCESS;
do you need this line?

http://codereview.chromium.org/62140/diff/33/1022#newcode107
Line 107: return &lock_;
I am torn about this, can the object not be thread safe? in the only place that
is being used isn't it under a lock?

http://codereview.chromium.org/62140/diff/33/1022#newcode217
Line 217: (*os) << "\t" << trace_[i] << "\n";
don't you want to break if the ostream is in a bad state?

http://codereview.chromium.org/62140/diff/33/1022#newcode224
Line 224: static const int kMaxNameLength = 256;
what does static does for you here?

http://codereview.chromium.org/62140/diff/33/1022#newcode231
Line 231: kMaxNameLength * sizeof(TCHAR) +
don't use TCHAR, we know that we only compile in unicode.

http://codereview.chromium.org/62140/diff/33/1022#newcode246
Line 246: memset(&line, 0, sizeof(IMAGEHLP_LINE64));
line = {0};  ?

http://codereview.chromium.org/62140/diff/33/1023
File base/logging.cc (right):

http://codereview.chromium.org/62140/diff/33/1023#newcode515
Line 515: #endif
I am fine doing this but if we do it as long the app is not being debugged. See
line 518

[awong, 2009-04-14 04:02:40]

Hi Carlos,

I've addressed most of your comments.

There are 3 issues I didn't quite know how to resolve.

  1) Slow symbol resolution if someone specifies a symbol server across the
network in their symbol path.
  2) Interaction with breakpad's use of dbghelp.dll
  3) I can't get the try bots to respect the vsprops changes.

For #1, I'm tempted to punt because, if someone has a debug build, isn't using a
debugger, and has set their symbol path in their environment, they probably know
enough to understand why it might be slow.

For #2, I talked with the breakpad devs.  The issue is a pretty narrow edge
case.   You'd have to be doing a LOG(FATAL) when a second thread crashes to
trigger the bad interaction.  The issue apparently already exists in breakpad
(if multiple threads crash, they may stomp on each other).

For #3, I don't know what to do.  The try bot on windows is barfing because it
can't resolve teh Sym* references.  This is almostly certainly because it isn't
respecting the chagne to essential.vsprops.  The code runs and works on my
desktop, but I can't test it on the try server.

Let me know what you think, and if you have any tips on how to resolve #3.

Thanks,
Albert

http://codereview.chromium.org/62140/diff/33/1022
File base/debug_util_win.cc (right):

http://codereview.chromium.org/62140/diff/33/1022#newcode83
Line 83: AutoLock lock(lock_);
On 2009/04/09 21:06:32, cpu wrote:
> what happens if the symbols are not available?
> what happens if the internet is slow or broken?
> 
> Is it going to go fetch the symbols from MS symbol server?
> 

Reading through the documentation on MSDN, this actually depends on environment
variables.  If someone specifies a remote symbol server, this will talk over the
network, and if it's slow, well, the whole call will be slow.  Looking through
the API, I don't think there's a way to disable remote server lookup.  On the
otherhand, I'm inclined to say that if someone is running a debug build with a
remote server listed in their path, they should know what they're doing...

If the symbols are not available, it just resolves to nothing and looks like
this:
Backtrace:
        (No symbol) [0x0125C5F2]
        (No symbol) [0x01113FE3]
        (No symbol) [0x013A66F8]
        (No symbol) [0x013A7378]
        (No symbol) [0x013A7B32]
        (No symbol) [0x013B0566]
        (No symbol) [0x013A7A9A]
        (No symbol) [0x013AB59B]
        (No symbol) [0x013AEDD6]
        (No symbol) [0x013AB4D4]
        (No symbol) [0x013AA766]
        (No symbol) [0x0118325B]
        (No symbol) [0x011830AD]
        (No symbol) [0x0143CBF3]
        (No symbol) [0x0143C9BD]
        BaseThreadInitThunk [0x77664911+18]
        RtlInitializeExceptionChain [0x778DE4B6+99]
        RtlInitializeExceptionChain [0x778DE489+54]

http://codereview.chromium.org/62140/diff/33/1022#newcode93
Line 93: init_error_ = ERROR_SUCCESS;
On 2009/04/09 21:06:32, cpu wrote:
> do you need this line?

I thought it was more clear to show that init_error_ is set to a known value as
a result of this code path, even though it's redundant with the constructor.

http://codereview.chromium.org/62140/diff/33/1022#newcode107
Line 107: return &lock_;
On 2009/04/09 21:06:32, cpu wrote:
> I am torn about this, can the object not be thread safe? in the only place
that
> is being used isn't it under a lock?
> 

I had a hard time with this too.  On the one-hand, this lock should be used by
any code that calls the Sym* family of functions.  On the other, there's no
solid use case, nor is it clear people would notice this function before coding
up a bug anyways.

I've moved the backtrace generation code into the SymbolContext object and
unexposed the lock.

http://codereview.chromium.org/62140/diff/33/1022#newcode217
Line 217: (*os) << "\t" << trace_[i] << "\n";
On 2009/04/09 21:06:32, cpu wrote:
> don't you want to break if the ostream is in a bad state?

I added a check in the loop condition here and below.

http://codereview.chromium.org/62140/diff/33/1022#newcode224
Line 224: static const int kMaxNameLength = 256;
On 2009/04/09 21:06:32, cpu wrote:
> what does static does for you here?

It's just the mantra I've always used for constant primitives.  Do you want me
to drop it? I feel the static is a bit conceptually correct, and in the case
where the compiler doesn't constant-propagate, it can make a difference (const
char foo[] = "abc" generated an extra memcpy copy on gcc per frame iirc).

http://codereview.chromium.org/62140/diff/33/1022#newcode231
Line 231: kMaxNameLength * sizeof(TCHAR) +
On 2009/04/09 21:06:32, cpu wrote:
> don't use TCHAR, we know that we only compile in unicode.
> 

Change to wchar_t per mappings listed in 
http://msdn.microsoft.com/en-us/library/c426s321(VS.80).aspx

http://codereview.chromium.org/62140/diff/33/1022#newcode246
Line 246: memset(&line, 0, sizeof(IMAGEHLP_LINE64));
On 2009/04/09 21:06:32, cpu wrote:
> line = {0};  ?

Crazy. I didn't realize that was actually spec compliant.  Now I know (8.5.1.7).
 Changed.

http://codereview.chromium.org/62140/diff/33/1023
File base/logging.cc (right):

http://codereview.chromium.org/62140/diff/33/1023#newcode515
Line 515: #endif
On 2009/04/09 21:06:32, cpu wrote:
> I am fine doing this but if we do it as long the app is not being debugged.
See
> line 518

Good point. Moved into else clause.

[cpu_(ooo_6.6-7.5), 2009-04-14 17:25:35]

LGTM on my side.

Ask maruel@ for advice about the delay load part. I am not sure how to help.

http://codereview.chromium.org/62140/diff/33/1022
File base/debug_util_win.cc (right):

http://codereview.chromium.org/62140/diff/33/1022#newcode83
Line 83: AutoLock lock(lock_);
On 2009/04/14 04:02:40, awong wrote:
> On 2009/04/09 21:06:32, cpu wrote:
> > what happens if the symbols are not available?
> > what happens if the internet is slow or broken?
> > 
> > Is it going to go fetch the symbols from MS symbol server?
> > 
> 
> Reading through the documentation on MSDN, this actually depends on
environment
> variables.  If someone specifies a remote symbol server, this will talk over
the
> network, and if it's slow, well, the whole call will be slow.  Looking through
> the API, I don't think there's a way to disable remote server lookup.  On the
> otherhand, I'm inclined to say that if someone is running a debug build with a
> remote server listed in their path, they should know what they're doing...
> 
> If the symbols are not available, it just resolves to nothing and looks like
> this:
> Backtrace:
>         (No symbol) [0x0125C5F2]
>         (No symbol) [0x01113FE3]
>         (No symbol) [0x013A66F8]
>         (No symbol) [0x013A7378]
>         (No symbol) [0x013A7B32]
>         (No symbol) [0x013B0566]
>         (No symbol) [0x013A7A9A]
>         (No symbol) [0x013AB59B]
>         (No symbol) [0x013AEDD6]
>         (No symbol) [0x013AB4D4]
>         (No symbol) [0x013AA766]
>         (No symbol) [0x0118325B]
>         (No symbol) [0x011830AD]
>         (No symbol) [0x0143CBF3]
>         (No symbol) [0x0143C9BD]
>         BaseThreadInitThunk [0x77664911+18]
>         RtlInitializeExceptionChain [0x778DE4B6+99]
>         RtlInitializeExceptionChain [0x778DE489+54]
> 

Ok. We shall give it  a try as is.

http://codereview.chromium.org/62140/diff/33/1022#newcode107
Line 107: return &lock_;
On 2009/04/14 04:02:40, awong wrote:
> On 2009/04/09 21:06:32, cpu wrote:
> > I am torn about this, can the object not be thread safe? in the only place
> that

Ok
> > is being used isn't it under a lock?
> > 
> 
> I had a hard time with this too.  On the one-hand, this lock should be used by
> any code that calls the Sym* family of functions.  On the other, there's no
> solid use case, nor is it clear people would notice this function before
coding
> up a bug anyways.
> 
> I've moved the backtrace generation code into the SymbolContext object and
> unexposed the lock.

http://codereview.chromium.org/62140/diff/33/1022#newcode224
Line 224: static const int kMaxNameLength = 256;
On 2009/04/14 04:02:40, awong wrote:
> On 2009/04/09 21:06:32, cpu wrote:
> > what does static does for you here?
> 
> It's just the mantra I've always used for constant primitives.  Do you want me
> to drop it? I feel the static is a bit conceptually correct, and in the case
> where the compiler doesn't constant-propagate, it can make a difference (const
> char foo[] = "abc" generated an extra memcpy copy on gcc per frame iirc).

I think is better to drop it for style consistency across the chrome codebase.
If in some special case it shows this is a perf gain we can use it there.

[awong, 2009-04-14 18:50:12]

[ +maruel ]

Could you check over the vsprops changes for essential.vsprops?  I'm trying to
add a delay load to dbghelp.dll for use in the backtrace generation code in
base/debug_util_win.cc.

Caveat: I haven't been able to make this build on the try bots.  They don't seem
to be respecting the vsprops changes.

Since this library dependency is now part of base, I think the dbghelp.dll needs
to be linked always.  Alternately, we could conditionally compile out the
backtrace code and move dbghelp.dll into debug.vsprops, but seems like
unnecessary forking of logic in the implementation level (the call to generate a
trace is already protected by an ifndef).

The rest of the code has been reviewed by scherkus, agl, and cpu.

Thanks,
Albert

[M-A Ruel, 2009-04-14 18:59:13]

lgtm

Yep, VS is dumb about vsprops. I'd recommend you to check-in the vsprops
separately first, warn folks about rebuilding on chromium-dev and then check-in
the rest maybe one day or two later. That would simplify the life of everyone.

http://codereview.chromium.org/62140/diff/4001/5005
File base/debug_util_win.cc (right):

http://codereview.chromium.org/62140/diff/4001/5005#newcode242
Line 242: static const int kMaxCallers = 256;
Remove static here.

[awong, 2009-04-14 19:32:44]

On 2009/04/14 18:59:13, M-A wrote:
> lgtm
> 
> Yep, VS is dumb about vsprops. I'd recommend you to check-in the vsprops
> separately first, warn folks about rebuilding on chromium-dev and then
check-in
> the rest maybe one day or two later. That would simplify the life of everyone.

Sent you a review here:

http://codereview.chromium.org/67137/show

 
> http://codereview.chromium.org/62140/diff/4001/5005
> File base/debug_util_win.cc (right):
> 
> http://codereview.chromium.org/62140/diff/4001/5005#newcode242
> Line 242: static const int kMaxCallers = 256;
> Remove static here.

fixed in local client.  Will upload patch once the vsprops is checked in and
synced back down.

Thanks,
Albert

[M-A Ruel, 2009-04-14 20:14:31]

BTW, you can't check-in this change as-is. You need to update chrome.dll.deps.
Do a release build and you'll see.

[awong, 2009-04-16 02:04:11]

On 2009/04/14 20:14:31, M-A wrote:
> BTW, you can't check-in this change as-is. You need to update chrome.dll.deps.
> Do a release build and you'll see.

I did a release build, and it seemed to work without the change.  However, I
added it anyways.

The lastest patch is #6. I also updated the code in the debug_util_posix.cc to
remove the static const.

Let me know if there are issues.  Otherwise, I will check in on friday morning.

[M-A Ruel, 2009-04-16 10:08:50]

On 2009/04/16 02:04:11, awong wrote:
> On 2009/04/14 20:14:31, M-A wrote:
> > BTW, you can't check-in this change as-is. You need to update
chrome.dll.deps.
> > Do a release build and you'll see.
> 
> I did a release build, and it seemed to work without the change.  However, I
> added it anyways.
> 
> The lastest patch is #6. I also updated the code in the debug_util_posix.cc to
> remove the static const.
> 
> Let me know if there are issues.  Otherwise, I will check in on friday
morning.

In fact I was wrong. Your patch is debug only, so chrome.dll.deps shouldn't be
modified after all. In fact, you shouldn't have modified essential.vsprops but
debug.vsprops instead if you wanted to add this dependency in debug. My bad. If
you do this change, nobody will need to clobber since it is removing an implicit
dependency so it'll build fine with the previous settings. It's more a "safety
measure" to make sure dbghelp is not imported in release.

[awong, 2009-04-17 03:51:34]

On 2009/04/16 10:08:50, M-A wrote:
> On 2009/04/16 02:04:11, awong wrote:
> > On 2009/04/14 20:14:31, M-A wrote:
> > > BTW, you can't check-in this change as-is. You need to update
> chrome.dll.deps.
> > > Do a release build and you'll see.
> > 
> > I did a release build, and it seemed to work without the change.  However, I
> > added it anyways.
> > 
> > The lastest patch is #6. I also updated the code in the debug_util_posix.cc
to
> > remove the static const.
> > 
> > Let me know if there are issues.  Otherwise, I will check in on friday
> morning.
> 
> In fact I was wrong. Your patch is debug only, so chrome.dll.deps shouldn't be
> modified after all. In fact, you shouldn't have modified essential.vsprops but
> debug.vsprops instead if you wanted to add this dependency in debug. My bad.
If
> you do this change, nobody will need to clobber since it is removing an
implicit
> dependency so it'll build fine with the previous settings. It's more a "safety
> measure" to make sure dbghelp is not imported in release.

hmm...looks like gmail might have ate my reply.

I think dbghelp.dll needs to be linked in release and debug mode even if the
code path is never hit in release.  This is because the library function for
generating the stack trace still exists in release mode.

Is it possible to leave it in essential.vsprops and maybe not put it in
chrome.dll.deps?

The alternative would be to #ifndef NDEBUG the whole code block, which feels
ugly as well.

Let me know which way I should go.  Ideally I'd like to get this code checked in
this friday if possible so I can feel like I'm making progress. :)

Thanks,
Albert

[M-A Ruel, 2009-04-18 01:16:32]

lgtm, please check-in. Apologies for the delay.
We discussed offline about refactoring some code in debug_util_win.cc but
nothing that should hold off this commit.

[awong, 2009-04-22 22:19:34]

One last pass.  This time for sure!

I added
  (a) handling of not getting any stack frames.
  (b) aborting the unittest (instead of failing) if we have no symbols.
  (c) delay load of dbghelp.dll into common.gypi since essential.vsprops is no
longer being read by all vsprojs (in particular net/tools/tld_cleanup)

(b) will make the test less "tight", but I think it's a sensible tradeoff.

-Albert

[scherkus (not reviewing), 2009-04-23 22:36:54]

LGTM

http://codereview.chromium.org/62140/diff/10001/10004
File base/debug_util_posix.cc (right):

http://codereview.chromium.org/62140/diff/10001/10004#newcode120
Line 120: // number, but window's but since we need a 0-check, we might as well
this comment is confusing me, especially the "but window's but" part

http://codereview.chromium.org/62140/diff/10001/10006
File base/debug_util_win.cc (right):

http://codereview.chromium.org/62140/diff/10001/10006#newcode210
Line 210: };
is DISALLOW_COPY_AND_ASSIGN needed here?

[stoyan, 2009-04-23 23:50:18]

IIRC if you have symbols around, it's better to use StackWalk64() instead of
RtlCaptureStackBackTrace(), since it will consider the FPO.

http://codereview.chromium.org/62140/diff/9030/8016
File base/debug_util_win.cc (right):

http://codereview.chromium.org/62140/diff/9030/8016#newcode258
Line 258: int count = CaptureStackBackTrace(0, kMaxCallers, callers, NULL);
From MSDN: "Windows Server 2003 and Windows XP:  The sum of the FramesToSkip and
FramesToCapture parameters must be less than 64."

In practice it has to be less than 63, otherwise you will always get 0 as result
(at least on XPSP2, don't have win2003 around).