Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2006183004:
Implement ldrex and strex in ARM simulator (Closed)
|
Created:
4 years, 7 months ago by binji Modified:
3 years, 11 months ago CC:
v8-reviews_googlegroups.com, v8-mips-ports_googlegroups.com, v8-ppc-ports_googlegroups.com, Benedikt Meurer Base URL:
http://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionImplement ldrex/strex instructions in ARM simulator
This CL implements ldrex, ldrexb, ldrexh, strex, strexb, and strexh in the
Simulator. These instructions provide "exclusive" access, which provides mutual
exclusion for concurrent threads of execution.
The ARM specification gives some leeway to implementors, but essentially
describes each processor as having Local Monitor and Global Monitor. The Local
Monitor is used to check the exclusivity state without having to synchronize
with other processors. The Global Monitor is shared between processors. We
model both to make it easier to match behavior with the spec.
When running with multiple OS threads, each thread has its own isolate, and
each isolate has its own Simulator. The Local Monitor is stored directly on the
Simulator, and the Global Monitor is stored as a lazy singleton. The Global
Monitor maintains a linked-list of all Simulators.
All loads/stores (even non-exclusive) are guarded by the Global Monitor's mutex.
BUG=v8:4614
Review-Url: https://codereview.chromium.org/2006183004
Cr-Commit-Position: refs/heads/master@{#42481}
Committed: https://chromium.googlesource.com/v8/v8/+/66ae5f7d71102b2351210f0c4220e96990330e0b
Patch Set 1 #
Total comments: 18
Patch Set 2 : some fixes #Patch Set 3 : Separating into Local+Global monitors #Patch Set 4 : clear monitor on transition to Open #Patch Set 5 : merge master #
Total comments: 16
Patch Set 6 : feedback #Patch Set 7 : merge master #Patch Set 8 : Some initial tests #Patch Set 9 : add tests #
Total comments: 6
Patch Set 10 : fixes #
Total comments: 4
Patch Set 11 : Lock on reads #
Messages
Total messages: 56 (33 generated)
binji@chromium.org changed reviewers: + bmeurer@chromium.org, jarin@chromium.org
Not sure the best way to test this. Thoughts?
binji@chromium.org changed reviewers: + rodolph.perfetta@arm.com
partial review https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4405: return 1; it is 0 on success and 1 for failure. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4427: return 1; 0 https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4449: return 1; 0 https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4488: access_state_ = Access::Open; You should make sure any other nodes with the same address is also marked open. Anyway how are threads supposed to add node to the list? I may be missing something obvious. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4514: } shouldn't next_ point to head_?
jacob.bramley@arm.com changed reviewers: + jacob.bramley@arm.com
I think it would be useful to implement separate local and global monitors. These can then implement the state machines described in the ARM ARM, so the operation is relatively easy to verify. The exclusive access logic is complicated to get right, and here we probably want to be more strict than realistic hardware, in order to stress simulated code. Is similar work also planned for ARM64? If so, it might be worth importing the implementation in VIXL. The only feature missing from the VIXL version is that it doesn't have multiple global monitors, but the framework exists for it. https://git.linaro.org/arm/vixl.git/blob/95372fcd05dda909b0c2e0e35b3432f874eb... https://git.linaro.org/arm/vixl.git/blob/95372fcd05dda909b0c2e0e35b3432f874eb... Finally, I think this could really benefit from some focussed tests. VIXL has single-threaded tests which could be adapted for ARM: https://git.linaro.org/arm/vixl.git/blob/95372fcd05dda909b0c2e0e35b3432f874eb... https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4404: simulator->WriteB(addr, value); Also, _any_ unrelated memory operation should _sometimes_ clear the monitor. The hardware is allowed to do this during cache evictions and the like; the ARM Software should avoid performing memory accesses and cache maintenance between ldrex and strex, so the simulator should be demanding in this regard. Finally, sometimes the lock should just fail. Again, this is allowed to happen on hardware, even if it's unlikely, so the simulator should push this to stress the generated code. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4476: if (access_state_ == Access::Exclusive) { This should be more strict, and check that the addresses and sizes match. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.h File src/arm/simulator-arm.h (right): https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.h#new... src/arm/simulator-arm.h:485: // exclusive access for some address range. The global monitor remembers a single outstanding exclusive access per core, along with its associated state machine. Rather than use a linked list, it would probably be simpler to use a simple vector, with one element per simulated core. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.h#new... src/arm/simulator-arm.h:486: static DataMonitor* head_; I can't see how the list is allocated. Does this currently only support a single thread?
I realize now that this is a bit confusing; I probably should have explained it better. :) My goal here is to allow us to use Atomics on SharedArrayBuffers, which requires supporting ldrex and strex, but only when accessing SAB memory. The tests that access this are run using a fake WebWorker-like API that I implemented for d8. Each Worker gets its own Isolate, and each Isolate has a Simulator. Since each Worker is (basically) a thread, each Simulator represents one core. The linked list makes this convenient; this way we don't need a new global allocation to be shared between the Simulators. But maybe it would be easier to understand that way? As for importing code from VIXL -- I'm not sure the policy on that. Have we imported code from them before? https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4404: simulator->WriteB(addr, value); On 2016/05/31 16:58:47, jbramley wrote: > Also, _any_ unrelated memory operation should _sometimes_ clear the monitor. The > hardware is allowed to do this during cache evictions and the like; the ARM > Software should avoid performing memory accesses and cache maintenance between > ldrex and strex, so the simulator should be demanding in this regard. OK, makes sense. > > Finally, sometimes the lock should just fail. Again, this is allowed to happen > on hardware, even if it's unlikely, so the simulator should push this to stress > the generated code. Hm, not sure how to implement "sometimes" fail. I suppose I could use the random number generator, knowing that the seed can be fixed. But it does make me a bit uncomfortable. :) Maybe just failing once every N times? https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4405: return 1; On 2016/05/25 17:38:29, Rodolph Perfetta (ARM) wrote: > it is 0 on success and 1 for failure. Oops! done. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4427: return 1; On 2016/05/25 17:38:29, Rodolph Perfetta (ARM) wrote: > 0 Done. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4449: return 1; On 2016/05/25 17:38:29, Rodolph Perfetta (ARM) wrote: > 0 Done. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4476: if (access_state_ == Access::Exclusive) { On 2016/05/31 16:58:47, jbramley wrote: > This should be more strict, and check that the addresses and sizes match. OK, I'll work on this after these simple fixes. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4488: access_state_ = Access::Open; On 2016/05/25 17:38:29, Rodolph Perfetta (ARM) wrote: > You should make sure any other nodes with the same address is also marked open. OK, I'll work on this after these simple fixes. > Anyway how are threads supposed to add node to the list? I may be missing > something obvious. A node is only added to the list via PrependNode_Locked, which happens during via ReadExInternal_Locked. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:4514: } On 2016/05/25 17:38:29, Rodolph Perfetta (ARM) wrote: > shouldn't next_ point to head_? Done. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.h File src/arm/simulator-arm.h (right): https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.h#new... src/arm/simulator-arm.h:485: // exclusive access for some address range. On 2016/05/31 16:58:47, jbramley wrote: > The global monitor remembers a single outstanding exclusive access per core, > along with its associated state machine. Rather than use a linked list, it would > probably be simpler to use a simple vector, with one element per simulated core. I was writing this as though each Simulator is one core. It's a bit strange, but it matches the way the things are currently generated: if you create two Workers, you get two Isolates, which gives you two Simulators. https://codereview.chromium.org/2006183004/diff/1/src/arm/simulator-arm.h#new... src/arm/simulator-arm.h:486: static DataMonitor* head_; On 2016/05/31 16:58:47, jbramley wrote: > I can't see how the list is allocated. Does this currently only support a single > thread? Each DataMonitor is a node in the list, so it is allocated along with the Simulator (and Isolate). This matches the Worker model which we're trying to emulate for the Atomics API.
OK, I'm coming back to this after a long vacation. :) This new patch is trying to separate into local monitor and global monitor components so it can more easily map to the state machine in the ARM ARM. PTAL
The CQ bit was checked by binji@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/14 22:45:59, binji wrote: > OK, I'm coming back to this after a long vacation. :) > > This new patch is trying to separate into local monitor and global monitor > components so it can more easily map to the state machine in the ARM ARM. > > PTAL ping
The separation is useful, thanks! https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.c... src/arm/simulator-arm.cc:4487: // or exclusive access. See ARM DDI 0406C.b, A3.4.1. That's true, but also note that software should avoid explicit memory accesses between a matched LDREX-STREX pair because these accesses could cause cache evictions that affect the monitor (A3.4.5). The simulator should be as strict as the architecture allows, so I'd argue that Store should unconditionally clear the local monitor. The same also applies to loads (whether exclusive or not). https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.c... src/arm/simulator-arm.cc:4499: if ((addr & kTaggedAddrMask) == tagged_addr_ && size_ == size) { I'd drop the mask here. The ARM ARM allows a processor to require that the address exactly matches (A3.4.5), so we should be as strict as possible. The same applies in the global monitor. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.c... src/arm/simulator-arm.cc:4503: return true; This should fail randomly from time to time. The ARM ARM says that this can occur as a result of background cache evictions (A3.4.5). For simulation purposes, it should happen much more often than on real hardware because it might expose bugs. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h File src/arm/simulator-arm.h (right): https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h... src/arm/simulator-arm.h:448: static const int32_t kTaggedAddrMask = ~7; Picking (2^)11 would probably be more demanding on software because it means that exclusive access locations must be spread out if they are to be accessed simultaneously. I'd also consider naming it something along the lines of "kExclusiveTaggedAddrMask". The term "tagged" is somewhat overloaded already so I think it would be useful to qualify it. Alternatively, you could replace it with kExclusivesReservationGranule, which perhaps has the clearest name but requires a bit more work to use since it should be set to a power of 2. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h... src/arm/simulator-arm.h:452: void WriteB(int32_t addr, int8_t value); There's no practical difference between storing signed or unsigned bytes or halfwords. Do you know why these are separated in this way? I realise that the existing methods were already separated, but the only difference is in an error message and I don't think it's worth the duplication. Have I missed something? These could all take just uint{8,16,32}_t types and they'd work fine. "uint*" is a better default than "int*" because it avoids implementation-defined casting behaviour, and the function doesn't need to do any arithmetic on it anyway. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h... src/arm/simulator-arm.h:455: void WriteW(int32_t addr, int value, Instruction* instr); I suggest using (u)int32_t explicitly, for consistency if nothing else. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h... src/arm/simulator-arm.h:475: void LoadExcl(int32_t addr, TransactionSize size); How about "NotifyLoadExcl" and so on? Then it's clear at the call site that no load is actually performed. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h... src/arm/simulator-arm.h:489: class Node { Perhaps "Processor" or something similar would be clearer. "Node" is a bit general, and I don't think it's a term used by the ARM ARM.
https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.c... src/arm/simulator-arm.cc:4487: // or exclusive access. See ARM DDI 0406C.b, A3.4.1. On 2016/07/20 16:09:24, jbramley wrote: > That's true, but also note that software should avoid explicit memory accesses > between a matched LDREX-STREX pair because these accesses could cause cache > evictions that affect the monitor (A3.4.5). The simulator should be as strict as > the architecture allows, so I'd argue that Store should unconditionally clear > the local monitor. > > The same also applies to loads (whether exclusive or not). Done. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.c... src/arm/simulator-arm.cc:4499: if ((addr & kTaggedAddrMask) == tagged_addr_ && size_ == size) { On 2016/07/20 16:09:24, jbramley wrote: > I'd drop the mask here. The ARM ARM allows a processor to require that the > address exactly matches (A3.4.5), so we should be as strict as possible. > > The same applies in the global monitor. Done. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.c... src/arm/simulator-arm.cc:4503: return true; On 2016/07/20 16:09:24, jbramley wrote: > This should fail randomly from time to time. The ARM ARM says that this can > occur as a result of background cache evictions (A3.4.5). For simulation > purposes, it should happen much more often than on real hardware because it > might expose bugs. Done, though I'm not sure this is the best way to do it. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h File src/arm/simulator-arm.h (right): https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h... src/arm/simulator-arm.h:452: void WriteB(int32_t addr, int8_t value); On 2016/07/20 16:09:24, jbramley wrote: > There's no practical difference between storing signed or unsigned bytes or > halfwords. Do you know why these are separated in this way? I realise that the > existing methods were already separated, but the only difference is in an error > message and I don't think it's worth the duplication. Have I missed something? > > These could all take just uint{8,16,32}_t types and they'd work fine. "uint*" is > a better default than "int*" because it avoids implementation-defined casting > behaviour, and the function doesn't need to do any arithmetic on it anyway. I agree, and I started to make this change, but it seemed like it might be better as a separate CL. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h... src/arm/simulator-arm.h:455: void WriteW(int32_t addr, int value, Instruction* instr); On 2016/07/20 16:09:24, jbramley wrote: > I suggest using (u)int32_t explicitly, for consistency if nothing else. Yes, but I was trying to have consistency with the current WriteW, which takes int. I agree it should probably be uint32_t (though all the registers are stored as int32_t), and bizarrely, this function then casts to intptr_t* to write the value...? Again, I think this might be better as a separate change. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h... src/arm/simulator-arm.h:475: void LoadExcl(int32_t addr, TransactionSize size); On 2016/07/20 16:09:24, jbramley wrote: > How about "NotifyLoadExcl" and so on? Then it's clear at the call site that no > load is actually performed. Done. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.h... src/arm/simulator-arm.h:489: class Node { On 2016/07/20 16:09:24, jbramley wrote: > Perhaps "Processor" or something similar would be clearer. "Node" is a bit > general, and I don't think it's a term used by the ARM ARM. Done.
The CQ bit was checked by binji@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by binji@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
resurrecting this CL after a long hiatus. @jbramley: ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think it needs some tests, but otherwise it looks good to me. https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): https://codereview.chromium.org/2006183004/diff/80001/src/arm/simulator-arm.c... src/arm/simulator-arm.cc:4503: return true; > Done, though I'm not sure this is the best way to do it. Some randomness would be useful but this is probably good enough.
bmeurer: ptal On 2016/11/10 11:19:01, jbramley wrote: > I think it needs some tests, but otherwise it looks good to me. Agreed on the tests. Though when I asked before about this, there didn't seem to be a good place to add simulator tests. I guess the idea is that the simulator will be tested by running the normal JavaScript tests? In this case, that will be true when these new instructions are used. Obviously there's a bit of a chicken-and-egg thing here, though, as I can't use the instructions until they've been added to the simulator.
bmeurer@chromium.org changed required reviewers: + jarin@chromium.org
Definitely something that jarin@ should review.
On 2016/11/14 21:54:33, binji wrote: > bmeurer: ptal > > On 2016/11/10 11:19:01, jbramley wrote: > > I think it needs some tests, but otherwise it looks good to me. > > Agreed on the tests. Though when I asked before about this, there didn't seem to > be a good place to add simulator tests. That's true. The easiest place to put them would be in test-assembler-arm.cc, but it's semantically awkward. Perhaps the _best_ thing to do would be to add a new test file, but I don't know what complications that introduces. > I guess the idea is that the simulator > will be tested by running the normal JavaScript tests? To an extent, yes, though low-level tests can more reliably exercise corner-cases, particularly where there is contention that depends upon thread timing. In real code, of course, we want to minimise contention (for performance reasons), so these corner-cases ought to be rare. If the JavaScript tests will cover that kind of thing, then perhaps they will suffice. I'm not terribly familiar with how these instructions will be used so I don't really know about that. The biggest risk here is that something works in the simulator, but (occasionally) not on hardware, so for example we'd want to prioritise tests where locks _fail_, to ensure that the generated code handles that correctly. Thanks, Jacob
On 2016/11/15 09:17:01, jbramley wrote: > On 2016/11/14 21:54:33, binji wrote: > > bmeurer: ptal > > > > On 2016/11/10 11:19:01, jbramley wrote: > > > I think it needs some tests, but otherwise it looks good to me. > > > > Agreed on the tests. Though when I asked before about this, there didn't seem > to > > be a good place to add simulator tests. > > That's true. The easiest place to put them would be in test-assembler-arm.cc, > but it's semantically awkward. Perhaps the _best_ thing to do would be to add a > new test file, but I don't know what complications that introduces. > > > I guess the idea is that the simulator > > will be tested by running the normal JavaScript tests? > > To an extent, yes, though low-level tests can more reliably exercise > corner-cases, particularly where there is contention that depends upon thread > timing. In real code, of course, we want to minimise contention (for performance > reasons), so these corner-cases ought to be rare. If the JavaScript tests will > cover that kind of thing, then perhaps they will suffice. I'm not terribly > familiar with how these instructions will be used so I don't really know about > that. I'm adding them to be used by the new Atomic.* functions (see https://github.com/tc39/ecmascript_sharedmem). It's a good point about adding some low-level tests; there are JavaScript tests that exercise some contention, but not directly for that purpose. I'll take a look at adding something more direct. > > The biggest risk here is that something works in the simulator, but > (occasionally) not on hardware, so for example we'd want to prioritise tests > where locks _fail_, to ensure that the generated code handles that correctly. AIUI, the normal way to translate SC atomic RMW operations to Arm is with a ldrex/strex instruction pair in a loop. So the failed lock will be handled naturally. That said, I suppose it's possible someone else will choose to use these instructions for a different purpose, where it may not be as straightforward. > > Thanks, > Jacob
The CQ bit was checked by binji@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry again for the delay, I've added some single- and multi-threaded tests. PTAL
I only have minor comments. Otherwise, it looks good. Benedikt said that Jarin should look so I'll let him give approval. https://codereview.chromium.org/2006183004/diff/160001/test/cctest/test-simul... File test/cctest/test-simulator-arm.cc (right): https://codereview.chromium.org/2006183004/diff/160001/test/cctest/test-simul... test/cctest/test-simulator-arm.cc:88: Register addr_reg, Register value_reg, I think it would be more natural to order them like the instructions (dest, value, addr). Of course, this means that you can't use the default value, but an overload would work. https://codereview.chromium.org/2006183004/diff/160001/test/cctest/test-simul... test/cctest/test-simulator-arm.cc:89: Register dest_reg = r0) { Shouldn't the default be no_reg? https://codereview.chromium.org/2006183004/diff/160001/test/cctest/test-simul... test/cctest/test-simulator-arm.cc:252: __ mov(pc, Operand(lr)); "bx(lr)" would be better.
https://codereview.chromium.org/2006183004/diff/160001/test/cctest/test-simul... File test/cctest/test-simulator-arm.cc (right): https://codereview.chromium.org/2006183004/diff/160001/test/cctest/test-simul... test/cctest/test-simulator-arm.cc:88: Register addr_reg, Register value_reg, On 2017/01/04 17:45:41, jbramley wrote: > I think it would be more natural to order them like the instructions (dest, > value, addr). Of course, this means that you can't use the default value, but an > overload would work. Done. https://codereview.chromium.org/2006183004/diff/160001/test/cctest/test-simul... test/cctest/test-simulator-arm.cc:89: Register dest_reg = r0) { On 2017/01/04 17:45:41, jbramley wrote: > Shouldn't the default be no_reg? I ended up just removing the default and using an overload. https://codereview.chromium.org/2006183004/diff/160001/test/cctest/test-simul... test/cctest/test-simulator-arm.cc:252: __ mov(pc, Operand(lr)); On 2017/01/04 17:45:41, jbramley wrote: > "bx(lr)" would be better. Done.
The CQ bit was checked by binji@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
friendly ping :)
lgtm, but the description on the CL should be improved to describe what exactly has been done here. https://codereview.chromium.org/2006183004/diff/180001/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): https://codereview.chromium.org/2006183004/diff/180001/src/arm/simulator-arm.... src/arm/simulator-arm.cc:1062: return *ptr; Would not this be a data race with writes in other threads? Technically, we should at least do NoBarrier_Load. Or perhaps lock with the global monitor's mutex. Same for the other synchronization-less Read* methods below. https://codereview.chromium.org/2006183004/diff/180001/src/arm/simulator-arm.... src/arm/simulator-arm.cc:5195: size_ = TransactionSize::None; Perhaps it would be better to introduce void Simulator::LocalMonitor::Clear() { access_state_ = MonitorAccess::Open; tagged_addr_ = 0; size_ = TransactionSize::None; } and then call it from the places below where the state is cleared.
Description was changed from ========== Implement ldrex and strex in ARM simulator ========== to ========== Implement ldrex/strex instructions in ARM simulator This CL implements ldrex, ldrexb, ldrexh, strex, strexb, and strexh in the Simulator. These instructions provide "exclusive" access, which provides mutual exclusion for concurrent threads of execution. The ARM specification gives some leeway to implementors, but essentially describes each processor as having Local Monitor and Global Monitor. The Local Monitor is used to check the exclusivity state without having to synchronize with other processors. The Global Monitor is shared between processors. We model both to make it easier to match behavior with the spec. When running with multiple OS threads, each thread has its own isolate, and each isolate has its own Simulator. The Local Monitor is stored directly on the Simulator, and the Global Monitor is stored as a lazy singleton. The Global Monitor maintains a linked-list of all Simulators. All loads/stores (even non-exclusive) are guarded by the Global Monitor's mutex. BUG=v8:4614 ==========
https://codereview.chromium.org/2006183004/diff/180001/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): https://codereview.chromium.org/2006183004/diff/180001/src/arm/simulator-arm.... src/arm/simulator-arm.cc:1062: return *ptr; On 2017/01/13 07:56:29, Jarin wrote: > Would not this be a data race with writes in other threads? > > Technically, we should at least do NoBarrier_Load. Or perhaps lock with the > global monitor's mutex. Same for the other synchronization-less Read* methods > below. Done. https://codereview.chromium.org/2006183004/diff/180001/src/arm/simulator-arm.... src/arm/simulator-arm.cc:5195: size_ = TransactionSize::None; On 2017/01/13 07:56:29, Jarin wrote: > Perhaps it would be better to introduce > > void Simulator::LocalMonitor::Clear() { > access_state_ = MonitorAccess::Open; > tagged_addr_ = 0; > size_ = TransactionSize::None; > } > > and then call it from the places below where the state is cleared. Done.
The CQ bit was checked by binji@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by binji@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org Link to the patchset: https://codereview.chromium.org/2006183004/#ps200001 (title: "Lock on reads")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1484777746738870,
"parent_rev": "9662547c15db6dc54169a3d2da91048d3d669fda", "commit_rev":
"66ae5f7d71102b2351210f0c4220e96990330e0b"}
Message was sent while issue was closed.
Description was changed from ========== Implement ldrex/strex instructions in ARM simulator This CL implements ldrex, ldrexb, ldrexh, strex, strexb, and strexh in the Simulator. These instructions provide "exclusive" access, which provides mutual exclusion for concurrent threads of execution. The ARM specification gives some leeway to implementors, but essentially describes each processor as having Local Monitor and Global Monitor. The Local Monitor is used to check the exclusivity state without having to synchronize with other processors. The Global Monitor is shared between processors. We model both to make it easier to match behavior with the spec. When running with multiple OS threads, each thread has its own isolate, and each isolate has its own Simulator. The Local Monitor is stored directly on the Simulator, and the Global Monitor is stored as a lazy singleton. The Global Monitor maintains a linked-list of all Simulators. All loads/stores (even non-exclusive) are guarded by the Global Monitor's mutex. BUG=v8:4614 ========== to ========== Implement ldrex/strex instructions in ARM simulator This CL implements ldrex, ldrexb, ldrexh, strex, strexb, and strexh in the Simulator. These instructions provide "exclusive" access, which provides mutual exclusion for concurrent threads of execution. The ARM specification gives some leeway to implementors, but essentially describes each processor as having Local Monitor and Global Monitor. The Local Monitor is used to check the exclusivity state without having to synchronize with other processors. The Global Monitor is shared between processors. We model both to make it easier to match behavior with the spec. When running with multiple OS threads, each thread has its own isolate, and each isolate has its own Simulator. The Local Monitor is stored directly on the Simulator, and the Global Monitor is stored as a lazy singleton. The Global Monitor maintains a linked-list of all Simulators. All loads/stores (even non-exclusive) are guarded by the Global Monitor's mutex. BUG=v8:4614 Review-Url: https://codereview.chromium.org/2006183004 Cr-Commit-Position: refs/heads/master@{#42481} Committed: https://chromium.googlesource.com/v8/v8/+/66ae5f7d71102b2351210f0c4220e969903... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/v8/v8/+/66ae5f7d71102b2351210f0c4220e969903... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
