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

Ran Benita ran234 at gmail.com
Sun Oct 12 09:38:19 PDT 2014


On Sun, Oct 12, 2014 at 07:02:04PM +0300, Ran Benita wrote:
> On Wed, Sep 10, 2014 at 08:10:25PM +0300, Ran Benita wrote:
> > On Tue, Sep 09, 2014 at 11:26:40PM +0200, Christian Linhart wrote:
> > > From: Christian Linhart <chris at DemoRecorder.com>
> > > 
> > > Fix the alignment computation inside switches which start at
> > > an unaligned pos.
> > > This affects both explicit and implicit align pads.
> > > 
> > > 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.
> > > 
> > > 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.)
> > > 
> > > 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.
> > > 
> > > V3: patch revised according to suggestion by Ran Benita:
> > > * only create and use xcb_padding_offset for switch
> > 
> > It's always unfortunate when c_client.py becomes even more obscure.. But
> > this looks good to me now:
> 
> Maybe as a follow-up (after this is merged) we can make this even a bit
> simpler, by unconditionally generating the `xcb_padding_offset`
> declaration, and using it freely, but just making sure it is initialized
> to 0 when `not is_switch`.
> 
> Anything to reduce the massive amounts of guess-why-I'm-here if-else's in
> the current generator...

Oops, that was the previous version, and what I asked to change :)
Before I looked mostly at the generated files, now I looked more at the
generator. So conflict of interest :)

Anyway, disregard this comment - a cleaner file is probably better.

Ran

