Revert "Record allocation stack traces".

Revert "Record allocation stack traces".

This reverts commit r17365 for memory corruption. The issues
are most probably related to storing a filler map without
checking the bump pointer first, and traversing the stack
when called from within a stub (or builtin), while the code
assumes that it's called from a JS function.

R=verwaest@chromium.org

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

[Toon Verwaest, 2013-11-05 13:31:53]

lgtm

[Benedikt Meurer, 2013-11-05 13:46:19]

Committed patchset #2 manually as r17488 (presubmit successful).

[yurys, 2013-11-05 13:52:22]

On 2013/11/05 13:31:53, Toon Verwaest wrote:
> lgtm

Benedikt, can you provide more details on the problem please? The feature is
disabled by default and I'm curious to learn more about the scenario where it
would lead to a memory corruption.

[Benedikt Meurer, 2013-11-05 14:10:49]

On 2013/11/05 13:52:22, Yury Semikhatsky wrote:
> On 2013/11/05 13:31:53, Toon Verwaest wrote:
> > lgtm
> 
> Benedikt, can you provide more details on the problem please? The feature is
> disabled by default and I'm curious to learn more about the scenario where it
> would lead to a memory corruption.

I discovered the problem while adding a new Hydrogen code stub that allocates.
There are two problems in here: (a) you cannot simply trace the stack if called
from somewhere within a Hydrogen code stub (and the same is true for native code
stubs), because the state of the stack is undefined and you need to take great
care to get a proper traceable stack, and (b) depending on the state of the
heap, you'll pass a bump pointer to your runtime function that points after the
heap, where you try to place a filler map. This happened with the
test-heap-profiler/TrackAllocations test, where in my case the new string add
stub is generated at runtime after your flag is turned on. You probably didn't
notice (at least with your test case) because all stubs were already part of the
snapshot and hence those allocations weren't tracked.

[yurys, 2013-11-06 12:35:52]

On 2013/11/05 14:10:49, Benedikt Meurer wrote:
> On 2013/11/05 13:52:22, Yury Semikhatsky wrote:
> > On 2013/11/05 13:31:53, Toon Verwaest wrote:
> > > lgtm
> > 
> > Benedikt, can you provide more details on the problem please? The feature is
> > disabled by default and I'm curious to learn more about the scenario where
it
> > would lead to a memory corruption.
> 
> I discovered the problem while adding a new Hydrogen code stub that allocates.
> There are two problems in here: (a) you cannot simply trace the stack if
called
> from somewhere within a Hydrogen code stub (and the same is true for native
code
> stubs), because the state of the stack is undefined and you need to take great
> care to get a proper traceable stack
FWIW, I was trying to address this in https://codereview.chromium.org/43693002/.
I believe  calling native code stub requires erecting valid exit frame so the
stack should already be in an iterable state. Now that you reverted r17365 let's
move the discussion to the design document that I've sent. Please provide your
feedback on walking call stack section there.

> and (b) depending on the state of the
> heap, you'll pass a bump pointer to your runtime function that points after
the
> heap, where you try to place a filler map. This happened with the
> test-heap-profiler/TrackAllocations test, where in my case the new string add
> stub is generated at runtime after your flag is turned on. You probably didn't
> notice (at least with your test case) because all stubs were already part of
the
> snapshot and hence those allocations weren't tracked.
I see, thank you. The problem is that when allocation is handled in C++ (in
spaces-inl.h), we report object allocation after checking the limit and when we
know for sure the new object's address but the generated code calls allocation
tracker after loading top pointer but before checking if the pointer is valid
and there is enough room for the new object. In that case we can end up with the
bump pointer pointing after the heap as you described. This could be easily
fixed by moving the hook at the place where all checks have successfully passed.

[yurys, 2013-11-06 13:30:45]

I'm wondering why the test didn't fail neither locally nor on the bots when
running with --nosnapshot flag.

[Benedikt Meurer, 2013-11-06 13:31:41]

On 2013/11/06 13:30:45, yurys wrote:
> I'm wondering why the test didn't fail neither locally nor on the bots when
> running with --nosnapshot flag.

Most probably because no Hydrogen code stub with HAllocate is covered.

[yurys, 2013-11-06 13:44:36]

On 2013/11/06 13:31:41, Benedikt Meurer wrote:
> On 2013/11/06 13:30:45, yurys wrote:
> > I'm wondering why the test didn't fail neither locally nor on the bots when
> > running with --nosnapshot flag.
> 
> Most probably because no Hydrogen code stub with HAllocate is covered.

How come there is such stub which is affected when you run the test on your
machine and it is missing when the test is executed on the bots. I'd assume that
the code stubs would be the same, am I missing something?

[Benedikt Meurer, 2013-11-06 13:51:59]

On 2013/11/06 13:44:36, yurys wrote:
> On 2013/11/06 13:31:41, Benedikt Meurer wrote:
> > On 2013/11/06 13:30:45, yurys wrote:
> > > I'm wondering why the test didn't fail neither locally nor on the bots
when
> > > running with --nosnapshot flag.
> > 
> > Most probably because no Hydrogen code stub with HAllocate is covered.
> 
> How come there is such stub which is affected when you run the test on your
> machine and it is missing when the test is executed on the bots. I'd assume
that
> the code stubs would be the same, am I missing something?

No, I'm working on converting existing native code stubs to Hydrogen. In this
case the bug was triggered by my experimental hydrogen StringAddStub (because
your test case uses string concatentation).