[Xcb] [PATCH proto 5/5] make xkb pass the alignment checker
Christian Linhart
chris at DemoRecorder.com
Thu Nov 5 21:41:16 PST 2015
Hi Ran,
On 2015-11-05 09:46, Ran Benita wrote:
> On Thu, Nov 05, 2015 at 04:53:27AM +0100, Christian Linhart wrote:
>> [...]
>>>> 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.
The checker does care about that.
It considers all possibilities, but not by trying all of them.
There is a more elegant way to do that...
> 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?
The padding is required by the spec but it cannot be derived automatically.
Reason: The struct KeyModMap consists of two 1 byte values.
Therefore it can be used at any alignment. The KeyVModMap
that could optionally occur, only needs 2-byte alignment
which is guaranteed after SetExplicit.
So, The 4-byte align-pad at the end of the SetExplicit case-branch can only be
derived from the spec.
There is no automatic way to detect its necessity because
this 4-byte align-pad is not necessary to get all subsequent data aligned to their
required alignment. Because the subsequent data (in all of its possible configurations)
is happy with the 2-byte alignment that is guaranteed after the SetExplicit case-branch.
This also means that there cannot be any implicit alignment here.
The lack of the 4-byte align-pad at the end of the SetExplicit case-branch
simply a protocol definition bug.
So, the checker can obviously not check for any protocol definition bugs.
It can only detect cases where there can be misalignment of primitive types.
These are the same cases where implicit alignment can occur.
(Implicit alignment is also an automatic mechanism and therefore
can also only handle stuff that can be detected automatically.
Anything beyond that needs human review, or precise work when writing
the original xml protocol definition.)
Cheers,
Chris
PS.: The "required alignment" of primitive types is their length, i.e.,
* 1 byte values can occur at any alignment
* 2 byte values have to have 2-byte alignment
* 4 byte values have to have 4-byte alignment
* usually, 8 byte values have to have 8-byte alignment,
but I am not sure whether this applies to the X-protocol
because the start of requests is only guaranteed to have 4 byte alignment.
Therefore it is not possible to guarantee 8-byte alignment by
protocol-design.
The reason for these alignment rules are some properties of the hardware,
e.g.
* On Sparc-CPUs, the program crashes with a Bus-error if data is not accessed with proper alignment
* On Intel-CPUs, you'll get a performance penalty for unaligned data access.
>
> 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
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb
More information about the Xcb
mailing list