Reland - Arm64: Ensure that csp is always aligned to 16 byte values even if jssp is not.

Reland - Arm64: Ensure that csp is always aligned to 16 byte values even if jssp is not.

Even although the Arm64 specification specifies that csp
only needs to be aligned to 16 bytes if it is dereferenced, some
implementations show poor performance.

Also makes the following change:
 - Enable CPU support for arm64 to enable probing of cpu implementer and cpu part.
 - Add ALWAYS_ALIGN_CSP CpuFeature for Arm64 and set it based on runtime probing of the cpu imp
 - Rename PrepareForPush and PrepareForPop to PushPreamble and PopPostamble and move PopPostabl

Original Review URL: https://codereview.chromium.org/264773004

R=ulan@chromium.org

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

[rmcilroy, 2014-05-07 18:08:21]

This is a reland of r21177.  

To make this easier to review I've updated the patchset's independently: 
 Patchset 1 contains the original patch. 
 Patchset 2 adds the fixes landed in r21179
 Patchset 3 adds additional fixes which should address the remaining crashes.
 Patchset 4 disables the AssertStackConsistency in BumpSystemStackPointer due to
what I think is an unrelated bug.  

PTAL.

https://codereview.chromium.org/271543004/diff/60001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64-inl.h (right):

https://codereview.chromium.org/271543004/diff/60001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64-inl.h:1288: // AssertStackConsistency(true);
If I enable this I get the following assert in debug-multiple-breakpoints.js on
a nosnapshot build.  I've replaced this with an equivalent number of nop()
instructions and still got the same result, so this looks like an unrelated
failure which I am tickling by changing the amount of code generated.  Any ideas
Ulan?
 

./out/arm64.debug/d8 --test --random-seed=1269372994 --stress-opt --always-opt
--nohard-abort --nodead-code-elimination --nofold-constants
--enable-slow-asserts --debug-code --verify-heap --expose-debug-as debug
test/mjsunit/mjsunit.js test/mjsunit/debug-multiple-breakpoints.js
============ Stress 1/2 ============
============ Stress 2/2 ============


#
# Fatal error in ../src/debug.cc, line 2041
# CHECK_EQ(Code::FUNCTION, function->code()->kind()) failed
#   Expected: 0
#   Found: 4
#

==== C stack trace ===============================

 1: V8_Fatal
 2: CheckEqualsHelper(char const*, int, char const*, int, char const*, int)
 3:
v8::internal::Debug::MaybeRecompileFunctionForDebugging(v8::internal::Handle<v8::internal::JSFunction>)
 4: v8::internal::Debug::PrepareForBreakPoints()
 5:
v8::internal::Debug::SetBreakPointForScript(v8::internal::Handle<v8::internal::Script>,
v8::internal::Handle<v8::internal::Object>, int*,
v8::internal::BreakPositionAlignment)
 6: ??
 7: v8::internal::Runtime_SetScriptBreakPoint(int, v8::internal::Object**,
v8::internal::Isolate*)
 8: v8::internal::Simulator::DoRuntimeCall(v8::internal::Instruction*)
 9: v8::internal::Simulator::VisitException(v8::internal::Instruction*)
10:
v8::internal::Decoder<v8::internal::Simulator>::DecodeBranchSystemException(v8::internal::Instruction*)
11:
v8::internal::Decoder<v8::internal::Simulator>::Decode(v8::internal::Instruction*)
12: v8::internal::Simulator::ExecuteInstruction()
13: v8::internal::Simulator::Run()
14: v8::internal::Simulator::CheckPCSComplianceAndRun()
15: v8::internal::Simulator::CallVoid(unsigned char*,
v8::internal::Simulator::CallArgument*)
16: v8::internal::Simulator::CallInt64(unsigned char*,
v8::internal::Simulator::CallArgument*)
17: v8::internal::Simulator::CallJS(unsigned char*, unsigned char*,
v8::internal::JSFunction*, v8::internal::Object*, long, v8::internal::Object***)
18: ??
19: v8::internal::Execution::Call(v8::internal::Isolate*,
v8::internal::Handle<v8::internal::Object>,
v8::internal::Handle<v8::internal::Object>, int,
v8::internal::Handle<v8::internal::Object>*, bool)
20: v8::Script::Run()
21: v8::Shell::ExecuteString(v8::Isolate*, v8::Handle<v8::String>,
v8::Handle<v8::Value>, bool, bool)
22: v8::SourceGroup::Execute(v8::Isolate*)
23: v8::Shell::RunMain(v8::Isolate*, int, char**)
24: v8::Shell::Main(int, char**)
25: main
26: __libc_start_main
27: ??
Aborted (core dumped)

[ulan, 2014-05-08 08:18:43]

I am looking into debug-multiple-breakpoints.js failure.

https://codereview.chromium.org/271543004/diff/40001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64-inl.h (right):

