Adds tests to the AssemblerX8632.

Adds basic tests to the AssemblerX8632.

More tests will be added during the AssemblerX8664 (on a separate CL.)

BUG=
R=jvoung@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=f746f3d39ff531cb7ad0350c9aab221616f3ab10

[John, 2015-07-10 00:04:39]

jpp@chromium.org changed reviewers:
+ jfb@chromium.org, jvoung@chromium.org, kschimpf@chromium.org,
stichnot@chromium.org

[jvoung (off chromium), 2015-07-10 18:17:32]

neat -- cool that you made this work with -m64 (so far =)) though it's testing
the x86-32 assembler

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
File unittest/IceAssemblerX8632Test.cpp (right):

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:55: //   __ ret();
Maybe the example should have __ ret() after, since that's the more usual
sequence.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:79: assert(Buffer[I] == Byte);
What's the need for this latter assert, if there is already the EXPECT_EQ(Byte,
Buffer[I])?

I see, to make it a fatal error so that later tests are not performed.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:88: assert(ByteCount == codeBytesSize());
Question for here and below -- why not use ASSERT_EQ instead of an EXPECT_EQ +
assert pair?

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:104: __ popl(GPRRegister::Encoded_Reg_eax);
This test is called PushRegs, but the method used is pop ?

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:116: constexpr uint8_t PushOpcode = 0x58;
PopOpcode?

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:126: TEST_F(AssemblerX8632LowLevelTest,
PopRegs) {
Rename this to "PushRegs"?

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:246: //       pop  %ebp
__ popl(GPRRegister::Encoded_Reg_ebp) in the code happens right after the call

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:250: //      mov %eax, $(1M + 0 -
(test$materialize_ip - test))(%ebp,,)
nit: indentation seems to change here (not sure if that's intentional)

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:250: //      mov %eax, $(1M + 0 -
(test$materialize_ip - test))(%ebp,,)
I think these slots need a bit more explanation.

So ebp will hold "test$materialize_ip" after the pop. The "(test$materialize_ip
- test)" will also have some "cancelling out". So really this is like "test + 1M
+ {0,4,8,...}") ?

