From d74d066949dbbbbbcb03bf7764e63f4347f99974 Mon Sep 17 00:00:00 2001 From: Christian Linhart Date: Sat, 6 Sep 2014 20:06:15 +0200 Subject: [PATCH 1/4] generator: support fixed size lists in var-sized structs V2: patch revised according to suggestions from Ran Benita: * removed blanks before an after parentheses of function-calls or tuples * replaced if by elif in "if field.type.is_list". ( this fixes old code ) Message-ID: <540B4D17.1080908@DemoRecorder.com> Patch-Thread-Subject: [Xcb] xinput:QueryDeviceState: full-support: generator and xml-changes Patch-Set: QueryDeviceState Patch-Number: libxcb 1/3 Patch-Version: V2 Signed-off-by: Christian Linhart Reviewed-By: Ran Benita --- src/c_client.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/c_client.py b/src/c_client.py index 87f268b..6f8f8f2 100644 --- a/src/c_client.py +++ b/src/c_client.py @@ -869,9 +869,13 @@ def _c_serialize_helper_fields_fixed_size(context, self, field, # total padding = sizeof(pad0) * nmemb length += " * %d" % field.type.nmemb - if field.type.is_list: - # no such case in the protocol, cannot be tested and therefore ignored for now - raise Exception('list with fixed number of elemens unhandled in _unserialize()') + elif field.type.is_list: + # list with fixed number of elements + # length of array = sizeof(arrayElementType) * nmemb + length += " * %d" % field.type.nmemb + # use memcpy because C cannot assign whole arrays with operator= + value = ' memcpy(%s, xcb_tmp, %s);' % (abs_field_name, length) + elif 'serialize' == context: value = ' xcb_parts[xcb_parts_idx].iov_base = (char *) ' From 277ea629def6728c9d826ff88e95b31c3e25915f Mon Sep 17 00:00:00 2001 From: Christian Linhart Date: Thu, 21 Aug 2014 22:35:55 +0200 Subject: [PATCH 2/4] generator: support lists of structs which contain a switch This essentially requires to have a correct sizeof-function for the struct. This in turn requires a sizeof-function for the switch, too. Making a sizeof-function for the switch is triggered by replacing "elif" by "if" in the first change of this patch. This way, c_need_sizeof is also set to True for switches if appropriate. The _c_serialize_helper_switch_field function has to support the context "sizeof": This is done in the second change of this patch The third change of this patch fixes an alignment error: It does not make sense to base the padding on the struct-type which is generated for switch because this struct does not represent the protocol. Rather it is the output of deserialization. ( The implicit padding for var-sized fields has other issues, IMHO, but I am not touching these now...) The effect on the generated code for the current xml-files is as follows: * several additional sizeof-functions are generated * the fix of the alignment error only changes one place in the XKB-extension for the GetKbdByName-reply. This is no problem because that reply in its current form is broken/unfinished anyways. Note: This patch also fixes a problem in the generator when a fixed-size list is the last field of a case or bitcase. Message-ID: <1408653356-21191-2-git-send-email-chris@demorecorder.com> Patch-Thread-Subject: [Xcb] xinput:QueryDeviceState: full-support: generator and xml-changes Patch-Set: QueryDeviceState Patch-Number: libxcb 2/3 Patch-Version: V1 Signed-off-by: Christian Linhart Reviewed-By: Ran Benita --- src/c_client.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/c_client.py b/src/c_client.py index 6f8f8f2..1d1bbf6 100644 --- a/src/c_client.py +++ b/src/c_client.py @@ -358,7 +358,8 @@ def _c_type_setup(self, name, postfix): field.c_pointer = '*' field.c_field_const_type = 'const ' + field.c_field_type self.c_need_aux = True - elif not field.type.fixed_size() and not field.type.is_case_or_bitcase: + + if not field.type.fixed_size() and not field.type.is_case_or_bitcase: self.c_need_sizeof = True field.c_iterator_type = _t(field.field_type + ('iterator',)) # xcb_fieldtype_iterator_t @@ -770,6 +771,10 @@ def _c_serialize_helper_switch_field(context, self, field, c_switch_variable, pr elif context in ('unserialize', 'unpack'): length = "%s(xcb_tmp, %s&%s%s)" % (field.type.c_unpack_name, c_field_names, prefix_str, field.c_field_name) + elif 'sizeof' == context: + # remove trailing ", " from c_field_names because it will be used at end of arglist + my_c_field_names = c_field_names[:-2] + length = "%s(xcb_tmp, %s)" % (field.type.c_sizeof_name, my_c_field_names) return length # _c_serialize_helper_switch_field() @@ -1046,7 +1051,12 @@ def _c_serialize_helper_fields(context, self, code_lines.append('%s xcb_parts_idx++;' % space) count += 1 - code_lines.append('%s xcb_align_to = ALIGNOF(%s);' % (space, 'char' if field.c_field_type == 'void' else field.c_field_type)) + code_lines.append( + '%s xcb_align_to = ALIGNOF(%s);' + % (space, + 'char' + if field.c_field_type == 'void' or field.type.is_switch + else field.c_field_type)) need_padding = True if self.c_var_followed_by_fixed_fields: From a7c75be5b1e2da32f7ce2c255972178e7d4b7598 Mon Sep 17 00:00:00 2001 From: Christian Linhart Date: Tue, 9 Sep 2014 23:26:40 +0200 Subject: [PATCH 3/4] generator: fix align-pads for switches which start at unaligned pos 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 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 Message-ID: <1410298000-24734-1-git-send-email-chris@demorecorder.com> Patch-Thread-Subject: [Xcb] xinput:QueryDeviceState: full-support: generator and xml-changes Patch-Set: QueryDeviceState Patch-Number: libxcb 4/4 Patch-Version: V3 Signed-off-by: Christian Linhart Reviewed-By: Ran Benita --- 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 @@ -657,9 +657,15 @@ def get_serialize_params(context, self, buffer_var='_buffer', aux_var='_aux'): 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) @@ -677,6 +683,8 @@ def _c_serialize_helper_insert_padding(context, code_lines, space, postpone): 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 @@ -1005,13 +1013,15 @@ def _c_serialize_helper_fields(context, self, # Variable length pad is 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, @@ -1100,7 +1110,7 @@ def _c_serialize_helper(context, complex_type, 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() @@ -1170,6 +1180,8 @@ def _c_serialize(context, self): _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' @@ -1193,6 +1205,8 @@ def _c_serialize(context, self): _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] @@ -1210,6 +1224,8 @@ def _c_serialize(context, self): 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' From fdb291b414a7afc2c2326124b8ca11d6846cd7b9 Mon Sep 17 00:00:00 2001 From: Christian Linhart Date: Wed, 3 Sep 2014 10:10:49 +0200 Subject: [PATCH 4/4] no typename for nested structs Nested structs which are generated for named case and bitcase do not get a typename anymore, i.e., they are anonymous structs. Reasons for this change: * Prior typenames have caused nameclashes * Prior typenames introduced names in the global namespace which did not start with the xcb prefix. This change is safe with respect to API compatibility because: I have searched for instances of named bitcases and there's only one place where they are used, and that's in xkb.xml: reply GetKbdByName. ( no need to search for because it was introduced after the last release ) The reply GetKbdByName is broken in its current form in the xkb.xml anyways, so it is most probably not used anywhere. So, my conclusion is that we can safely omit named types for nested structs. No need for an attribute. Message-ID: <1409731849-51897-1-git-send-email-chris@demorecorder.com> Patch-Thread-Subject: Re: [Xcb] names of nested structs of named bitcase/case are prone to nameclashes. Solution? Patch-Set: NestedStructTypenames Patch-Number: libxcb 1/1 Patch-Version: V1 Signed-off-by: Christian Linhart Reviewed-By: Ran Benita --- src/c_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/c_client.py b/src/c_client.py index 0a1f877..56a1766 100644 --- a/src/c_client.py +++ b/src/c_client.py @@ -1804,7 +1804,7 @@ def _c_complex(self, force_packed = False): for b in self.bitcases: space = '' if b.type.has_name: - _h(' struct _%s {', b.c_field_name) + _h(' struct {') space = ' ' for field in b.type.fields: _c_complex_field(self, field, space)