629435 - Android VMPI_memoryBarrier() requires compiler-level barriers

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Simon Wilkinson]

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).

Comment 1

[Simon Wilkinson]

Attachment: Initial Patch. MQ rev 242

This patches fixes VMPI_memoryBarrier() and all of the VMPI_*WithBarrier functions

Comment 2

[Simon Wilkinson]

Attachment: MQ rev 249

Doh! Need compiler-barriers before and after the atomic API calls.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

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

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 515552 [details] [diff] [review]
MQ rev 249

(doing review myself)

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

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

[Felix S. Klock II [:pnkfelix, :fklock]]

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.)

Comment 7

[Tamarin Bot]

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

[Felix S. Klock II [:pnkfelix, :fklock]]

(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

[Edwin Smith]

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?