Fwd: Re: [OPW] Generated swapping and size-checking code integration
Asal Mirzaieva
asalle.kim at gmail.com
Tue Mar 10 11:47:52 PDT 2015
Good afternoon all,
Yesterday was the last official day of my internship and I am here to
report about work I've done and also to respond about fixes according to
previous letter from Peter Hutterer.
I worked and tested mainly on xproto and shape extensions, but almost
all the extensions are covered with existing code, excluding such
complex ones, as xinput and xkb.
The main goal of internship was to implement and size checking and
byteswapping code to xserver.
The generator of code is in proto/ directory and is called
gen_swap_check.py. It takes as an arguments path to directory where the
generated c-files will be stored and an xml-file to generate from.
So far, the generator supports:
1. Requests
and
* lists (of both variable size and fixed size)
* simple fields (as CARD32)
* pads (not completely)
* switches
* structs
within requests
2. Enums
3. Structs
valueparams and unions are deprecated and, thanks to Jaya Tiwari and
Christian Linhart, are no more used in xproto.
Errors and events handlers are not yet implemented.
Integration into xserver consisted in refactoring the Xext/shape.c so it
used the generated code.
We tested the generated code with SPARC machine and checking works OK,
but there is some bug with byte-swapping, I am working on.
I attached the diff with the newest changes to the reply to this letter
because these two take more than 100 KB together and sending such a big
letter requires moderator approval. You can also check the bitbucket
<https://bitbucket.org/AsalleKim/gen_swap_check> repo on branch master.
I inserted some comments below:
>> Here is the link tobitbucket <bitbucket.org/AsalleKim/gen_swap_check>. I
>> didn't create patch, because in my opinion it's pretty inconvenient to send
>> patches with code written from scratch, though it's a powerful tool for
>> maintaining already existing code. The repo contains forked xserver from
>> main xserver repo<http://cgit.freedesktop.org/xorg/xserver/>. It uses some
>> changes in xcb made by Christian Linhart and Jaya Tiwari, the changes are in
>> pending patches and will be soon applied (if they are not already). This
>> mainly touches the valueparam to switch replacement in xproto.xml.
>> As valueparam is deprecated, the generator, I am working on, does not
>> support it.
> I mentioned this in private email already, but for the archives: next time
> please submit a git-formatted patch. If you're not quite ready yet submit it
> as [RFC PATCH] ... so people know immediately that this is still work in
> progress.
OK. Sorry for inconvenience.
>
>> Generator's code can process almost all extensions, but I am now mainly
>> working on Shape and Xproto.
>> For now it creates source file and header file in the directory proto/gen
>> and then they are built with automake (proto/Makefile.am).
>>
>> There are several things I was not sure about.
>> 1. The working name of the swapping and size-checking functions generator is
>> gen_swap_check, which is rather awkward. But I couldn't invent anything
>> better.
>>
>> 2. To test the code I need to integrate it to the xserver. I already made
>> some changes into Xext/shape.c so it uses the generated code now. The
>> Xext/shape.h must include the swapcheck_shape.h (generated one). Would it be
>> okay if I copy all generated headers to {installdir}/include/xcb, mixing the
>> genswap-generated code and the regular code, generated by c_client.py? And
>> copy the compiled swapcheck_* files to {installdir}/lib.
> I don't think you'd want to install it to $installdir, rather it should be
> written to $builddir (which you already do) and then used from there, i.e.
> add the directory to CORE_INCS in configure.ac
>
>> 3. proto/Makefile.am is written in such way that no make targets are
>> declared. How can I add cp commad to it?
> depends, it's not clear to me what you want to copy, maybe you can comment
> on this?
To two previous comments: I understood that there should be no copying,
I just added generated-headers-and-libs-path to Xext_INCS and Xext_LIBS
respectively. I guess this is how it should look like.
>
>> 4. When building several warnings in included file os.h appeared. How can I
>> get rid of them? Or should I ignore them?
>>
>> /home/asalle/xorg/Debug2/test-install/include/xorg/os.h:541:1: warning: redundant redeclaration of 'strndup' [-Wredundant-decls]
>> 1055 strndup(const char *str, size_t n);
> don't worry about these. unless warnings are added by your code you can
> ignore them (well, or fix them if you have the time and motivation)
> always helps to switch back to the master branch to check if a warning is
> there.
OK.
>> This is the short overview of swapping and size-checking functions
>> generator,
>> I would be glad to hear your comments and messages about found bugs,
>> Asalle
> ok, I ran a diff to make the review a bit easier. Most of the diff is in the
> parser code anyway so that won't change. see my comments in-line
>
>> diff --git a/.gitignore b/.gitignore
>> index dc56b46..b45e8cb 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -80,3 +80,6 @@ core
>> doltcompile
>> doltlibtool
>> xserver.ent
>> +
>> +# vim swap files
>> +*.swp
> please submit this as separate commit.
As far as I understand, it's already done (link
<https://bitbucket.org/AsalleKim/gen_swap_check/commits/77bb0d891e391d1af02837d88c030af097ed65e8>).
>
>> diff --git a/Makefile.am b/Makefile.am
>> index f0fa2d8..1c214b4 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -42,6 +42,7 @@ SUBDIRS = \
>> dix \
>> fb \
>> mi \
>> + proto \
>> Xext \
>> miext \
>> os \
>> @@ -62,7 +63,7 @@ SUBDIRS = \
>> $(GLAMOR_DIR) \
>> config \
>> hw \
>> - test
>> + test
> please drop this hunk. There's a couple of whitespace issues in the various
> hunks, I'll point some of them out but double check the whole diff please.
> trailing whitespaces can be removed with s/[ ]*$// in vim.
>
> you should also configure your vimrc to highlight such issues, it makes life
> a bit easier:
> let c_space_errors=1
Thanks for vim-hint. I fixed it on all hunks.
>>
>> if XORG
>> aclocaldir = $(datadir)/aclocal
>> @@ -116,7 +117,8 @@ DIST_SUBDIRS = \
>> dri3 \
>> present \
>> hw \
>> - test
>> + test \
>> + proto
>>
>> # gross hack
>> relink: all
>> diff --git a/Xext/Makefile.am b/Xext/Makefile.am
>> index a9a4468..92a3b72 100644
>> --- a/Xext/Makefile.am
>> +++ b/Xext/Makefile.am
>> @@ -1,6 +1,7 @@
>> noinst_LTLIBRARIES = libXext.la libXextdpmsstubs.la
>>
>> -AM_CFLAGS = $(DIX_CFLAGS)
>> +
>> +AM_CFLAGS = $(DIX_CFLAGS) $(PROTO_CFLAGS)
> see my XCBPROTO_CFLAGS comment in the configure.ac hunk
I guess, everything is OK here, because we cannot simply replace
PROTO_CFLAGS by XCBPROTO_CFLAGS, because PROTO_CFLAGS contains also
XSERVER_CFLAGS beside the XCBPROTO_CFLAGS. Or had I misunderstood anything?
>
>> if XORG
>> sdk_HEADERS = xvdix.h xvmcext.h geext.h geint.h shmint.h syncsdk.h
>> diff --git a/Xext/shape.c b/Xext/shape.c
>> index bb479b1..54e71d6 100644
>> --- a/Xext/shape.c
>> +++ b/Xext/shape.c
>> @@ -1025,9 +1025,25 @@ ProcShapeGetRectangles(ClientPtr client)
>> return Success;
>> }
>>
>> +#include "xcb/swapcheck_shape.h"
>> +
>> static int
>> ProcShapeDispatch(ClientPtr client)
>> {
>> + REQUEST(xReq);
>> + if(xcb_shape_Check_dispatch(client) == Success)
>> + {
>> + return ProcShapeDispatch_Unchecked(client);
>> + }
>> + else
>> + {
>> + return BadLength;
>> + }
>> +}
> couple of coding style issues:
> { on same line as the condition
> space after if, before the actual condition.
> { on same line as else please (but not })
> xserver coding style is indent of 4 spaces, no tabs.
>
> Please fix this in all hunks.
Fixed
>
>> +
>> +static int
>> +ProcShapeDispatch_Unchecked(ClientPtr client)
>> +{
>> REQUEST(xReq);
>> switch (stuff->data) {
>> case X_ShapeQueryVersion:
>> @@ -1073,6 +1089,22 @@ ProcShapeDispatch(ClientPtr client)
>> }
>> }
>>
>> +static int
>> +SProcShapeDispatch(ClientPtr client)
>> +{
>> + REQUEST(xReq);
>> +
>> + if(xcb_shape_SwapFromClient_dispatch(client))
>> + {
>> + return ProcShapeDispatch_Unchecked(client);
>> + }
>> + else
>> + {
>> + return BadLength;
>> + }
>> +}
>> +
>> +
>> static void
>> SShapeNotifyEvent(xShapeNotifyEvent * from, xShapeNotifyEvent * to)
>> {
>> diff --git a/configure.ac b/configure.ac
>> index 96524c5..ef9f0bc 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -33,6 +33,10 @@ AC_CONFIG_SRCDIR([Makefile.am])
>> AC_CONFIG_MACRO_DIR([m4])
>> AM_INIT_AUTOMAKE([foreign dist-bzip2])
>> AC_USE_SYSTEM_EXTENSIONS
>> +AM_PATH_PYTHON([2.7])
>> +
>> +
>> +
> no spare empty lines please (in hunk above too)
Fixed.
>>
>> # Require xorg-macros minimum of 1.14 for XORG_COMPILER_BRAND in XORG_DEFAULT_OPTIONS
>> m4_ifndef([XORG_MACROS_VERSION],
>> @@ -1779,6 +1783,31 @@ AC_MSG_RESULT([yes])], AC_MSG_RESULT([no]))
>> XSERVER_CFLAGS="$XSERVER_CFLAGS $CORE_INCS $XEXT_INC $COMPOSITE_INC $DAMAGE_INC $FIXES_INC $XI_INC $MI_INC $MIEXT_SYNC_INC $MIEXT_SHADOW_INC $MIEXT_LAYER_INC $MIEXT_DAMAGE_INC $RENDER_INC $RANDR_INC $FB_INC $DBE_INC $PRESENT_INC"
>>
>> dnl ---------------------------------------------------------------------------
>> +dnl proto section.
>> +dnl ---------------------------------------------------------------------------
>> +
> run PKG_CHECK_MODULES for xcb-proto here so you can be sure it exists before
> you access the various variables.
Fixed.
>> +# Find the xcb-proto protocol descriptions
>> +AC_MSG_CHECKING(XCBPROTO_XCBINCLUDEDIR)
>> +XCBPROTO_XCBINCLUDEDIR=`$PKG_CONFIG --variable=xcbincludedir xcb-proto`
>> +AC_MSG_RESULT($XCBPROTO_XCBINCLUDEDIR)
>> +AC_SUBST(XCBPROTO_XCBINCLUDEDIR)
> quick check shows that xcbincludedir was added in 2006, you can just grab it
> directly and skip the AC_MSG_CHECKING/AC_MSG_RESULT
I haven't found any. There are some random code in configure.ac, line
2006. I also searched the whole dir with grep and found no other
'xcbincludedir' entries. Could you please point out what 2006 do you mean?
Anyways, I removed the re-checking lines.
>> +
>> +# Find the xcb-proto version
>> +XCBPROTO_VERSION=`$PKG_CONFIG --modversion xcb-proto`
>> +AC_SUBST(XCBPROTO_VERSION)
> you don't seem to use this one anywhere, is this still needed?
I guess, we don't. So I Removed them.
>> +
>> +# Find the xcbgen Python package
>> +AC_MSG_CHECKING(XCBPROTO_XCBPYTHONDIR)
>> +XCBPROTO_XCBPYTHONDIR=`$PKG_CONFIG --variable=pythondir xcb-proto`
>> +AC_MSG_RESULT($XCBPROTO_XCBPYTHONDIR)
>> +AC_SUBST(XCBPROTO_XCBPYTHONDIR)
> likewise, pythondir was added in 2008, just require a new-ish version and
> skip the CHECKING/RESULTS
Couldn't find other mentions of variable 'pythondir' in line 2008 of
configure.ac or anywhere else, as in previous hunk.
>> +
>> +# CFLAGS
>> +libxcb_inc=`$PKG_CONFIG --cflags xcb`
>> +PROTO_CFLAGS="$XSERVER_CFLAGS $libxcb_inc"
>> +AC_SUBST([PROTO_CFLAGS])
> if you use PKG_CHECK_MODULES([XCBPROTO], [xcb-proto]) it will automatically
> supply XCBPROTO_CFLAGS and XCBPROTO_LIBS. no extra work needed.
Fixed.
>
>> +
>> +dnl ---------------------------------------------------------------------------
>> dnl DDX section.
>> dnl ---------------------------------------------------------------------------
>>
>> @@ -2546,6 +2575,7 @@ xfixes/Makefile
>> exa/Makefile
>> dri3/Makefile
>> present/Makefile
>> +proto/Makefile
>> hw/Makefile
>> hw/xfree86/Makefile
>> hw/xfree86/Xorg.sh
>> diff --git a/proto/.gitignore b/proto/.gitignore
>> new file mode 100644
>> index 0000000..cc4611b
>> --- /dev/null
>> +++ b/proto/.gitignore
>> @@ -0,0 +1,5 @@
>> +# Python "compiled" files
>> +*.pyc
>> +
>> +# Autogenerated by gen_swap_check.py in proto files
>> +gen/*
>> diff --git a/proto/Makefile.am b/proto/Makefile.am
>> new file mode 100644
>> index 0000000..b2d5768
>> --- /dev/null
>> +++ b/proto/Makefile.am
>> @@ -0,0 +1,19 @@
>> +noinst_LTLIBRARIES = libproto.la
>> +
>> +AM_CFLAGS = $(PROTO_CFLAGS)
>> +
>> +prefix='gen/swapcheck_'
> I think "generated" instead of "gen" is better here. It's not like we need
> to type this often so spelling it out makes it quicker to understand.
Fixed.
>> +
>> +EXTSOURCES = \
>> + gen/swapcheck_xproto.c \
>> + gen/swapcheck_shape.c
> huh, I had to google to check if EXTSOURCES was a special automake variable.
> I'd recommend writing it lowercase or even "extension_sources" to make this
> more obvious.
Fixed.
>> +
>> +libproto_la_SOURCES = $(EXTSOURCES)
>> +
>> +ech = $(subst gen/swapcheck_, ,$(@))
> "ech"? I wonder what that stand for... :)
Silly mistake, I changed it to 'sources'
>
>> +
>> +$(EXTSOURCES): $(XCBPROTO_XCBINCLUDEDIR)/$(ech:.c=.xml) $(srcdir)/gen_swap_check.py
>> +
>> + $(AM_V_GEN)$(PYTHON) $(srcdir)/gen_swap_check.py \
>> + -p $(XCBPROTO_XCBPYTHONDIR) \
>> + $(XCBPROTO_XCBINCLUDEDIR)/$(ech:.c=.xml)
>> diff --git a/proto/gen_swap_check.py b/proto/gen_swap_check.py
>> new file mode 100644
>> index 0000000..b1937c2
>> --- /dev/null
>> +++ b/proto/gen_swap_check.py
>> @@ -0,0 +1,753 @@
>> +#!/usr/bin/env python
>> +import getopt
>> +import os
>> +import sys
>> +import errno
>> +import re
>> +
>> +def _i():
>> + '''
>> + Indent function
>> + '''
>> + return ''.join(_indent)
>> +
>> +#Helper functions and definitions copy-pasted from c_client.py, to replace with xcbutils.py
> I'm struggling to find any reference to xcbutils.py in google. is this
> something proposed on the xcb list? or a TODO for this patch?
It was an idea of file, that stores common for c_client.py and
gen_swap_check.py proposed by me, as Christian Linhart has already
clarified. We are still disscussing if it's really necessary to create
new dependencies in order to enhance code utilization.
>> +
>> +def _h(fmt, *args):
>> + '''
>> + Writes the given line to the header file.
>> + '''
>> + _hlines[_hlevel].append(fmt % args)
>> +
>> +def _c(fmt, *args):
>> + '''
>> + Writes the given line to the source file.
>> + '''
>> + _clines[_clevel].append(fmt % args)
>> +
>> +def _hc(fmt, *args):
>> + '''
>> + Writes the given line to both the header and source files.
>> + '''
>> + _h(fmt, *args)
>> + _c(fmt, *args)
>> +
>> +def _c_wr_stringlist(indent, strlist):
>> + '''
>> + Writes the given list of strings to the source file.
>> + Each line is prepended by the indent string
>> + '''
>> + for str in strlist:
>> + _c("%s%s", indent, str)
> can you pop a comment in where the copy/pasted helper functions stop? I suspect here,
> but a comment makes this more obvious
There is a comment, I made it even more obvious, as it turned out to be
misunderstood.
> # Copy-pasted from c_client.py code ends here+
> +# Some hacks to make the API more readable and to keep backwards compability
> +_cname_special_cases = {'DECnet':'decnet'}
> +_extension_special_cases = ['XPrint', 'XCMisc', 'BigRequests']
> +_cname_re = re.compile('([A-Z0-9][a-z]+|[A-Z0-9]+(?![a-z])|[a-z]+)')
> please add a comment to describe what the regex should do (roughly anyway).
> I can probably figure this out with a bit of time, but then I likely still
> won't know whether it's what you intended to do :)
>
>> +
>> +_hlines = []
>> +_hlevel = 0
>> +_clines = []
>> +_clevel = 0
>> +_ns = None
>> +_filename = ''
>> +requests = {}
>> +_indent = []
> huh, I know that python uses _ to try to hide things, it's not clear here
> why requests doesn't do this.
> Also, afaict this is just a standalone program, not a library. I don't think
> we need to worry about visibility anyway.
I used underscore to hide global variables, because I read that it's a
good tone and to follow c_client.py's naming traditions (with 'requests'
I just forgot to name it properly). But, when thinking about separating
reused code to a different file, the main reason I've not done it yet is
that under-scored named vars cannot be imported via import command, they
are visible only within their file. Still don't know if it's a feature
or a bug.
>
>> +
>> +def _increase_indent():
>> + global _indent
>> + _indent.append(' ')
>> +
>> +def _decrease_indent():
>> + global _indent
>> + if _indent: # if not empty
>> + _indent.pop()
>> +
>> +# XXX See if this level thing is really necessary.
>> +def _h_setlevel(idx):
>> + '''
>> + Changes the array that header lines are written to.
>> + Supports writing different sections of the header file.
>> + '''
>> + global _hlevel
>> + while len(_hlines) <= idx:
>> + _hlines.append([])
>> + _hlevel = idx
>> +
>> +def _c_setlevel(idx):
>> + '''
>> + Changes the array that source lines are written to.
>> + Supports writing to different sections of the source file.
>> + '''
>> + global _clevel
>> + while len(_clines) <= idx:
>> + _clines.append([])
>> + _clevel = idx
>> +
>> +def _n_item(str):
> "n_item" usually refers to "number of items". can we rename this into
> something more expressive?
> same goes for _ext, _n and _t, no idea what they do based on the function
> name alone.
>
>> + '''
>> + Does C-name conversion on a single string fragment.
> an example of the conversion would be nice here (same below)
This issue and the one above are about c_client.py code that I use, as
C. Linhart said before.
>
>> + Uses a regexp with some hard-coded special cases.
>> + '''
>> + if str in _cname_special_cases:
>> + return _cname_special_cases[str]
>> + else:
>> + split = _cname_re.finditer(str)
>> + name_parts = [match.group(0) for match in split]
>> + return '_'.join(name_parts)
>> +
>> +def _ext(str):
>> + '''
>> + Does C-name conversion on an extension name.
>> + Has some additional special cases on top of _n_item.
>> + '''
>> + if str in _extension_special_cases:
>> + return _n_item(str).lower()
>> + else:
>> + return str.lower()
>> +
>> +def _n(list):
>> + '''
>> + Does C-name conversion on a tuple of strings.
>> + Different behavior depending on length of tuple, extension/not extension, etc.
>> + Basically C-name converts the individual pieces, then joins with underscores.
>> + '''
>> + if len(list) == 1:
>> + parts = list
>> + elif len(list) == 2:
>> + parts = [list[0], _n_item(list[1])]
>> + elif _ns.is_ext:
>> + parts = [list[0], _ext(list[1])] + [_n_item(i) for i in list[2:]]
>> + else:
>> + parts = [list[0]] + [_n_item(i) for i in list[1:]]
>> + return '_'.join(parts).lower()
>> +
>> +def _t(list):
>> + '''
>> + Does C-name conversion on a tuple of strings representing a type.
>> + Same as _n but adds a "_t" on the end.
>> + '''
>> + if len(list) == 1:
>> + parts = list
>> + elif len(list) == 2:
>> + parts = [list[0], _n_item(list[1]), 't']
>> + elif _ns.is_ext:
>> + parts = [list[0], _ext(list[1])] + [_n_item(i) for i in list[2:]] + ['t']
>> + else:
>> + parts = [list[0]] + [_n_item(i) for i in list[1:]] + ['t']
>> + return '_'.join(parts).lower()
>> +
>> +#to replace part ends here
>> +
>> +
>> +# Fuctions required by output
> typo, Functions
>
>> +
>> +def c_open(self):
>> + '''
>> + Exported function that handles module open.
>> + Opens the files and writes out the auto-generated comment,
>> + header file includes, etc.
>> + '''
> tab/space indentation mixup
>
>> + global _ns
>> + _ns = self.namespace
>> + _ns.c_ext_global_name = _n(_ns.prefix + ('id',))
>> +
>> + global _filename
>> + _filename = ''.join(('swapcheck_',_ns.header))
>> +
>> + _h_setlevel(0)
>> + _c_setlevel(0)
>> +
>> + _hc('/*')
>> + _hc(' * This file generated automatically from %s by gen_swap_check.py.', _ns.file)
>> + _hc(' * Edit at your peril.')
>> + _hc(' */')
>> + _hc('')
>> +
>> + _h('/**')
>> + _h(' * @defgroup XCB_%s_API XCB %s API', _ns.ext_name, _ns.ext_name)
>> + _h(' * @brief %s XCB Protocol Implementation.', _ns.ext_name)
>> + _h(' * @{')
>> + _h(' **/')
>> + _h('')
>> + _h('#ifndef __%s_H', _ns.header.upper())
>> + _h('#define __%s_H', _ns.header.upper())
>> + _h('')
>> +
>> + # swapcheck_xproto.h is include in all the others' extensions header files, so
>> + # it's very convenient to include the common libs into this header
>> + if _ns.header == 'xproto':
>> + _h('#include "xorg/misc.h"')
>> + _h('#include "X11/X.h"')
>> + _h('#include "X11/Xproto.h"')
>> + else:
>> + _hc('#include "swapcheck_xproto.h"')
>> +
>> + _c('#include "%s.h"', _filename)
>> + _c('#include <stdlib.h>')
>> + _c('#include <assert.h>')
>> + _c('#include <stddef.h> /* for offsetof() */')
>> + _c('#include <errno.h>')
>> +
>> + _c('')
>> + _c('#define ALIGNOF(type) offsetof(struct { char dummy; type member; }, member)')
>> +
>> + _h('')
>> + _h('#ifdef __cplusplus')
>> + _h('extern "C" {')
>> + _h('#endif')
>> +
>> + if _ns.is_ext:
>> + _h('')
>> + _h('#define XCB_%s_MAJOR_VERSION %s', _ns.ext_name.upper(), _ns.major_version)
>> + _h('#define XCB_%s_MINOR_VERSION %s', _ns.ext_name.upper(), _ns.minor_version)
>> + _h('') #XXX
>> + #_h('extern xcb_extension_t %s;', _ns.c_ext_global_name)
>> +
>> + _c('')
>> + #_c('xcb_extension_t %s = { "%s", 0 };', _ns.c_ext_global_name, _ns.ext_xname)
>> +
>> + _hc('')
>> +
>> +def c_close(self):
>> + '''
>> + Exported function that handles module close.
>> + Writes out all the stored content lines, then closes the files.
>> + '''
>> + _c_setlevel(0)
>> + for reqnum in requests:
>> + _c('#define %s %s', 'GEN_' + _n(requests[reqnum]).upper(), reqnum)
>> + _c('')
>> +
>> + _h_setlevel(2)
>> + _c_setlevel(2)
>> + _hc('')
>> +
>> + for kind in swapCheckDescendants:
>> + generate_dispatch(kind(RequestHandler()), self.namespace.header)
>> +
>> + _h('')
>> + _h('#ifdef __cplusplus')
>> + _h('}')
>> + _h('#endif')
>> +
>> + _h('')
>> + _h('#endif')
>> + _h('')
>> + _h('/**')
>> + _h(' * @}')
>> + _h(' */')
>> +
>> + # Ensure the gen subdirectory exists
>> + _gendir = 'gen'
> shouldn't this come from the Makefile?
> or at last be defined at the top somewhere so it's easy to find.
Moved it to proto/Makefile.am.
>
>> + try:
>> + os.mkdir(_gendir)
>> + except OSError as e:
>> + if e.errno != errno.EEXIST:
>> + raise
>> +
>> + # Write header file
>> + hfile = open('%s/%s.h' % (_gendir, _filename), 'w')
>> + for list in _hlines:
>> + for line in list:
>> + hfile.write(line)
>> + hfile.write('\n')
>> + hfile.close()
>> +
>> + # Write source file
>> + cfile = open('%s/%s.c' % (_gendir, _filename), 'w')
>> + for list in _clines:
>> + for line in list:
>> + cfile.write(line)
>> + cfile.write('\n')
>> +
>> + cfile.close()
>> +
>> +def c_simple(self, name):
>> + '''
>> + Exported function that handles cardinal type declarations.
>> + These are types which are typedef'd to one of the CARDx's, char, float, etc.
>> + '''
>> + #print(name)
>> + #print(self)
> can we drop the functions that don't do anything? Or at least comment why
> they don't do anything?
They still need an implementation, I will be working on them in couple
of days. Commented as 'needs a future implementation'.
>> +
>> +
>> +def c_enum(self, name):
>> + '''
>> + Exported function that handles enum declarations.
>> +
>> + Private fields:
>> + * fields dictonary contains (enum_number -> enum_name) pair, which
>> + represents enum entry
>> + '''
>> + _fields = {}
>> +
>> + if(self.values):
>> + for entry in self.values:
>> + _fields[entry[1]] = entry[0]
>> + else: #if self.bits
>> + for entry in self.bits:
>> + _fields[2**entry[1]] = entry[0] # dunno if it's right
>> +
>> + _c('%stypedef enum %s {', _i(), _n(name))
>> + _increase_indent()
>> + for enum_num in _fields:
>> + _c('%s%s = %s,', _i(), (_n(name)+'_'+_fields[enum_num]).upper(), enum_num)
>> +
>> + _decrease_indent()
>> + _c('%s} %s_t;', _i(), _n(name))
>> + _c('')
>> +
>> +def c_struct(self, name):
>> + '''
>> + Exported function that handles struct declarations.
>> + '''
>> + _h_setlevel(1)
>> + _c_setlevel(1)
>> +
>> + for kind in swapCheckDescendants:
>> + _swapcheck = kind(StructHandler())
>> + _swapcheck.generate(self, name)
>> +
>> +def c_union(self, name):
>> + '''
>> + Exported function that handles union declarations (union is deprecated)
>> + '''
>> + #print(name)
>> +
>> +class SwapCheck:
>> + '''
>> + Represent abstract swapper/checker, that generates appropriate c-code
>> +
>> + Created combined because these classes share a lot functions
>> + _name is hardcoded literal that represents name of it's class, is used to concatenate it with other stuff to generate functions
>> + _docheck is bool that determines accurate position to define afterEnd by calling determine_afterEnd
>> + '''
>> + _name = None
>> + _docheck = False
>> + _reusedvars = []
>> + _typeHandler = None
>> + _has_struct = False
>> +
>> + def __init__(self, typeHandler):
>> + self._typeHandler = typeHandler
>> +
>> + def access_check( self, size ):
>> + if self._docheck:
>> + _c('%sif( p + %u > (uint8_t*)afterEnd)', _i(), size)
>> + _c('%s{', _i())
>> + _increase_indent()
>> + _c('%sreturn BadLength;', _i())
>> + _decrease_indent()
>> + _c('%s}', _i())
>> +
>> + def process_fieldvalue(self, fname, ftype):
>> + self._docheck = self._typeHandler.determine_afterEnd(self._docheck, fname)
>> + if(fname in self._reusedvars):
>> + _varname = 'fieldvalue' + '_' + ''.join(fname)
>> + _datatype = _n(ftype.name)
>> + _c('%s%s %s;', _i(), _datatype, _varname)
>> + _c('%s%s = *(%s*)p;', _i(), _varname, _datatype)
>> +
>> + def funcName(self, name):
>> + return '_'.join(name) + '_' + self.nameToString()
>> +
>> + def printHeader(self, type):
>> + _hc('int')
>> + self._typeHandler.printSwapCheckFuncSignature(self.funcName(type.name))
>> + _increase_indent()
>> + _c('%s//type, field_type, field_name, visible, wire, auto, enum, isfd', _i())
>> + _c('')
>> + _c('%suint8_t* p = (uint8_t*)data;', _i())
>> +
>> + if self._has_struct:
>> + self._typeHandler.initAfterStruct()
>> + self._typeHandler.init_afterEnd()
>> +
>> + _c('')
>> +
>> + def printFieldHeader(self, field):
>> + _c('%s//%s, %s, %s, %s, %s, %s, %s', _i(),
>> + field.field_type,
>> + field.field_name,
>> + field.visible,
>> + field.wire,
>> + field.auto,
>> + field.enum,
>> + field.isfd)
> whoah, what happened here? that's a bit excessive on the indenting
I hope, it is more good-looking now :).
>> +
>> + def process_simple(self, fname, ftype):
>> + pass
>> +
>> + def process_list(self, ftype, it, llen):
>> + _c('%s{', _i())
>> + _increase_indent()
>> + _c('%sunsigned int %s;', _i(), it)
>> +
>> + _c('%sfor(%s = 0; %s < %s; %s++)', _i(), it, it, llen, it)
>> + _c('%s{', _i())
>> +
>> + _increase_indent()
>> + self.check(ftype.member, None)
>> + _decrease_indent()
>> +
>> + _c('%s}', _i())
>> + _decrease_indent()
>> + _c('%s}', _i())
>> +
>> + def process_struct(self, tname):
>> + self._typeHandler.process_struct(self.funcName(tname))
>> +
>> + def process_switch(self, ftype):
>> + _casevarn = 'fieldvalue_' + ftype.expr.lenfield_name # case variable name
>> + _c('\n%s//switch begins\n', _i())
>> +
>> + for case in ftype.bitcases:
>> + _eq_sign = '==' if case.type.is_case else '&'
>> +
>> + if case.type.expr: # if bitcase/case has enumref
>> + _enumn = case.type.expr[0].lenfield_type.name # enum name
>> + _enument = case.type.expr[0].lenfield_name # enum entry name #TODO WHAT IF NOT 0?
>> + _c('%sif (%s %s %s)', _i(),
>> + _casevarn, _eq_sign,
>> + (_n(_enumn)+'_'+_enument).upper())
>> + _c('%s{', _i())
>> + _increase_indent()
>> + for field in case.type.fields:
>> + _c('%s//%s %s', _i(), field.type.name, field.field_name)
>> + self.check(field.type, field.field_name)
>> + _decrease_indent()
>> + _c('%s}', _i())
>> + _c('')
>> +
>> + _c('%s//switch ends\n', _i())
>> +
>> + def check(self, ftype, fname):
>> + _size = ftype.size
>> + if ftype.is_simple or ftype.is_expr:
>> + self.process_simple(fname, ftype)
>> + _c('%sp += %u;', _i(), _size)
>> + elif ftype.is_pad and ftype.fixed_size():
>> + byts = ftype.nmemb
>> + _c('%sp += %u;', _i(), byts)
>> + elif ftype.is_pad and not ftype.fixed_size():
>> + al = ftype.align
>> + _c('%sp += %u;', _i(), al)
>> + elif ftype.is_list and ftype.fixed_size():
>> + self.process_list(ftype, 'i_'+fname, ftype.nmemb)
>> + elif ftype.is_list and not ftype.fixed_size():
>> + self.process_list(ftype,
>> + 'i_'+ftype.expr.lenfield_name,
>> + 'fieldvalue_' + ftype.expr.lenfield_name)
>> + elif ftype.is_switch:
>> + self.process_switch(ftype)
>> + elif ftype.is_container:
>> + self.process_struct(ftype.name)
>> + else:
>> + _c( '%s#error yet not implemented', _i())
>> +
>> + def generate(self, item, name):
>> + self._docheck = self._typeHandler.init_docheck(self._docheck)
>> + _afterEnd = None
>> +
>> + self._reusedvars = self.checkReusedVariables(item, [])
>> + for field in item.fields:
>> + if(self.isAfterStructNedeed(field.type)):
>> + break
>> + self.printHeader(item)
>> +
>> + for field in item.fields:
>> + self.printFieldHeader(field)
>> + self.check(field.type, field.field_name)
>> + self.printEmpty()
>> + self._typeHandler.printFooter()
>> + self._typeHandler.printReturn()
>> +
>> + def nameToString(self):
>> + return self._name
>> +
>> + def isAfterStructNedeed(self, ftype):
>> + '''
>> + recurce over every field in request to find out whether it contains structs
>> +
>> + to avoid afterEnd is declared but not used warning we iterate over all
>> + fields in the request and if it contains a single struct entry we must
>> + declare it, otherwise we must not.
>> + '''
>> + if ftype.is_list:
>> + self._has_struct = self.isAfterStructNedeed(ftype.member) # check if members of list are structs
>> + elif ftype.is_switch:
>> + for sfield in ftype.fields: # check if any field of switch is a struct
>> + if self.isAfterStructNedeed(sfield.type):
>> + self._has_struct = True
>> + elif ftype.is_container:
>> + self._has_struct = True
>> + else:
>> + self._has_struct = False
>> + return self._has_struct
>> +
>> + def printEmpty(self):
>> + _c('')
>> +
>> + def checkReusedVariables(self, _container, other):
>> + appendvars = []
>> + listvars = []
>> + othervars = other
>> + for field in _container.fields:
>> + if field.type.is_list or field.type.is_switch:
>> + listvars.append(field.type.expr.lenfield_name)
>> + if field.type.is_switch:
>> + appendvars.extend(self.checkReusedVariables(field.type, othervars))
>> + else:
>> + othervars.append(field.field_name)
>> + listvars = list(set(listvars) & set(othervars))
>> + listvars.extend(appendvars)
>> + return listvars
>> +
>> +class SwapParent(SwapCheck):
>> + '''
>> + Represents abstract class to generate toClient and fromClient swapping functions
>> + '''
>> + def process_simple(self, fname, ftype):
>> + self.before_swap_simplefield(fname, ftype) #before swap hook
>> + self.swap_simplefield(ftype.size)
>> + self.after_swap_simplefield(fname, ftype) #after swap hook
>> +
>> + def swap_simplefield(self, _size ):
>> + if _size == 1:
>> + pass
>> + elif _size == 2:
>> + self.access_check(_size )
>> + self.swap_with_swapper('swaps', 'uint16_t')
>> + elif _size ==4 :
>> + self.access_check( _size )
>> + self.swap_with_swapper('swapl', 'uint32_t')
>> + else:
>> + _c( ' #error swap of size %d not implemented', _size )
>> +
>> + def swap_with_swapper( self, swapper, size ):
>> + _c('%s%s((%s*)p);', _i(), swapper, size)
>> +
>> + def before_swap_simplefield( self, fname, ftype ):
>> + pass
>> +
>> + def after_swap_simplefield( self, fname, ftype ):
>> + pass
>> +
>> +class SwapFromClient(SwapParent):
>> + _name = 'SwapFromClient'
>> +
>> + def after_swap_simplefield( self, fname, ftype):
>> + self.process_fieldvalue( fname, ftype )
>> +
>> +class SwapToClient(SwapParent):
>> + _name = 'SwapToClient'
>> +
>> + def before_swap_simplefield( self, fname, ftype):
>> + self.process_fieldvalue( fname, ftype)
>> +
>> +class Check(SwapCheck):
>> + _name = 'Check'
>> +
>> + def process_simple(self, fname, _size):
>> + self.process_fieldvalue(fname, _size)
>> +
>> +class TypeHandler:
>> + def printFooter(self):
>> + pass
>> +
>> + def determine_afterEnd(self, _docheck, fname):
>> + pass
>> +
>> + def printReturn(self):
>> + _c(' return Success;')
>> + _c(' else')
>> + _c(' return BadLength;')
>> + _c('}')
>> + _decrease_indent()
>> + _hc('')
>> +
>> + def printSwapCheckFuncSignature(self, name):
>> + pass
>> +
>> + def init_afterEnd(self):
>> + pass
>> +
>> + def init_docheck(self, docheck):
>> + pass
>> +
>> + def initAfterStruct(self):
>> + pass
>> +
>> + def process_struct(self, name):
>> + pass
>> +
>> +class RequestHandler(TypeHandler):
>> + def printFooter(self):
>> + _c(' if (p == afterEnd)')
>> +
>> + def determine_afterEnd(self, _docheck, fname):
>> + """
>> + Overloaded function to return the appropriate value of _docheck,
>> +
>> + unlike the StructHandler does
>> + """
>> + if fname == 'length':
>> + _c(' afterEnd = ((uint8_t*)data) + 4 * ( *(uint16_t*)p );')
>> + return True
>> + else:
>> + return _docheck
>> + def printSwapCheckFuncSignature(self, name):
>> + _h('%s(void *data);', name)
>> + _c('%s(void *data)\n{', name)
>> +
>> + def init_afterEnd(self):
>> + _c(' uint8_t* afterEnd = NULL;')
>> +
>> + def init_docheck(self, docheck):
>> + return False
>> +
>> + def initAfterStruct(self):
>> + _c(' uint8_t* afterStruct = NULL;')
>> +
>> + def process_struct(self, name):
>> + _c('%sif(%s(p, afterEnd, &afterStruct) != Success)\n\t\t\treturn BadLength;', _i(), name)
>> + _c('%sp = afterStruct;', _i())
>> +
>> +class StructHandler(TypeHandler):
>> + def printFooter(self):
>> + _c(' *afterStruct = p;')
>> + _c(' if (p <= (uint8_t*)afterEnd)')
>> +
>> + def printSwapCheckFuncSignature(self, name):
>> + _h('%s(void *data, void *afterEnd, uint8_t **afterStruct);', name)
>> + _c('%s(void *data, void *afterEnd, uint8_t **afterStruct)\n{', name)
>> +
>> + def init_docheck(self, docheck):
>> + return True
>> +
>> + def process_struct(self, name):
>> + _c('%sif(%s(p, afterEnd, afterStruct) != Success)\n\t\t\treturn BadLength;', _i(), name)
>> + _c('%sp = (uint8_t*)(*afterStruct);', _i())
>> +
>> + def determine_afterEnd(self, _docheck, fname):
>> + """
>> + Overloaded function to return the same value as _docheck had,
>> +
>> + unlike the RequestHandler's one
>> + """
>> + return _docheck
>> +
>> +def generate_dispatch(kind, name):
>> + """
>> + Function to generate dispatch function
>> +
>> + kind is an object of class SwapCheck
>> + name is the name of extension
>> + """
>> + #print function header-signature
>> + _hc('%sint', _i())
>> + _h('%sxcb_%s_dispatch(void *req);', _i(), name+'_'+kind._name)
>> + _c('%sxcb_%s_dispatch(void *req)\n{', _i(), name+'_'+kind._name)
>> + _increase_indent()
>> +
>> + # init variables according to c90 std
>> + _c('%slong switch_item;', _i())
>> + _c('%sint return_val = 0;', _i())
>> + _c('%sxReq* request = (xReq*)req;', _i())
>> +
>> + # define varible to switch, depends on extension
>> + _switch_item = 'request->'
>> + _switch_item = _switch_item + 'reqType' if name == 'xproto' else _switch_item + 'data'
>> + _c('%sswitch_item = %s;', _i(), _switch_item)
>> +
>> + # print switch header
>> + _c('%sswitch(switch_item)', _i())
>> + _c('%s{', _i())
>> + _increase_indent()
>> +
>> + # for every opcode in generated (opcode -> name) dict "requests"
>> + # print case
>> + for reqnum in requests:
>> + _c('%scase %s:', _i(), 'GEN_'+_n(requests[reqnum]).upper())
>> + _increase_indent()
>> + _c('%sreturn_val = %s((void*)request);', _i(),
>> + kind.funcName(requests[reqnum]))
>> + _c('%sbreak;', _i())
>> + _decrease_indent()
>> +
>> + _c('%s}', _i())
>> + _decrease_indent()
>> + _c('%sreturn return_val;', _i())
>> + _decrease_indent()
>> + _c('%s}', _i())
>> +
>> +def c_request(self, name):
>> + '''
>> + Exported function that handles request declarations.
>> + '''
>> + _h_setlevel(1)
>> + _c_setlevel(1)
>> + global requests
>> + requests[self.opcode] = name
>> + for kind in swapCheckDescendants:
>> + swapcheck = kind(RequestHandler())
>> + swapcheck.generate(self, name)
>> +
>> +swapCheckDescendants = {SwapFromClient, SwapToClient, Check}
> move this to the top of the file somewhere pls
Christian already commented on this; I can add that, as the list
declaration should proceed the classes declarations, I can move the
whole declaration bunch of code to the top but this way or another the
list will be declared after a huge piece of code.
>
>> +
>> +def c_event(self, name):
>> + '''
>> + Exported function that handles event declarations.
>> + '''
>> + #print(name)
>> +
>> +def c_error(self, name):
>> + '''
>> + Exported function that handles error declarations.
>> + '''
>> + #print(self)
>> + #print(name)
> hmm, so gut feeling tells me that you probably want some error message here
> so you don't accidentally create swapping code that skips events or errors.
Christian already commented about the functions we cannot drop, because
they are required by output dictionary (declared at the end).
>
>> +
>> +# Must create an "output" dictionary before any xcbgen imports.
>> +output = {'open' : c_open,
>> + 'close' : c_close,
>> + 'simple' : c_simple,
>> + 'enum' : c_enum,
>> + 'struct' : c_struct,
>> + 'union' : c_union,
>> + 'request' : c_request,
>> + 'event' : c_event,
>> + 'error' : c_error,
>> + }
>> +
>> +# Check for the argument that specifies path to the xcbgen python package.
>> +try:
>> + opts, args = getopt.getopt(sys.argv[1:], 'p:')
>> +except getopt.GetoptError as err:
>> + print(err)
>> + print('Usage: gen_swap_check.py [-p path] file.xml')
>> + sys.exit(1)
>> +
>> +for (opt, arg) in opts:
>> + if opt == '-p':
>> + sys.path.insert(1, arg)
>> +
>> +# Import the module class
>> +try:
>> + from xcbgen.state import Module
>> + from xcbgen.xtypes import *
>> +except ImportError:
>> + print('''
>> +Failed to load the xcbgen Python package!
>> +Make sure that xcb/proto installed it on your Python path.
>> +If not, you will need to create a .pth file or define $PYTHONPATH
>> +to extend the path.
>> +Refer to the README file in xcb/proto for more info.
>> +''')
>> + raise
>> +
>> +module = Module(args[0], output)
>> +module.register()
>> +module.resolve()
>> +module.generate()
> this is the C programmer in me, but I've really started liking the python
> approach of:
>
> def main(argv):
> # do stuff
>
> if __name__ == "__main__":
> main(sys.argv)
>
> IMO it nicely groups the code to run when the file is executed.
>
> That's about all I have for now. I admit the python bits made my head spin,
> they're a bit hard to decypher (not your fault). I haven't run this yet, so
> I need to look at the output to comment any further.
>
> Meanwhile, most of what I pointed out above is easy to fix and a trivial
> change, please incorporate those, it'll make review a lot easier.
>
> Cheers,
> Peter
Lots and lots lines in the patch changed only because I removed trailing
whitespaces and spare new lines.
I would like to work on project during this week informally.
Thanks,
best regards,
Asalle
P. S. Sorry for the delay, I needed to handle with some health problems.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20150310/209bbd61/attachment-0001.html>
More information about the xorg-devel
mailing list