[Xcb] [PATCH v2 resend 1/4] xkb: Work around alignment problems in GetNames and GetMap replies
Ran Benita
ran234 at gmail.com
Mon Aug 12 00:14:10 PDT 2013
Hi Daniel,
On Sun, Aug 11, 2013 at 10:17:16AM +0200, Daniel Martin wrote:
> On Tue, Aug 06, 2013 at 02:12:00PM +0300, Ran Benita wrote:
> > The basic situation is this: a list of CARD8/CARD16s followed by a list
> > of CARD16/CARD32s. In the current code, the second list is aligned to
> > 1/2 bytes according the size of the first list. However, in some cases
> > the second list needs to be aligned to 4 bytes per the xkbproto spec:
> > http://www.x.org/releases/current/doc/kbproto/xkbproto.html#appD::Requests
> >
> > XkbGetMap reply (xkb-opcode 8):
> > [...]
> > a LISTofCARD8 actsRtrn.count
> > p unused,p=pad(a)
> > 8A LISTofKB_ACTION actsRtrn.acts
> > 4B LISTofKB_SETBEHAVIOR behaviorsRtrn
> > v LISTofSETofKEYMASK vmodsRtrn
> > p unused, p=pad(v)
> > 2E LISTofKB_SETEXPLICIT explicitRtrn
> > p unused,p=pad(2E)
> > 2M LISTofKB_KEYMODMAP modmapRtrn
> > p unused, p=pad(2M)
> > [...]
> >
> > XkbGetNames reply (xkb-opcode 17):
> > [...]
> > l LISTofCARD8 nLevelsPerType, sum of all elements=L
> > p unused, p=pad(l)
> > [...]
> >
> > The server and Xlib handle this with calls to XkbPaddedSize(), which is
> > a good way to see where the extra padding is needed.
> >
> > Signed-off-by: Ran Benita <ran234 at gmail.com>
> > ---
> > src/xkb.xml | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 70 insertions(+)
> >
> > diff --git a/src/xkb.xml b/src/xkb.xml
> > index 0e263c4..15b3368 100644
> > --- a/src/xkb.xml
> > +++ b/src/xkb.xml
> > @@ -1350,6 +1350,20 @@ authorization from the authors.
> > <list name="acts_rtrn_count" type="CARD8">
> > <fieldref>nKeyActions</fieldref>
> > </list>
> > + <list type="CARD8" name="alignment_pad">
> > + <op op="-">
> > + <op op="&">
> > + <op op="+">
> > + <fieldref>nKeyActions</fieldref>
> > + <value>3</value>
> > + </op>
> > + <unop op="~">
> > + <value>3</value>
> > + </unop>
> > + </op>
> > + <fieldref>nKeyActions</fieldref>
> > + </op>
> > + </list>
> > <list name="acts_rtrn_acts" type="Action">
> > <fieldref>totalActions</fieldref>
> > </list>
> > @@ -1365,18 +1379,60 @@ authorization from the authors.
> > <list name="vmods_rtrn" type="CARD8" mask="ModMask">
> > <popcount><fieldref>virtualMods</fieldref></popcount>
> > </list>
> > + <list type="CARD8" name="alignment_pad2">
> > + <op op="-">
> > + <op op="&">
> > + <op op="+">
> > + <popcount><fieldref>virtualMods</fieldref></popcount>
> > + <value>3</value>
> > + </op>
> > + <unop op="~">
> > + <value>3</value>
> > + </unop>
> > + </op>
> > + <popcount><fieldref>virtualMods</fieldref></popcount>
> > + </op>
> > + </list>
> > </bitcase>
> > <bitcase>
> > <enumref ref="MapPart">ExplicitComponents</enumref>
> > <list name="explicit_rtrn" type="SetExplicit">
> > <fieldref>totalKeyExplicit</fieldref>
> > </list>
> > + <list type="CARD16" name="alignment_pad3">
> > + <op op="-">
> > + <op op="&">
> > + <op op="+">
> > + <fieldref>totalKeyExplicit</fieldref>
> > + <value>1</value>
> > + </op>
> > + <unop op="~">
> > + <value>1</value>
> > + </unop>
> > + </op>
> > + <fieldref>totalKeyExplicit</fieldref>
> > + </op>
> > + </list>
> > </bitcase>
> > <bitcase>
> > <enumref ref="MapPart">ModifierMap</enumref>
> > <list name="modmap_rtrn" type="KeyModMap">
> > <fieldref>totalModMapKeys</fieldref>
> > </list>
> > + <list type="CARD16" name="alignment_pad4">
> > + <op op="-">
> > + <op op="&">
> > + <op op="+">
> > + <fieldref>totalModMapKeys</fieldref>
> > + <value>1</value>
> > + </op>
> > + <unop op="~">
> > + <value>1</value>
> > + </unop>
> > + </op>
> > + <fieldref>totalModMapKeys</fieldref>
> > + </op>
> > + </list>
> > </bitcase>
> > <bitcase>
> > <enumref ref="MapPart">VirtualModMap</enumref>
> > @@ -1657,6 +1713,20 @@ authorization from the authors.
> > <fieldref>nKTLevels</fieldref> -->
> > <fieldref>nTypes</fieldref>
> > </list>
> > + <list type="CARD8" name="alignment_pad">
> > + <op op="-">
> > + <op op="&">
> > + <op op="+">
> > + <fieldref>nTypes</fieldref>
> > + <value>3</value>
> > + </op>
> > + <unop op="~">
> > + <value>3</value>
> > + </unop>
> > + </op>
> > + <fieldref>nTypes</fieldref>
> > + </op>
> > + </list>
> > <list name="ktLevelNames" type="ATOM">
> > <sumof ref="nLevelsPerType" />
> > </list>
> > --
> > 1.8.3.4
>
> Could you make the alignment pad lists of type 'void' please? Other
> alignment pads use void too.
I've changed this intentionally (which was worth a mention I guess):
1. Using 'void' cause some compiler warning, e.g.:
xkb.c: In function 'xcb_xkb_get_map_map_alignment_pad_end':
xkb.c:6043:41: warning: pointer of type 'void *' used in arithmetic [-Wpedantic]
i.data = /* map */ S->alignment_pad + (((R->nKeyActions + 3) & (~3)) - R->nKeyActions);
Not sure why this doesn't happen in the existing cases.
And maybe related, I recall 'void' doing the wrong thing in some cases,
but I haven't rechecked now though.
2. In a couple of places I'm using 'CARD16' rather than 'CARD8', because
I need to match the type of the value which precedes the alignment_pad.
Otherwise it may not align correctly. So it seems more consistent to use
the explicit type in both cases.
But anyway, why is 'void' preferred? (At least the useless accessor
functions are generated in both cases).
> Additionally, I could think of: That we may patch the generator to omit
> those list (based upon the condition: type is void and name starts with
> alignment_pad) when generating the surrounding structure. That'd be a
> hack until we've <pad align=.../>.
I don't see a reason to replace a hack with another hack. <pad align>
would be great, and I don't think would break ABI (I'd say if someone
uses the generated alignment_pad functions, it's their fault...). So
let's wait for that (that is until you write it :)
Thanks for the review,
Ran
More information about the Xcb
mailing list