Correctly setup exit frame when calling into allocation tracker

Correctly setup exit frame when calling into allocation tracker

The change introduces RecordObjectAllocationStub which is passed newly allocated object address and size as values on the stack. The stub constructs EXIT frame, saves registers, puts arguments into registers in accord with the calling convention and calls into the HeapProfiler.

allow_builtins_on_stack flag was added to stack iterators to let them know that we may start iterating stack when paused inside a builtin (this is not uncommon to have heap allocations inside builtins) and that the assert inside StackFrame::ComputeType should be relaxed.

Before this change we didn't write exit frame pointer into Isolate::c_entry_fp so the stack iteration would fail in many cases.

BUG=None

[yurys, 2013-10-25 09:47:10]

I've got a test for this change (https://codereview.chromium.org/43713002/)
which I'd love to include into this patch but the test wouldn't work because of
https://code.google.com/p/v8/issues/detail?id=2960.

[loislo, 2013-10-28 07:50:30]

lgtm

[yurys, 2013-10-28 07:51:17]

Hannes, can you do OWNER's review please.

[loislo, 2013-10-28 08:40:48]

The patch is failing on x64.debug 

Program received signal SIGSEGV, Segmentation fault.
0x00000000009275da in v8::internal::MemoryChunk::skip_list (this=0x7ffffff00000)
at ../src/spaces.h:634
634	    return skip_list_;

[loislo, 2013-10-28 08:46:12]

On 2013/10/28 08:40:48, loislo wrote:
> The patch is failing on x64.debug 
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00000000009275da in v8::internal::MemoryChunk::skip_list
(this=0x7ffffff00000)
> at ../src/spaces.h:634
> 634	    return skip_list_;

/src/loislo/chromium/src/v8/out/x64.debug/cctest --stress-opt --always-opt
test-heap-profiler/TrackHeapAllocations

[yurys, 2013-10-30 10:40:08]

Fixed crash. Also introduced new code stub that makes all preparation and calls
HeapProfiler::RecordObjectAllocationFromMasm. PTAL.

[loislo, 2013-10-30 11:30:41]

lgtm

[alph, 2013-10-30 11:44:27]

lgtm

https://codereview.chromium.org/43693002/diff/150001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/43693002/diff/150001/src/code-stubs.h#newcode...
src/code-stubs.h:1404: class RecordObjectAllocationStub : public
PlatformCodeStub {
Consider overriding PrintName function.

[yurys, 2013-10-30 11:59:53]

https://codereview.chromium.org/43693002/diff/150001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/43693002/diff/150001/src/code-stubs.h#newcode...
src/code-stubs.h:1404: class RecordObjectAllocationStub : public
PlatformCodeStub {
On 2013/10/30 11:44:27, alph wrote:
> Consider overriding PrintName function.

Done.

[Yang, 2013-10-30 15:01:24]

I do not agree with this change yet:
- We are introducing a new flag to the StackTraceFrameIterator and all its
parent classes just to disable an assert.
- Do we actually understand why the stackframe iterator cannot work when being
called from a builtin (I assume that you are referring to C++ builtins in like
ArrayShift)? I was under the assumption that the StackFrameIterator should be
able to handle all types of frames on the stack.

https://codereview.chromium.org/43693002/diff/200001/src/x64/macro-assembler-...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/43693002/diff/200001/src/x64/macro-assembler-...
src/x64/macro-assembler-x64.cc:4097: addq(rsp, Immediate(2 * kPointerSize));
Use Drop(2) instead.

[Yang, 2013-10-30 15:12:57]

https://codereview.chromium.org/43693002/diff/200001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/43693002/diff/200001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:5835: __ movq(arg_reg_1, isolate,
RelocInfo::EXTERNAL_REFERENCE);
If you need to erect a frame and also pass the isolate, why don't you simply
call into a runtime function using CallRuntime? That uses the CEntryStub and
takes care of the isolate.

[yurys, 2013-10-31 08:45:07]

On 2013/10/30 15:01:24, Yang wrote:
> I do not agree with this change yet:
> - We are introducing a new flag to the StackTraceFrameIterator and all its
> parent classes just to disable an assert.
> - Do we actually understand why the stackframe iterator cannot work when being
> called from a builtin (I assume that you are referring to C++ builtins in like
> ArrayShift)? I was under the assumption that the StackFrameIterator should be
> able to handle all types of frames on the stack.
> 
The assert was there since the Crankshaft implementation was landed in
bleeding_edge. Digging into the history of this code before it was published
didn't give much additional info.

My understanding of this code is that determining frame type based on the return
address saved on the stack may be more reliable than casting frame marker value
to a function. Otherwise I don't see why not simply cast the marker to a
function and return its code type. 

It must be the assert is there because the function may well be calling some
code stub without erecting a frame in which case return address on the stack
would point inside that code stub. Based on the stub's code's type we couldn't
tell the type of the function's code. I believe the assert checks that we cannot
end up in a situation where call stack contains JSFunction -> Builtin ->
JSFunction, in other words if we are inside a JSFunction then return address on
the stack may belong only to a JSFunction's code (given the caller's frame type
is JS).

