[Xcb] [PATCH v2] c_client.py: Add padding after full_sequence for certain XGE events.

[Josh Triplett]

On Mon, Dec 30, 2013 at 12:03:19AM -0800, Kenneth Graunke wrote:
> On 12/29/2013 08:50 PM, Kenneth Graunke wrote:
> > With the advent of the Present extension, some events (such as
> > PresentCompleteNotify) now use native 64-bit types on the wire.
> > 
> > For XGE events, we insert an extra "uint32_t full_sequence" field
> > immediately after the first 32 bytes of data.  Normally, this causes the
> > subsequent fields to be shifted over by 4 bytes, and the structure to
> > grow in size by 4 bytes.  Everything works fine.
> > 
> > However, if event contains 64-bit extended fields, this may result in
> > the compiler adding an extra 4 bytes of padding so that those fields
> > remain properly aligned on 64-bit boundaries.  This causes the structure
> > to grow by 8 bytes, not 4.  Unfortunately, XCB doesn't realize this.
> > read_packet() fails to malloc enough memory to hold the event, and the
> > event processing code uses the wrong offsets.
> > 
> > To correct this, we add an additional uint32_t field (called "pad_fs")
> > just after full_sequence.
> > 
> > This breaks the ABI for 32-bit systems, but any affected code would have
> > been completely broken on 64-bit systems.  Thus, it's unlikely that
> > anyone will be affected.
> > 
> > v2: Check for /any/ extended fields where size == 8, not just the first.
> >     Also, only look for fields whose type is classified as "simple" to
> >     avoid triggering on struct fields whose size happens to add up to 8
> >     (such as pairs of uint32_t values).
> > 
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/c_client.py | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > Keith,
> > 
> > You're right, of course - we have to look at all the extended fields,
> > not just the first one.
> > 
> > With that fixed, this patch now affects:
> >  - xcb_present_complete_notify_event_t (same as v1)
> >  - xcb_present_redirect_notify_event_t (new in v2)
> > 
> > diff --git a/src/c_client.py b/src/c_client.py
> > index 99fd307..937b4c7 100644
> > --- a/src/c_client.py
> > +++ b/src/c_client.py
> > @@ -2911,6 +2911,14 @@ def c_event(self, name):
> >                  full_sequence = Field(tcard32, tcard32.name, 'full_sequence', False, True, True)
> >                  idx = self.fields.index(field)
> >                  self.fields.insert(idx + 1, full_sequence)
> > +
> > +                # If the event contains any 64-bit extended fields, they need
> > +                # to remain aligned on a 64-bit boundary.  Adding full_sequence
> > +                # broke that; add an additional 32-bit pad to fix it.
> > +                if True in (f.type.size == 8 and f.type.is_simple for f in self.fields[(idx+2):]):
> > +                    pad_fs = Field(tcard32, tcard32.name, 'pad_fs', False, True, True)
> > +                    self.fields.insert(idx + 2, pad_fs)
> > +
> >                  break
> >  
> >      _c_type_setup(self, name, ('event',))
> > 
> 
> Scratch this, I failed to account for the extra field when malloc'ing,
> so it doesn't actually fix the problem.  I could've sworn I tested it,
> but apparently was still running a different version.  Sorry for the noise.

For that matter, you're inserting padding even if it would turn out that
a subsequent 64-bit field is already properly aligned.

- Josh Triplett