https://codereview.chromium.org/271543004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64-inl.h:1253: Sub(temp, StackPointer(), space);
What was the bug here?

https://codereview.chromium.org/271543004/diff/40001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64.h (right):

https://codereview.chromium.org/271543004/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.h:726: void AssertStackConsistency(bool
csp_was_modified);
Nit: we usually pass flags as enums with descriptive names, that improves
readability at call-sites.

[ulan, 2014-05-08 09:05:29]

I uncommented 

src/arm64/macro-assembler-arm64-inl.h:1288: // AssertStackConsistency(true);

but cannot reproduce the debug failure at r21195. Ross, did you try ToT?

[rmcilroy, 2014-05-08 09:35:42]

On 2014/05/08 09:05:29, ulan wrote:
> I uncommented 
> 
> src/arm64/macro-assembler-arm64-inl.h:1288: // AssertStackConsistency(true);
> 
> but cannot reproduce the debug failure at r21195. Ross, did you try ToT?

I tried it at r21191, but let me try at r21195.  Did you do a nosnapshot build
(it only exhibits in snapshot=off builds).

Also, after running all the debug tests overnight with r21191 there were also
errors in:
 Fatal error in ../test/cctest/test-spaces.cc, line 411
 # CHECK_EQ(1, isolate->heap()->paged_space(i)->CountTotalPages()) failed

andin test-serialize:
 Fatal error in ../src/spaces.cc, line 2360
 # CHECK(size_in_bytes <= kMaxBlockSize) failed

So looks like we still need to trim the amount of debug code down.

[rmcilroy, 2014-05-08 09:40:09]

On 2014/05/08 09:35:42, rmcilroy wrote:
> On 2014/05/08 09:05:29, ulan wrote:
> > I uncommented 
> > 
> > src/arm64/macro-assembler-arm64-inl.h:1288: // AssertStackConsistency(true);
> > 
> > but cannot reproduce the debug failure at r21195. Ross, did you try ToT?
> 
> I tried it at r21191, but let me try at r21195.  Did you do a nosnapshot build
> (it only exhibits in snapshot=off builds).
> 
> Also, after running all the debug tests overnight with r21191 there were also
> errors in:
>  Fatal error in ../test/cctest/test-spaces.cc, line 411
>  # CHECK_EQ(1, isolate->heap()->paged_space(i)->CountTotalPages()) failed
> 
> andin test-serialize:
>  Fatal error in ../src/spaces.cc, line 2360
>  # CHECK(size_in_bytes <= kMaxBlockSize) failed
> 
> So looks like we still need to trim the amount of debug code down.

I still get the error at r21197.

[ulan, 2014-05-08 09:46:20]

On 2014/05/08 09:40:09, rmcilroy wrote:
> On 2014/05/08 09:35:42, rmcilroy wrote:
> > On 2014/05/08 09:05:29, ulan wrote:
> > > I uncommented 
> > > 
> > > src/arm64/macro-assembler-arm64-inl.h:1288: //
AssertStackConsistency(true);
> > > 
> > > but cannot reproduce the debug failure at r21195. Ross, did you try ToT?
> > 
> > I tried it at r21191, but let me try at r21195.  Did you do a nosnapshot
build
> > (it only exhibits in snapshot=off builds).
> > 
> > Also, after running all the debug tests overnight with r21191 there were
also
> > errors in:
> >  Fatal error in ../test/cctest/test-spaces.cc, line 411
> >  # CHECK_EQ(1, isolate->heap()->paged_space(i)->CountTotalPages()) failed
> > 
> > andin test-serialize:
> >  Fatal error in ../src/spaces.cc, line 2360
> >  # CHECK(size_in_bytes <= kMaxBlockSize) failed
> > 
> > So looks like we still need to trim the amount of debug code down.
> 
> I still get the error at r21197.

Thanks. With snapshot=off it reproduces.

[ulan, 2014-05-08 10:17:09]

Here is what happens in debug-multiple-breakpoints.js:

1. A hot function is marked for optimization by replacing its code with
CompileOptimized or CompileOptimizedConcurrent builtin.
2. Debug::PrepareForBreakPoints deoptimizes optimized functions but doesn't
restore code of functions that were marked for optimization.
3. ASSERT_EQ(Code::FUNCTION, function->code()->kind()) triggers in
Debug::MaybeRecompileFunctionForDebugging

Yang, is this a bug in Debug::PrepareForBreakPoints that functions marked for
optimization do not get their code restored?

[Yang, 2014-05-08 10:36:53]

On 2014/05/08 10:17:09, ulan wrote:
> Here is what happens in debug-multiple-breakpoints.js:
> 
> 1. A hot function is marked for optimization by replacing its code with
> CompileOptimized or CompileOptimizedConcurrent builtin.
> 2. Debug::PrepareForBreakPoints deoptimizes optimized functions but doesn't
> restore code of functions that were marked for optimization.
> 3. ASSERT_EQ(Code::FUNCTION, function->code()->kind()) triggers in
> Debug::MaybeRecompileFunctionForDebugging
> 
> Yang, is this a bug in Debug::PrepareForBreakPoints that functions marked for
> optimization do not get their code restored?

