Skip samples where top function's stack frame is not setup properly

Skip samples where top function's stack frame is not setup properly

Stack iterator takes return address based on the frame pointer (ebp) and detects JS frames based on value at fp + StandardFrameConstants::kMarkerOffset. So in order the iterator to work correctly this values should be already setup for the current function. Stack frame is constructed at the very beginning of JS function code and destroyed before return. If sample is taken before before the frame construction is completed or after it was destroyed the stack iterator will wrongly think that FP points at the current functions frame base and will skip callers frame. To avoid this we mark code ranges where  stack frame doesn't exist and completely ignore such samples.

This fixes cctest/test-cpu-profiler/CollectCpuProfile flakiness.

BUG=v8:2628
R=jkummerow@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=14670

[yurys, 2013-04-26 13:24:35]

The patch provides implementation only for x64. I'd like to get feedback on the
approach in general before moving forward with other platforms.

[loislo, 2013-04-26 13:36:56]

https://codereview.chromium.org/14253015/diff/2001/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

https://codereview.chromium.org/14253015/diff/2001/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:510: printf("CompilationInfo size = %lu\n",
sizeof(CompilationInfo));
please remove that

https://codereview.chromium.org/14253015/diff/2001/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:510: printf("CompilationInfo size = %lu\n",
sizeof(CompilationInfo));
please remove that

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator-inl.h
File src/profile-generator-inl.h (right):

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator-inl....
src/profile-generator-inl.h:59: frame_setup_offset_(-1),
please use -1 or the constant everywhere

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator-inl....
src/profile-generator-inl.h:59: frame_setup_offset_(-1),
please use -1 or the constant everywhere

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator.cc
File src/profile-generator.cc (right):

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator.cc#n...
src/profile-generator.cc:915: return;
looks like you are skipping the other stack traces.

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator.cc#n...
src/profile-generator.cc:919: return;
ditto

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator.cc#n...
src/profile-generator.cc:921: *entry++ = pc_entry;
sounds like you are adding zero entry if the mapping wasn't found.

[yurys, 2013-04-26 13:43:51]

https://codereview.chromium.org/14253015/diff/2001/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

https://codereview.chromium.org/14253015/diff/2001/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:510: printf("CompilationInfo size = %lu\n",
sizeof(CompilationInfo));
On 2013/04/26 13:36:56, loislo wrote:
> please remove that

Done.

https://codereview.chromium.org/14253015/diff/2001/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:510: printf("CompilationInfo size = %lu\n",
sizeof(CompilationInfo));
On 2013/04/26 13:36:56, loislo wrote:
> please remove that

Done.

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator.cc
File src/profile-generator.cc (right):

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator.cc#n...
src/profile-generator.cc:915: return;
On 2013/04/26 13:36:56, loislo wrote:
> looks like you are skipping the other stack traces.
Yes, this is intentional, the comment above describes that.

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator.cc#n...
src/profile-generator.cc:919: return;
On 2013/04/26 13:36:56, loislo wrote:
> ditto
See above.

https://codereview.chromium.org/14253015/diff/2001/src/profile-generator.cc#n...
src/profile-generator.cc:921: *entry++ = pc_entry;
On 2013/04/26 13:36:56, loislo wrote:
> sounds like you are adding zero entry if the mapping wasn't found.

This part didn't change, it is normal case - pc could have pointed into some
stub code e.g.

[loislo, 2013-04-26 13:46:19]

https://codereview.chromium.org/14253015/diff/5001/src/x64/lithium-codegen-x6...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/14253015/diff/5001/src/x64/lithium-codegen-x6...
src/x64/lithium-codegen-x64.cc:2547:
info_->set_frame_destroy_offset(masm_->pc_offset());
do we have the same code on the other platforms

[yurys, 2013-04-26 13:52:32]

On 2013/04/26 13:46:19, loislo wrote:
>
https://codereview.chromium.org/14253015/diff/5001/src/x64/lithium-codegen-x6...
> File src/x64/lithium-codegen-x64.cc (right):
> 
>
https://codereview.chromium.org/14253015/diff/5001/src/x64/lithium-codegen-x6...
> src/x64/lithium-codegen-x64.cc:2547:
> info_->set_frame_destroy_offset(masm_->pc_offset());
> do we have the same code on the other platforms
Of cause. I just want to collect some feedback on the approach first.

[loislo, 2013-04-26 16:11:00]

sounds good to me

[yurys, 2013-04-30 15:57:25]

Added support for the rest architectures. PTAL.

