From 05b3c97be6a9a5efa27edb40023e329f680837c5 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 25 Jan 2010 10:49:31 -0800 Subject: [PATCH] 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 | 1 - build-aux/.gitignore | 4 + build-aux/check-structs | 267 ++++++++++++++++++++++++++++++++++ configure.ac | 5 +- include/openflow/.gitignore | 1 + include/openflow/automake.mk | 16 ++ include/openflow/nicira-ext.h | 4 +- m4/openvswitch.m4 | 7 +- tests/atlocal.in | 6 +- tests/automake.mk | 1 + tests/check-structs.at | 41 ++++++ tests/testsuite.at | 1 + 12 files changed, 347 insertions(+), 7 deletions(-) create mode 100644 build-aux/.gitignore create mode 100755 build-aux/check-structs create mode 100644 include/openflow/.gitignore create mode 100644 tests/check-structs.at diff --git a/.gitignore b/.gitignore index a9377fbd..4e1fccba 100644 --- a/.gitignore +++ b/.gitignore @@ -23,7 +23,6 @@ /aclocal.m4 /autom4te.cache /build-arch-stamp -/build-aux /build-indep-stamp /compile /config.guess diff --git a/build-aux/.gitignore b/build-aux/.gitignore new file mode 100644 index 00000000..999eae2a --- /dev/null +++ b/build-aux/.gitignore @@ -0,0 +1,4 @@ +/compile +/depcomp +/install-sh +/missing diff --git a/build-aux/check-structs b/build-aux/check-structs new file mode 100755 index 00000000..545c80a8 --- /dev/null +++ b/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() diff --git a/configure.ac b/configure.ac index 92d5ac03..2f5c87ff 100644 --- a/configure.ac +++ b/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 diff --git a/include/openflow/.gitignore b/include/openflow/.gitignore new file mode 100644 index 00000000..db736db5 --- /dev/null +++ b/include/openflow/.gitignore @@ -0,0 +1 @@ +/openflow.h.stamp diff --git a/include/openflow/automake.mk b/include/openflow/automake.mk index d4731550..8c48b1cb 100644 --- a/include/openflow/automake.mk +++ b/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 diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h index 8434a30a..c2cd1ef5 100644 --- a/include/openflow/nicira-ext.h +++ b/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 { diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4 index 0890b9f4..bdb8b198 100644 --- a/m4/openvswitch.m4 +++ b/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])]) diff --git a/tests/atlocal.in b/tests/atlocal.in index d0149c1e..5ff0cd2f 100644 --- a/tests/atlocal.in +++ b/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@' diff --git a/tests/automake.mk b/tests/automake.mk index dc677eb1..044a1091 100644 --- a/tests/automake.mk +++ b/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 \ diff --git a/tests/check-structs.at b/tests/check-structs.at new file mode 100644 index 00000000..52e92ec0 --- /dev/null +++ b/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 diff --git a/tests/testsuite.at b/tests/testsuite.at index 93d7e6eb..06858922 100644 --- a/tests/testsuite.at +++ b/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]) -- 2.30.2