No. This seems to be a result of the refactoring in
https://codereview.chromium.org/260423002/diff/40001/src/debug.cc

The assertion that is triggering has been newly introduced. I think we should
remove it. What should happen there is that if the function is on the stack at
the point of Debug::PrepareForBreakPoints, it should be recompiled for debugging
regardless of what it currently is (including being marked by builtin). The only
exception to that rule is that if it already has been recompiled for debugging.

I will make this easier to understand in the future, but for now, just revert
that part of the refactoring there.

[ulan, 2014-05-08 10:47:11]

On 2014/05/08 10:36:53, Yang wrote:
> On 2014/05/08 10:17:09, ulan wrote:
> > Here is what happens in debug-multiple-breakpoints.js:
> > 
> > 1. A hot function is marked for optimization by replacing its code with
> > CompileOptimized or CompileOptimizedConcurrent builtin.
> > 2. Debug::PrepareForBreakPoints deoptimizes optimized functions but doesn't
> > restore code of functions that were marked for optimization.
> > 3. ASSERT_EQ(Code::FUNCTION, function->code()->kind()) triggers in
> > Debug::MaybeRecompileFunctionForDebugging
> > 
> > Yang, is this a bug in Debug::PrepareForBreakPoints that functions marked
for
> > optimization do not get their code restored?
> 
> No. This seems to be a result of the refactoring in
> https://codereview.chromium.org/260423002/diff/40001/src/debug.cc
> 
> The assertion that is triggering has been newly introduced. I think we should
> remove it. What should happen there is that if the function is on the stack at
> the point of Debug::PrepareForBreakPoints, it should be recompiled for
debugging
> regardless of what it currently is (including being marked by builtin). The
only
> exception to that rule is that if it already has been recompiled for
debugging.
> 
> I will make this easier to understand in the future, but for now, just revert
> that part of the refactoring there.

Thanks, Yang. So we can just remove the assertions
2006   ASSERT_EQ(Code::FUNCTION, function->code()->kind());
2007   ASSERT_EQ(function->code(), function->shared()->code());
in Debug::MaybeRecompileFunctionForDebugging? Because the function code will be
replaced anyway.

[Yang, 2014-05-08 10:47:47]

On 2014/05/08 10:47:11, ulan wrote:
> On 2014/05/08 10:36:53, Yang wrote:
> > On 2014/05/08 10:17:09, ulan wrote:
> > > Here is what happens in debug-multiple-breakpoints.js:
> > > 
> > > 1. A hot function is marked for optimization by replacing its code with
> > > CompileOptimized or CompileOptimizedConcurrent builtin.
> > > 2. Debug::PrepareForBreakPoints deoptimizes optimized functions but
doesn't
> > > restore code of functions that were marked for optimization.
> > > 3. ASSERT_EQ(Code::FUNCTION, function->code()->kind()) triggers in
> > > Debug::MaybeRecompileFunctionForDebugging
> > > 
> > > Yang, is this a bug in Debug::PrepareForBreakPoints that functions marked
> for
> > > optimization do not get their code restored?
> > 
> > No. This seems to be a result of the refactoring in
> > https://codereview.chromium.org/260423002/diff/40001/src/debug.cc
> > 
> > The assertion that is triggering has been newly introduced. I think we
should
> > remove it. What should happen there is that if the function is on the stack
at
> > the point of Debug::PrepareForBreakPoints, it should be recompiled for
> debugging
> > regardless of what it currently is (including being marked by builtin). The
> only
> > exception to that rule is that if it already has been recompiled for
> debugging.
> > 
> > I will make this easier to understand in the future, but for now, just
revert
> > that part of the refactoring there.
> 
> Thanks, Yang. So we can just remove the assertions
> 2006   ASSERT_EQ(Code::FUNCTION, function->code()->kind());
> 2007   ASSERT_EQ(function->code(), function->shared()->code());
> in Debug::MaybeRecompileFunctionForDebugging? Because the function code will
be
> replaced anyway.

Yes.

[Yang, 2014-05-08 11:13:33]

