libXi regression (was Re: XQuartz tablet mouse events are at the wrong location and other Xinput headaches)

[Jeremy Huddleston]

So looking through the original patch is making my head spin... here are a couple things that stood out to me:

1) This is odd.  It looks like the original code should never have worked because of the sizeof(XIButtonClass).  My issue does not run through this block, but it's still a head scratcher.

+static void
+sizeXIButtonClassType(int num_buttons, int* structure, int* state, int* atoms)
+{
...
+    *structure = pad_to_double(sizeof(XIButtonClassInfo));

...

-                    bout = next_block(&ptr, sizeof(XIButtonClass));
+                    sizeXIButtonClassType(bin->num_buttons, &struct_size,
+                                          &state_size, &labels_size);
+                    bout = next_block(&ptr, struct_size);

2) sizeDeviceClassType(XIButtonClass) is now longer 

 sizeDeviceClassType(int type, int num_elements)
 {
     int l = 0;
+    int extra1 = 0;
+    int extra2 = 0;
     switch(type)
     {
         case XIButtonClass:
-            l = sizeof(XIButtonClassInfo);
-            l += num_elements * sizeof(Atom);
-            /* Force mask alignment with longs to avoid
-             * unaligned access when accessing the atoms. */
-            l += ((((num_elements + 7)/8) + 3)/4) * sizeof(Atom);
+            sizeXIButtonClassType(num_elements, &l, &extra1, &extra2);
+            l += extra1 + extra2;
             break;

^^^ Why is extra1 (the size of the state mask) now being added here?  It wasn't before.  Was this just a latent bug that was fixed with this change rather than as a separate commit?

3) copy_classes seems to be the culprit.  With the new changes, we're passing larger values to next_block 

-                    cls_lib = next_block(&ptr_lib, sizeof(XIButtonClassInfo));
                     cls_wire = (xXIButtonInfo*)any_wire;
+                    sizeXIButtonClassType(cls_wire->num_buttons,
+                                          &struct_size, &state_size,
+                                          &labels_size);
+                    cls_lib = next_block(&ptr_lib, struct_size);
...
-                    cls_lib->state.mask = next_block(&ptr_lib, size * sizeof(Atom));
+                    cls_lib->state.mask_len = state_size;
+                    cls_lib->state.mask = next_block(&ptr_lib, state_size);
...
-                    cls_lib->labels = next_block(&ptr_lib, cls_lib->num_buttons * sizeof(Atom));
+                    cls_lib->labels = next_block(&ptr_lib, labels_size);

A comparison of old steps and new steps:
~ $ ./a.out 
sizeof(XIButtonClassInfo): 40, struct_size: 40
size * sizeof(Atom): 8, state_size: 8
old_mask_len: 4, state_size: 8
num_buttons * sizeof(Atom): 56, labels_size: 64

So the issue seems to be that the new "cls_lib->state.mask = next_block(&ptr_lib, state_size)" steps over our first button.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: testa.c
Type: application/octet-stream
Size: 1506 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20120314/627664b7/attachment.obj>
-------------- next part --------------


On Mar 14, 2012, at 6:00 PM, Jeremy Huddleston <jeremyhu at apple.com> wrote:

> Reverting c1a5a70b51f12dedf354102217c7cd4247ed3a4b from libXi fixed the issue for me.  I haven't looked into why, but the patch's changes are certainly related:
> 
> good: libXi-1.5.99.3
> bad: libXi-1.6.0
> 
> $ git bisect log
> # bad: [70b730b0548ca9e408f14f2576b972beb32a0ad0] libXi 1.6.0
> # good: [34964b05c16161de65709d60799b9ad97ce56296] libXi 1.5.99.3
> git bisect start 'libXi-1.6.0' 'libXi-1.5.99.3'
> # bad: [1b9f0394c3d4d3833f8560ae8170a4d5842419ab] Fix XIScrollClass increment value on 32-bit machines
> git bisect bad 1b9f0394c3d4d3833f8560ae8170a4d5842419ab
> # bad: [c1a5a70b51f12dedf354102217c7cd4247ed3a4b] Fix bus error on MIPS N32 for bug #38331.
> git bisect bad c1a5a70b51f12dedf354102217c7cd4247ed3a4b
> 
> c1a5a70b51f12dedf354102217c7cd4247ed3a4b is the first bad commit
> commit c1a5a70b51f12dedf354102217c7cd4247ed3a4b
> Author: Micha? Mas?owski <mtjm at mtjm.eu>
> Date:   Tue Feb 21 20:54:40 2012 +0100
> 
>    Fix bus error on MIPS N32 for bug #38331.
> 
>    XIValuatorClassInfo and XIScrollClassInfo might have an address
>    of 4 bytes modulo 8, while they contain doubles which need 8 byte
>    alignment.  This is fixed by adding extra padding after each structure
>    or array in sizeDeviceClassType and adding helper functions to
>    determine sizes and padding only in one place.
> 
>    Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=38331
>    Signed-off-by: Micha? Mas?owski <mtjm at mtjm.eu>
>    Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> 
> :040000 040000 6b49f07dcdd66ae31add8e9b543e6d6a0586231c a389cffc1796584cc7e90ea296b57f7ecf2224d9 M	src
> 
> 
> On Mar 14, 2012, at 5:06 PM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
>>> (gdb) print b->num_buttons
>>> $24 = 7
>>> (gdb) print b->labels[-1]
>>> $25 = 498216206336
>>> 
>>> Based on xlsatoms, I'd expect 116-122 as the button atoms:
>>> 116	Button Left
>>> 117	Button Middle
>>> 118	Button Right
>>> 119	Button Wheel Up
>>> 120	Button Wheel Down
>>> 121	Button Horiz Wheel Left
>>> 122	Button Horiz Wheel Right
>>> 
>>> It works when the client is i386/Linux or i386/darwin and fails when x86_64/darwin, ppc/Linux, or ppc64/Linux ... is this possibly an inputproto issue?
>> 
>> libXi or xinput, I suspect. curious though, since it works fine here in
>> x86_64. Can you bisect libXi from 1.5.99.2 to 1.6.0, I wonder if we
>> introduced a bug here with the alignments.
>> 
>> 2d638fc37b0dbf28e5c826f74f68ada83a8c3e2b is another possible candidate to
>> revert and test.
>> 
>> put a breakpoint in libXi's copy_classes on the XIButtonClass case and see
>> what comes down the wire and where the offset is introduced. if it's already
>> wrong on the wire, then the server has a bug here.