Add build checks for portable OpenFlow structure padding and alignment.

This causes the build to fail with an error message if openflow.h contains
a structure whose members are not aligned in a portable way.

.gitignore

@@ -23,7 +23,6 @@
 /aclocal.m4
 /autom4te.cache
 /build-arch-stamp
-/build-aux
 /build-indep-stamp
 /compile
 /config.guess

build-aux/.gitignore

@@ -0,0 +1,4 @@
+/compile
+/depcomp
+/install-sh
+/missing

build-aux/check-structs

@@ -0,0 +1,267 @@
+#! /usr/bin/python
+
+import sys
+import re
+
+macros = {}
+
+anyWarnings = False
+
+types = {}
+types['char'] = {"size": 1, "alignment": 1}
+types['uint8_t'] = {"size": 1, "alignment": 1}
+types['uint16_t'] = {"size": 2, "alignment": 2}
+types['uint32_t'] = {"size": 4, "alignment": 4}
+types['uint64_t'] = {"size": 8, "alignment": 8}
+
+token = None
+line = ""
+idRe = "[a-zA-Z_][a-zA-Z_0-9]*"
+tokenRe = "#?" + idRe + "|[0-9]+|."
+inComment = False
+inDirective = False
+def getToken():
+    global token
+    global line
+    global inComment
+    global inDirective
+    while True:
+        line = line.lstrip()
+        if line != "":
+            if line.startswith("/*"):
+                inComment = True
+                line = line[2:]
+            elif inComment:
+                commentEnd = line.find("*/")
+                if commentEnd < 0:
+                    line = ""
+                else:
+                    inComment = False
+                    line = line[commentEnd + 2:]
+            else:
+                match = re.match(tokenRe, line)
+                token = match.group(0)
+                line = line[len(token):]
+                if token.startswith('#'):
+                    inDirective = True
+                elif token in macros and not inDirective:
+                    line = macros[token] + line
+                    continue
+                return True
+        elif inDirective:
+            token = "$"
+            inDirective = False
+            return True
+        else:
+            global lineNumber
+            line = inputFile.readline()
+            lineNumber += 1
+            while line.endswith("\\\n"):
+                line = line[:-2] + inputFile.readline()
+                lineNumber += 1
+            if line == "":
+                if token == None:
+                    fatal("unexpected end of input")
+                token = None
+                return False
+    
+def fatal(msg):
+    sys.stderr.write("%s:%d: error at \"%s\": %s\n" % (fileName, lineNumber, token, msg))
+    sys.exit(1)
+    
+def warn(msg):
+    global anyWarnings
+    anyWarnings = True
+    sys.stderr.write("%s:%d: warning: %s\n" % (fileName, lineNumber, msg))
+
+def skipDirective():
+    getToken()
+    while token != '$':
+        getToken()
+
+def isId(s):
+    return re.match(idRe + "$", s) != None
+
+def forceId():
+    if not isId(token):
+        fatal("identifier expected")
+
+def forceInteger():
+    if not re.match('[0-9]+$', token):
+        fatal("integer expected")
+
+def match(t):
+    if token == t:
+        getToken()
+        return True
+    else:
+        return False
+
+def forceMatch(t):
+    if not match(t):
+        fatal("%s expected" % t)
+
+def parseTaggedName():
+    assert token in ('struct', 'union')
+    name = token
+    getToken()
+    forceId()
+    name = "%s %s" % (name, token)
+    getToken()
+    return name
+
+def parseTypeName():
+    if token in ('struct', 'union'):
+        name = parseTaggedName()
+    elif isId(token):
+        name = token
+        getToken()
+    else:
+        fatal("type name expected")
+
+    if name in types:
+        return name
+    else:
+        fatal("unknown type \"%s\"" % name)
+
+def parseStruct():
+    isStruct = token == 'struct'
+    structName = parseTaggedName()
+    if token == ";":
+        return
+
+    ofs = size = 0
+    alignment = 4               # ARM has minimum 32-bit alignment
+    forceMatch('{')
+    while not match('}'):
+        typeName = parseTypeName()
+        typeSize = types[typeName]['size']
+        typeAlignment = types[typeName]['alignment']
+
+        forceId()
+        memberName = token
+        getToken()
+
+        if match('['):
+            if token == ']':
+                count = 0
+            else:
+                forceInteger()
+                count = int(token)
+                getToken()
+            forceMatch(']')
+        else:
+            count = 1
+
+        nBytes = typeSize * count
+        if isStruct:
+            if ofs % typeAlignment:
+                shortage = typeAlignment - (ofs % typeAlignment)
+                warn("%s member %s is %d bytes short of %d-byte alignment"
+                     % (structName, memberName, shortage, typeAlignment))
+                size += shortage
+                ofs += shortage
+            size += nBytes
+            ofs += nBytes
+        else:
+            if nBytes > size:
+                size = nBytes
+        if typeAlignment > alignment:
+            alignment = typeAlignment
+
+        forceMatch(';')
+    if size % alignment:
+        shortage = alignment - (size % alignment)
+        if (structName == "struct ofp_packet_in" and
+            shortage == 2 and
+            memberName == 'data' and
+            count == 0):
+            # This is intentional
+            pass
+        else:
+            warn("%s needs %d bytes of tail padding" % (structName, shortage))
+        size += shortage
+    types[structName] = {"size": size, "alignment": alignment}
+
+def checkStructs():
+    if len(sys.argv) < 2:
+        sys.stderr.write("at least one non-option argument required; "
+                         "use --help for help")
+        sys.exit(1)
+
+    if '--help' in sys.argv:
+        argv0 = sys.argv[0]
+        slash = argv0.rfind('/')
+        if slash:
+            argv0 = argv0[slash + 1:]
+        print '''\
+%(argv0)s, for checking struct and struct member alignment
+usage: %(argv0)s HEADER [HEADER]...
+
+This program reads the header files specified on the command line and
+verifies that all struct members are aligned on natural boundaries
+without any need for the compiler to add additional padding.  It also
+verifies that each struct's size is a multiple of 32 bits (because
+some ABIs for ARM require all structs to be a multiple of 32 bits), or
+64 bits if the struct has any 64-bit members, again without the
+compiler adding additional padding.  Finally, it checks struct size
+assertions using OFP_ASSERT.
+
+Header files are read in the order specified.  #include directives are
+not processed, so specify them in dependency order.
+
+This program is specialized for reading include/openflow/openflow.h
+and include/openflow/nicira-ext.h.  It will not work on arbitrary
+header files without extensions.''' % {"argv0": argv0}
+        sys.exit(0)
+
+    global fileName
+    for fileName in sys.argv[1:]:
+        global inputFile
+        global lineNumber
+        inputFile = open(fileName)
+        lineNumber = 0
+        while getToken():
+            if token in ("#ifdef", "#ifndef", "#include",
+                         "#endif", "#elif", "#else"):
+                skipDirective()
+            elif token == "#define":
+                getToken()
+                name = token
+                if line.startswith('('):
+                    skipDirective()
+                else:
+                    definition = ""
+                    getToken()
+                    while token != '$':
+                        definition += token
+                        getToken()
+                    macros[name] = definition
+            elif token == "enum":
+                while token != ';':
+                    getToken()
+            elif token in ('struct', 'union'):
+                parseStruct()
+            elif match('OFP_ASSERT') or match('BOOST_STATIC_ASSERT'):
+                forceMatch('(')
+                forceMatch('sizeof')
+                forceMatch('(')
+                typeName = parseTypeName()
+                forceMatch(')')
+                forceMatch('=')
+                forceMatch('=')
+                forceInteger()
+                size = int(token)
+                getToken()
+                forceMatch(')')
+                if types[typeName]['size'] != size:
+                    warn("%s is %d bytes long but declared as %d" % (
+                            typeName, types[typeName]['size'], size))
+            else:
+                fatal("parse error")
+        inputFile.close()
+    if anyWarnings:
+        sys.exit(1)
+
+if __name__ == '__main__':
+    checkStructs()