On 2014/05/08 10:47:47, Yang wrote:
> On 2014/05/08 10:47:11, ulan wrote:
> > On 2014/05/08 10:36:53, Yang wrote:
> > > On 2014/05/08 10:17:09, ulan wrote:
> > > > Here is what happens in debug-multiple-breakpoints.js:
> > > > 
> > > > 1. A hot function is marked for optimization by replacing its code with
> > > > CompileOptimized or CompileOptimizedConcurrent builtin.
> > > > 2. Debug::PrepareForBreakPoints deoptimizes optimized functions but
> doesn't
> > > > restore code of functions that were marked for optimization.
> > > > 3. ASSERT_EQ(Code::FUNCTION, function->code()->kind()) triggers in
> > > > Debug::MaybeRecompileFunctionForDebugging
> > > > 
> > > > Yang, is this a bug in Debug::PrepareForBreakPoints that functions
marked
> > for
> > > > optimization do not get their code restored?
> > > 
> > > No. This seems to be a result of the refactoring in
> > > https://codereview.chromium.org/260423002/diff/40001/src/debug.cc
> > > 
> > > The assertion that is triggering has been newly introduced. I think we
> should
> > > remove it. What should happen there is that if the function is on the
stack
> at
> > > the point of Debug::PrepareForBreakPoints, it should be recompiled for
> > debugging
> > > regardless of what it currently is (including being marked by builtin).
The
> > only
> > > exception to that rule is that if it already has been recompiled for
> > debugging.
> > > 
> > > I will make this easier to understand in the future, but for now, just
> revert
> > > that part of the refactoring there.
> > 
> > Thanks, Yang. So we can just remove the assertions
> > 2006   ASSERT_EQ(Code::FUNCTION, function->code()->kind());
> > 2007   ASSERT_EQ(function->code(), function->shared()->code());
> > in Debug::MaybeRecompileFunctionForDebugging? Because the function code will
> be
> > replaced anyway.
> 
> Yes.

Wait. Please check for (Code::FUNCTION == function->code()->kind()) before
checking whether it already has_debug_break_slots(). Otherwise the latter may
assert. Something like
if (Code::FUNCTION == function->code()->kind() &&
function->code()->has_debug_break_slots()) return;

[rmcilroy, 2014-05-08 12:40:48]

On 2014/05/08 11:13:33, Yang wrote:
> On 2014/05/08 10:47:47, Yang wrote:
> > On 2014/05/08 10:47:11, ulan wrote:
> > > On 2014/05/08 10:36:53, Yang wrote:
> > > > On 2014/05/08 10:17:09, ulan wrote:
> > > > > Here is what happens in debug-multiple-breakpoints.js:
> > > > > 
> > > > > 1. A hot function is marked for optimization by replacing its code
with
> > > > > CompileOptimized or CompileOptimizedConcurrent builtin.
> > > > > 2. Debug::PrepareForBreakPoints deoptimizes optimized functions but
> > doesn't
> > > > > restore code of functions that were marked for optimization.
> > > > > 3. ASSERT_EQ(Code::FUNCTION, function->code()->kind()) triggers in
> > > > > Debug::MaybeRecompileFunctionForDebugging
> > > > > 
> > > > > Yang, is this a bug in Debug::PrepareForBreakPoints that functions
> marked
> > > for
> > > > > optimization do not get their code restored?
> > > > 
> > > > No. This seems to be a result of the refactoring in
> > > > https://codereview.chromium.org/260423002/diff/40001/src/debug.cc
> > > > 
> > > > The assertion that is triggering has been newly introduced. I think we
> > should
> > > > remove it. What should happen there is that if the function is on the
> stack
> > at
> > > > the point of Debug::PrepareForBreakPoints, it should be recompiled for
> > > debugging
> > > > regardless of what it currently is (including being marked by builtin).
> The
> > > only
> > > > exception to that rule is that if it already has been recompiled for
> > > debugging.
> > > > 
> > > > I will make this easier to understand in the future, but for now, just
> > revert
> > > > that part of the refactoring there.
> > > 
> > > Thanks, Yang. So we can just remove the assertions
> > > 2006   ASSERT_EQ(Code::FUNCTION, function->code()->kind());
> > > 2007   ASSERT_EQ(function->code(), function->shared()->code());
> > > in Debug::MaybeRecompileFunctionForDebugging? Because the function code
will
> > be
> > > replaced anyway.
> > 
> > Yes.
> 
> Wait. Please check for (Code::FUNCTION == function->code()->kind()) before
> checking whether it already has_debug_break_slots(). Otherwise the latter may
> assert. Something like
> if (Code::FUNCTION == function->code()->kind() &&
> function->code()->has_debug_break_slots()) return;

Thanks Yang! I guess you mean:
  if (Code::FUNCTION != function->code()->kind() ||
      function->code()->has_debug_break_slots()) return;
?  I'll give that a go and send out a separate CL if this fixes things.

[Yang, 2014-05-08 12:42:40]

