[PATCH] Byte-swapping and size-checking generator all-in-one

Peter Hutterer peter.hutterer at who-t.net
Tue Mar 17 00:25:00 PDT 2015


On Wed, Mar 11, 2015 at 08:46:41PM +0200, asalle.kim at gmail.com wrote:
> ---
>  .gitignore              |   3 +
>  Makefile.am             |   4 +-
>  Xext/Makefile.am        |   2 +-
>  Xext/shape.c            | 167 ++---------
>  configure.ac            |  41 ++-
>  proto/.gitignore        |   5 +
>  proto/Makefile.am       |  21 ++
>  proto/gen_swap_check.py | 761 ++++++++++++++++++++++++++++++++++++++++++++++++
>  8 files changed, 860 insertions(+), 144 deletions(-)
>  create mode 100644 proto/.gitignore
>  create mode 100644 proto/Makefile.am
>  create mode 100644 proto/gen_swap_check.py
> 
> 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

this can/should be a separate patch

> diff --git a/Makefile.am b/Makefile.am
> index f0fa2d8..aac0a69 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -42,6 +42,7 @@ SUBDIRS = \
>  	dix  \
>  	fb \
>  	mi \
> +	proto \
>  	Xext \
>  	miext \
>  	os \
> @@ -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..73ef643 100644
> --- a/Xext/Makefile.am
> +++ b/Xext/Makefile.am
> @@ -1,6 +1,6 @@
>  noinst_LTLIBRARIES = libXext.la libXextdpmsstubs.la
>  
> -AM_CFLAGS = $(DIX_CFLAGS)
> +AM_CFLAGS = $(DIX_CFLAGS) $(PROTO_CFLAGS)
>  
>  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..f1e59aa 100644
> --- a/Xext/shape.c
> +++ b/Xext/shape.c
> @@ -1026,7 +1026,7 @@ ProcShapeGetRectangles(ClientPtr client)
>  }
>  
>  static int
> -ProcShapeDispatch(ClientPtr client)
> +ProcShapeDispatch_Unchecked(ClientPtr client)
>  {
>      REQUEST(xReq);
>      switch (stuff->data) {
> @@ -1073,6 +1073,36 @@ ProcShapeDispatch(ClientPtr client)
>      }
>  }
>  
> +
> +#include "swapcheck_shape.h"
> +
> +static int
> +ProcShapeDispatch(ClientPtr client)
> +{
> +    int check_result = xcb_shape_Check_dispatch(client);
> +    if(check_result == Success){
> +        return ProcShapeDispatch_Unchecked(client);
> +    }
> +    else
> +    {
> +        return check_result;
> +    }
> +}
> +
> +static int
> +SProcShapeDispatch(ClientPtr client)
> +{
> +    int swap_result = xcb_shape_SwapFromClient_dispatch(client);
> +    if(swap_result == Success){
> +        return ProcShapeDispatch_Unchecked(client);
> +    }
> +    else
> +    {
> +        return swap_result;
> +    }
> +}
> +
> +
>  static void
>  SShapeNotifyEvent(xShapeNotifyEvent * from, xShapeNotifyEvent * to)
>  {
> @@ -1088,141 +1118,6 @@ SShapeNotifyEvent(xShapeNotifyEvent * from, xShapeNotifyEvent * to)
>      to->shaped = from->shaped;
>  }
>  
> -static int
> -SProcShapeQueryVersion(ClientPtr client)
> -{
> -    REQUEST(xShapeQueryVersionReq);
> -
> -    swaps(&stuff->length);
> -    return ProcShapeQueryVersion(client);
> -}
> -
> -static int
> -SProcShapeRectangles(ClientPtr client)
> -{
> -    REQUEST(xShapeRectanglesReq);
> -
> -    swaps(&stuff->length);
> -    REQUEST_AT_LEAST_SIZE(xShapeRectanglesReq);
> -    swapl(&stuff->dest);
> -    swaps(&stuff->xOff);
> -    swaps(&stuff->yOff);
> -    SwapRestS(stuff);
> -    return ProcShapeRectangles(client);
> -}
> -
> -static int
> -SProcShapeMask(ClientPtr client)
> -{
> -    REQUEST(xShapeMaskReq);
> -
> -    swaps(&stuff->length);
> -    REQUEST_SIZE_MATCH(xShapeMaskReq);
> -    swapl(&stuff->dest);
> -    swaps(&stuff->xOff);
> -    swaps(&stuff->yOff);
> -    swapl(&stuff->src);
> -    return ProcShapeMask(client);
> -}
> -
> -static int
> -SProcShapeCombine(ClientPtr client)
> -{
> -    REQUEST(xShapeCombineReq);
> -
> -    swaps(&stuff->length);
> -    REQUEST_SIZE_MATCH(xShapeCombineReq);
> -    swapl(&stuff->dest);
> -    swaps(&stuff->xOff);
> -    swaps(&stuff->yOff);
> -    swapl(&stuff->src);
> -    return ProcShapeCombine(client);
> -}
> -
> -static int
> -SProcShapeOffset(ClientPtr client)
> -{
> -    REQUEST(xShapeOffsetReq);
> -
> -    swaps(&stuff->length);
> -    REQUEST_SIZE_MATCH(xShapeOffsetReq);
> -    swapl(&stuff->dest);
> -    swaps(&stuff->xOff);
> -    swaps(&stuff->yOff);
> -    return ProcShapeOffset(client);
> -}
> -
> -static int
> -SProcShapeQueryExtents(ClientPtr client)
> -{
> -    REQUEST(xShapeQueryExtentsReq);
> -
> -    swaps(&stuff->length);
> -    REQUEST_SIZE_MATCH(xShapeQueryExtentsReq);
> -    swapl(&stuff->window);
> -    return ProcShapeQueryExtents(client);
> -}
> -
> -static int
> -SProcShapeSelectInput(ClientPtr client)
> -{
> -    REQUEST(xShapeSelectInputReq);
> -
> -    swaps(&stuff->length);
> -    REQUEST_SIZE_MATCH(xShapeSelectInputReq);
> -    swapl(&stuff->window);
> -    return ProcShapeSelectInput(client);
> -}
> -
> -static int
> -SProcShapeInputSelected(ClientPtr client)
> -{
> -    REQUEST(xShapeInputSelectedReq);
> -
> -    swaps(&stuff->length);
> -    REQUEST_SIZE_MATCH(xShapeInputSelectedReq);
> -    swapl(&stuff->window);
> -    return ProcShapeInputSelected(client);
> -}
> -
> -static int
> -SProcShapeGetRectangles(ClientPtr client)
> -{
> -    REQUEST(xShapeGetRectanglesReq);
> -    swaps(&stuff->length);
> -    REQUEST_SIZE_MATCH(xShapeGetRectanglesReq);
> -    swapl(&stuff->window);
> -    return ProcShapeGetRectangles(client);
> -}
> -
> -static int
> -SProcShapeDispatch(ClientPtr client)
> -{
> -    REQUEST(xReq);
> -    switch (stuff->data) {
> -    case X_ShapeQueryVersion:
> -        return SProcShapeQueryVersion(client);
> -    case X_ShapeRectangles:
> -        return SProcShapeRectangles(client);
> -    case X_ShapeMask:
> -        return SProcShapeMask(client);
> -    case X_ShapeCombine:
> -        return SProcShapeCombine(client);
> -    case X_ShapeOffset:
> -        return SProcShapeOffset(client);
> -    case X_ShapeQueryExtents:
> -        return SProcShapeQueryExtents(client);
> -    case X_ShapeSelectInput:
> -        return SProcShapeSelectInput(client);
> -    case X_ShapeInputSelected:
> -        return SProcShapeInputSelected(client);
> -    case X_ShapeGetRectangles:
> -        return SProcShapeGetRectangles(client);
> -    default:
> -        return BadRequest;
> -    }
> -}
> -
>  void
>  ShapeExtensionInit(void)
>  {
> diff --git a/configure.ac b/configure.ac
> index 96524c5..5b89705 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -33,6 +33,7 @@ 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])
>  
>  # Require xorg-macros minimum of 1.14 for XORG_COMPILER_BRAND in XORG_DEFAULT_OPTIONS
>  m4_ifndef([XORG_MACROS_VERSION],
> @@ -576,6 +577,7 @@ AC_ARG_WITH(khronos-spec-dir, AS_HELP_STRING([--with-khronos-spec-dir=PATH], [Pa
>  				[KHRONOS_SPEC_DIR=auto])
>  
>  dnl Extensions.
> +AC_ARG_ENABLE(proto,          AS_HELP_STRING([--disable-composite], [Build Proto (default: enabled)]), [PROTO=$enableval], [PROTO=yes])

I'm gonna go out on a limb here and say that the help string isn't quite
what you were looking for :)
      
>fwiw, "proto" is a rather generic term, I'd use xcb-proto-parsing or
something. But really, I think this should be unconditional anyway
so you don't need this flag at all.

>  AC_ARG_ENABLE(composite,      AS_HELP_STRING([--disable-composite], [Build Composite extension (default: enabled)]), [COMPOSITE=$enableval], [COMPOSITE=yes])
>  AC_ARG_ENABLE(mitshm,         AS_HELP_STRING([--disable-mitshm], [Build SHM extension (default: auto)]), [MITSHM=$enableval], [MITSHM=auto])
>  AC_ARG_ENABLE(xres,           AS_HELP_STRING([--disable-xres], [Build XRes extension (default: enabled)]), [RES=$enableval], [RES=yes])
> @@ -1036,6 +1038,13 @@ if test "x$COMPOSITE" = xyes; then
>  	COMPOSITE_INC='-I$(top_srcdir)/composite'
>  fi
>  
> +AM_CONDITIONAL(PROTO, [test "x$PROTO" = xyes])
> +if test "x$PROTO" = xyes; then
> +	AC_DEFINE(PROTO, 1, [Support Proto])
> +	PROTO_LIB='$(top_builddir)/proto/libproto.la'
> +	PROTO_INC='-I$(top_srcdir)/proto/generated'
> +fi
> +
>  if test "x$MITSHM" = xauto; then
>  	MITSHM="$ac_cv_sysv_ipc"
>  fi
> @@ -1776,7 +1785,26 @@ AC_EGREP_CPP([I_AM_SVR4],[
>  AC_DEFINE([SVR4],1,[Define to 1 on systems derived from System V Release 4])
>  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"
> +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 $PROTO_INC"
> +
> +dnl ---------------------------------------------------------------------------
> +dnl proto section.
> +dnl ---------------------------------------------------------------------------
> +
> +# Find the xcb-proto protocol descriptions
> +PKG_CHECK_MODULES([XCBPROTO], [xcb-proto])
> +XCBPROTO_XCBINCLUDEDIR=`$PKG_CONFIG --variable=xcbincludedir xcb-proto`
> +AC_SUBST(XCBPROTO_XCBINCLUDEDIR)
> +
> +# 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)
> +
> +# CFLAGS
> +PROTO_CFLAGS="$XSERVER_CFLAGS $XCBPROTO_CFLAGS"
> +AC_SUBST([PROTO_CFLAGS])
>  
>  dnl ---------------------------------------------------------------------------
>  dnl DDX section.
> @@ -1789,7 +1817,7 @@ AC_MSG_RESULT([$XVFB])
>  AM_CONDITIONAL(XVFB, [test "x$XVFB" = xyes])
>  
>  if test "x$XVFB" = xyes; then
> -	XVFB_LIBS="$FB_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB"
> +	XVFB_LIBS="$FB_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $PROTO_LIB"
>  	XVFB_SYS_LIBS="$XVFBMODULES_LIBS $GLX_SYS_LIBS"
>  	AC_SUBST([XVFB_LIBS])
>  	AC_SUBST([XVFB_SYS_LIBS])
> @@ -1810,7 +1838,7 @@ if test "x$XNEST" = xyes; then
>  	if test "x$have_xnest" = xno; then
>  		AC_MSG_ERROR([Xnest build explicitly requested, but required modules not found.])
>  	fi
> -	XNEST_LIBS="$FB_LIB $FIXES_LIB $MI_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB  $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $MAIN_LIB $DIX_LIB $OS_LIB"
> +	XNEST_LIBS="$FB_LIB $FIXES_LIB $MI_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB  $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $MAIN_LIB $DIX_LIB $OS_LIB $PROTO_LIB"
>  	XNEST_SYS_LIBS="$XNESTMODULES_LIBS $GLX_SYS_LIBS"
>  	AC_SUBST([XNEST_LIBS])
>  	AC_SUBST([XNEST_SYS_LIBS])
> @@ -1835,7 +1863,7 @@ if test "x$XORG" = xyes; then
>  	XORG_OSINCS='-I$(top_srcdir)/hw/xfree86/os-support -I$(top_srcdir)/hw/xfree86/os-support/bus -I$(top_srcdir)/os'
>  	XORG_INCS="$XORG_DDXINCS $XORG_OSINCS"
>  	XORG_CFLAGS="$XORGSERVER_CFLAGS -DHAVE_XORG_CONFIG_H"
> -	XORG_LIBS="$COMPOSITE_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $XI_LIB $XKB_LIB"
> +	XORG_LIBS="$COMPOSITE_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $XI_LIB $XKB_LIB $PROTO_LIB"
>  
>  	dnl ==================================================================
>  	dnl symbol visibility
> @@ -2391,11 +2419,11 @@ if test "$KDRIVE" = yes; then
>      KDRIVE_INC='-I$(top_srcdir)/hw/kdrive/src'
>      KDRIVE_PURE_INCS="$KDRIVE_INC $MIEXT_SYNC_INC $MIEXT_DAMAGE_INC $MIEXT_SHADOW_INC $XEXT_INC $FB_INC $MI_INC"
>      KDRIVE_OS_INC='-I$(top_srcdir)/hw/kdrive/linux'
> -    KDRIVE_INCS="$KDRIVE_PURE_INCS $KDRIVE_OS_INC"
> +    KDRIVE_INCS="$KDRIVE_PURE_INCS $KDRIVE_OS_INC $PROTO_INC"
>      
>      KDRIVE_CFLAGS="$XSERVER_CFLAGS -DHAVE_KDRIVE_CONFIG_H $TSLIB_CFLAGS"
>  
> -    KDRIVE_PURE_LIBS="$FB_LIB $MI_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $OS_LIB"
> +    KDRIVE_PURE_LIBS="$FB_LIB $MI_LIB $FIXES_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB $GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $DRI3_LIB $PRESENT_LIB $MIEXT_SYNC_LIB $MIEXT_DAMAGE_LIB $MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $OS_LIB $PROTO_LIB"
>      KDRIVE_LIB='$(top_builddir)/hw/kdrive/src/libkdrive.la'
>      case $host_os in
>  	*linux*)
> @@ -2546,6 +2574,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..34af921
> --- /dev/null
> +++ b/proto/Makefile.am
> @@ -0,0 +1,21 @@
> +noinst_LTLIBRARIES = libproto.la
> +
> +AM_CFLAGS = $(PROTO_CFLAGS)
> +
> +prefx=swapcheck_

is prefix a reserved word? or do we just disagree about the spelling? :)

> +PROTO_GENERATEDDIR=generated
> +
> +extension_sources =	\
> +	${PROTO_GENERATEDDIR}/${prefx}xproto.c \
> +	${PROTO_GENERATEDDIR}/${prefx}shape.c
> +
> +libproto_la_SOURCES = $(extension_sources)
> +
> +sources = $(subst ${PROTO_GENERATEDDIR}/${prefx}, ,$(@))
> +
> +$(extension_sources): $(XCBPROTO_XCBINCLUDEDIR)/$(sources:.c=.xml) $(srcdir)/gen_swap_check.py
> +

you can drop this empty line here

> +	$(AM_V_GEN)$(PYTHON) $(srcdir)/gen_swap_check.py \
> +		-p $(XCBPROTO_XCBPYTHONDIR) \
> +		-d ${PROTO_GENERATEDDIR} \
> +		$(XCBPROTO_XCBINCLUDEDIR)/$(sources:.c=.xml)
> diff --git a/proto/gen_swap_check.py b/proto/gen_swap_check.py
> new file mode 100644
> index 0000000..f0ca86e
> --- /dev/null
> +++ b/proto/gen_swap_check.py
> @@ -0,0 +1,761 @@
> +#!/usr/bin/env python
> +import getopt
> +import os
> +import sys
> +import errno
> +import re
> +
> +def _i():
> +	'''
> +	Indent function
> +
> +	Returns joined _indent to be concatenated with code string in _c()
> +	'''
> +	return ''.join(_indent)
> +
> +# Helping functions and definitions copy-pasted from c_client.py
> +
> +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 _code(fmt, *args):
> +	global _codelines
> +	_codelines.append(fmt % args)
> +
> +def output_code():
> +	global _codelines
> +	for line in _codelines:
> +		_c("%s", line)
> +	_codelines = []
> +
> +# Some hacks to make the API more readable and to keep backwards compability
> +_cname_special_cases = {'DECnet':'decnet'}
> +_extension_special_cases = ['XPrint', 'XCMisc', 'BigRequests']
> +
> +# The regex matches three types of strings:
> +#	1. Those starting with one and only one upper-case letter or a digit
> +#		and proceeding with lower-case letters, no upper-case letters or
> +#		digits are allowed except for the first letter
> +#	2. Those staring and proceeding with upper-case letters or digits and
> +#		containing no lower-case letters at all
> +#	3. Those starting and proceeding with lower-case letters and containing
> +#		no upper-case letters or digits at all
> +_cname_re = re.compile('([A-Z0-9][a-z]+|[A-Z0-9]+(?![a-z])|[a-z]+)')
> +
> +_hlines = []
> +_hlevel = 0
> +_clines = []
> +_clevel = 0
> +_codelines = []
> +
> +_ns = None
> +_filename = ''
> +_requests = {}
> +_indent = []
> +
> +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):
> +	'''
> +	Does C-name conversion on a single string fragment.
> +	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()
> +
> +#			Copy-pasted from c_client.py code ends here
> +
> +# Check for the argument that specifies path to the xcbgen python package.
> +try:
> +	opts, args = getopt.getopt(sys.argv[1:], 'd:p:')
> +except getopt.GetoptError as err:
> +	print(err)
> +	print('Usage: gen_swap_check.py -d generated_dir [-p path] file.xml')
> +	sys.exit(1)
> +
> +for (opt, arg) in opts:
> +	if opt == '-d':
> +		gendir = arg
> +	if opt == '-p':
> +		sys.path.insert(1, arg)
> +
> +def c_open(self):
> +	'''
> +	Exported function that handles module open.
> +	Opens the files and writes out the auto-generated comment,
> +	header file includes, etc.
> +	'''
> +	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 included 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
> +	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.
> +	'''
> +	#Needs 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)
> +	'''
> +	#Needs future implementation
> +
> +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

this should be linewrapped. I'm also struggling - where is the copied code
and where is the new code? again, this should be marked with comments to
help focusing on the actual review

> +	'''
> +	_name = None
> +	_docheck = False
> +	_reusedvars = []
> +	_declaredvars = []
> +	_typeHandler = None
> +	_has_struct = False
> +
> +	def __init__(self, typeHandler):
> +		self._typeHandler = typeHandler
> +
> +	def access_check( self, size ):
> +		if self._docheck:
> +			_code('%sif( p + %u > (uint8_t*)afterEnd) {', _i(), size)
> +			_increase_indent()
> +			_code('%sreturn BadLength;', _i())
> +			_decrease_indent()
> +			_code('%s}',  _i())

general nitpick: it's usually better to invert a condition and
return/break/continue early than have the whole block indented. so in this
case you'd do a
        if not self._docheck:
                return

        .... other code

> +
> +	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)
> +				if(fname not in self._declaredvars):
> +					_c('%s%s %s;', _i(), _datatype, _varname)
> +				self._declaredvars.append(fname)
> +				_code('%s%s = *(%s*)p;', _i(), _varname, _datatype)

