[Xcb] [PATCH] Force XCB event structures with 64-bit extended fields to be packed.

[Daniel Martin]

On Mon, Dec 30, 2013 at 08:31:55PM -0800, 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 aligned on 64-bit boundaries.  This causes the structure to grow
> by 8 bytes, not 4.  Unfortunately, XCB doesn't realize this, and
> always believes that the length only increased by 4.  read_packet()
> then fails to malloc enough memory to hold the event, and the event
> processing code uses the wrong offsets.
> 
> To fix this, mark any event structures containing 64-bit extended
> fields with __attribute__((__packed__)).
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> Reviewed-by: Keith Packard <keithp at keithp.com>
> ---
>  src/c_client.py | 12 +++++++++---
>  src/xcb.h       |  2 ++
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> Keith noted that the compiler is required to generate code to do an
> unaligned load, so straight reads from this should not cause problems.
> 
> On #xorg-devel, dalias pointed out that on some architectures, it isn't
> safe to take the address of an unaligned field and pass it around as a
> pointer.  Keith and Eric noted that this is really unlikely, given that
> we're only applying this workaround to events, which should only ever be
> read...
> 
> This preserves the existing ABI on x86 systems, and fixes amd64 systems.
> Unlike other proposed fixes, it doesn't require a major .so bump, nor a
> new release of Xlib.  Not being able to take a pointer to certain fields
> in two of the new Present events is an irritating restriction.  But we've
> only come up with one other solution that doesn't break ABI - handling
> uint64_t as a pair of uint32_t with funky accessor functions.  That seems
> even more irritating.  As I see it, this is awful, but strictly an
> improvement over what we have today.
> 
> In the future, if we decide to bump the .so version and break ABI, I
> agree with Daniel that we should clean up this mess by removing the
> full_sequence field (or at least not putting it in the middle!).
> 
> Keith suggested only applying the "packed" attribute to structures that
> required packing.  So I've added a bit of Python code to limit that.
> 
> Feedback welcome...thanks all.
> 
> diff --git a/src/c_client.py b/src/c_client.py
> index 99fd307..ff27439 100644
> --- a/src/c_client.py
> +++ b/src/c_client.py
> @@ -1762,7 +1762,7 @@ def c_simple(self, name):
>          # Iterator
>          _c_iterator(self, name)
>  
> -def _c_complex(self):
> +def _c_complex(self, force_packed = False):
>      '''
>      Helper function for handling all structure types.
>      Called for all structs, requests, replies, events, errors.
> @@ -1772,7 +1772,7 @@ def _c_complex(self):
>      _h('/**')
>      _h(' * @brief %s', self.c_type)
>      _h(' **/')
> -    _h('typedef %s %s {', self.c_container, self.c_type)
> +    _h('typedef %s%s %s {', self.c_container, ' XCB_PACKED' if force_packed else '', self.c_type)
>  
>      struct_fields = []
>      maxtypelen = 0
> @@ -2902,6 +2902,7 @@ def c_event(self, name):
>      # events while generating the structure for them. Otherwise we would read
>      # garbage (the internal full_sequence) when accessing normal event fields
>      # there.
> +    force_packed = False
>      if hasattr(self, 'is_ge_event') and self.is_ge_event and self.name == name:
>          event_size = 0
>          for field in self.fields:
> @@ -2911,6 +2912,11 @@ 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
> +                # would normally break that; force the struct to be packed.
> +                force_packed = True in (f.type.size == 8 and f.type.is_simple for f in self.fields[(idx+1):])

Just a nitpick, the "True in ()" could become an any().

>                  break
>  
>      _c_type_setup(self, name, ('event',))
> @@ -2920,7 +2926,7 @@ def c_event(self, name):
>  
>      if self.name == name:
>          # Structure definition
> -        _c_complex(self)
> +        _c_complex(self, force_packed)
>      else:
>          # Typedef
>          _h('')
> diff --git a/src/xcb.h b/src/xcb.h
> index e62c985..73c77a3 100644
> --- a/src/xcb.h
> +++ b/src/xcb.h
> @@ -51,6 +51,8 @@ extern "C" {
>   * @file xcb.h
>   */
>  
> +#define XCB_PACKED __attribute__((__packed__))

Would it be usefull to have such a "packed" abstraction in util-macros
to be able to handle it for other compilers (like the _Alignas() from
C11, which Mark mentioned) in the future? I see that it wasn't necessary
before and might not be used by more libs then xcb soon - or ever.

>  /**
>   * @defgroup XCB_Core_API XCB Core API
>   * @brief Core API of the XCB library.
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb

Reviewed-by: Daniel Martin <consume.noise at gmail.com>