Initial revision of ARM simulator and (empty) MIPS simulator.

Initial revision of ARM simulator and (empty) MIPS simulator.
Committed: https://code.google.com/p/dart/source/detail?r=17623

[siva, 2013-01-24 02:03:09]

LGTM with some comments.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:304: #endif
Is the #if defined(USING_SIMULATOR) necessary here?

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:364: }
Shouldn't we be creating a simulator here and assign it to
the isolate?

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/os_linux.cc
File runtime/vm/os_linux.cc (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/os_linux.cc...
runtime/vm/os_linux.cc:322: #endif
As discussed offline we have moved away from using asm statements in our code
because Mads found that they are not supported on win64.

I think this facility of generating a debug break is useful for generated code,
not sure why it needed for C++ code.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator.h
File runtime/vm/simulator.h (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator.h...
runtime/vm/simulator.h:15: #define USING_SIMULATOR
in globals.h we have used the pattern "#define <variable> 1"
There are some inconsistencies with the WIN32 and Android defines.

If HOST_ARCH_ARM and TARGET_ARCH_ARM are defined we don't want USING_SIMULATOR
to be defined right?

You have a #if !defined(HOST_ARCH_ARM) ... #endif wrapping in vm/simulator_arm.h
maybe the wrapping should be moved here into this file or "#define
USING_SIMULATOR 1" moved to vm/simulator_arm.h

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator.h...
runtime/vm/simulator.h:19: #include "vm/simulator_mips.h"
Ditto comment for the MIPS case.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
File runtime/vm/simulator_arm.cc (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
runtime/vm/simulator_arm.cc:549: Mutex* Simulator::exclusive_access_lock_;
 = NULL;

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
File runtime/vm/simulator_arm.h (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
runtime/vm/simulator_arm.h:38: // for each isolate.
There should be one for each isolate right, not sure why you say Potentially. Do
we want to run multiple isolates on the same simulator?

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
runtime/vm/simulator_arm.h:188: } exclusive_access_state_[kNumAddressTags];
Not sure if we will require this as there will be none of these mutual exclusion
instructions in the generated code.

Even when we support concurrent GC the other threads will only be running C++
code.

Not suggesting that we remove this now, maybe we should a TODO to clean this up
once we have stuff working on ARM and realize that we don't use this structure
at all.

[Kevin Millikin (Google), 2013-01-24 08:26:36]

Drive by.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/os_linux.cc
File runtime/vm/os_linux.cc (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/os_linux.cc...
runtime/vm/os_linux.cc:322: #endif
On 2013/01/24 02:03:09, siva wrote:
> I think this facility of generating a debug break is useful for generated
code,
> not sure why it needed for C++ code.

It's pretty minor, but it's nice to have because gdb's support for conditional
breakpoints is weak/broken.  For complicated conditions it's much easier to
recompile the project with a conditional OS::DebugBreak().  You can get the same
effect with a conditional ASSERT(true) and set a gdb breakpoint on the ASSERT. 
This just saves having to find the line and add the breakpoint, since you're
recompiling anyway.

I've just been using asm("int $3") directly since we originally removed this
handy function, but it's annoying to have to remember how to spell it on all
architectures if you work on more than one.  As for Win64, I have not yet
encountered a Win64-specific bug, but I suppose not having OS::DebugBreak
available there is the same as the current situation.

[Ivan Posva, 2013-01-24 18:25:57]

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

https://codereview.chromium.org/12041056/diff/1/runtime/vm/constants_arm.h#ne...
runtime/vm/constants_arm.h:143: const Register kExceptionObjectReg = R0;
Are you sure this is the right register? Or do you want to use kNoRegister here
to hit an assertion once it is being used?

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

https://codereview.chromium.org/12041056/diff/1/runtime/vm/isolate.h#newcode371
runtime/vm/isolate.h:371: Simulator* simulator_;
#ifdef USING_SIMULATOR?

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

https://codereview.chromium.org/12041056/diff/1/runtime/vm/os.h#newcode84
runtime/vm/os.h:84: static uword NullPageSize() { return null_page_size_; }
I am not sure what we need this for.

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

https://codereview.chromium.org/12041056/diff/1/runtime/vm/os_linux.cc#newcod...
runtime/vm/os_linux.cc:322: #endif
On 2013/01/24 02:03:09, siva wrote:
> As discussed offline we have moved away from using asm statements in our code
> because Mads found that they are not supported on win64.
> 
> I think this facility of generating a debug break is useful for generated
code,
> not sure why it needed for C++ code.

Since this is in a OS specific file we can add the correct way to trigger the
debugger on Windows without having to resort to inline assembly.

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

https://codereview.chromium.org/12041056/diff/1/runtime/vm/os_win.cc#newcode162
runtime/vm/os_win.cc:162: UNIMPLEMENTED();
http://msdn.microsoft.com/en-us/library/windows/desktop/ms679297(v=vs.85).aspx

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

https://codereview.chromium.org/12041056/diff/1/runtime/vm/simulator_arm.h#ne...
runtime/vm/simulator_arm.h:38: // for each isolate.
On 2013/01/24 02:03:09, siva wrote:
> There should be one for each isolate right, not sure why you say Potentially.
Do
> we want to run multiple isolates on the same simulator?

Actually the Simulator should be tied to the thread instead of the isolate. Each
thread in the thread pool should have its own Simulator instance, but then we
get into life cycle issues especially for the main thread. Having said that it
might be easier just to attach a Simulator with each Isolate for now.

[regis, 2013-01-24 21:47:48]

Thanks!
Please, have another look.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/constants_a...
File runtime/vm/constants_arm.h (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/constants_a...
runtime/vm/constants_arm.h:143: const Register kExceptionObjectReg = R0;
On 2013/01/24 18:25:57, Ivan Posva wrote:
> Are you sure this is the right register? Or do you want to use kNoRegister
here
> to hit an assertion once it is being used?

The exception register is EAX/RAX on ia32/x64, so I expect it to be R0 on ARM.
If I set it to kNoRegister, the compiler complains about an out of bounds array
access. I left it as R0, but added a comment.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:304: #endif
On 2013/01/24 02:03:09, siva wrote:
> Is the #if defined(USING_SIMULATOR) necessary here?

We would need to declare a dummy empty Simulator class for ia32 and x64,
otherwise, without the if-def, we get a warning (=error) that no destructor or
delete operator will be called, even if the forwarded declared class Simulator
is defined (it is not defined).

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:364: }
On 2013/01/24 02:03:09, siva wrote:
> Shouldn't we be creating a simulator here and assign it to
> the isolate?

We create the simulator on demand, when it is accessed for the first time. See
Simulator::Current(). Is there any benefit of doing it here? Either way is fine
with me.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:371: Simulator* simulator_;
On 2013/01/24 18:25:57, Ivan Posva wrote:
> #ifdef USING_SIMULATOR?

It's tricky. It would require including simulator.h, which itself includes
isolate.h, and this leads into nasty cycles.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/os.h
File runtime/vm/os.h (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/os.h#newcode84
runtime/vm/os.h:84: static uword NullPageSize() { return null_page_size_; }
On 2013/01/24 18:25:57, Ivan Posva wrote:
> I am not sure what we need this for.

Good point. Since we do not install handlers for segmentation violations, the
simulator can implement null checks independently of the actual page size.
Removed.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/os_linux.cc
File runtime/vm/os_linux.cc (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/os_linux.cc...
runtime/vm/os_linux.cc:322: #endif
On 2013/01/24 18:25:57, Ivan Posva wrote:
> On 2013/01/24 02:03:09, siva wrote:
> > As discussed offline we have moved away from using asm statements in our
code
> > because Mads found that they are not supported on win64.
> > 
> > I think this facility of generating a debug break is useful for generated
> code,
> > not sure why it needed for C++ code.
> 
> Since this is in a OS specific file we can add the correct way to trigger the
> debugger on Windows without having to resort to inline assembly.

As Kevin, I think that DebugBreak() is nice to have. I replaced the
UNIMPLEMENTED() on Windows by something that should do it in most cases:
void OS::DebugBreak() {
#if defined(_MSC_VER)
  // Microsoft Visual C/C++ or drop-in replacement.
  __debugbreak();
#elif defined(__GCC__)
  // MinGW?
  asm("int $3");
#else
  // Microsoft style assembly.
   __asm {
      int 3
   }
#endif
}

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/os_win.cc
File runtime/vm/os_win.cc (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/os_win.cc#n...
runtime/vm/os_win.cc:162: UNIMPLEMENTED();
On 2013/01/24 18:25:57, Ivan Posva wrote:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms679297%28v=vs.85%29...

I had already checked this one:
http://msdn.microsoft.com/en-us/library/f408b4et.aspx

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator.h
File runtime/vm/simulator.h (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator.h...
runtime/vm/simulator.h:15: #define USING_SIMULATOR
On 2013/01/24 02:03:09, siva wrote:
> in globals.h we have used the pattern "#define <variable> 1"
> There are some inconsistencies with the WIN32 and Android defines.
> 
> If HOST_ARCH_ARM and TARGET_ARCH_ARM are defined we don't want USING_SIMULATOR
> to be defined right?
> 
> You have a #if !defined(HOST_ARCH_ARM) ... #endif wrapping in
vm/simulator_arm.h
> maybe the wrapping should be moved here into this file or "#define
> USING_SIMULATOR 1" moved to vm/simulator_arm.h
Good point. Fixed.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator.h...
runtime/vm/simulator.h:19: #include "vm/simulator_mips.h"
On 2013/01/24 02:03:09, siva wrote:
> Ditto comment for the MIPS case.

Done.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
File runtime/vm/simulator_arm.cc (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
runtime/vm/simulator_arm.cc:549: Mutex* Simulator::exclusive_access_lock_;
On 2013/01/24 02:03:09, siva wrote:
>  = NULL;

Done.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
File runtime/vm/simulator_arm.h (right):

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
runtime/vm/simulator_arm.h:38: // for each isolate.
On 2013/01/24 02:03:09, siva wrote:
> There should be one for each isolate right, not sure why you say Potentially.
Do
> we want to run multiple isolates on the same simulator?

No, there is zero or one simulator per isolate. If we are here, there is
obviously one. Changed the comment.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
runtime/vm/simulator_arm.h:38: // for each isolate.
On 2013/01/24 18:25:57, Ivan Posva wrote:
> On 2013/01/24 02:03:09, siva wrote:
> > There should be one for each isolate right, not sure why you say
Potentially.
> Do
> > we want to run multiple isolates on the same simulator?
> 
> Actually the Simulator should be tied to the thread instead of the isolate.
Each
> thread in the thread pool should have its own Simulator instance, but then we
> get into life cycle issues especially for the main thread. Having said that it
> might be easier just to attach a Simulator with each Isolate for now.

OK, leaving as is for now.

https://chromiumcodereview.appspot.com/12041056/diff/1/runtime/vm/simulator_a...
runtime/vm/simulator_arm.h:188: } exclusive_access_state_[kNumAddressTags];
On 2013/01/24 02:03:09, siva wrote:
> Not sure if we will require this as there will be none of these mutual
exclusion
> instructions in the generated code.
> 
> Even when we support concurrent GC the other threads will only be running C++
> code.
> 
> Not suggesting that we remove this now, maybe we should a TODO to clean this
up
> once we have stuff working on ARM and realize that we don't use this structure
> at all.

TODO(regis) added.

[Ivan Posva, 2013-01-25 00:33:08]

LGTM -ip