[Xcb] [PATCH proto 5/5] make xkb pass the alignment checker

[Ran Benita]

On Thu, Nov 05, 2015 at 04:53:27AM +0100, Christian Linhart wrote:
> Hi Ran,
> 
> Thank you for looking at this patch.
> 
> On 2015-11-05 01:15, Ran Benita wrote:
> > On Sun, Nov 01, 2015 at 06:26:34PM +0100, Christian Linhart wrote:
> >> These are just minimal adjustments to get xkb through
> >> the checks of the alignment checker.
> >>
> >> It is not the big fixup which I have already posted an RFC patch
> >> a while ago.
> > I haven't looked at the code for the checker yet, but the results here
> > look good. I have a question below though (hopefully you haven't
> > answered it already in the other commit message).
> >
> > And one style issue: the file uses tabs everywhere, but the patch uses
> > spaces.
> Thank you for noticing that, and sorry for that.
> This must have slipped through my final style check.
> I'll update the patch.
> >
> >> Signed-off-by: Christian Linhart <chris at demorecorder.com>
> >> ---
> >>  src/xkb.xml | 25 +++++++++++--------------
> >>  1 file changed, 11 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/src/xkb.xml b/src/xkb.xml
> >> index ad52ea2..0c5fa3f 100644
> >> --- a/src/xkb.xml
> >> +++ b/src/xkb.xml
> >> @@ -741,14 +741,15 @@ <struct name="Section">
> >>  
> >>  	<struct name="Listing">
> >>  		<field name="flags" type="CARD16" />
> >>  		<field name="length" type="CARD16" />
> >>  		<list name="string" type="STRING8">
> >>  			<fieldref>length</fieldref>
> >>  		</list>
> >> +		<pad align="2" />
> >>  	</struct>
> >>  
> >>  	<struct name="DeviceLedInfo">
> >>  		<field name="ledClass" type="LedClassSpec" enum="LedClass" />
> >>  		<field name="ledID" type="IDSpec" altenum="ID" />
> >>  		<field name="namesPresent" type="CARD32" />
> >>  		<field name="mapsPresent" type="CARD32" />
> >> @@ -1439,14 +1440,15 @@ <request name="SetMap" opcode="9">
> >>  				</list>
> >>  			</bitcase>
> >>  			<bitcase>
> >>  				<enumref ref="MapPart">KeyActions</enumref>
> >>  				<list name="actionsCount" type="CARD8">
> >>  					<fieldref>nKeyActions</fieldref>
> >>  				</list>
> >> +                                <pad align="4" />
> >>  				<list name="actions" type="Action">
> >>  					<fieldref>totalActions</fieldref>
> >>  				</list>
> >>  			</bitcase>
> >>  			<bitcase>
> >>  				<enumref ref="MapPart">KeyBehaviors</enumref>
> >>  				<list name="behaviors" type="SetBehavior">
> >> @@ -1454,14 +1456,15 @@ <request name="SetMap" opcode="9">
> >>  				</list>
> >>  			</bitcase>
> >>  			<bitcase>
> >>  				<enumref ref="MapPart">VirtualMods</enumref>
> >>  				<list name="vmods" type="CARD8">
> >>  					<popcount><fieldref>virtualMods</fieldref></popcount>
> >>  				</list>
> >> +                                <pad align="4" />
> >>  			</bitcase>
> >>  			<bitcase>
> >>  				<enumref ref="MapPart">ExplicitComponents</enumref>
> >>  				<list name="explicit" type="SetExplicit">
> >>  					<fieldref>totalKeyExplicit</fieldref>
> >>  				</list>
> > I wonder why this one (for example) doesn't need an align? `SetExplicit`
> > is 2 bytes long. Is there still implicit padding here?
> No, there is no implicit padding.
> The start of the bitcase is already aligned because all prior bitcases
> end on a 4-byte aligned address.
> 
> I guess that the checker complained about SetExplicit non-alignment
> before I added the 4-byte align-pad at the end of the "VirtualMods" bitcase.
> The protocol spec probably states that all bitcases have to start
> at a 4-byte aligned address. Therefore the proper way to fix
> non-alignment of the "explicit"-list is to fix the alignment at the end
> of the other bitcases.

I am not sure I understand why the other bitcases should affect the
alignment/padding due to this one. After all, all the bitcases are
optional, so it's possible that only "excplicits" are sent, or they are
sent first/last. Or to quote the full example (after the patch):

    <bitcase>
    »       <enumref ref="MapPart">ExplicitComponents</enumref>
    »       <list name="explicit" type="SetExplicit">
    »       »       <fieldref>totalKeyExplicit</fieldref>
    »       </list>
    </bitcase>
    <bitcase>
    »       <enumref ref="MapPart">ModifierMap</enumref>
    »       <list name="modmap" type="KeyModMap">
    »       »       <fieldref>totalModMapKeys</fieldref>
    »       </list>
    </bitcase>

both the `SetExplicit` and `KeyModMap` structs are 2 bytes. Let's say
these are the only two bits set in `MapPart`. And let's say we have 1
`SetExplicit` in the list and 1 `KeyModMap`. Will there be padding
between them?

For reference the spec says:

    2E     LISTofKB_SETEXPLICIT          explicit
    p               unused,p=pad(2E)
    2M     LISTofKB_KEYMODMAP          modmap
    P               unused, p=pad(2M)

> Chris
> 
> BTW,  the align-fixes in XKB are just there to get it through the checker.
> But XKB is a good testcase for the checker, due to the complexity of the XKB protocol.

Explicit padding in the protocol files is nicer anyway, so I like the
changes irregardless.

> Xkb really needs complete rework, as by my RFC post a while ago.

For some reason patchwork does show your patches. I must have missed
this one, can you point me at the mail?

> I simply did not have the time to split that xkb-work into pieces and cross-reference them with the spec.
> That's a huge amount of work, but that's the investment needed for having everything reviewed,
> which is the basis of open-source development.
> So, maybe I can make time for that, or somebody volunteers to document my prior XKB RFC-patch :-)

Cross-referencing everything does take a lot of time. If the fixes were
found automatically, I think it should be sufficient to review the
method, and review a sample of the change against the spec. But let's
see what you mean by "huge" first :)

Ran