VM: Refactor assembler test that rely on the constant pool.

VM: Refactor assembler test that rely on the constant pool.

This makes the test code independent of the way we call Dart code and
makes it easier to change the calling convention.

The code for calling test code is now encapsulated in unit_test.h instead
of being spread over many different files.

I only changed the tests that need a constant pool set up, but the same strategy
could be used for all other assembler tests to avoid unnecessary duplicated
boilerplate code.

BUG=
R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/25966d228d65ce1722bba854e62f0b1c7433018b

[Florian Schneider, 2015-08-10 11:29:11]

fschneider@google.com changed reviewers:
+ srdjan@google.com, zra@google.com

[srdjan, 2015-08-10 16:23:51]

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:360: #if defined(USING_SIMULATOR)
I am not sure I found this more readable and maintainable than the original. It
look smart though ;-).

[zra, 2015-08-10 16:43:32]

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:360: #if defined(USING_SIMULATOR)
This makes writing the assembler tests a bit easier, but please add comments
about how to use Invoke, and the restrictions on the template arguments.

What does the compiler error look like if you try to use a two-argument version
of Invoke that doesn't exist? If it looks bad, maybe add a comment or TODO
somewhere explaining how to fix.

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:366: }
Missing newline

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:375: }
Missing newline

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:380: ResultType Invoke(Arg1Type arg1, Arg2Type arg2,
Arg3Type arg3) {
The first two Invokes appear to be used for assembler tests, while this one is
used for something else. Maybe give it a different name to avoid confusion.

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:400: typedef ResultType (*FunctionType) (Arg1Type);
Is there some reason not to have the same asserts here as in the simulated
version?

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:409: typedef ResultType (*FunctionType) (Arg1Type,
Arg2Type, Arg3Type);
Same question.

[Florian Schneider, 2015-08-11 07:58:29]

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:360: #if defined(USING_SIMULATOR)
On 2015/08/10 16:23:51, srdjan wrote:
> I am not sure I found this more readable and maintainable than the original.
It
> look smart though ;-).

There may be even more elegant ways to solve this >) I just needed a common
place for the actual invocation (Invoke method)

I only addressed the tests that need the constant pool, since this will change
when we change the calling convention and move the constant pool out of the
Instructions object.

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:360: #if defined(USING_SIMULATOR)
On 2015/08/10 16:43:32, zra wrote:
> This makes writing the assembler tests a bit easier, but please add comments
> about how to use Invoke, and the restrictions on the template arguments.
> 
> What does the compiler error look like if you try to use a two-argument
version
> of Invoke that doesn't exist? If it looks bad, maybe add a comment or TODO
> somewhere explaining how to fix.

Yes, I just added the variants necessary.
The compiler would complain about not finding a methods with the signature
requested. e.g. adding the 2-template-argument variant is very straight-forward
using the same pattern.

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:375: }
On 2015/08/10 16:43:32, zra wrote:
> Missing newline