On 2014/05/08 12:40:48, rmcilroy wrote:
> On 2014/05/08 11:13:33, Yang wrote:
> > On 2014/05/08 10:47:47, Yang wrote:
> > > On 2014/05/08 10:47:11, ulan wrote:
> > > > On 2014/05/08 10:36:53, Yang wrote:
> > > > > On 2014/05/08 10:17:09, ulan wrote:
> > > > > > Here is what happens in debug-multiple-breakpoints.js:
> > > > > > 
> > > > > > 1. A hot function is marked for optimization by replacing its code
> with
> > > > > > CompileOptimized or CompileOptimizedConcurrent builtin.
> > > > > > 2. Debug::PrepareForBreakPoints deoptimizes optimized functions but
> > > doesn't
> > > > > > restore code of functions that were marked for optimization.
> > > > > > 3. ASSERT_EQ(Code::FUNCTION, function->code()->kind()) triggers in
> > > > > > Debug::MaybeRecompileFunctionForDebugging
> > > > > > 
> > > > > > Yang, is this a bug in Debug::PrepareForBreakPoints that functions
> > marked
> > > > for
> > > > > > optimization do not get their code restored?
> > > > > 
> > > > > No. This seems to be a result of the refactoring in
> > > > > https://codereview.chromium.org/260423002/diff/40001/src/debug.cc
> > > > > 
> > > > > The assertion that is triggering has been newly introduced. I think we
> > > should
> > > > > remove it. What should happen there is that if the function is on the
> > stack
> > > at
> > > > > the point of Debug::PrepareForBreakPoints, it should be recompiled for
> > > > debugging
> > > > > regardless of what it currently is (including being marked by
builtin).
> > The
> > > > only
> > > > > exception to that rule is that if it already has been recompiled for
> > > > debugging.
> > > > > 
> > > > > I will make this easier to understand in the future, but for now, just
> > > revert
> > > > > that part of the refactoring there.
> > > > 
> > > > Thanks, Yang. So we can just remove the assertions
> > > > 2006   ASSERT_EQ(Code::FUNCTION, function->code()->kind());
> > > > 2007   ASSERT_EQ(function->code(), function->shared()->code());
> > > > in Debug::MaybeRecompileFunctionForDebugging? Because the function code
> will
> > > be
> > > > replaced anyway.
> > > 
> > > Yes.
> > 
> > Wait. Please check for (Code::FUNCTION == function->code()->kind()) before
> > checking whether it already has_debug_break_slots(). Otherwise the latter
may
> > assert. Something like
> > if (Code::FUNCTION == function->code()->kind() &&
> > function->code()->has_debug_break_slots()) return;
> 
> Thanks Yang! I guess you mean:
>   if (Code::FUNCTION != function->code()->kind() ||
>       function->code()->has_debug_break_slots()) return;
> ?  I'll give that a go and send out a separate CL if this fixes things.

No! I don't mean that. In MaybeRecompileFunctionForDebugging, what we want to do
is to make sure the function code afterwards is unoptimized and contains debug
break slots. So only if that's already true, we return prematurely. If the
function code kind is not FUNCTION, it could be a builtin, or optimized code. In
either of those cases, we want to recompile.

[Yang, 2014-05-08 12:43:42]

On 2014/05/08 12:42:40, Yang wrote:
> On 2014/05/08 12:40:48, rmcilroy wrote:
> > On 2014/05/08 11:13:33, Yang wrote:
> > > On 2014/05/08 10:47:47, Yang wrote:
> > > > On 2014/05/08 10:47:11, ulan wrote:
> > > > > On 2014/05/08 10:36:53, Yang wrote:
> > > > > > On 2014/05/08 10:17:09, ulan wrote:
> > > > > > > Here is what happens in debug-multiple-breakpoints.js:
> > > > > > > 
> > > > > > > 1. A hot function is marked for optimization by replacing its code
> > with
> > > > > > > CompileOptimized or CompileOptimizedConcurrent builtin.
> > > > > > > 2. Debug::PrepareForBreakPoints deoptimizes optimized functions
but
> > > > doesn't
> > > > > > > restore code of functions that were marked for optimization.
> > > > > > > 3. ASSERT_EQ(Code::FUNCTION, function->code()->kind()) triggers in
> > > > > > > Debug::MaybeRecompileFunctionForDebugging
> > > > > > > 
> > > > > > > Yang, is this a bug in Debug::PrepareForBreakPoints that functions
> > > marked
> > > > > for
> > > > > > > optimization do not get their code restored?
> > > > > > 
> > > > > > No. This seems to be a result of the refactoring in
> > > > > > https://codereview.chromium.org/260423002/diff/40001/src/debug.cc
> > > > > > 
> > > > > > The assertion that is triggering has been newly introduced. I think
we
> > > > should
> > > > > > remove it. What should happen there is that if the function is on
the
> > > stack
> > > > at
> > > > > > the point of Debug::PrepareForBreakPoints, it should be recompiled
for
> > > > > debugging
> > > > > > regardless of what it currently is (including being marked by
> builtin).
> > > The
> > > > > only
> > > > > > exception to that rule is that if it already has been recompiled for
> > > > > debugging.
> > > > > > 
> > > > > > I will make this easier to understand in the future, but for now,
just
> > > > revert
> > > > > > that part of the refactoring there.
> > > > > 
> > > > > Thanks, Yang. So we can just remove the assertions
> > > > > 2006   ASSERT_EQ(Code::FUNCTION, function->code()->kind());
> > > > > 2007   ASSERT_EQ(function->code(), function->shared()->code());
> > > > > in Debug::MaybeRecompileFunctionForDebugging? Because the function
code
> > will
> > > > be
> > > > > replaced anyway.
> > > > 
> > > > Yes.
> > > 
> > > Wait. Please check for (Code::FUNCTION == function->code()->kind()) before
> > > checking whether it already has_debug_break_slots(). Otherwise the latter
> may
> > > assert. Something like
> > > if (Code::FUNCTION == function->code()->kind() &&
> > > function->code()->has_debug_break_slots()) return;
> > 
> > Thanks Yang! I guess you mean:
> >   if (Code::FUNCTION != function->code()->kind() ||
> >       function->code()->has_debug_break_slots()) return;
> > ?  I'll give that a go and send out a separate CL if this fixes things.
> 
> No! I don't mean that. In MaybeRecompileFunctionForDebugging, what we want to
do
> is to make sure the function code afterwards is unoptimized and contains debug
> break slots. So only if that's already true, we return prematurely. If the
> function code kind is not FUNCTION, it could be a builtin, or optimized code.
In
> either of those cases, we want to recompile.