configure.ac

@@ -1,4 +1,4 @@
-# Copyright (c) 2008, 2009 Nicira Networks
+# Copyright (c) 2008, 2009, 2010 Nicira Networks
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -88,4 +88,7 @@ datapath/linux-2.6/Makefile
 datapath/linux-2.6/Makefile.main
 tests/atlocal])
 
+dnl This makes sure that include/openflow gets created in the build directory.
+AC_CONFIG_COMMANDS([include/openflow/openflow.h.stamp])
+
 AC_OUTPUT

include/openflow/.gitignore

@@ -0,0 +1 @@
+/openflow.h.stamp

include/openflow/automake.mk

@@ -2,3 +2,19 @@ noinst_HEADERS += \
 	include/openflow/openflow-mgmt.h \
 	include/openflow/nicira-ext.h \
 	include/openflow/openflow.h
+
+if HAVE_PYTHON
+all-local: include/openflow/openflow.h.stamp
+include/openflow/openflow.h.stamp: \
+	include/openflow/openflow.h build-aux/check-structs
+	$(PYTHON) $(srcdir)/build-aux/check-structs $(srcdir)/include/openflow/openflow.h
+	touch $@
+
+all-local: include/openflow/nicira-ext.h.stamp
+include/openflow/nicira-ext.h.stamp: include/openflow/openflow.h include/openflow/nicira-ext.h build-aux/check-structs
+	$(PYTHON) $(srcdir)/build-aux/check-structs $(srcdir)/include/openflow/openflow.h $(srcdir)/include/openflow/nicira-ext.h
+	touch $@
+endif
+
+EXTRA_DIST += build-aux/check-structs
+DISTCLEANFILES += include/openflow/openflow.h.stamp 

