[OPW] Generated swapping and size-checking code integration
Peter Hutterer
peter.hutterer at who-t.net
Mon Feb 16 23:36:37 PST 2015
Hi Asalle,
On Mon, Feb 16, 2015 at 09:46:47PM +0200, Asal Mirzaieva wrote:
> 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.
> 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?
> 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.
> 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.
> 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
>
> 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
>
> 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.
> +
> +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)
>
> # 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.
> +# 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
> +
> +# 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?
> +
> +# 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
> +
> +# 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.
> +
> +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.
> +
> +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.
> +
> +libproto_la_SOURCES = $(EXTSOURCES)
> +
> +ech = $(subst gen/swapcheck_, ,$(@))
"ech"? I wonder what that stand for... :)
> +
> +$(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?
> +
> +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
> +
> +# 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.
> +
> +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)
> + 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.
> + 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?
> +
> +
> +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
> +
> + 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
> +
> +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.
> +
> +# 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
More information about the xorg-devel
mailing list