Support profiler stack sampling in any situation.

Support profiler stack sampling in any situation. After this change, almost all profiler ticks (except GC ones) have a stack sample data associated.

Tested under Linux, OS X, and Windows.

Committed: http://code.google.com/p/v8/source/detail?r=1565

[Mikhail Naganov, 2009-03-20 11:15:13]

SConstruct has only trailing spaces removed. Sorry for that.

[Søren Thygesen Gjesse, 2009-03-20 13:31:32]

LGTM

There is a lot of details in this CL, so I might have missed something.

A general coment is regarding using unsigned int for addresses. How about
changing that to Address all over. I know class StackTracer already uses
unsigned int, but that could be changed to Address as well. You could do that in
a new CL.

http://codereview.chromium.org/50052/diff/1/6
File src/frames.cc (right):

http://codereview.chromium.org/50052/diff/1/6#newcode86
Line 86: if (use_top || fp != NULL) {
Isn't fp != NULL sufficient?

http://codereview.chromium.org/50052/diff/1/6#newcode208
Line 208: StackFrame* frame = iterator_.frame();
Maybe call this variable last_frame and prev_frame for frame.

http://codereview.chromium.org/50052/diff/1/6#newcode222
Line 222: bool SafeStackFrameIterator::IsValidFrame(StackFrame* frame) const {
Add a comment on this check (JavaScript frame needs a valid function whereas
other frames does not)

http://codereview.chromium.org/50052/diff/1/6#newcode348
Line 348: (void)GetCallerState(state);
Is the (void) cast required?

http://codereview.chromium.org/50052/diff/1/7
File src/frames.h (right):

http://codereview.chromium.org/50052/diff/1/7#newcode422
Line 422: inline Object* function_no_check() const;
How about naming this function_slot_object instead?

http://codereview.chromium.org/50052/diff/1/7#newcode523
Line 523: explicit StackFrameIterator(bool use_top, Address fp, Address sp);
explicit not needed anymore.

http://codereview.chromium.org/50052/diff/1/7#newcode545
Line 545: void (StackFrameIterator::*advance_)(void);
(void) -> ()

http://codereview.chromium.org/50052/diff/1/7#newcode621
Line 621: Address low_bound, Address high_bound);
explicit not required.

http://codereview.chromium.org/50052/diff/1/8
File src/log.cc (right):

http://codereview.chromium.org/50052/diff/1/8#newcode928
Line 928: msg.Append(",0x%x", reinterpret_cast<int>(sample->stack[i]));
Change int to uint32_t. Btw. why does %p now work anymore?

http://codereview.chromium.org/50052/diff/1/10
File src/platform-win32.cc (right):

http://codereview.chromium.org/50052/diff/1/10#newcode1763
Line 1763: if (sampler_->IsProfiling()) {
Good move!

http://codereview.chromium.org/50052/diff/1/11
File test/cctest/test-log-ia32.cc (right):

http://codereview.chromium.org/50052/diff/1/11#newcode4
Line 4: 
How about changing unsigned int to Address where it is an address in this file?
Then use casting in the use of CHECK_GE macros, or add a CHECK_ADDR_GE macros.
In test-debug.cc I have added helpers for the CHECK_EQ macros for Address, but
these will not help as you need CHECK_GE unless we change these to call helpers
as well, which might be overkill for this.

http://codereview.chromium.org/50052/diff/1/11#newcode34
Line 34: StackTracer* tracer;
Initialize tracer and sampler to NULL.

http://codereview.chromium.org/50052/diff/1/11#newcode49
Line 49: reinterpret_cast<unsigned int>(trace_env.sample) - 10240;
Why is 100 not enough any more?

http://codereview.chromium.org/50052/diff/1/11#newcode210
Line 210: 
Please describe this function (e.g. create a JS function which calls another JS
function with the current fp value).

[Mikhail Naganov, 2009-03-20 14:39:39]

Thanks! OK, I will migrate to using Addresses in TickSampler.

http://codereview.chromium.org/50052/diff/1/6
File src/frames.cc (right):

http://codereview.chromium.org/50052/diff/1/6#newcode86
Line 86: if (use_top || fp != NULL) {
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> Isn't fp != NULL sufficient?

No. It is possible to have a case when neither Top, nor registers contain valid
pointers. Then we do nothing.

http://codereview.chromium.org/50052/diff/1/6#newcode208
Line 208: StackFrame* frame = iterator_.frame();
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> Maybe call this variable last_frame and prev_frame for frame.

Done.

http://codereview.chromium.org/50052/diff/1/6#newcode222
Line 222: bool SafeStackFrameIterator::IsValidFrame(StackFrame* frame) const {
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> Add a comment on this check (JavaScript frame needs a valid function whereas
> other frames does not)

Done.

http://codereview.chromium.org/50052/diff/1/6#newcode348
Line 348: (void)GetCallerState(state);
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> Is the (void) cast required?

No. Just to indicate that the result is thrown away. Ok, removed.

http://codereview.chromium.org/50052/diff/1/7
File src/frames.h (right):

http://codereview.chromium.org/50052/diff/1/7#newcode422
Line 422: inline Object* function_no_check() const;
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> How about naming this function_slot_object instead?

Done.

http://codereview.chromium.org/50052/diff/1/7#newcode523
Line 523: explicit StackFrameIterator(bool use_top, Address fp, Address sp);
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> explicit not needed anymore.

Done.

http://codereview.chromium.org/50052/diff/1/7#newcode545
Line 545: void (StackFrameIterator::*advance_)(void);
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> (void) -> ()

Done.

http://codereview.chromium.org/50052/diff/1/7#newcode621
Line 621: Address low_bound, Address high_bound);
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> explicit not required.

Done.

http://codereview.chromium.org/50052/diff/1/8
File src/log.cc (right):

http://codereview.chromium.org/50052/diff/1/8#newcode928
Line 928: msg.Append(",0x%x", reinterpret_cast<int>(sample->stack[i]));
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> Change int to uint32_t. Btw. why does %p now work anymore?

Done. Under Windows %p doesn't produce the "0x" prefix.

http://codereview.chromium.org/50052/diff/1/10
File src/platform-win32.cc (right):

http://codereview.chromium.org/50052/diff/1/10#newcode1763
Line 1763: if (sampler_->IsProfiling()) {
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> Good move!

Yes. Otherwise we are sampling the moving stack ;)

http://codereview.chromium.org/50052/diff/1/11
File test/cctest/test-log-ia32.cc (right):

http://codereview.chromium.org/50052/diff/1/11#newcode4
Line 4: 
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> How about changing unsigned int to Address where it is an address in this
file?
> Then use casting in the use of CHECK_GE macros, or add a CHECK_ADDR_GE macros.
> In test-debug.cc I have added helpers for the CHECK_EQ macros for Address, but
> these will not help as you need CHECK_GE unless we change these to call
helpers
> as well, which might be overkill for this.

Let's make this change together with using Address in TickSample.

http://codereview.chromium.org/50052/diff/1/11#newcode34
Line 34: StackTracer* tracer;
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> Initialize tracer and sampler to NULL.

Done.

http://codereview.chromium.org/50052/diff/1/11#newcode49
Line 49: reinterpret_cast<unsigned int>(trace_env.sample) - 10240;
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> Why is 100 not enough any more?

The main change here is to use Sample's address as a base (remember, Sample's
address is used as the lower bound), because FP can contain garbage. 100 then
becomes insufficient.

http://codereview.chromium.org/50052/diff/1/11#newcode210
Line 210: 
On 2009/03/20 13:31:33, Søren Gjesse wrote:
> Please describe this function (e.g. create a JS function which calls another
JS
> function with the current fp value).

Done.