[yurys, 2013-05-12 17:28:07]

Jakob could you review this?

[Jakob Kummerow, 2013-05-14 12:38:51]

LGTM with nits.

Please also update the test expectations for the CPU profiler test (see
https://code.google.com/p/v8/source/browse/branches/bleeding_edge/test/cctest...
)

https://codereview.chromium.org/14253015/diff/14001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/14253015/diff/14001/src/arm/full-codegen-arm....
src/arm/full-codegen-arm.cc:431: Label exit_codesize;
I don't see what we need this label for.

https://codereview.chromium.org/14253015/diff/14001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/14253015/diff/14001/src/compiler.cc#newcode106
src/compiler.cc:106: no_frame_ranges_ = isolate->cpu_profiler()->is_profiling()
?
nit: putting the '?' on the second line is preferable.

https://codereview.chromium.org/14253015/diff/14001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/14253015/diff/14001/src/compiler.h#newcode268
src/compiler.h:268: // samples where top frame is not setup.
nit: s/setup/set up/

https://codereview.chromium.org/14253015/diff/14001/src/profile-generator.cc
File src/profile-generator.cc (right):

https://codereview.chromium.org/14253015/diff/14001/src/profile-generator.cc#...
src/profile-generator.cc:547: *start = locator.key();
nit: {} please

https://codereview.chromium.org/14253015/diff/14001/src/profile-generator.cc#...
src/profile-generator.cc:912: // If pc is in the function code before it setup
stack frame or after the
nit: s/setup/set up/

https://codereview.chromium.org/14253015/diff/14001/src/profile-generator.cc#...
src/profile-generator.cc:914: // ebp cotains return address of the current
function and skips caller's
nit: contains

[yurys, 2013-05-14 22:51:19]

https://codereview.chromium.org/14253015/diff/14001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/14253015/diff/14001/src/arm/full-codegen-arm....
src/arm/full-codegen-arm.cc:431: Label exit_codesize;
On 2013/05/14 12:38:51, Jakob wrote:
> I don't see what we need this label for.

Removed. Thanks.

https://codereview.chromium.org/14253015/diff/14001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/14253015/diff/14001/src/compiler.cc#newcode106
src/compiler.cc:106: no_frame_ranges_ = isolate->cpu_profiler()->is_profiling()
?
On 2013/05/14 12:38:51, Jakob wrote:
> nit: putting the '?' on the second line is preferable.

Done.

https://codereview.chromium.org/14253015/diff/14001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/14253015/diff/14001/src/compiler.h#newcode268
src/compiler.h:268: // samples where top frame is not setup.
On 2013/05/14 12:38:51, Jakob wrote:
> nit: s/setup/set up/

Done.

https://codereview.chromium.org/14253015/diff/14001/src/profile-generator.cc
File src/profile-generator.cc (right):

https://codereview.chromium.org/14253015/diff/14001/src/profile-generator.cc#...
src/profile-generator.cc:547: *start = locator.key();
On 2013/05/14 12:38:51, Jakob wrote:
> nit: {} please

Done.

https://codereview.chromium.org/14253015/diff/14001/src/profile-generator.cc#...
src/profile-generator.cc:912: // If pc is in the function code before it setup
stack frame or after the
On 2013/05/14 12:38:51, Jakob wrote:
> nit: s/setup/set up/

Done.

https://codereview.chromium.org/14253015/diff/14001/src/profile-generator.cc#...
src/profile-generator.cc:914: // ebp cotains return address of the current
function and skips caller's
On 2013/05/14 12:38:51, Jakob wrote:
> nit: contains

Done.

[yurys, 2013-05-14 22:51:49]

Committed patchset #7 manually as r14670 (presubmit successful).

[Vyacheslav Egorov (Google), 2013-06-03 13:51:03]

Late DBC

https://codereview.chromium.org/14253015/diff/29001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/14253015/diff/29001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:2782: void LCodeGen::DoReturn(LReturn* instr) {
optimized code can have arbitrary many HReturns unlike non-optimized code which
branches to the single return sequence from all return points.

[yurys, 2013-06-04 05:59:25]

https://codereview.chromium.org/14253015/diff/29001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/14253015/diff/29001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:2782: void LCodeGen::DoReturn(LReturn* instr) {
On 2013/06/03 13:51:03, Vyacheslav Egorov (Google) wrote:
> optimized code can have arbitrary many HReturns unlike non-optimized code
which
> branches to the single return sequence from all return points.

Profiler part is designed to allow multiple returns, did I miss some points in
the code generator where returns should be reported?