* Modify simulator and ARM code generator to avoid swi...

* Modify simulator and ARM code generator to avoid swi
instructions.  The intention is that the snapshots generated
by the simulator should be usable on the hardware.  Instead of
swi instructions we generate a branch to a swi instruction that
is not part of the snapshot.  The call/jump is patched up in
the same way as other external references when the snapshot
is deserialized.  This only works for EABI targets: on old ABI
targets we still emit some instructions not supported by the
simulator (fp coprocessor instructions).

Committed: http://code.google.com/p/v8/source/detail?r=2127

[Kevin Millikin (Chromium), 2009-06-03 12:56:08]

I think it looks OK, but Ivan should definitely look at the simulator changes.

http://codereview.chromium.org/119036/diff/32/1038
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/119036/diff/32/1038#newcode1058
Line 1058: emit((intptr_t) msg);
reinterpret_cast<intptr_t>(msg)

http://codereview.chromium.org/119036/diff/32/1036
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/119036/diff/32/1036#newcode4433
Line 4433: // Actually call the add routine written in C.
"Actually call" sounds weird in its new non-conditional-compilation context.

http://codereview.chromium.org/119036/diff/32/1031
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/119036/diff/32/1031#newcode31
Line 31: // The simulator emulates the EABI so we define the __ARM_EABI__ macro
if we
if and only if?

http://codereview.chromium.org/119036/diff/32/1031#newcode33
Line 33: // old ABI used fp registers in the calling convention and the
simulator does
used ==> uses

http://codereview.chromium.org/119036/diff/32/1032
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/119036/diff/32/1032#newcode449
Line 449: }
How about more blank lines in here?

http://codereview.chromium.org/119036/diff/32/1032#newcode478
Line 478: 
How about fewer blank lines in here?

http://codereview.chromium.org/119036/diff/32/1026
File src/assembler.h (right):

http://codereview.chromium.org/119036/diff/32/1026#newcode359
Line 359: // An ExternalReference represents a C++ address called from the
generated
"called from" ==> "used in"

http://codereview.chromium.org/119036/diff/32/1026#newcode424
Line 424: // Used by the simulator to catch calls to external references.
"by the ARM simulator"

http://codereview.chromium.org/119036/diff/32/1030
File src/version.h (right):

http://codereview.chromium.org/119036/diff/32/1030#newcode1
Line 1: // Copyright 2009 the V8 project authors. All rights reserved.
Bizarre that this file shows up as "replaced" in this change.

[iposva, 2009-06-04 08:25:42]

Overall I appreciate to complexity of trying to support snapshots generated in a
simulated environment when building on real hardware. Nonetheless I do think
there is potential to limit the intrusion into other parts of the code. Here is
my first round of comments. I will gladly take a second look once these have
been addressed (and I promise faster turnaround on the next set of changes.)

Please also fix the description: simultor->simulator

Thanks,
-Ivan

http://codereview.chromium.org/119036/diff/32/1033
File src/arm/disasm-arm.cc (right):

http://codereview.chromium.org/119036/diff/32/1033#newcode761
Line 761: "%08x  ",
Any reason for the formatting change here? Just curious...

