Verify/document assumption about stack resource addresses.

Verify/document assumption about stack resource addresses.

We assume that addresses of stack resources refer to the actual stack and are ordered accordingly.
Using the AddressSanitizer with detect_stack_use_after_return will break this assumption.

R=zra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43858

[zra, 2015-02-17 19:00:23]

lgtm with question

https://codereview.chromium.org/927363004/diff/60001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/927363004/diff/60001/runtime/vm/exceptions.cc...
runtime/vm/exceptions.cc:293: ASSERT(reinterpret_cast<uword>(current) >
stack_pointer);
Is this the only place where this assertion is needed? Do we walk the stack
resources and reclaim anywhere else?

[koda, 2015-02-17 19:07:54]

https://codereview.chromium.org/927363004/diff/60001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/927363004/diff/60001/runtime/vm/exceptions.cc...
runtime/vm/exceptions.cc:293: ASSERT(reinterpret_cast<uword>(current) >
stack_pointer);
On 2015/02/17 19:00:23, zra wrote:
> Is this the only place where this assertion is needed? Do we walk the stack
> resources and reclaim anywhere else?

Yes, we do the same thing in the simulators. We should share the code. I'll send
out an updated CL.

[koda, 2015-02-17 19:36:35]

New patchsets have been uploaded after l-g-t-m from zra@google.com

[zra, 2015-02-17 19:45:24]

https://codereview.chromium.org/927363004/diff/80001/runtime/vm/simulator_arm.cc
File runtime/vm/simulator_arm.cc (right):

https://codereview.chromium.org/927363004/diff/80001/runtime/vm/simulator_arm...
runtime/vm/simulator_arm.cc:3859: 
rm extra whitespace

https://codereview.chromium.org/927363004/diff/100001/runtime/vm/allocation.cc
File runtime/vm/allocation.cc (right):

https://codereview.chromium.org/927363004/diff/100001/runtime/vm/allocation.c...
runtime/vm/allocation.cc:41: (reinterpret_cast<uword>(isolate->top_resource()) <
stack_pointer)) {
Should this be <= ?

https://codereview.chromium.org/927363004/diff/100001/runtime/vm/allocation.c...
runtime/vm/allocation.cc:48: ASSERT(reinterpret_cast<uword>(current) >
stack_pointer);
Or should this be >= ?

Or is current == stack_pointer not possible?

If not possible please add a comment explaining why.

[koda, 2015-02-17 20:01:15]

Addressed comments and also moved similar unwinding code from longjump.cc into
StackResource, to collect all explicit destructor calls in one place.

PTAL

https://codereview.chromium.org/927363004/diff/80001/runtime/vm/simulator_arm.cc
File runtime/vm/simulator_arm.cc (right):

https://codereview.chromium.org/927363004/diff/80001/runtime/vm/simulator_arm...
runtime/vm/simulator_arm.cc:3859: 
On 2015/02/17 19:45:24, zra wrote:
> rm extra whitespace

Done.

https://codereview.chromium.org/927363004/diff/100001/runtime/vm/allocation.cc
File runtime/vm/allocation.cc (right):

https://codereview.chromium.org/927363004/diff/100001/runtime/vm/allocation.c...
runtime/vm/allocation.cc:41: (reinterpret_cast<uword>(isolate->top_resource()) <
stack_pointer)) {
On 2015/02/17 19:45:24, zra wrote:
> Should this be <= ?

No, made it exclusive and documented that.

https://codereview.chromium.org/927363004/diff/100001/runtime/vm/allocation.c...
runtime/vm/allocation.cc:48: ASSERT(reinterpret_cast<uword>(current) >
stack_pointer);
On 2015/02/17 19:45:24, zra wrote:
> Or should this be >= ?
> 
> Or is current == stack_pointer not possible?
> 
> If not possible please add a comment explaining why.

With out current usage, it's not possible, since we pass in a stack pointer
that's between two C++ frames. However, for completeness, I've made this a ">="
and documented that it's exclusive (that's also consistent with the added
overloaded version of this method).

[zra, 2015-02-17 20:43:37]

lgtm

[koda, 2015-02-17 21:43:25]

New patchsets have been uploaded after l-g-t-m from zra@google.com

[koda, 2015-02-17 21:45:51]

Ivan, PTAL

https://codereview.chromium.org/927363004/diff/140001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/927363004/diff/140001/runtime/vm/exceptions.c...
runtime/vm/exceptions.cc:287: ASSERT(isolate->top_resource() != NULL);
Added this ASSERT, which is now the first thing that fails when we run with
detect_stack_use_after_return=1 (whether it pops too much or too little depends
on the order of the malloc'ed fake stack addresses).

[koda, 2015-02-18 23:21:42]

Committed patchset #9 (id:160001) manually as r43858 (presubmit successful).