Merge ~mitya57/compiz:s390x-test into compiz:master

[Dmitry Shachnev]

Use unsigned long for getting/setting _NET_WM_USER_TIME property, to fix tests on s390x.

Use unsigned long for getting/setting _NET_WM_USER_TIME property

The XChangeProperty / XGetWindowProperty man page says:

> If the specified format is 32, the property data must be a long array.

and:

> If the returned format is 32, the returned data is represented as a long
> array and should be cast to that type to obtain the elements.

This fixes the test failure on s390x.

Thanks to Jakub Jelinek for pointing me to it!

[Sam Spilsbury (smspillaz)]

I think it may be worthwhile to try and figure out why this test is failing - could be a timing related issue perhaps?

[Dmitry Shachnev (mitya57)]

Hi Sam!

I tried to add 5 seconds sleep before getting ct::NET_CLIENT_LIST_STACKING, that did not help. I also tried to SetUserTime 200 for w2 and 100 for w1, that also did not help. Maybe you have more ideas how to debug it?

For some reason it started happening only on one architecture, and I don't know what caused it — it is not Xorg or xorg-gtest as these packages were not updated for a while.

[Sam Spilsbury (smspillaz)]

Hi :-)

It has been a long time since I have worked with this code, but my hunch is as follows:

1. The test is checking that a window denied focus will not be stacked above some other window.
2. It does this by setting the user time on the incoming window to zero, then attempting to raise it.
3. The test is only failing on PowerPC and a key difference there is that PowerPC is MSB-First.

XChangeProperty stores bytes directly, so any internal byte-swapping performed does matter. Having a look at the code (https://git.launchpad.net/~mitya57/compiz/tree/tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp?h=lp1882792#n83), there are three parts where we use XChangeProperty:

    void MakeDock (Display *dpy, Window w)
    {
 Atom _NET_WM_WINDOW_TYPE = XInternAtom (dpy, "_NET_WM_WINDOW_TYPE", false);
 Atom _NET_WM_WINDOW_TYPE_DOCK = XInternAtom (dpy, "_NET_WM_WINDOW_TYPE_DOCK", false);

 XChangeProperty (dpy,
    w,
    _NET_WM_WINDOW_TYPE,
    XA_ATOM,
    32,
    PropModeReplace,
    reinterpret_cast <const unsigned char *> (&_NET_WM_WINDOW_TYPE_DOCK),
    1);
    }

    void SetUserTime (Display *dpy, Window w, Time time)
    {
 Atom _NET_WM_USER_TIME = XInternAtom (dpy, "_NET_WM_USER_TIME", false);
 unsigned int value = (unsigned int) time;

 XChangeProperty (dpy, w,
    _NET_WM_USER_TIME,
    XA_CARDINAL, 32, PropModeReplace,
    (unsigned char *) &value, 1);
    }

    void SetClientLeader (Display *dpy, Window w, Window leader)
    {
 Atom WM_CLIENT_LEADER = XInternAtom (dpy, "WM_CLIENT_LEADER", false);

 XChangeProperty (dpy, w,
    WM_CLIENT_LEADER,
    XA_WINDOW, 32, PropModeReplace,
    (unsigned char *) &leader, 1);
    }

Looking at the documentation, the fifth argument is the number of bytes in the data. Particularly for SetUserTime, we pass an unsigned int and tell the server that it is 32 bytes. unsigned int has an implementation-dependent size, with 16 bits at minimum. I guess if it is 16 bits on s390x and 32 bits on other platforms, this would cause a problem, since XChangeProperty would be reading and swapping an extra two bytes.

Probably a quick fix might be to try and change the "unsigned int value" to "uint32_t value". This is all a hunch though.

[Dmitry Shachnev (mitya57)]

Thanks a lot for pointing me to this code!

unsigned int (and signed too) is 32 bits on all platforms supported by Debian/Ubuntu [1], so changing it to uint32_t won't change anything.

However, this fragment is suspicious:

(unsigned char *) &value

It points to the first byte of a 4 bytes value, however it may be the most or the least significant byte depending on endianness.

I think it may be solved by creating a temporary unsigned char variable, like I once did in [2].

I will give it a try later today.

[1]: https://wiki.debian.org/ArchitectureSpecificsMemo#Summary
[2]: https://github.com/thiagomacieira/tinycbor/pull/1

[Dmitry Shachnev (mitya57)]

My hypothesis above was wrong.

However, I noticed an interesting thing: if I add __attribute__((noinline)) to SetUserTime function, this bug goes away.

So I am now almost sure that it's a compiler regression. Will now try to get a minimal example.

[Dmitry Shachnev (mitya57)]

> So I am now almost sure that it's a compiler regression.

In fact it was a bug in Compiz. One should use a long/unsigned long variable for getting/setting 32-bit properties. uint32_t is not enough.

Thanks to Jakub Jelinek for pointing me to it in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96594#c3.

The latest commit should fix the issue properly. It built fine on s390x in https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/4202/+packages.

[Sam Spilsbury (smspillaz)]

Aha! Great find.

It is indeed perplexing that passing "32" to XChangeProperty causes it to expect a *64* bit value. Legacy code/assumptions bite once again.

[Sam Spilsbury (smspillaz)]

It may be that the xlib manual should be updated. The documentation on the "format" parameter is a tad misleading, especially if you don't read the next section:

> format: Specifies whether the data should be viewed as a list of 8-bit, 16-bit, or 32-bit quantities. Possible values are 8, 16, and 32. This information allows the X server to correctly perform byte-swap operations as necessary. *If the format is 16-bit or 32-bit, you must explicitly cast your data pointer to an (unsigned char *) in the call to XChangeProperty().*

> If the specified format is 8, the property data must be a char array. If the specified format is 16, the property data must be a short array. If the specified format is 32, the property data must be a long array.

The first section implies that "16" means 16 bits and "32" means 32 bits, the next section says "16" means short (16 bits everywhere), "32" means long, which in turn means 32 bits on 32 bit platforms and 64 bits on 64 bit platforms.

[Sam Spilsbury (smspillaz)]

(Anyway, this has been fun. Its not up to me to approve/disapprove MRs, so I'll leave that to whoever is responsible for it these days).

[Alberts Muktupāvels (muktupavels)]

review: Approve

[Dmitry Shachnev (mitya57)]

> I'll leave that to whoever is responsible for it these days

There is no one really responsible. I usually ask Alberts to review the changes, but of course I welcome your opinion too (or anyone else's).

diff --git a/src/window.cpp b/src/window.cpp
index a967c62..d5aafe6 100644
--- a/src/window.cpp
+++ b/src/window.cpp
@@ -4848,9 +4848,9 @@ PrivateWindow::getUserTime (Time& time)
     {
 	if (n)
 	{
-	    CARD32 value;
+	    unsigned long value;
 
-	    memcpy (&value, data, sizeof (CARD32));
+	    memcpy (&value, data, sizeof (unsigned long));
 	    retval = true;
 	    time   = (Time) value;
 	}
diff --git a/tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp b/tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp
index d2d7a62..9c806e2 100644
--- a/tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp
+++ b/tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp
@@ -83,7 +83,7 @@ namespace
     void SetUserTime (Display *dpy, Window w, Time time)
     {
 	Atom _NET_WM_USER_TIME = XInternAtom (dpy, "_NET_WM_USER_TIME", false);
-	unsigned int value = (unsigned int) time;
+	unsigned long value = (unsigned long) time;
 
 	XChangeProperty (dpy, w,
 			 _NET_WM_USER_TIME,