include/openflow/nicira-ext.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009 Nicira Networks
+ * Copyright (c) 2008, 2009, 2010 Nicira Networks
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -65,7 +65,7 @@ struct nicira_header {
     uint32_t vendor;            /* NX_VENDOR_ID. */
     uint32_t subtype;           /* One of NXT_* above. */
 };
-OFP_ASSERT(sizeof(struct nicira_header) == sizeof(struct ofp_vendor_header) + 4);
+OFP_ASSERT(sizeof(struct nicira_header) == 16);
 
 
 enum nx_action_subtype {

m4/openvswitch.m4

@@ -256,7 +256,12 @@ else:
           done
         done
       fi])
+   AC_SUBST([HAVE_PYTHON])
    AM_MISSING_PROG([PYTHON], [python])
    if test $ovs_cv_python != no; then
      PYTHON=$ovs_cv_python
-   fi])
+     HAVE_PYTHON=yes
+   else
+     HAVE_PYTHON=no
+   fi
+   AM_CONDITIONAL([HAVE_PYTHON], [test "$HAVE_PYTHON" = yes])])

tests/atlocal.in

@@ -1,4 +1,6 @@
 # -*- shell-script -*-
-PERL='@PERL@'
-LCOV='@LCOV@'
 HAVE_OPENSSL='@HAVE_OPENSSL@'
+HAVE_PYTHON='@HAVE_PYTHON@'
+LCOV='@LCOV@'
+PERL='@PERL@'
+PYTHON='@PYTHON@'

tests/automake.mk

@@ -9,6 +9,7 @@ TESTSUITE_AT = \
 	tests/ovsdb-macros.at \
 	tests/lcov-pre.at \
 	tests/library.at \
+	tests/check-structs.at \
 	tests/daemon.at \
 	tests/vconn.at \
 	tests/dir_name.at \

tests/check-structs.at

@@ -0,0 +1,41 @@
+AT_BANNER([struct alignment checker unit tests])
+
+m4_define([check_structs], [$top_srcdir/build-aux/check-structs])
+m4_define([RUN_STRUCT_CHECKER], 
+  [AT_SKIP_IF([test $HAVE_PYTHON = no])
+   AT_DATA([test.h], [$1
+])
+   AT_CHECK_UNQUOTED([$PYTHON check_structs test.h], [$2], [$3], [$4])])
+
+AT_SETUP([check struct tail padding])
+RUN_STRUCT_CHECKER(
+[struct xyz {
+    uint16_t x;
+};], 
+  [1], [], 
+  [test.h:3: warning: struct xyz needs 2 bytes of tail padding
+])
+AT_CLEANUP
+
+AT_SETUP([check struct internal alignment])
+RUN_STRUCT_CHECKER(
+[struct xyzzy {
+    uint16_t x;
+    uint32_t y;
+};], 
+  [1], [], 
+  [test.h:3: warning: struct xyzzy member y is 2 bytes short of 4-byte alignment
+])
+AT_CLEANUP
+
+AT_SETUP([check struct declared size])
+RUN_STRUCT_CHECKER(
+[struct wibble {
+    uint64_t z;
+};
+OFP_ASSERT(sizeof(struct wibble) == 12);
+], 
+  [1], [], 
+  [test.h:4: warning: struct wibble is 8 bytes long but declared as 12
+])
+AT_CLEANUP

tests/testsuite.at

@@ -39,6 +39,7 @@ m4_include([tests/ovsdb-macros.at])
 m4_include([tests/lcov-pre.at])
 
 m4_include([tests/library.at])
+m4_include([tests/check-structs.at])
 m4_include([tests/daemon.at])
 m4_include([tests/vconn.at])
 m4_include([tests/dir_name.at])