Mir

Merge lp://qastaging/~afrantzis/mir/valgrind-suppressions-armhf into lp://qastaging/mir

valgrind-suppressions-armhf
Merge into development-branch

Proposed by Alexandros Frantzis on 2014-02-19

Status:

Merged

Approved by:

Alexandros Frantzis on 2014-02-21

Approved revision:

no longer in the source branch.

Merged at revision:

1419

Proposed branch:

lp://qastaging/~afrantzis/mir/valgrind-suppressions-armhf

Merge into:

lp://qastaging/mir

Diff against target:

371 lines (+166/-19)

6 files modified

CMakeLists.txt (+5/-1)
cmake/MirCommon.cmake (+8/-4)
cmake/src/mir/mir_discover_gtest_tests.cpp (+24/-10)
include/test/mir_test_doubles/mock_hwc_composer_device_1.h (+2/-0)
tests/unit-tests/test_udev_wrapper.cpp (+16/-4)
tools/valgrind_suppressions_armhf (+111/-0)

To merge this branch:

bzr merge lp://qastaging/~afrantzis/mir/valgrind-suppressions-armhf

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alexandros Frantzis (community)			Abstain on 2014-02-21
PS Jenkins bot (community)	continuous-integration		Approve on 2014-02-21
Daniel van Vugt			Approve on 2014-02-20
Kevin DuBois (community)			Abstain on 2014-02-20
Alan Griffiths		2014-02-19	Approve on 2014-02-19
Review via email: mp+207188@code.qastaging.launchpad.net

Commit message

tests: Suppress spurious memory errors occuring when running the unit tests on armhf with valgrind

Description of the change

tests: Suppress spurious memory errors occuring when running the unit tests on armhf with valgrind

This MP adds and enables use of a valgrind suppression file for the mir_unit_tests on armhf. This should allow us to renable valgrind in CI armhf jobs (https://bugs.launchpad.net/mir/+bug/1279438).

This MP also fixes a real memory error found in mir_unit_tests.

A concern I have with the suppression file is that it may be too fragile, i.e., we may need to amend it often as the stack changes. If it turns out that the cost of maintance is not acceptable, an alternative would be to enable an x86 Android build + valgrind unit tests, so at least we can continue to cover the Android build.

An exercise for later would be to investigate the errors we get on armhf/android when running the integration and acceptance tests, to enable running them under valgrind, too.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-02-19:

I too have concerns, but there's currently too much "noise" to detect "signal". This offers a way forward.

181 external_set = false;
182 virtual_prepare = false;
183 virtual_set = false;
184 + fb_fence = -1;
185 + retire_fence = -1;
186
187 common.version = HWC_DEVICE_API_VERSION_1_1;

This whole block of code uses assignment where initializer lists or in-class initializers would be better.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-19:

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-19:

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-02-20:

like mentioned in the description, having the mangled name in there seems fragile:
304 + fun:_ZN3mir13SharedLibraryC1EPKc
but I couldn't think of another way, and having valgrind checks seems better than not.

review: Abstain

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-02-20:

I'm concerned about fun:_ZN3mir13SharedLibraryC1EPKc too. Only because the comment says it's a dlopen bug, but we have to suppress it using a Mir symbol. So I'd prefer to see that suppression written in terms of libc if possible. But I don't know if it's possible. So won't block.

review: Approve

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-02-20:

Note: don't top approve until we have run pre-merge tests on this.

See, https://code.launchpad.net/~afrantzis/pbuilderjenkins/mir-enable-testing-temp/+merge/207252

review: Needs Information

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-02-20:

> I'm concerned about fun:_ZN3mir13SharedLibraryC1EPKc too. Only because the comment says it's a dlopen bug,
> but we have to suppress it using a Mir symbol. So I'd prefer to see that suppression written in terms of
> libc if possible. But I don't know if it's possible. So won't block.

Unfortunately the memory error the rule is suppressing is in Mir code, triggered by:

mir::SharedLibrary::SharedLibrary(char const* library_name) :
    so(dlopen(library_name, RTLD_NOW | RTLD_GLOBAL))
{
    if (!so) <===== **** This check triggers the error
    {
        BOOST_THROW_EXCEPTION(std::runtime_error(dlerror()));
    }
}