I could relax this assert instead of plumbing the boolean flag through the
iterator constructors but even with the allocation stack traces the built-ins
are allowed only on top of the call stack and I preferred to preserve the check
for all current cases. Do you think it would make sense to relax that assert
instead?


>
https://codereview.chromium.org/43693002/diff/200001/src/x64/macro-assembler-...
> File src/x64/macro-assembler-x64.cc (right):
> 
>
https://codereview.chromium.org/43693002/diff/200001/src/x64/macro-assembler-...
> src/x64/macro-assembler-x64.cc:4097: addq(rsp, Immediate(2 * kPointerSize));
> Use Drop(2) instead.
Done.

[yurys, 2013-10-31 08:56:00]

https://codereview.chromium.org/43693002/diff/200001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/43693002/diff/200001/src/x64/code-stubs-x64.c...
src/x64/code-stubs-x64.cc:5835: __ movq(arg_reg_1, isolate,
RelocInfo::EXTERNAL_REFERENCE);
On 2013/10/30 15:12:57, Yang wrote:
> If you need to erect a frame and also pass the isolate, why don't you simply
> call into a runtime function using CallRuntime? That uses the CEntryStub and
> takes care of the isolate.

There are two reasons for this:
1. CallRuntime doesn't preserve registers. Since we call HeapProfiler in the
middle of allocation we should be careful not to change current state of the
instrumented code.
2. Does a lot of checks related to GC which are unnecessary in our case since
the allocation hook is guaranteed not to do any heap allocations. As long as
we'd like to minimize perf impact of the allocation tracker we'd rather avoid
unnecessary checks.

[yurys, 2013-10-31 08:57:40]

https://codereview.chromium.org/43693002/diff/200001/src/x64/macro-assembler-...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/43693002/diff/200001/src/x64/macro-assembler-...
src/x64/macro-assembler-x64.cc:4097: addq(rsp, Immediate(2 * kPointerSize));
On 2013/10/30 15:01:25, Yang wrote:
> Use Drop(2) instead.

Done.

[yurys, 2013-11-05 08:57:57]

-hpayer since he is on vacation, +mstarzinger

[loislo, 2013-11-05 08:58:41]

https://codereview.chromium.org/43693002/diff/200001/src/x64/macro-assembler-...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/43693002/diff/200001/src/x64/macro-assembler-...
src/x64/macro-assembler-x64.cc:4093: push(Immediate(object_size));
please add  AllowStubCallsScope allow_scope(this, true); call here.
Otherwise it will fail on compilation StringAdd stub.

[yurys, 2013-11-05 09:09:12]

https://codereview.chromium.org/43693002/diff/200001/src/x64/macro-assembler-...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/43693002/diff/200001/src/x64/macro-assembler-...
src/x64/macro-assembler-x64.cc:4093: push(Immediate(object_size));
On 2013/11/05 08:58:42, loislo wrote:
> please add  AllowStubCallsScope allow_scope(this, true); call here.
> Otherwise it will fail on compilation StringAdd stub.

Done.