Ok, so the buffer is actually allocating memory beyond these instructions  at
"test + 1M". The mov %eax, etc... is to save the final values to that "far" away
scratch area in the allocated buffer for later inspection (so you can inspect
more than just the return value from eax).

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:274: // the jitted code should not perform
any arithmetic with ebp (not that
not that -> note that

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:276: // sign-extenteded.)
extenteded -> extended

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:299: // scratchpad area that tests may use --
all tests use this scraychpad area
scraychpad -> scratchpad

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:317: explicit AssembledBuffer(const uint8_t
*Data, const size_t MySize,
doesn't need "explicit" w/ multiple params?

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:391: if (NeedsEpilogue) {
Is it ever kosher to run assemble() twice?

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:409: // the spcified register. These are all
private for, when jitting the test
spcified -> specified

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:641: __ mov(IceType_i32,
GPRRegister::Encoded_Reg_##Dest, Immediate(0));        \
How about set to something other than 0 (and other than C0FFEE, since the
registers are initialized to 0 in the prologue?

[John, 2015-07-10 23:26:37]

I do not intend to add any other tests to this CL -- I'll add them before
modifying the assembler template, in a later CL.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
File unittest/IceAssemblerX8632Test.cpp (right):

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:55: //   __ ret();
On 2015/07/10 18:17:31, jvoung wrote:
> Maybe the example should have __ ret() after, since that's the more usual
> sequence.

A very unfortunate example. Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:79: assert(Buffer[I] == Byte);
On 2015/07/10 18:17:31, jvoung wrote:
> What's the need for this latter assert, if there is already the
EXPECT_EQ(Byte,
> Buffer[I])?
> 
> I see, to make it a fatal error so that later tests are not performed.

Yes, exactly: I want verifyBytes to report all errors before an assertion
happens.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:88: assert(ByteCount == codeBytesSize());
On 2015/07/10 18:17:31, jvoung wrote:
> Question for here and below -- why not use ASSERT_EQ instead of an EXPECT_EQ +
> assert pair?

I don't like ASSERT_<<cond>>, they only work within TEST() and TEST_F() bodies.
They expand to something that return;s on errors, which is not really asserting.

They might make sense in test cases (one failure should not prevent other tests
from running), but then you can't ASSERT_EQ() inside a member function.

the use

EXPECT_<<COND>>
assert();

gives me, roughly, what

CHECK_<<COND>>

gives me: either the condition holds, or the whole program/test will abort.
Anyway, I changed the instances in test bodies use ASSERT_<<COND>>, but I left
the other uses (as they don't even compile.)

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:104: __ popl(GPRRegister::Encoded_Reg_eax);
On 2015/07/10 18:17:32, jvoung wrote:
> This test is called PushRegs, but the method used is pop ?

I was tired... Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:116: constexpr uint8_t PushOpcode = 0x58;
On 2015/07/10 18:17:31, jvoung wrote:
> PopOpcode?

Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:126: TEST_F(AssemblerX8632LowLevelTest,
PopRegs) {
On 2015/07/10 18:17:31, jvoung wrote:
> Rename this to "PushRegs"?

Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:246: //       pop  %ebp
On 2015/07/10 18:17:32, jvoung wrote:
> __ popl(GPRRegister::Encoded_Reg_ebp) in the code happens right after the call

Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:250: //      mov %eax, $(1M + 0 -
(test$materialize_ip - test))(%ebp,,)
On 2015/07/10 18:17:31, jvoung wrote:
> nit: indentation seems to change here (not sure if that's intentional)

Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:250: //      mov %eax, $(1M + 0 -
(test$materialize_ip - test))(%ebp,,)
On 2015/07/10 18:17:32, jvoung wrote:
> I think these slots need a bit more explanation.
But I already explain about the scratchpad area above. I added a note that these
"slots" use the tests scratchpad area. I also changed the comment so that they
read

mov %<reg>, {0 + $ScratchpadOffset}(%ebp)

see if this helps.

> 
> So ebp will hold "test$materialize_ip" after the pop. The
"(test$materialize_ip
> - test)" will also have some "cancelling out". So really this is like "test +
1M
> + {0,4,8,...}") ?
ebp will point right after the call (at the pop %ebp instruction.)

you're right about the cancelling out. I could simply adjust ebp with a

sub $12, %ebp

but that instruction behaves differently x86-32 and x86-64, which would
effectively destroy the contents of %rbp in x86-64. Bummer, it would be nice to
be able to use

(1M + dword * 4)(%ebp)


> 
> Ok, so the buffer is actually allocating memory beyond these instructions  at
> "test + 1M". The mov %eax, etc... is to save the final values to that "far"
away
> scratch area in the allocated buffer for later inspection (so you can inspect
> more than just the return value from eax).

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:274: // the jitted code should not perform
any arithmetic with ebp (not that
On 2015/07/10 18:17:32, jvoung wrote:
> not that -> note that

Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:276: // sign-extenteded.)
On 2015/07/10 18:17:32, jvoung wrote:
> extenteded -> extended 

Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:299: // scratchpad area that tests may use --
all tests use this scraychpad area
On 2015/07/10 18:17:32, jvoung wrote:
> scraychpad -> scratchpad

Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:317: explicit AssembledBuffer(const uint8_t
*Data, const size_t MySize,
On 2015/07/10 18:17:32, jvoung wrote:
> doesn't need "explicit" w/ multiple params?

Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:391: if (NeedsEpilogue) {
On 2015/07/10 18:17:32, jvoung wrote:
> Is it ever kosher to run assemble() twice?

Yes! :)

assemble **could** be a const method, if it weren't for the need to set
NeedsEpilogue to false.

This method just copies the bytes in the assembler buffer to a PROT_EXEC mmap'ed
buffer (wrapped around a AssemblerBuffer so that we can use destructors for
unmmap()ing.)

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:409: // the spcified register. These are all
private for, when jitting the test
On 2015/07/10 18:17:32, jvoung wrote:
> spcified -> specified

Done.

https://codereview.chromium.org/1231903002/diff/40001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:641: __ mov(IceType_i32,
GPRRegister::Encoded_Reg_##Dest, Immediate(0));        \
On 2015/07/10 18:17:32, jvoung wrote:
> How about set to something other than 0 (and other than C0FFEE, since the
> registers are initialized to 0 in the prologue?

Done.

[jvoung (off chromium), 2015-07-11 00:17:04]

lgtm

https://codereview.chromium.org/1231903002/diff/60001/unittest/IceAssemblerX8...
File unittest/IceAssemblerX8632Test.cpp (right):

https://codereview.chromium.org/1231903002/diff/60001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:240: //    mov offset(%rsp), %reg (64-bit
only)
You mean rbp instead of rsp?

Also note that this is the 32-bit %reg, or movl and not movq?

https://codereview.chromium.org/1231903002/diff/60001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:675: EXPECT_EQ(0xBEEFul, test.Dest()) <<
"Br_" #C ", " #Near;                   \
yum

[John, 2015-07-11 15:28:42]

https://codereview.chromium.org/1231903002/diff/60001/unittest/IceAssemblerX8...
File unittest/IceAssemblerX8632Test.cpp (right):

https://codereview.chromium.org/1231903002/diff/60001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:240: //    mov offset(%rsp), %reg (64-bit
only)
On 2015/07/11 00:17:04, jvoung wrote:
> You mean rbp instead of rsp?
> 
> Also note that this is the 32-bit %reg, or movl and not movq?
Good point about movl. 

Done.

https://codereview.chromium.org/1231903002/diff/60001/unittest/IceAssemblerX8...
unittest/IceAssemblerX8632Test.cpp:675: EXPECT_EQ(0xBEEFul, test.Dest()) <<
"Br_" #C ", " #Near;                   \
On 2015/07/11 00:17:04, jvoung wrote:
> yum

hehehe.

[John, 2015-07-11 15:29:26]

Committed patchset #5 (id:80001) manually as
f746f3d39ff531cb7ad0350c9aab221616f3ab10 (presubmit successful).