> Ran
> 
> > Reviewed-by: Ran Benita <ran234 at gmail.com>
> > 
> > > ---
> > >  src/c_client.py | 26 +++++++++++++++++++++-----
> > >  1 file changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/c_client.py b/src/c_client.py
> > > index 1d1bbf6..0a1f877 100644
> > > --- a/src/c_client.py
> > > +++ b/src/c_client.py
> > > @@ -653,17 +653,23 @@ def get_serialize_params(context, self, buffer_var='_buffer', aux_var='_aux'):
> > >          for p in param_fields:
> > >              if not p.type.fixed_size():
> > >                  add_param(params, (p.c_field_const_type, '*', p.c_field_name))
> > >  
> > >      return (param_fields, wire_fields, params)
> > >  # get_serialize_params()
> > >  
> > > -def _c_serialize_helper_insert_padding(context, code_lines, space, postpone):
> > > +def _c_serialize_helper_insert_padding(context, code_lines, space, postpone, is_case_or_bitcase):
> > >      code_lines.append('%s    /* insert padding */' % space)
> > > -    code_lines.append('%s    xcb_pad = -xcb_block_len & (xcb_align_to - 1);' % space)
> > > +    if is_case_or_bitcase:
> > > +        code_lines.append(
> > > +            '%s    xcb_pad = -(xcb_block_len + xcb_padding_offset) & (xcb_align_to - 1);'
> > > +            % space)
> > > +    else:
> > > +        code_lines.append(
> > > +            '%s    xcb_pad = -xcb_block_len & (xcb_align_to - 1);' % space)
> > >  #    code_lines.append('%s    printf("automatically inserting padding: %%%%d\\n", xcb_pad);' % space)
> > >      code_lines.append('%s    xcb_buffer_len += xcb_block_len + xcb_pad;' % space)
> > >  
> > >      if not postpone:
> > >          code_lines.append('%s    if (0 != xcb_pad) {' % space)
> > >  
> > >          if 'serialize' == context:
> > > @@ -673,14 +679,16 @@ def _c_serialize_helper_insert_padding(context, code_lines, space, postpone):
> > >          elif context in ('unserialize', 'unpack', 'sizeof'):
> > >              code_lines.append('%s        xcb_tmp += xcb_pad;' % space)
> > >  
> > >          code_lines.append('%s        xcb_pad = 0;' % space)
> > >          code_lines.append('%s    }' % space)
> > >  
> > >      code_lines.append('%s    xcb_block_len = 0;' % space)
> > > +    if is_case_or_bitcase:
> > > +        code_lines.append('%s    xcb_padding_offset = 0;' % space)
> > >  
> > >      # keep tracking of xcb_parts entries for serialize
> > >      return 1
> > >  # _c_serialize_helper_insert_padding()
> > >  
> > >  def _c_serialize_helper_switch(context, self, complex_name,
> > >                                 code_lines, temp_vars,
> > > @@ -1001,21 +1009,23 @@ def _c_serialize_helper_fields(context, self,
> > >  
> > >          # fields with variable size
> > >          else:
> > >              if field.type.is_pad:
> > >                  # Variable length pad is <pad align= />
> > >                  code_lines.append('%s    xcb_align_to = %d;' % (space, field.type.align))
> > >                  count += _c_serialize_helper_insert_padding(context, code_lines, space,
> > > -                                                        self.c_var_followed_by_fixed_fields)
> > > +                                                            self.c_var_followed_by_fixed_fields,
> > > +                                                            is_case_or_bitcase)
> > >                  continue
> > >              else:
> > >                  # switch/bitcase: always calculate padding before and after variable sized fields
> > >                  if need_padding or is_case_or_bitcase:
> > >                      count += _c_serialize_helper_insert_padding(context, code_lines, space,
> > > -                                                            self.c_var_followed_by_fixed_fields)
> > > +                                                                self.c_var_followed_by_fixed_fields,
> > > +                                                                is_case_or_bitcase)
> > >  
> > >                  value, length = _c_serialize_helper_fields_variable_size(context, self, field,
> > >                                                                           code_lines, temp_vars,
> > >                                                                           space, prefix)
> > >                  prev_field_was_variable = True
> > >  
> > >          # save (un)serialization C code
> > > @@ -1096,15 +1106,15 @@ def _c_serialize_helper(context, complex_type,
> > >              code_lines.append('%s    xcb_buffer_len += xcb_block_len;' % space)
> > >              code_lines.append('%s    xcb_block_len = 0;' % space)
> > >  
> > >          count += _c_serialize_helper_fields(context, self,
> > >                                              code_lines, temp_vars,
> > >                                              space, prefix, False)
> > >      # "final padding"
> > > -    count += _c_serialize_helper_insert_padding(context, code_lines, space, False)
> > > +    count += _c_serialize_helper_insert_padding(context, code_lines, space, False, self.is_switch)
> > >  
> > >      return count
> > >  # _c_serialize_helper()
> > >  
> > >  def _c_serialize(context, self):
> > >      """
> > >      depending on the context variable, generate _serialize(), _unserialize(), _unpack(), or _sizeof()
> > > @@ -1166,14 +1176,16 @@ def _c_serialize(context, self):
> > >              _c('    unsigned int xcb_out_pad = -sizeof(%s) & 3;', self.c_type)
> > >              _c('    unsigned int xcb_buffer_len = sizeof(%s) + xcb_out_pad;', self.c_type)
> > >              _c('    unsigned int xcb_align_to = 0;')
> > >          else:
> > >              _c('    char *xcb_out = *_buffer;')
> > >              _c('    unsigned int xcb_buffer_len = 0;')
> > >              _c('    unsigned int xcb_align_to = 0;')
> > > +        if self.is_switch:
> > > +            _c('    unsigned int xcb_padding_offset = ((size_t)xcb_out) & 7;')
> > >          prefix = [('_aux', '->', self)]
> > >          aux_ptr = 'xcb_out'
> > >  
> > >      elif context in ('unserialize', 'unpack'):
> > >          _c('    char *xcb_tmp = (char *)_buffer;')
> > >          if not self.is_switch:
> > >              if not self.c_var_followed_by_fixed_fields:
> > > @@ -1189,14 +1201,16 @@ def _c_serialize(context, self):
> > >                  aux_var = '(*_aux)' # unserialize: double pointer (!)
> > >              prefix = [(aux_var, '->', self)]
> > >          aux_ptr = '*_aux'
> > >          _c('    unsigned int xcb_buffer_len = 0;')
> > >          _c('    unsigned int xcb_block_len = 0;')
> > >          _c('    unsigned int xcb_pad = 0;')
> > >          _c('    unsigned int xcb_align_to = 0;')
> > > +        if self.is_switch:
> > > +            _c('    unsigned int xcb_padding_offset = ((size_t)_buffer) & 7;')
> > >  
> > >      elif 'sizeof' == context:
> > >          param_names = [p[2] for p in params]
> > >          if self.is_switch:
> > >              # switch: call _unpack()
> > >              _c('    %s _aux;', self.c_type)
> > >              _c('    return %s(%s, &_aux);', self.c_unpack_name, reduce(lambda x,y: "%s, %s" % (x, y), param_names))
> > > @@ -1206,14 +1220,16 @@ def _c_serialize(context, self):
> > >              # special case: call _unserialize()
> > >              _c('    return %s(%s, NULL);', self.c_unserialize_name, reduce(lambda x,y: "%s, %s" % (x, y), param_names))
> > >              _c('}')
> > >              return
> > >          else:
> > >              _c('    char *xcb_tmp = (char *)_buffer;')
> > >              prefix = [('_aux', '->', self)]
> > > +            if self.is_switch:
> > > +                _c('    unsigned int xcb_padding_offset = 0;')
> > >  
> > >      count = _c_serialize_helper(context, self, code_lines, temp_vars, prefix=prefix)
> > >      # update variable size fields (only important for context=='serialize'
> > >      variable_size_fields = count
> > >      if 'serialize' == context:
> > >          temp_vars.append('    unsigned int xcb_pad = 0;')
> > >          temp_vars.append('    char xcb_pad0[3] = {0, 0, 0};')
> > > -- 
> > > 2.0.1
> > > 
> > > _______________________________________________
> > > Xcb mailing list
> > > Xcb at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/xcb


More information about the Xcb mailing list