Not between template functions

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:380: ResultType Invoke(Arg1Type arg1, Arg2Type arg2,
Arg3Type arg3) {
On 2015/08/10 16:43:32, zra wrote:
> The first two Invokes appear to be used for assembler tests, while this one is
> used for something else. Maybe give it a different name to avoid confusion.

This one is used ony for one test right now (StoreIntoObject), from
assembler_test.cc

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:400: typedef ResultType (*FunctionType) (Arg1Type);
On 2015/08/10 16:43:31, zra wrote:
> Is there some reason not to have the same asserts here as in the simulated
> version?

No, regular invocation just works with all types. For simulator-calls we need to
pass true/false for fp_return and fp_args accordingly. I didn't want to eagerly
add all variants, so I just added the asserts for now.

[zra, 2015-08-11 19:48:17]

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:400: typedef ResultType (*FunctionType) (Arg1Type);
On 2015/08/11 07:58:28, Florian Schneider wrote:
> On 2015/08/10 16:43:31, zra wrote:
> > Is there some reason not to have the same asserts here as in the simulated
> > version?
> 
> No, regular invocation just works with all types. For simulator-calls we need
to
> pass true/false for fp_return and fp_args accordingly. I didn't want to
eagerly
> add all variants, so I just added the asserts for now.

I think it would be good to have consistent behavior between simulated and
non-simulated builds. My suggestion would be to have the asserts here, too.

https://codereview.chromium.org/1283733003/diff/20001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1283733003/diff/20001/runtime/vm/unit_test.h#...
runtime/vm/unit_test.h:360: #if defined(USING_SIMULATOR)
Please add comments here about how to use Invoke() and the restrictions on the
argument/return types.

[Florian Schneider, 2015-08-12 07:30:53]

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:400: typedef ResultType (*FunctionType) (Arg1Type);
On 2015/08/11 19:48:17, zra wrote:
> On 2015/08/11 07:58:28, Florian Schneider wrote:
> > On 2015/08/10 16:43:31, zra wrote:
> > > Is there some reason not to have the same asserts here as in the simulated
> > > version?
> > 
> > No, regular invocation just works with all types. For simulator-calls we
need
> to
> > pass true/false for fp_return and fp_args accordingly. I didn't want to
> eagerly
> > add all variants, so I just added the asserts for now.
> 
> I think it would be good to have consistent behavior between simulated and
> non-simulated builds. My suggestion would be to have the asserts here, too.

Why add assertions that don't guard against anything? That would be even more
confusing.

Also I'd rather not want this CL to grow beyond what's I need for making
changing the calling convention as smooth as possible.

https://codereview.chromium.org/1283733003/diff/20001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1283733003/diff/20001/runtime/vm/unit_test.h#...
runtime/vm/unit_test.h:360: #if defined(USING_SIMULATOR)
On 2015/08/11 19:48:17, zra wrote:
> Please add comments here about how to use Invoke() and the restrictions on the
> argument/return types.

If you happen to write a test that requires a new return-type which needs
special handling, the compiler will complain.

The point of using templates is that you get a compile-time error when something
is not right. Therefore, I don't see the need for additional comments.

[srdjan, 2015-08-13 20:24:27]

We have reached an impasse with this review. Let's talk about it on Monday's
compiler meeting.

[zra, 2015-08-17 16:35:43]

lgtm only with the asserts I'm suggesting, as explained below.

I'd also still encourage you to add comments to templated code like this. Mere
mortals cannot read template errors from the compiler.

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1283733003/diff/1/runtime/vm/unit_test.h#newc...
runtime/vm/unit_test.h:400: typedef ResultType (*FunctionType) (Arg1Type);
On 2015/08/12 07:30:53, Florian Schneider wrote:
> On 2015/08/11 19:48:17, zra wrote:
> > On 2015/08/11 07:58:28, Florian Schneider wrote:
> > > On 2015/08/10 16:43:31, zra wrote:
> > > > Is there some reason not to have the same asserts here as in the
simulated
> > > > version?
> > > 
> > > No, regular invocation just works with all types. For simulator-calls we
> need
> > to
> > > pass true/false for fp_return and fp_args accordingly. I didn't want to
> > eagerly
> > > add all variants, so I just added the asserts for now.
> > 
> > I think it would be good to have consistent behavior between simulated and
> > non-simulated builds. My suggestion would be to have the asserts here, too.
> 
> Why add assertions that don't guard against anything? That would be even more
> confusing.

The assertions guard against doing something in architecture/simulator agnostic
code, like in assembler_test.cc, that won't work on the simulator. You will find
out early that the use of Invoke won't work.

https://codereview.chromium.org/1283733003/diff/20001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1283733003/diff/20001/runtime/vm/unit_test.h#...
runtime/vm/unit_test.h:360: #if defined(USING_SIMULATOR)
On 2015/08/12 07:30:53, Florian Schneider wrote:
> On 2015/08/11 19:48:17, zra wrote:
> > Please add comments here about how to use Invoke() and the restrictions on
the
> > argument/return types.
> 
> If you happen to write a test that requires a new return-type which needs
> special handling, the compiler will complain.
> 
> The point of using templates is that you get a compile-time error when
something
> is not right. Therefore, I don't see the need for additional comments.
> 

Template errors from the compiler are hard to read. I'm not sure this is a good
trade-off.

[Florian Schneider, 2015-08-18 09:36:10]

Committed patchset #4 (id:60001) manually as
25966d228d65ce1722bba854e62f0b1c7433018b (presubmit successful).