Added slow_assert macro and flag for slow development assertions in the VM.

Added slow_assert macro and flag for slow development assertions in the VM.


R=kmillikin@google.com

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

[Kevin Millikin (Google), 2012-10-01 10:53:25]

We should not commit such a limited-use python script.

We can plumb the flag through the test script to the VM, but for now it's better
to just stick the script up on a wiki somewhere for VM developers to use.

https://codereview.chromium.org/11014013/diff/1/runtime/platform/assert.h
File runtime/platform/assert.h (right):

https://codereview.chromium.org/11014013/diff/1/runtime/platform/assert.h#new...
runtime/platform/assert.h:264: if (FLAG_slow_assert) ASSERT(cond);              
                  \
Align the \.  You can add something like

(defun my-c++-mode-hook ()
  (setq c-backslash-column 79)
  (setq c-backslash-max-column 79))
(add-hook 'c++-mode-hook 'my-c++-mode-hook)

You can also write the one-liner:

#define SLOW_ASSERT(cond) ASSERT(!FLAG_slow_assert || (cond))

https://codereview.chromium.org/11014013/diff/1/runtime/platform/assert.h#new...
runtime/platform/assert.h:276: #define SLOW_ASSERT(cond)
This should probably just expand into ASSERT(cond) so it ensures that cond can
compile.

https://codereview.chromium.org/11014013/diff/1/runtime/vm/flow_graph_inliner.cc
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/11014013/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:24: DECLARE_FLAG(bool, slow_assert);
I guess I'd rather have DECLARE_FLAG and #define SLOW_ASSERT in one place, say
"vm/assert.h".

[zerny-google, 2012-10-01 11:12:04]

Thanks. This should address your comments.

http://codereview.chromium.org/11014013/diff/1/runtime/platform/assert.h
File runtime/platform/assert.h (right):

http://codereview.chromium.org/11014013/diff/1/runtime/platform/assert.h#newc...
runtime/platform/assert.h:264: if (FLAG_slow_assert) ASSERT(cond);              
                  \
On 2012/10/01 10:53:25, kmillikin wrote:
> Align the \.  You can add something like
> 
> (defun my-c++-mode-hook ()
>   (setq c-backslash-column 79)
>   (setq c-backslash-max-column 79))
> (add-hook 'c++-mode-hook 'my-c++-mode-hook)
> 
> You can also write the one-liner:
> 
> #define SLOW_ASSERT(cond) ASSERT(!FLAG_slow_assert || (cond))

Done.

http://codereview.chromium.org/11014013/diff/1/runtime/platform/assert.h#newc...
runtime/platform/assert.h:276: #define SLOW_ASSERT(cond)
On 2012/10/01 10:53:25, kmillikin wrote:
> This should probably just expand into ASSERT(cond) so it ensures that cond can
> compile.

Well, in the case of the verification code, it's not there when compiling
release mode.

http://codereview.chromium.org/11014013/diff/1/runtime/vm/flow_graph_inliner.cc
File runtime/vm/flow_graph_inliner.cc (right):

http://codereview.chromium.org/11014013/diff/1/runtime/vm/flow_graph_inliner....
runtime/vm/flow_graph_inliner.cc:24: DECLARE_FLAG(bool, slow_assert);
On 2012/10/01 10:53:25, kmillikin wrote:
> I guess I'd rather have DECLARE_FLAG and #define SLOW_ASSERT in one place, say
> "vm/assert.h".

Done.

[Kevin Millikin (Google), 2012-10-01 11:38:26]

LGTM.

https://codereview.chromium.org/11014013/diff/5001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/11014013/diff/5001/runtime/vm/compiler.cc#new...
runtime/vm/compiler.cc:52: DEFINE_FLAG(bool, slow_assert, false, "Enable slow
assertions");
Let's spell it "slow_asserts".

https://codereview.chromium.org/11014013/diff/5001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/11014013/diff/5001/runtime/vm/flow_graph.cc#n...
runtime/vm/flow_graph.cc:137: ASSERT(1 == MembershipCount(use,
use->definition()->input_use_list()));
Can we make this a slow assert?

And the same for the other calls to MembershipCount in non-validation code?

[zerny-google, 2012-10-01 11:50:36]

http://codereview.chromium.org/11014013/diff/5001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

http://codereview.chromium.org/11014013/diff/5001/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:52: DEFINE_FLAG(bool, slow_assert, false, "Enable slow
assertions");
On 2012/10/01 11:38:26, kmillikin wrote:
> Let's spell it "slow_asserts".

Done.

http://codereview.chromium.org/11014013/diff/5001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

http://codereview.chromium.org/11014013/diff/5001/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:137: ASSERT(1 == MembershipCount(use,
use->definition()->input_use_list()));
On 2012/10/01 11:38:26, kmillikin wrote:
> Can we make this a slow assert?
> 
> And the same for the other calls to MembershipCount in non-validation code?

Done.

[siva, 2012-10-01 23:35:40]

Please do not add a "vm/assert.h" file.

https://codereview.chromium.org/11014013/diff/10/runtime/vm/assert.h
File runtime/vm/assert.h (right):

https://codereview.chromium.org/11014013/diff/10/runtime/vm/assert.h#newcode3
runtime/vm/assert.h:3: // BSD-style license that can be found in the LICENSE
file.
I think it is not a good idea to add this file as we will end up with multiple
versions of assert.h and it can be very confusing.

https://codereview.chromium.org/11014013/diff/10/runtime/vm/assert.h#newcode20
runtime/vm/assert.h:20: #endif
I don't quite understand the need for a VM wide SLOW_ASSERT macro. We want to
run with ASSERT on all the time in many parts of the VM.

https://codereview.chromium.org/11014013/diff/10/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/11014013/diff/10/runtime/vm/compiler.cc#newco...
runtime/vm/compiler.cc:53: DEFINE_FLAG(bool, slow_asserts, false, "Enable slow
assertions");
Ditto comment regarding slow_asserts.

https://codereview.chromium.org/11014013/diff/10/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/11014013/diff/10/runtime/vm/flow_graph.cc#new...
runtime/vm/flow_graph.cc:137: SLOW_ASSERT(1 == MembershipCount(use,
use->definition()->input_use_list()));
It sounds like this needs to be
ASSERT(VerifyMembershipCount(use, use->definition()->input_use_list()));

if MemberShipCount is a very expensive operation and you do not want it executed
all the time then maybe you could have a '--verify-compiler-state' or some
equivalent flag and all these verification could be conditional on this flag.

https://codereview.chromium.org/11014013/diff/10/runtime/vm/flow_graph.cc#new...
runtime/vm/flow_graph.cc:285: SLOW_ASSERT(ValidateUseLists());
Ditto comment, this would be
ASSERT(ValidateUseLists());

and if that is an expensive operation you could conditionalize it under
'--verify-compiler-state'