- Use the simulator to do atomic operations that could also

- Use the simulator to do atomic operations that could also
  be executed in generated code.

R=koda@google.com, zra@google.com

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

[Ivan Posva, 2014-10-24 23:38:11]

iposva@google.com changed reviewers:
+ koda@google.com, zra@google.com

[koda, 2014-10-27 14:05:11]

First pass...

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/atomic.h#newcode20
runtime/vm/atomic.h:20: // NOTE: Not to be used for any atomic operations
involving generated code.
This is a bit vague. "involving memory locations that are accessed by generated
code" maybe?

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/atomic_simulator....
runtime/vm/atomic_simulator.h:9: #error Do not include atomic_linux.h directly.
Use atomic.h instead.
Update error message.

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1145: // This lock is initialized in
Simulator::InitOnce().
Why keep this comment but remove the other two instances?

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1147: // We do not check for a reservation as it
would be guaranteed to be found. No
Why not ASSERT this then?

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_mips.h#...
runtime/vm/simulator_mips.h:217: // Synchronization primitives support.
This code seems currently unused on MIPS, right?

If you think all three copies of this code will remain the same, then I strongly
urge you to consider sharing it in a utility class.

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_mips.h#...
runtime/vm/simulator_mips.h:220: intptr_t WriteExclusiveW(uword addr, intptr_t
value, Instr* instr);
Document return value.

[zra, 2014-10-27 14:53:13]

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1089: exclusive_access_state_[i].isolate = isolate;
Should we assert here that exclusive_access_state_[i].addr == 0? Or instead
search for an entry where that's the case?

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1103: // Check whether the current isolates address
reservation matches.
isolate's

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_mips.h#...
runtime/vm/simulator_mips.h:217: // Synchronization primitives support.
On 2014/10/27 14:05:11, koda wrote:
> This code seems currently unused on MIPS, right?
> 
> If you think all three copies of this code will remain the same, then I
strongly
> urge you to consider sharing it in a utility class.

Agree.

[Ivan Posva, 2014-10-27 17:58:24]

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/atomic.h#newcode20
runtime/vm/atomic.h:20: // NOTE: Not to be used for any atomic operations
involving generated code.
On 2014/10/27 14:05:10, koda wrote:
> This is a bit vague. "involving memory locations that are accessed by
generated
> code" maybe?

Done.

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/atomic_simulator....
runtime/vm/atomic_simulator.h:9: #error Do not include atomic_linux.h directly.
Use atomic.h instead.
On 2014/10/27 14:05:10, koda wrote:
> Update error message.

Done.

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1089: exclusive_access_state_[i].isolate = isolate;
On 2014/10/27 14:53:12, zra wrote:
> Should we assert here that exclusive_access_state_[i].addr == 0?
You cannot assert this. As it is perfectly legal to:
ldrex Rt, some_addr
then branch out if Rt is not the expected value and then later
ldrex Rt, some_other_addr
At this point exclusive_access_state's addr for this isolate is not zero and
you'd get a wrong assertion failure.

> Or instead search for an entry where that's the case?
The CPU the simulator is modeling has only one reservation slot to be very
conservative. Thus you need to invalidate the existing reservation if it exists
for this isolate.

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1103: // Check whether the current isolates address
reservation matches.
On 2014/10/27 14:53:12, zra wrote:
> isolate's

Done.

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1145: // This lock is initialized in
Simulator::InitOnce().
On 2014/10/27 14:05:10, koda wrote:
> Why keep this comment but remove the other two instances?

Done. Oversight, thanks for noticing.

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1147: // We do not check for a reservation as it
would be guaranteed to be found. No
On 2014/10/27 14:05:10, koda wrote:
> Why not ASSERT this then?

Maybe the comment needs rephrasing: What it is supposed to convey that since we
hold the lock the load would get the reservation and nobody else can get it and
steal it while the lock is held.

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_mips.h#...
runtime/vm/simulator_mips.h:217: // Synchronization primitives support.
On 2014/10/27 14:53:12, zra wrote:
> On 2014/10/27 14:05:11, koda wrote:
> > This code seems currently unused on MIPS, right?
> > 
> > If you think all three copies of this code will remain the same, then I
> strongly
> > urge you to consider sharing it in a utility class.
> 
> Agree.

How about atomic_simulator.h?

[zra, 2014-10-27 18:09:18]

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1089: exclusive_access_state_[i].isolate = isolate;
On 2014/10/27 17:58:24, Ivan Posva wrote:
> On 2014/10/27 14:53:12, zra wrote:
> > Should we assert here that exclusive_access_state_[i].addr == 0?
> You cannot assert this. As it is perfectly legal to:
> ldrex Rt, some_addr
> then branch out if Rt is not the expected value and then later
> ldrex Rt, some_other_addr
> At this point exclusive_access_state's addr for this isolate is not zero and
> you'd get a wrong assertion failure.
> 
> > Or instead search for an entry where that's the case?
> The CPU the simulator is modeling has only one reservation slot to be very
> conservative. Thus you need to invalidate the existing reservation if it
exists
> for this isolate.

What happens when you run out of address tags?

[zra, 2014-10-27 18:13:48]

lgtm

[koda, 2014-10-27 18:32:48]

LGTM if you add comment and assert

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1147: // We do not check for a reservation as it
would be guaranteed to be found. No
On 2014/10/27 17:58:23, Ivan Posva wrote:
> On 2014/10/27 14:05:10, koda wrote:
> > Why not ASSERT this then?
> 
> Maybe the comment needs rephrasing: What it is supposed to convey that since
we
> hold the lock the load would get the reservation and nobody else can get it
and
> steal it while the lock is held.

Then you should assert that the current isolate holds the lock when
SetExclusiveAccess/HasExclusiveAccessAndOpen are called, and document it in the
header.

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

https://codereview.chromium.org/677193002/diff/20001/runtime/vm/simulator_mip...
runtime/vm/simulator_mips.h:200: // In Dart, there is at most one thread per
isolate.
At most one thread *that executes Dart code*. Point out that GC is concurrent.
Otherwise, some poor noogler might be misled to think the synchronization here
is used for cross-isolate purposes...

[Ivan Posva, 2014-10-27 19:21:08]

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

https://codereview.chromium.org/677193002/diff/1/runtime/vm/simulator_arm.cc#...
runtime/vm/simulator_arm.cc:1147: // We do not check for a reservation as it
would be guaranteed to be found. No
On 2014/10/27 18:32:48, koda wrote:
> On 2014/10/27 17:58:23, Ivan Posva wrote:
> > On 2014/10/27 14:05:10, koda wrote:
> > > Why not ASSERT this then?
> > 
> > Maybe the comment needs rephrasing: What it is supposed to convey that since
> we
> > hold the lock the load would get the reservation and nobody else can get it
> and
> > steal it while the lock is held.
> 
> Then you should assert that the current isolate holds the lock when
> SetExclusiveAccess/HasExclusiveAccessAndOpen are called, and document it in
the
> header.

Done.

[Ivan Posva, 2014-10-27 19:21:43]

Committed patchset #3 (id:40001) manually as r41332 (presubmit successful).