[Xcb] [PATCH libxcb 4/4 V2] generator: fix align-pads for switches which start at unaligned pos

[Christian Linhart]

On 09/09/14 18:35, Ran Benita wrote:
> On Tue, Sep 02, 2014 at 06:13:04PM +0200, Christian Linhart wrote:
>> Fix the alignment computation inside switches which start at
>> an unaligned pos.
>> This affects both explicit and implicit align pads.
> 
> Can you describe the problem a bit more? Like, what the current code
> does wrong? (What happens if you add the <pad align="4"> mentioned
> below?).

The current code computes alignment based on the number of bytes counted
from the start of the *switch*, but it should compute alignment based on
the number of bytes counted from the start of the enclosing *struct*.
( or better: from the start of the enclosing protocol frame, like
request, reply, ... )

This is no problem if the switch starts at an aligned position.

But we have several examples in XInput where a switch starts at position 2
which is clearly not 4 byte aligned.

Simplified Example:
    <struct name="Foo">
        <field type="CARD16" name="class_id" />
        <switch name="data">
            <fieldref>class_id</fieldref>
            <case name="bar">
                <enumref ref="Classes">Bar</enumref>
	        <field type="Baz" name="myBaz" />
	        <pad align="4" />
	        <field type="CARD32" name="bar" />
            </case>
            ...
        </switch>
    </struct>

Let us assume that 
* class_id has value "Bar" so that case "bar" is chosen, 
* and "myBaz" is a var-sized field so that the align pad makes sense.
  but lets assume it has size 3 in this example

Then this *should* have the following layout:

	Start-Offset	Start-Offset	Size	Field
	rel		rel		in
	to struct	to switch	Bytes
	0		n/a		2	class_id
	2		0		3	myBaz
	5		3		3	align-pad. 
	8		6		4	bar

	To get 4-byte alignment this has to have size 3 because 5 + 3 = 8 which is 4-byte aligned


The current code bases the alignment on the start-offset 
of the switch. So the align-pad will get a size of 1 because 3 + 1 = 4 which is 4-byte aligned.
Therefore, "bar" will start on offset 6 relative to the start of the struct
and therefore "bar" is *not* 4-byte aligned in the struct, even though it is after a 4-byte align-pad.

So, the current code creates the following, which is not correct:

	Start-Offset	Start-Offset	Size	Field
	rel		rel		in
	to struct	to switch	Bytes
	0		n/a		2	class_id
	2		0		3	myBaz
	5		3		1=WRONG	align-pad. 
	6		4		4	bar

***

In the code, the start-offset relative to the start of the switch
is somehow [1] represented by variable "xcb_block_len".
xcb_padding_offset corrects for that so it takes into account
that the switch starts at a non-4-byte-aligned offset.

Of course, deriving "xcb_padding_offset" from the pointer value
assumes that the protocol data is store at a 4-byte aligned address
which is usually a safe assumption.

BTW, On some hardware platforms ( like SPARC ), 
unaligned data-access leads to a crash. ( with a SIGBUS on SPARC )
So, there, the protocol data has to be stored at an aligned address
or otherwise the program would crash very very soon.
Besides performance considerations thats probably
the main reason why all 4-byte values in the X-protocol
are stored on 4-byte aligned offsets relative to the 
start of the protocol frame. ( same with 2-byte values ).

I hope I could explain this in a comprehensible way.

Footnotes:
[1] Above, I write "somehow" because xcb_block_len 
    is set to 0 after an align-pad. 
    This probably creates other problems,
    when a 2-byte align is later followed by 4-byte align.

    So there probably needs to be another fix in that area, 
    but that is unrelated to this one.
    I'll create another patch for that unrelated, old potential bug.
    I will make that other patch in an extra patchset
    because it affects all code and may need more
    thorough review and testing.

    We probably should use ( xcb_buffer_len + xcb_padding_offset )
    as bases for alignment, and update xcb_buffer_len
    before computing the alignment.

    I have to think about this second problem a bit
    and make a testcase.
    I just realized it while writing the explanation. :-)


> 
>> The alignment offset is derived from the lowest 3 bits of
>> the pointer to the protocol-data at the start of the switch.
>> This is sufficient for correcting all alignments up to 8-byte alignment.
>> As far as I know there is no bigger alignment than 8-byte for the
>> X-protocol.
> 
> This sounds undefined-behavior-y to me... Maybe it's not...
It needs the assumption that the protocol-data is stored
at an aligned address.
The address needs to be 4-byte aligned if we deal with 4-byte align-pads.
I think we can generally assume that. 
But it is not ideal.

If protocol data is 4-byte aligned then every request, reply, event, etc
starts on a 4-byte aligned address because all X11 protocol frames
have a size which is a multiple of 4.

> Is there no easy way to figure out this number "statically"?

Unfortunately, this is not possible. :-(
(Well, it were be possible if we'd add an additional parameter to 
all of those functions. But that'd change the API in incompatible ways.)

For a discussion of that, see the messages 
    <53FF3818.10208 at DemoRecorder.com>
    <53FF450D.5040800 at opentext.com>
in the discussion for [PATCH proto 2/2].

In that discussion we reached agreement that
deriving the alignment offset from the pointer is
the lesser evil.

But maybe we need to discuss that again.

> 
>> Example:
>> struct InputState, where the switch starts after two 1-byte fields,
>> which is a 2 byte offset for 4-byte and 8-byte alignment.
>>
>> The previous problem can be demonstrated when adding a
>> <pad align="4"/> at the end of case "key".
>>
>> (Or when finding a testcase which reports the case "valuator" not
>> at the last position of the QueryDeviceState-reply.
>> I didn't find such a testcase, so I have used the pad align
>> as described above.)
> 
> This patch creates a lot of
> 
>     xcb_padding_offset = 0;
>     <...>
>     ... + xcb_padding_offset ...
> 
> Which is a no-op. Is looks like if `is_switch` is `False` you can avoid
> generating those?
Yes, I can do that. 
This will then change muss less code, which will make a review
of the generator-output easier.
I'll change the patch accordingly 
and post a new version of this patch
(after testing it of course)

> 
>> V2: patch modified in order to fix bugs which I found when working on the
>> next issue:
>> * xcb_padding_offset has to be set 0 when xcb_block_len is set 0
>> * xcb_padding_offset cannot be "const" therefore
>> * for unpack and unserialize, the padding_offset must computed
>>   from _buffer instead of from the aux_var.
> 
> There are some tabs mixed with spaces below which doesn't go well in
> python. But in the QueryDeviceState-V3 branch it is already fixed.
Sorry for the tabs.
I try to avoid these, but manual stuff is not perfect. :-(
However, since today, they get auto-corrected before being added to the repo. :-)

Chris

> 
> I'll wait for you reply to look at this further.
> 
> Ran
>