http://codereview.chromium.org/119036/diff/32/1033#newcode763
Line 763: if (instr->ConditionField() == special_condition) {
This should go away as Assembler::stop() is now using the bkpt instruction. Also
you are missing the special decoding of the bkpt instruction. Have you tested
using a stop instruction, disassembling the code and running through it with the
simulator debugger?

http://codereview.chromium.org/119036/diff/32/1033#newcode885
Line 885: d.set_regs_affected(NULL);
This is a "useless" statement. The decoder d is being destroyed as part of the
return. Which makes it possible to go back to "return
d.InstructionDecode(instruction);"

http://codereview.chromium.org/119036/diff/32/1032
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/119036/diff/32/1032#newcode442
Line 442: explicit Redirection(void* external_function,
No need for explicit in a constructor with more than one argument.

http://codereview.chromium.org/119036/diff/32/1032#newcode449
Line 449: }
On 2009/06/03 12:56:08, Kevin Millikin wrote:
> How about more blank lines in here?

Yes, please.

And also please group the static functions together at the end.

http://codereview.chromium.org/119036/diff/32/1032#newcode456
Line 456: for (current = list_; current != NULL; current = current->next_) {
Can you move the declaration into the for statement?
As in:
for (Redirection* current = list_; ...

http://codereview.chromium.org/119036/diff/32/1032#newcode464
Line 464: char* addr_of_redirection = addr_of_swi - sizeof(void*);  // NOLINT
Shouldn't you use OFFSET_OF(Redirection, swi_instruction_) here? That would be
safer and future proof to reordering of the fields. For example if somebody
wanted to bring the fields into the order they are mentioned in the comment
above ("We write the original destination of the jump just after the swi
instruction so the simulator knows what to call.")

http://codereview.chromium.org/119036/diff/32/1032#newcode482
Line 482: bool fp_return) {
same as in .h file:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/119036/diff/32/1032#newcode483
Line 483: // This leaks memory but it doesn't matter because it only happens in
the ARM
I do not see how this is leaking memory. Everyone of these redirections is
needed when running with the simulator and you make sure to not duplicate
entries inside the Get() function. I would just say that it is a linear lookup
inside the Get function, but that the number of entries and lookups needed is
fairly limited.

http://codereview.chromium.org/119036/diff/32/1032#newcode1024
Line 1024: int32_t saved_lr = get_register(lr);
to be consistent with the code above this should be intptr_t

http://codereview.chromium.org/119036/diff/32/1032#newcode1026
Line 1026: int32_t external =
ditto: intptr_t

http://codereview.chromium.org/119036/diff/32/1032#newcode1039
Line 1039: int32_t external =
ditto: intptr_t

http://codereview.chromium.org/119036/diff/32/1032#newcode1373
Line 1373: dbg.Debug();
Can you please roll this into the Debugger.Stop() functionality? This way the
special decoding in InstructionDecode() would disappear. You already did change
the Assembler::stop() to correctly deal with this, but why did you not handle it
here? Also by not calling Debugger::Stop() here all the special handling in Stop
to deal with generated code coverage is missing.

http://codereview.chromium.org/119036/diff/32/1032#newcode1615
Line 1615: if (instr->ConditionField() == special_condition) {
This whole block can be entirely dropped by using the bkpt instruction.

http://codereview.chromium.org/119036/diff/32/1032#newcode1634
Line 1634: for (int i = 0; i < 15; i++) {  // Don't print pc.
What is the benefit of the register value tracing and is it worth polluting the
disassembler API for it?
Also wouldn't it be useful to print the value as it changed? Meaning you could
put the register printing into the Simulator::get_register and
Simulator::set_register methods which also intercepts all registers affected by
this instruction as it is being executed. In addition you can print the value
being assigned to a register. In order to not slow down the simulator too much,
this is probably code that should only be executed in DEBUG for the register
accesses inside the simulator.

http://codereview.chromium.org/119036/diff/32/1034
File src/arm/simulator-arm.h (right):

http://codereview.chromium.org/119036/diff/32/1034#newcode184
Line 184: bool fp_return);
Please adjust according to
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

[Erik Corry, 2009-06-05 11:11:22]

Updated, Ivan please take another look.

http://codereview.chromium.org/119036/diff/32/1031
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/119036/diff/32/1031#newcode31
Line 31: // The simulator emulates the EABI so we define the __ARM_EABI__ macro
if we
On 2009/06/03 12:56:08, Kevin Millikin wrote:
> if and only if?

Just if.

I'll indent these to make them easier to read.  The reason I am undefing is that
some compilers will warn you if you define something that was already defined.

http://codereview.chromium.org/119036/diff/32/1032
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/119036/diff/32/1032#newcode456
Line 456: for (current = list_; current != NULL; current = current->next_) {
On 2009/06/04 08:25:42, iposva wrote:
> Can you move the declaration into the for statement?
> As in:
> for (Redirection* current = list_; ...

No.  80 column limit.

http://codereview.chromium.org/119036/diff/32/1032#newcode1024
Line 1024: int32_t saved_lr = get_register(lr);
On 2009/06/04 08:25:42, iposva wrote:
> to be consistent with the code above this should be intptr_t

I made them all into int32_t for consistency.  This is an ARM simulator after
all, the registers aren't going to change size.

http://codereview.chromium.org/119036/diff/32/1032#newcode1026
Line 1026: int32_t external =
On 2009/06/04 08:25:42, iposva wrote:
> ditto: intptr_t

Done.  This doesn't have to be consistent with the register above.  It's a host
pointer so intptr_t is correct here.

http://codereview.chromium.org/119036/diff/32/1032#newcode1373
Line 1373: dbg.Debug();
On 2009/06/04 08:25:42, iposva wrote:
> Can you please roll this into the Debugger.Stop() functionality? This way the
> special decoding in InstructionDecode() would disappear. You already did
change
> the Assembler::stop() to correctly deal with this, but why did you not handle
it
> here? Also by not calling Debugger::Stop() here all the special handling in
Stop
> to deal with generated code coverage is missing.

I don't understand this stuff, so I'm just going to remove this from the change.

[iposva, 2009-06-08 21:48:41]

LGTM after addressing small comments below.

-Ivan

http://codereview.chromium.org/119036/diff/35/40
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/119036/diff/35/40#newcode39
Line 39: # define __ARM_EABI__ 1
It makes me a bit uncomfortable to be defining "compiler reserved" macros, but I
guess this should be fine.

http://codereview.chromium.org/119036/diff/35/41
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/119036/diff/35/41#newcode432
Line 432: // the simulator.  We write the original destination of the jump just
after the
Please change the comment to say "We write the original destination of the jump
at a known offset from the swi instruction so the simulator knows what to call."
to be somewhat independent of the implementation.

http://codereview.chromium.org/119036/diff/35/41#newcode487
Line 487: v8::internal::Thread::CreateThreadLocalKey();
Please move this static initializer into the Initialize() method while we are
adding the new initializing functionality.

http://codereview.chromium.org/119036/diff/35/41#newcode1051
Line 1051: set_register(r0, result);
Isn't the second write to r0 redundant here? Strangely the compiler does not
complain that you are passing an int64_t into an int32_t parameter.

http://codereview.chromium.org/119036/diff/35/37
File src/v8.cc (right):

http://codereview.chromium.org/119036/diff/35/37#newcode70
Line 70: #if !defined(__arm__) && V8_TARGET_ARCH_ARM
For consistency we should start using V8_HOST_ARCH_ARM instead of
"defined(__arm__)".

[Erik Corry, 2009-06-09 09:27:00]

http://codereview.chromium.org/119036/diff/35/40
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/119036/diff/35/40#newcode39
Line 39: # define __ARM_EABI__ 1
On 2009/06/08 21:48:41, iposva wrote:
> It makes me a bit uncomfortable to be defining "compiler reserved" macros, but
I
> guess this should be fine.

Fixed with the introduction of USE_ARM_EABI macro instead.

http://codereview.chromium.org/119036/diff/35/41
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/119036/diff/35/41#newcode1051
Line 1051: set_register(r0, result);
On 2009/06/08 21:48:41, iposva wrote:
> Isn't the second write to r0 redundant here? Strangely the compiler does not
> complain that you are passing an int64_t into an int32_t parameter.

Fixed.

http://codereview.chromium.org/119036/diff/35/37
File src/v8.cc (right):

http://codereview.chromium.org/119036/diff/35/37#newcode70
Line 70: #if !defined(__arm__) && V8_TARGET_ARCH_ARM
On 2009/06/08 21:48:41, iposva wrote:
> For consistency we should start using V8_HOST_ARCH_ARM instead of
> "defined(__arm__)".

done.