Merge lp:~vorlon/mir/explicit-gcc-version-and-g++4.9-compatibility into lp:mir/ubuntu

[Steve Langasek]

Explicitly select g++-4.9 to prevent from ABI breaks. (LP: #1329089)

Explicitly select g++-4.9 to prevent from ABI breaks. (LP: #1329089)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1730
http://jenkins.qa.ubuntu.com/job/mir-ci/1667/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/927
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/933
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/5
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/5/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-armhf-ci/5
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-armhf-ci/5/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/1667/rebuild

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

This looks reasonable to me.

review: Approve

[Daniel van Vugt (vanvugt)]

Two issues here...

(1) The path to archive is preferably to land things in the stable maintenance branch first: lp:mir/0.4 . Unfortunately lp:mir points to an archive-only branch that represents neither the active development branch nor our stable maintenance branch.

(2) If we did land this in lp:mir/0.4 then that's kind of an ABI break of sorts, which isn't meant to happen in stable maintenance branches because it just breaks things and confuses everyone.

I think if you want bug 1329089 fixed in Ubuntu then the cleanest way to do it is wait until we've bumped Ubuntu up to the 0.5 series. Ubuntu should be on 0.4 for the foreseeable future which means no binary compatibility changes occur:
  https://launchpad.net/ubuntu/+source/mir

But there's an argument that changing the C++ ABI dependency is not a libmir* ABI change. Land at your own risk I guess. Things may explode.

review: Abstain

[Daniel van Vugt (vanvugt)]

I think this needs more discussion. There's a risk that the ABI changes from g++ 4.8 to 4.9 directly changes the libmirserver C++ ABI also. If so, we would have to defer the change until the next major release 0.5.

review: Needs Information

[Daniel van Vugt (vanvugt)]

Alright, my hypothetical concerns are probably less important than fixing the bug. But we should land it in the upstream stable maintenance branch instead --> lp:mir/0.4
and then it will go into the next utopic update.

review: Needs Resubmitting

[Cemil Azizoglu (cemil-azizoglu)]

We are starting a 0.5.0 release very soon. Please generate this MP against devel.

review: Needs Resubmitting

[Cemil Azizoglu (cemil-azizoglu)]

> We are starting a 0.5.0 release very soon. Please generate this MP against
> devel.

Scratch that. These changes are already part of devel. We'll be landing on trunk in a day or two.

[Daniel van Vugt (vanvugt)]

Yep, we're working on getting Mir 0.5 into archive, which means this fix will land in Ubuntu archive soon anyway. It's probably not useful re-proposing to the 0.4 series.

review: Disapprove

Unmerged revisions

1730. By Alexandros Frantzis on 2014-07-12

android,tests: Fix g++ 4.9 build

It seems that g++ 4.9 has become more aggressive about constant folding of
expressions that use sizeof(), thus triggering warnings/errors related to
constant overflows that didn't occur with g++ 4.8.

For example, g++-4.8 warns about overflowing i3, but not i4, whereas g++-4.9
warns about both i3 and i4:

#define VALUE1 0x70000000UL
#define VALUE2 (VALUE1 >> sizeof(unsigned char))
#define VALUE3 0x80000000UL
#define VALUE4 (VALUE1 << sizeof(unsigned char))

int main()
{
    int i1 = VALUE1;
    int i2 = VALUE2;
    int i3 = VALUE3;
    int i4 = VALUE4;
}

This affects our ioctl() wrapper calls because we use 'int' for the request
parameter, whereas the request numbers produced by the kernel macros (using
sizeof() in the calculation) are 'unsigned long'. This MP fixes the problem by
casting the request numbers to 'int', which is a safe conversion since the
request numbers are constrained to 32 bits by design.

Note that we don't have a problem with all ioctl request numbers, although they
are produced by the same set of macros, because almost all of them don't have
the high bit set (like VALUE2 above) and don't trigger the overflow error. The
one we have a problem with, SYNC_IOC_MERGE, has the high bit set (like VALUE4
above).

Also note that the glibc ioctl() function signature uses 'unsigned long'
instead of 'int' for the request number. So, alternatively, we could fix the
problem by changing our wrappers to use 'unsigned long', too. However, the
glibc signature goes against POSIX and there have been discussions about
switching it back. See https://sourceware.org/bugzilla/show_bug.cgi?id=14362 .

=== modified file 'debian/control'
--- debian/control	2014-06-27 06:10:24 +0000
+++ debian/control	2014-07-12 06:22:08 +0000
@@ -10,6 +10,10 @@
                doxygen,
                xsltproc,
                graphviz,
+# We rely on C++11 features, and to prevent from ABI breaks
+# in libstdc++ causing us issues, we explicitly select a G++
+# version.
+               g++-4.9,
                libboost-dev,
                libboost-chrono-dev,
                libboost-date-time-dev,

=== modified file 'debian/rules'
--- debian/rules	2014-04-15 17:11:14 +0000
+++ debian/rules	2014-07-12 06:22:08 +0000
@@ -6,6 +6,13 @@
 
 export DPKG_GENSYMBOLS_CHECK_LEVEL = 4
 
+include /usr/share/dpkg/default.mk
+
+# Explicitly selecting a G{CC,++}-version here to avoid accidental
+# ABI breaks introduced by toolchain updates.
+export CC=$(DEB_HOST_GNU_TYPE)-gcc-4.9
+export CXX=$(DEB_HOST_GNU_TYPE)-g++-4.9
+
 %:
 	dh $@ --parallel --fail-missing
 

=== modified file 'src/shared/graphics/android/syncfence.cpp'
--- src/shared/graphics/android/syncfence.cpp	2014-03-06 06:05:17 +0000
+++ src/shared/graphics/android/syncfence.cpp	2014-07-12 06:22:08 +0000
@@ -65,7 +65,7 @@
     {
         //both fences were valid, must merge
         struct sync_merge_data data { merge_fd, "mirfence", infinite_timeout };
-        ops->ioctl(fence_fd, SYNC_IOC_MERGE, &data);
+        ops->ioctl(fence_fd, static_cast<int>(SYNC_IOC_MERGE), &data);
         ops->close(fence_fd);
         ops->close(merge_fd);
         fence_fd = data.fence;

=== modified file 'tests/unit-tests/graphics/android/test_sync_fence.cpp'
--- tests/unit-tests/graphics/android/test_sync_fence.cpp	2014-03-06 06:05:17 +0000
+++ tests/unit-tests/graphics/android/test_sync_fence.cpp	2014-07-12 06:22:08 +0000
@@ -107,7 +107,7 @@
 
     struct sync_merge_data expected_data_in { dummy_fd2, "name", 0 };
 
-    EXPECT_CALL(*mock_fops, ioctl(dummy_fd, SYNC_IOC_MERGE, MergeMatches(expected_data_in)))
+    EXPECT_CALL(*mock_fops, ioctl(dummy_fd, static_cast<int>(SYNC_IOC_MERGE), MergeMatches(expected_data_in)))
         .Times(1)
         .WillOnce(Invoke(&setter, &IoctlSetter::merge_setter));
 
@@ -119,7 +119,7 @@
 TEST_F(SyncSwTest, sync_merge_with_invalid_fd)
 {
     using namespace testing;
-    EXPECT_CALL(*mock_fops, ioctl(dummy_fd, SYNC_IOC_MERGE, _))
+    EXPECT_CALL(*mock_fops, ioctl(dummy_fd, static_cast<int>(SYNC_IOC_MERGE), _))
         .Times(0);
 
     mga::SyncFence fence1(mock_fops, invalid_fd);