We could actually rename it to EnsureFunctionHasDebugBreakSlots.

[rmcilroy, 2014-05-08 13:53:45]

On 2014/05/08 12:43:42, Yang wrote:
> On 2014/05/08 12:42:40, Yang wrote:
> > On 2014/05/08 12:40:48, rmcilroy wrote:
> > > On 2014/05/08 11:13:33, Yang wrote:
> > > > On 2014/05/08 10:47:47, Yang wrote:
> > > > > On 2014/05/08 10:47:11, ulan wrote:
> > > > > > On 2014/05/08 10:36:53, Yang wrote:
> > > > > > > On 2014/05/08 10:17:09, ulan wrote:
> > > > > > > > Here is what happens in debug-multiple-breakpoints.js:
> > > > > > > > 
> > > > > > > > 1. A hot function is marked for optimization by replacing its
code
> > > with
> > > > > > > > CompileOptimized or CompileOptimizedConcurrent builtin.
> > > > > > > > 2. Debug::PrepareForBreakPoints deoptimizes optimized functions
> but
> > > > > doesn't
> > > > > > > > restore code of functions that were marked for optimization.
> > > > > > > > 3. ASSERT_EQ(Code::FUNCTION, function->code()->kind()) triggers
in
> > > > > > > > Debug::MaybeRecompileFunctionForDebugging
> > > > > > > > 
> > > > > > > > Yang, is this a bug in Debug::PrepareForBreakPoints that
functions
> > > > marked
> > > > > > for
> > > > > > > > optimization do not get their code restored?
> > > > > > > 
> > > > > > > No. This seems to be a result of the refactoring in
> > > > > > > https://codereview.chromium.org/260423002/diff/40001/src/debug.cc
> > > > > > > 
> > > > > > > The assertion that is triggering has been newly introduced. I
think
> we
> > > > > should
> > > > > > > remove it. What should happen there is that if the function is on
> the
> > > > stack
> > > > > at
> > > > > > > the point of Debug::PrepareForBreakPoints, it should be recompiled
> for
> > > > > > debugging
> > > > > > > regardless of what it currently is (including being marked by
> > builtin).
> > > > The
> > > > > > only
> > > > > > > exception to that rule is that if it already has been recompiled
for
> > > > > > debugging.
> > > > > > > 
> > > > > > > I will make this easier to understand in the future, but for now,
> just
> > > > > revert
> > > > > > > that part of the refactoring there.
> > > > > > 
> > > > > > Thanks, Yang. So we can just remove the assertions
> > > > > > 2006   ASSERT_EQ(Code::FUNCTION, function->code()->kind());
> > > > > > 2007   ASSERT_EQ(function->code(), function->shared()->code());
> > > > > > in Debug::MaybeRecompileFunctionForDebugging? Because the function
> code
> > > will
> > > > > be
> > > > > > replaced anyway.
> > > > > 
> > > > > Yes.
> > > > 
> > > > Wait. Please check for (Code::FUNCTION == function->code()->kind())
before
> > > > checking whether it already has_debug_break_slots(). Otherwise the
latter
> > may
> > > > assert. Something like
> > > > if (Code::FUNCTION == function->code()->kind() &&
> > > > function->code()->has_debug_break_slots()) return;
> > > 
> > > Thanks Yang! I guess you mean:
> > >   if (Code::FUNCTION != function->code()->kind() ||
> > >       function->code()->has_debug_break_slots()) return;
> > > ?  I'll give that a go and send out a separate CL if this fixes things.
> > 
> > No! I don't mean that. In MaybeRecompileFunctionForDebugging, what we want
to
> do
> > is to make sure the function code afterwards is unoptimized and contains
debug
> > break slots. So only if that's already true, we return prematurely. If the
> > function code kind is not FUNCTION, it could be a builtin, or optimized
code.
> In
> > either of those cases, we want to recompile.
> 
> We could actually rename it to EnsureFunctionHasDebugBreakSlots.

