[Xcb] [PATCH libxcb 4/4 V2] generator: fix align-pads for switches which start at unaligned pos
Ran Benita
ran234 at gmail.com
Tue Sep 9 09:35:07 PDT 2014
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 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...
Is there no easy way to figure out this number "statically"?
> 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?
> 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.
I'll wait for you reply to look at this further.
Ran
> ---
> src/c_client.py | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/src/c_client.py b/src/c_client.py
> index 6af21fb..6ca3a30 100644
> --- a/src/c_client.py
> +++ b/src/c_client.py
> @@ -655,15 +655,17 @@ def get_serialize_params(context, self, buffer_var='_buffer', aux_var='_aux'):
> 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):
> code_lines.append('%s /* insert padding */' % space)
> - code_lines.append('%s xcb_pad = -xcb_block_len & (xcb_align_to - 1);' % space)
> + code_lines.append(
> + '%s xcb_pad = -( xcb_block_len + xcb_padding_offset ) & (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 +675,15 @@ 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)
> + 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,
> @@ -1091,14 +1094,15 @@ def _c_serialize_helper(context, complex_type,
> else:
> # unserialize & fixed size fields: simply cast the buffer to the respective xcb_out type
> if context in ('unserialize', 'unpack', 'sizeof') and not self.c_var_followed_by_fixed_fields:
> code_lines.append('%s xcb_block_len += sizeof(%s);' % (space, self.c_type))
> code_lines.append('%s xcb_tmp += xcb_block_len;' % space)
> code_lines.append('%s xcb_buffer_len += xcb_block_len;' % space)
> code_lines.append('%s xcb_block_len = 0;' % space)
> + code_lines.append('%s xcb_padding_offset = 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)
>
> @@ -1166,14 +1170,18 @@ 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;')
> + else:
> + _c(' unsigned int xcb_padding_offset = 0;')
> 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 +1197,18 @@ 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;')
> + else:
> + _c(' unsigned int xcb_padding_offset = 0;')
>
> 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 +1218,15 @@ 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)]
> + _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