Mir

Merge lp://qastaging/~vanvugt/mir/fix-1282886 into lp://qastaging/mir

  1. fix-1282886
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 1425
Proposed branch: lp://qastaging/~vanvugt/mir/fix-1282886
Merge into: lp://qastaging/mir
Diff against target: 131 lines (+38/-36)
1 file modified
tests/integration-tests/compositor/test_swapping_swappers.cpp (+38/-36)
To merge this branch: bzr merge lp://qastaging/~vanvugt/mir/fix-1282886
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Kevin DuBois (community) Approve
Andreas Pokorny (community) Approve
Alan Griffiths Approve
Review via email: mp+207596@code.qastaging.launchpad.net

Commit message

Fix mutex data race reported by helgrind in integration test:
SwapperSwappingStress (LP: #1282886)

Description of the change

.

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Admittedly solving the std::atomic races is ugly. We really should create a class for that or get helgrind itself fixed, somehow.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1419
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/915/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1005
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1003
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/597
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/645
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/645/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/652
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/652/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/598
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/598/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/572
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4168

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

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

There are two parts to this fix:

the change to client_acquire_blocking has much the same effect as https://code.launchpad.net/~alan-griffiths/mir/keep-helgrind-happy/+merge/207389 and I'm happy with it.

But the other part is writing ugly code to get around the std::atomic issue with helgrind. I don't feel this is an adequate justification for such code (and doesn't solve similar cases elsewhere).

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1421
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/917/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1008
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1006
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/600
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/647
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/647/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/654
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/654/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/601
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/601/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/574
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4172

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

review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Alan, you're right. They're two separate issues which really require separate fixes. I've reverted the bespoke Atomic template and just propose fixing the original mutex/lock race now.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1423
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/925/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1019
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1017
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/611/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/655
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/655/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/662
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/662/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/612
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/612/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/586/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4216

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> Alan, you're right. They're two separate issues which really require separate
> fixes. I've reverted the bespoke Atomic template and just propose fixing the
> original mutex/lock race now.

You've changed a member atomic<bool> to a local atomic_bool instead of reverting, but OK.

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

So long as the C++ library provides std::atomic_bool, it should be faster to compile and eliminates any risk of template bloat (since it's not a template but a class implemented in the C++ library).

Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

ok

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

looks good to me

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/411/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1032
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1030
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/624
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/316/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/317
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/317/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/625
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/625/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/599
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4262

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches