Add support for debug break in Dart source.

Add support for debug break in Dart source.
The new flag --enable-debug-break turns the otherwise illegal Dart statement
  break "message";
into a break instruction preceded with a debug message, the equivalent of
emitting an Assembler::Stop("message").
Add a language test expecting a syntax error without the flag.
Change expected break instruction in arm64 simulator.
Remove constants related to now deleted simulator tracing on mips and arm64.

R=johnmccutchan@google.com, srdjan@google.com

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

[regis, 2015-04-15 23:35:22]

regis@google.com changed reviewers:
+ iposva@google.com, johnmccutchan@google.com, srdjan@google.com

[regis, 2015-04-15 23:35:22]

This is the next attempt.

Srdjan, can you please check that my new StopInstr class is declaring the proper
virtual calls? Thanks. 

The stop instruction is apparently not handled correctly on simarm64. I'll fix
it, but that is independent of this cl.

Thanks,
Regis

[Cutch, 2015-04-16 00:17:00]

LGTM with a test that fails to compile without the flag.

[regis, 2015-04-16 18:14:31]

On 2015/04/16 00:17:00, Cutch wrote:
> LGTM with a test that fails to compile without the flag.

Thanks!

I added a test, fixed a bad constant in arm64 simulator, and removed unused
constants on mips and arm64.
I was also missing a NULL check in parser

PTAL

[srdjan, 2015-04-16 18:23:02]

lgtm

[regis, 2015-04-16 19:33:04]

Committed patchset #3 (id:40001) manually as r45218 (presubmit successful).

[Ivan Posva, 2015-04-16 21:44:51]

Thanks for changing this around so much!

Small comments added...

-Ivan

https://codereview.chromium.org/1087383002/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1087383002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9569: const char* message =
strdup(CurrentLiteral()->ToCString());
Who is responsible for freeing this memory?

https://codereview.chromium.org/1087383002/diff/40001/tests/language/vm/debug...
File tests/language/vm/debug_break_vm_test.dart (right):

https://codereview.chromium.org/1087383002/diff/40001/tests/language/vm/debug...
tests/language/vm/debug_break_vm_test.dart:4: //
VMOptions=--optimization-counter-threshold=5
Is there any particular reason for using a special optimization count threshold
in this test? Is so (optimized code?) then please add a comment.

[Ivan Posva, 2015-04-16 21:47:05]

https://codereview.chromium.org/1087383002/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1087383002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9569: const char* message =
strdup(CurrentLiteral()->ToCString());
On 2015/04/16 21:44:51, Ivan Posva wrote:
> Who is responsible for freeing this memory?

Got the answer for this one: Stop does not do a strdup. It is the related
Unimplemented and similar which do allocate the final string.

[regis, 2015-04-16 22:17:04]

> Thanks for changing this around so much!

You are welcome.

https://codereview.chromium.org/1087383002/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1087383002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9569: const char* message =
strdup(CurrentLiteral()->ToCString());
On 2015/04/16 21:47:05, Ivan Posva wrote:
> On 2015/04/16 21:44:51, Ivan Posva wrote:
> > Who is responsible for freeing this memory?
> 
> Got the answer for this one: Stop does not do a strdup. It is the related
> Unimplemented and similar which do allocate the final string.

Yes, the strdup is necessary, since the literal string gets freed before
execution. We have many other places allocating error strings with malloc.

https://codereview.chromium.org/1087383002/diff/40001/tests/language/vm/debug...
File tests/language/vm/debug_break_vm_test.dart (right):

https://codereview.chromium.org/1087383002/diff/40001/tests/language/vm/debug...
tests/language/vm/debug_break_vm_test.dart:4: //
VMOptions=--optimization-counter-threshold=5
On 2015/04/16 21:44:51, Ivan Posva wrote:
> Is there any particular reason for using a special optimization count
threshold
> in this test? Is so (optimized code?) then please add a comment.

Yes and no.
Ideally, I would like to test two different flag configurations, which I could
specify with 2 lines:
// VMOptions=--optimization-counter-threshold=5
// VMOptions=--optimization-counter-threshold=5 --enable-debug-break

However, I cannot specify different expected results for the second line. In
that case, I do not expect a compile-time error, but I want to hit the debug
break at runtime. I also do not want to crash at compile time while optimizing
the new StopInstr. This actually happened with patch #1, because I wrongly
assumed that the optimizer would not see this instruction (see
intermediate_language.h in patch #1).

In short, the optimization count threshold is only useful when running this test
by hand and adding the --enable-debug-break flag. I allowed me to test and fix
patch #1.

I will add a comment.

[regis, 2015-04-16 22:37:21]

On 2015/04/16 22:17:04, regis wrote:
> > Thanks for changing this around so much!
> 
> You are welcome.
> 
> https://codereview.chromium.org/1087383002/diff/40001/runtime/vm/parser.cc
> File runtime/vm/parser.cc (right):
> 
>
https://codereview.chromium.org/1087383002/diff/40001/runtime/vm/parser.cc#ne...
> runtime/vm/parser.cc:9569: const char* message =
> strdup(CurrentLiteral()->ToCString());
> On 2015/04/16 21:47:05, Ivan Posva wrote:
> > On 2015/04/16 21:44:51, Ivan Posva wrote:
> > > Who is responsible for freeing this memory?
> > 
> > Got the answer for this one: Stop does not do a strdup. It is the related
> > Unimplemented and similar which do allocate the final string.
> 
> Yes, the strdup is necessary, since the literal string gets freed before
> execution. We have many other places allocating error strings with malloc.
> 
>
https://codereview.chromium.org/1087383002/diff/40001/tests/language/vm/debug...
> File tests/language/vm/debug_break_vm_test.dart (right):
> 
>
https://codereview.chromium.org/1087383002/diff/40001/tests/language/vm/debug...
> tests/language/vm/debug_break_vm_test.dart:4: //
> VMOptions=--optimization-counter-threshold=5
> On 2015/04/16 21:44:51, Ivan Posva wrote:
> > Is there any particular reason for using a special optimization count
> threshold
> > in this test? Is so (optimized code?) then please add a comment.
> 
> Yes and no.
> Ideally, I would like to test two different flag configurations, which I could
> specify with 2 lines:
> // VMOptions=--optimization-counter-threshold=5
> // VMOptions=--optimization-counter-threshold=5 --enable-debug-break
> 
> However, I cannot specify different expected results for the second line. In
> that case, I do not expect a compile-time error, but I want to hit the debug
> break at runtime. I also do not want to crash at compile time while optimizing
> the new StopInstr. This actually happened with patch #1, because I wrongly
> assumed that the optimizer would not see this instruction (see
> intermediate_language.h in patch #1).
> 
> In short, the optimization count threshold is only useful when running this
test
> by hand and adding the --enable-debug-break flag. I allowed me to test and fix
> patch #1.
> 
> I will add a comment.

One more detail: Instead of adding a different VMOptions= line in the same test
(not possible because a different outcome cannot be specified), I could have
added a copy of the test with both --optimization-counter-threshold=5 and
--enable-debug-break, but I was not sure how to make the difference between an
optimizer crash or hitting the breakpoint at runtime, which is also a crash.

Now that I think of it, the second test could actually be written so that the
breakpoint is only compiled but never hit, so no crash. That would only test the
optimizer, not the actual debug break functionality.

Let me try that.