different indentation to the rest of the code, and the above comment applies
here too

> +	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):
> +		_code('%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, that is a lot of tabs...

> +
> +	def process_simple(self, fname, ftype):
> +		pass
> +
> +	def process_list(self, ftype, it, llen):
> +		#_c('%s{', _i())
> +		if it not in self._declaredvars:
> +			_c('%sunsigned int %s;', _i(), it)
> +		self._declaredvars.append(it)
> +		#_increase_indent()
> +		_code('%sfor(%s = 0; %s < %s; %s++)', _i(), it, it, llen, it)
> +		_code('%s{', _i())
> +
> +		_increase_indent()
> +		self.check(ftype.member, None)
> +		_decrease_indent()
> +
> +		_code('%s}', _i())
> +		#_decrease_indent()
> +		#_code('%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
> +		_code('\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?
> +			_code('%sif (%s %s %s)', _i(),
> +								_casevarn, _eq_sign,
> +								(_n(_enumn)+'_'+_enument).upper())
> +			_code('%s{', _i())
> +			_increase_indent()
> +			for field in case.type.fields:
> +				_code('%s//%s %s', _i(), field.type.name, field.field_name)
> +				self.check(field.type, field.field_name)
> +			_decrease_indent()
> +			_code('%s}', _i())
> +			_code('')
> +
> +		_code('%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)
> +			_code('%sp += %u;', _i(), _size)
> +		elif ftype.is_pad and ftype.fixed_size():
> +			byts = ftype.nmemb
> +			_code('%sp += %u;', _i(), byts)

> +		elif ftype.is_pad and not ftype.fixed_size():
> +			al = ftype.align
> +			_code('%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)

indentation

> +		elif ftype.is_switch:
> +			self.process_switch(ftype)
> +		elif ftype.is_container:
> +			self.process_struct(ftype.name)
> +		else:
> +			_code(	'%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, [])
> +		self._declaredvars = []
> +		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()
> +		self.directPrintEmpty()
> +		output_code()
> +
> +	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):
> +		_code('')
> +
> +	def directPrintEmpty(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 ):
> +		_code('%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):

whitespace issue here

> +		_code('		return Success;')
> +		_code('	else')
> +		_code('		return BadLength;')
> +		_code('}')
> +		_decrease_indent()
> +		_h('')
> +		_code('')
> +
> +	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):
> +		_code('	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':
> +			_code('	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;')

shouldn't you be using _i() here instead of the tab? in the generated code
Chris sent late Feb this line is always over-indented

> +
> +	def init_docheck(self, docheck):
> +		return False
> +
> +	def initAfterStruct(self):
> +		_c('	uint8_t* afterStruct = NULL;')

same here

> +
> +	def process_struct(self, name):
> +		_code('%sif(%s(p, afterEnd, &afterStruct) != Success)\n\t\t\treturn BadLength;', _i(), name)

you need to use _i() consistently here, and best to have multiple print
lines. otherwise the indentation is messed up

> +		_code('%sp = afterStruct;', _i())
> +
> +class StructHandler(TypeHandler):
> +	def printFooter(self):
> +		_code('	*afterStruct = p;')
> +		_code('	if (p <= (uint8_t*)afterEnd)')

same here

afaict the output is correct, so this looks good. going through the final
patch with a fine-tooth-comb will be interesting. Have you run the X test
suite against this by any chance? That's probably the most reliable check to
see if something is buggy.

Cheers,
   Peter

> +
> +	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):
> +		_code('%sif(%s(p, afterEnd, afterStruct) != Success)\n\t\t\treturn BadLength;', _i(), name)
> +		_code('%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('%sdefault:', _i())
> +	_increase_indent()
> +	_c('%sreturn BadRequest;', _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}
> +
> +def c_event(self, name):
> +	'''
> +	Exported function that handles event declarations.
> +	'''
> +	#Needs future implementation
> +
> +def c_error(self, name):
> +	'''
> +	Exported function that handles error declarations.
> +	'''
> +	#Needs future implementation
> +
> +# 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,
> +		  }
> +
> +# 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()
> -- 
> 1.9.1
> 
 


More information about the xorg-devel mailing list