Merge lp:~alan-griffiths/mir/use-compare_exchange_weak-correctly into lp:mir

[Alan Griffiths]

common: correct usage of compare_exchange_weak()

common: correct usage of compare_exchange_weak()

compare_exchange_weak() is allowed to "fail spuriously". That is, it may act as if *this != expected even if they are equal. This would cause it to return false and the expected-variable to be unaltered, leading, e.g. to null pointer access in BasicObservers<Observer>::add().

(This may well not apply to our platform, but I'm correcting the code anyway.)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1942
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2827/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1964
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1971
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1913
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1349
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1349/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/850
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/850/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2930
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/13945

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2827/rebuild

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

Oh yeah!

review: Approve

[Daniel van Vugt (vanvugt)]

(1) 3rd_party/android-deps/std/atomic.h: I think it's probably a better idea to not fix 3rd party code when there is nothing provably wrong with the current form (yet). We should assume Google's using it in a way that is safe until proven otherwise. Or is it definitely not safe right now?

Happy for our own code to get fixes though...

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> (1) 3rd_party/android-deps/std/atomic.h: I think it's probably a better idea
> to not fix 3rd party code when there is nothing provably wrong with the
> current form (yet). We should assume Google's using it in a way that is safe
> until proven otherwise. Or is it definitely not safe right now?

1.1. The code is ours (.../std/... is a re-implementation using the standard library to replace android code).

1.2. It is wrong for the reasons described in the MP (it doesn't correctly handle spurious failures).

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

=== modified file '3rd_party/android-deps/std/atomic.h'
--- 3rd_party/android-deps/std/atomic.h	2013-03-13 04:54:15 +0000
+++ 3rd_party/android-deps/std/atomic.h	2014-09-30 16:10:51 +0000
@@ -80,7 +80,7 @@
 //int android_atomic_acquire_cas(int32_t oldvalue, int32_t newvalue,
 //        volatile int32_t* addr);
 inline int android_atomic_release_cas(int32_t oldvalue, int32_t newvalue,
-    android_atomic_int32_t* addr) { return !addr->compare_exchange_weak(oldvalue, newvalue); }
+    android_atomic_int32_t* addr) { return !addr->compare_exchange_strong(oldvalue, newvalue); }
 }
 
 /*

=== modified file 'src/include/common/mir/basic_observers.h'
--- src/include/common/mir/basic_observers.h	2014-09-11 05:51:44 +0000
+++ src/include/common/mir/basic_observers.h	2014-09-30 16:10:51 +0000
@@ -95,7 +95,7 @@
         !current_item->next.compare_exchange_weak(expected, new_item);
         expected = nullptr)
     {
-        current_item = expected;
+        if (expected) current_item = expected;
     }
 }