Closed
Bug 629435
Opened 14 years ago
Closed 14 years ago
Android VMPI_memoryBarrier() requires compiler-level barriers
Categories
(Tamarin Graveyard :: Virtual Machine, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: siwilkin, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
pnkfelix
:
review+
edwsmith
:
superreview+
|
Details | Diff | Splinter Review |
Although the level of hardware memory barrier support provided by Android's atomics APIs is currently a little vague (as documented in unix-platform.h), we should still insert a compiler memory barrier to fulfill the documented semantics of vmbase::MemoryBarrier::readWrite().
(Note that we should really be striving to fulfill the documented semantics of VMPI_memoryBarrier(), but these are currently missing. Bug 629426 will add this documentation).
Reporter | ||
Comment 1•14 years ago
|
||
This patches fixes VMPI_memoryBarrier() and all of the VMPI_*WithBarrier functions
Reporter | ||
Updated•14 years ago
|
Attachment #514938 -
Flags: superreview?(edwsmith)
Attachment #514938 -
Flags: review?(kpalacz)
Reporter | ||
Comment 2•14 years ago
|
||
Doh! Need compiler-barriers before and after the atomic API calls.
Attachment #514938 -
Attachment is obsolete: true
Attachment #515552 -
Flags: superreview?(edwsmith)
Attachment #514938 -
Flags: superreview?(edwsmith)
Attachment #514938 -
Flags: review?(kpalacz)
Updated•14 years ago
|
Attachment #515552 -
Flags: review?(kpalacz)
Comment 3•14 years ago
|
||
Simon: you asked me to land this as part of a larger series of patches, but neither the patch nor its obsoleted predecessor have gotten r+'ed.
Did you get an offline R+ from kpalacz? (Or is it somehow implied by his R+ on Bug 629426?)
Comment 4•14 years ago
|
||
Comment on attachment 515552 [details] [diff] [review]
MQ rev 249
(doing review myself)
Attachment #515552 -
Flags: review?(fklockii)
Comment 5•14 years ago
|
||
Question that is not about this patch but rather about this code in general.
VMPI.h says that VMPI_atomicIncAndGet32WithBarrier and VMPI_atomicIncAndGet32 return the _incremented_ value.
Your code, both before and after the patch, is subtracting 1 from the result of calling the underlying increment routine. I am have much difficulty understanding how that could correspond to the incremented value. (I could be convinced it corresponds to the old unincremented value.)
From what I can tell, functions like android_atomic_inc often follow a similar pattern of subtracting 1 from the result of calling an underlying routine like OSAtomicIncrement32Barrier, but that's because OSAtomicIncrement32Barrier says in its man page on my Mac that it returns the new value, while android_atomic_inc seems like it is supposed to return in the old value [1], so it needs to turn the new value into the old original value. If you're trying to satisfy the contract of VMPI_atomicIncAndGet32WithBarrier, you should never be trying to turn a new value into the old original value ...
[1] I say "seems like it is supposed to" because I am having an absurdly hard time documentation for the spec of android_atomic_inc. My statement above is based on the comments here:
http://www.netmite.com/android/mydroid/2.0/system/core/libcutils/atomic-android-arm.S
Comment 6•14 years ago
|
||
Comment on attachment 515552 [details] [diff] [review]
MQ rev 249
R+: Comment 5 really is orthogonal to the change represented by this patch, since the patch is preserving pre-existing behavior. (But I would like an answer to my question raised there.)
My initial reaction was to wonder whether inlining code with inline asm is going to cause problems for GCC to optimize unrelated statements in the same function body.
From reading
http://en.wikipedia.org/wiki/Memory_ordering#Compiler_memory_barrier
it sounds like GCC does not offer a more direct intrinsic for expressing this. So I guess I have to hope that GCC is smart enough to treat this particular assembly snippet as a special case. (That, or spend some time comparing generated assembly.)
Attachment #515552 -
Flags: review?(fklockii) → review+
Comment 7•14 years ago
|
||
changeset: 6007:ef2c3cd02f96
user: Simon Wilkinson <siwilkin>
summary: Bug 629435: Fixes memory barriers for Android (r=fklockii,r pending kpalacz, sr pending edwsmith)
http://hg.mozilla.org/tamarin-redux/rev/ef2c3cd02f96
Comment 8•14 years ago
|
||
(In reply to comment #5)
> Question that is not about this patch but rather about this code in general.
D'oh! This whole question was already raised by Bug 637233 (which I'm about to land). Sorry about the noise, Simon.
Comment 9•14 years ago
|
||
Comment on attachment 515552 [details] [diff] [review]
MQ rev 249
I see that VMPI_atomicIncAndGet32WithBarrier() is being called in a loop to make sure the right number of increments happen, in ST_vmpi_threads.st. But the test doesn't check the exact correctness of the returned value (before or after value)
each time.
worth adding more tests?
Attachment #515552 -
Flags: superreview?(edwsmith)
Attachment #515552 -
Flags: superreview+
Attachment #515552 -
Flags: review?(kpalacz)
Reporter | ||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•