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

[Ran Benita]

On Tue, Sep 09, 2014 at 07:50:28PM +0200, Christian Linhart wrote:
> [snip]
> I hope I could explain this in a comprehensible way.

Thanks for the detailed explanation. That made things clear.
The proposed solution is acceptable, at least I can't see why it
wouldn't work. I defer to you and Peter on the safety of it.

> 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.

I'd be pretty surprised to ever see a <pad align!=4>.

>     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. :-)

Glad to help :)

> >> 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. 

Right, if this were not the case there would have been many unaligned
accesses.

> 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.

I've missed these messages. I agree this alternative is better, even
with the & 7.

> > 
> >> 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".

Well, if the pad-align here is really needed, I would have opted for the
easy way of just putting it after the switch. But fixing this properly
is better.

Ran

> >> (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
> >