Ulan / Jacob: With the debug fix (which I'll land separately) and the additional
changed in patchset 5 - which avoids generating too much code in debug mode by
avoiding reentering AssertStackConsistency from the Push in Abort, and only
generating code AssertStackConsistency if we are using real aborts (since the
PrintF's in a non-real Abort are massive) - we pass the remaining tests AFAICT. 
I also modified test-spaces to not check if CODE_SPACE == 1 page if
FLAG_debug_code, since we are still at 2 pages after this change due to the
added assert code.  PTAL.

[jbramley, 2014-05-08 14:01:47]

I've hit code-size problems before. The ARM64 back-end has a lot more assertions
than the others (because they were useful whilst we worked on the port).

I can't really comment on the page/space stuff because I don't know how that
mechanism works, but the rest looks fine.

https://codereview.chromium.org/271543004/diff/80001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/271543004/diff/80001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.cc:1191: if (emit_debug_code() &&
use_real_aborts()) {
I think a comment is in order.

https://codereview.chromium.org/271543004/diff/80001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.cc:1207: { DontEmitDebugCodeScope
dont_emit_debug_code_scope(this);
Ditto.

[ulan, 2014-05-08 14:23:29]

LGTM. Out of curiosity why we need to use macro-assembler Sub() in
BumpSystemStackPointer?

[rmcilroy, 2014-05-08 14:33:07]

> LGTM. Out of curiosity why we need to use macro-assembler Sub() in
> BumpSystemStackPointer?

Otherwise if the Operand is an immediate which is !uint_12, then the sub()
operation will fail with an Assert.

https://codereview.chromium.org/271543004/diff/80001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/271543004/diff/80001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.cc:1191: if (emit_debug_code() &&
use_real_aborts()) {
On 2014/05/08 14:01:48, jbramley wrote:
> I think a comment is in order.

Done.

https://codereview.chromium.org/271543004/diff/80001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.cc:1207: { DontEmitDebugCodeScope
dont_emit_debug_code_scope(this);
On 2014/05/08 14:01:48, jbramley wrote:
> Ditto.

Done.

[rmcilroy, 2014-05-09 09:41:34]

On 2014/05/08 14:33:07, rmcilroy wrote:
> > LGTM. Out of curiosity why we need to use macro-assembler Sub() in
> > BumpSystemStackPointer?
> 
> Otherwise if the Operand is an immediate which is !uint_12, then the sub()
> operation will fail with an Assert.
> 
>
https://codereview.chromium.org/271543004/diff/80001/src/arm64/macro-assemble...
> File src/arm64/macro-assembler-arm64.cc (right):
> 
>
https://codereview.chromium.org/271543004/diff/80001/src/arm64/macro-assemble...
> src/arm64/macro-assembler-arm64.cc:1191: if (emit_debug_code() &&
> use_real_aborts()) {
> On 2014/05/08 14:01:48, jbramley wrote:
> > I think a comment is in order.
> 
> Done.
> 
>
https://codereview.chromium.org/271543004/diff/80001/src/arm64/macro-assemble...
> src/arm64/macro-assembler-arm64.cc:1207: { DontEmitDebugCodeScope
> dont_emit_debug_code_scope(this);
> On 2014/05/08 14:01:48, jbramley wrote:
> > Ditto.
> 
> Done.

One final change to always align (and check alignment) if FLAG_debug_code is
true in order to give us better test coverage of the alignment.  This also
removes the was_csp_modified flag from AssertStackConstency.  I will land this
after ensuring it passes all the tests in debug if there are no objections.

[rmcilroy, 2014-05-09 11:18:45]

https://codereview.chromium.org/271543004/diff/140001/test/cctest/test-assemb...
File test/cctest/test-assembler-arm64.cc (right):

https://codereview.chromium.org/271543004/diff/140001/test/cctest/test-assemb...
test/cctest/test-assembler-arm64.cc:8390: static RegList const allowed =
~(ip0.Bit() | ip1.Bit() | jssp.Bit());
One last change required here - I think this is a leftover from when you were
using x8/x9 instead of ip0/ip1 as default tmp registers.

[jbramley, 2014-05-09 11:23:51]

https://codereview.chromium.org/271543004/diff/140001/test/cctest/test-assemb...
File test/cctest/test-assembler-arm64.cc (right):

https://codereview.chromium.org/271543004/diff/140001/test/cctest/test-assemb...
test/cctest/test-assembler-arm64.cc:8390: static RegList const allowed =
~(ip0.Bit() | ip1.Bit() | jssp.Bit());
On 2014/05/09 11:18:46, rmcilroy wrote:
> One last change required here - I think this is a leftover from when you were
> using x8/x9 instead of ip0/ip1 as default tmp registers.

We used x8/x9 _and_ ip0/ip1: ip0/ip1 were for general MacroAssembler utilities,
and x8/x9 were used in debug code only (and rarely). Since it worked before, we
should be able to remove them from this mask entirely. It might be a good idea
to clear TmpList() to be safe.

[rmcilroy, 2014-05-09 11:30:55]

https://codereview.chromium.org/271543004/diff/140001/test/cctest/test-assemb...
File test/cctest/test-assembler-arm64.cc (right):

https://codereview.chromium.org/271543004/diff/140001/test/cctest/test-assemb...
test/cctest/test-assembler-arm64.cc:8390: static RegList const allowed =
~(ip0.Bit() | ip1.Bit() | jssp.Bit());
On 2014/05/09 11:23:51, jbramley wrote:
> On 2014/05/09 11:18:46, rmcilroy wrote:
> > One last change required here - I think this is a leftover from when you
were
> > using x8/x9 instead of ip0/ip1 as default tmp registers.
> 
> We used x8/x9 _and_ ip0/ip1: ip0/ip1 were for general MacroAssembler
utilities,
> and x8/x9 were used in debug code only (and rarely). Since it worked before,
we
> should be able to remove them from this mask entirely. It might be a good idea
> to clear TmpList() to be safe.

The reason this fails now without this change is because a tmp from tmplist (ip0
or ip1) will be used to align csp during the push/pops.  I could clear Tmplist
and remove these from mask such that it falls back to the no-tmp available
branch in BumpSystemStackPointer -  would you prefer I did this?

[jbramley, 2014-05-09 11:40:54]

On 2014/05/09 11:30:55, rmcilroy wrote:
>
https://codereview.chromium.org/271543004/diff/140001/test/cctest/test-assemb...
> File test/cctest/test-assembler-arm64.cc (right):
> 
>
https://codereview.chromium.org/271543004/diff/140001/test/cctest/test-assemb...
> test/cctest/test-assembler-arm64.cc:8390: static RegList const allowed =
> ~(ip0.Bit() | ip1.Bit() | jssp.Bit());
> On 2014/05/09 11:23:51, jbramley wrote:
> > On 2014/05/09 11:18:46, rmcilroy wrote:
> > > One last change required here - I think this is a leftover from when you
> were
> > > using x8/x9 instead of ip0/ip1 as default tmp registers.
> > 
> > We used x8/x9 _and_ ip0/ip1: ip0/ip1 were for general MacroAssembler
> utilities,
> > and x8/x9 were used in debug code only (and rarely). Since it worked before,
> we
> > should be able to remove them from this mask entirely. It might be a good
idea
> > to clear TmpList() to be safe.
> 
> The reason this fails now without this change is because a tmp from tmplist
(ip0
> or ip1) will be used to align csp during the push/pops.  I could clear Tmplist
> and remove these from mask such that it falls back to the no-tmp available
> branch in BumpSystemStackPointer -  would you prefer I did this?

That, or initialize `allowed` with `~TmpList().list()`.

[rmcilroy, 2014-05-09 11:58:28]

On 2014/05/09 11:40:54, jbramley wrote:
> On 2014/05/09 11:30:55, rmcilroy wrote:
> >
>
https://codereview.chromium.org/271543004/diff/140001/test/cctest/test-assemb...
> > File test/cctest/test-assembler-arm64.cc (right):
> > 
> >
>
https://codereview.chromium.org/271543004/diff/140001/test/cctest/test-assemb...
> > test/cctest/test-assembler-arm64.cc:8390: static RegList const allowed =
> > ~(ip0.Bit() | ip1.Bit() | jssp.Bit());
> > On 2014/05/09 11:23:51, jbramley wrote:
> > > On 2014/05/09 11:18:46, rmcilroy wrote:
> > > > One last change required here - I think this is a leftover from when you
> > were
> > > > using x8/x9 instead of ip0/ip1 as default tmp registers.
> > > 
> > > We used x8/x9 _and_ ip0/ip1: ip0/ip1 were for general MacroAssembler
> > utilities,
> > > and x8/x9 were used in debug code only (and rarely). Since it worked
before,
> > we
> > > should be able to remove them from this mask entirely. It might be a good
> idea
> > > to clear TmpList() to be safe.
> > 
> > The reason this fails now without this change is because a tmp from tmplist
> (ip0
> > or ip1) will be used to align csp during the push/pops.  I could clear
Tmplist
> > and remove these from mask such that it falls back to the no-tmp available
> > branch in BumpSystemStackPointer -  would you prefer I did this?
> 
> That, or initialize `allowed` with `~TmpList().list()`.

Done.

[rmcilroy, 2014-05-09 12:52:08]

Committed patchset #9 manually as r21221.