libXi regression (was Re: XQuartz tablet mouse events are at the wrong location and other Xinput headaches)
jeremyhu at apple.com
Wed Mar 14 18:58:43 PDT 2012
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.
+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;
- 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;
^^^ 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;
+ &struct_size, &state_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...
Size: 1506 bytes
Desc: not available
-------------- 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-188.8.131.52
> bad: libXi-1.6.0
> $ git bisect log
> # bad: [70b730b0548ca9e408f14f2576b972beb32a0ad0] libXi 1.6.0
> # good: [34964b05c16161de65709d60799b9ad97ce56296] libXi 184.108.40.206
> git bisect start 'libXi-1.6.0' 'libXi-220.127.116.11'
> # 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 18.104.22.168 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.
More information about the xorg-devel