Update process for ABI testing

Jay Berkenbilt · Jay Berkenbilt
1 parent 4c0addfe
Showing 8 changed files with 560 additions and 20 deletions
CMakeLists.txt
README-maintainer
abi-perf-test
check_abi
libqpdf/CMakeLists.txt
manual/installation.rst
qpdf/CMakeLists.txt
qpdf/sizes.cc
@@ -36,6 +36,9 @@ CMAKE_DEPENDENT_OPTION(
   WERROR "Treat compiler warnings as errors" OFF
   "NOT MAINTAINER_MODE; NOT CI_MODE" ON)
 CMAKE_DEPENDENT_OPTION(
+  CHECK_SIZES "Compare sizes.cc with classes in public API" OFF
+  "NOT MAINTAINER_MODE" ON)
+CMAKE_DEPENDENT_OPTION(
   GENERATE_AUTO_JOB "Automatically regenerate job files" OFF
   "NOT MAINTAINER_MODE" ON)
 CMAKE_DEPENDENT_OPTION(
@@ -297,26 +297,17 @@ RELEASE PREPARATION
   testing, do performance testing.
  
 * Test for performance and binary compatibility:
-  * Check out the last release
-  * cmake -S . -B build \
-    -DMAINTAINER_MODE=1 -DBUILD_STATIC_LIBS=0 -DBUILD_DOC=0 \
-    -DCMAKE_BUILD_TYPE=RelWithDebInfo
-  * cmake --build build -j$(nproc)
-  * Check out the current version
-  * ./performance_check | tee -a /tmp/perf
-  * cmake -S . -B build \
-    -DMAINTAINER_MODE=1 -DBUILD_STATIC_LIBS=0 -DBUILD_DOC=0 \
-    -DCMAKE_BUILD_TYPE=RelWithDebInfo
-  * cmake --build build -j$(nproc) --target libqpdf
-  * Checkout the last release
-  * (cd build; ctest --verbose)
-  * (some failures are normal -- looking for binary compatibility)
-  * Check out the current version
-  * cmake -S . -B build \
-    -DMAINTAINER_MODE=1 -DBUILD_STATIC_LIBS=0 -DBUILD_DOC=0 \
-    -DCMAKE_BUILD_TYPE=RelWithDebInfo
-  * cmake --build build -j$(nproc)
-  * ./performance_check | tee -a /tmp/perf
+
+  ./abi-perf-test release-<old> @
+
+  Prefix with SKIP_PERF=1 to skip performance test.
+  Prefix with SKIP_TESTS=1 to skip test suite run.
+
+  See "ABI checks" for details about the process.
+  End state:
+  * /tmp/check-abi/old contains old sizes and library
+  * /tmp/check-abi/new contains new sizes and library
+  * run check_abi manually to compare
  
 * Run pikepdf's test suite. Do this in a separate shell.
  
@@ -512,3 +503,39 @@ manual tests were done:
 We are using RelWithDebInfo for mingw and other non-Windows builds but
 Release for MSVC. There are linker warnings if MSVC is built with
 RelWithDebInfo when using external-libs.
+
+
+ABI checks
+
+Until the conversion of the build to cmake, we relied on running the
+test suite with old executables and a new library. When QPDFJob was
+introduced, this method got much less reliable since a lot of public
+API doesn't cross the shared library boundary. Also, when switching to
+cmake, we wanted a stronger check that the library had the expected
+ABI.
+
+Our ABI check now consists of three parts:
+
+* The same check as before: run the test suite with old executables
+  and a new library
+
+* Do a literal comparison of the symbols in the old and new shared
+  libraries -- this is a strong test of ABI change
+
+* Do a check to ensure that object sizes didn't change -- even with no
+  changes to the API of exported functions, size changes break API
+
+The combination of these checks is pretty strong, though there are
+still things that could potentially break ABI, such as
+
+* Changes to the types of public or protected data members without
+  changing the size
+
+* Changes to the meanings of parameters with changing the signature
+
+Not breaking ABI/API still requires care.
+
+The check_abi script is responsible for performing many of these
+steps. See comments in check_abi for additional notes. Running
+"check_abi check-sizes" is run by ctest on Linux when CHECK_SIZES is
+on.
+#!/usr/bin/env bash
+set -eo pipefail
+cd $(dirname $0)
+whoami=$(basename $0)
+
+if [[ $(git status -s | egrep -v abi-perf-test | wc -l) != 0 ]]; then
+    echo 1>&2 "${whoami}: git is not clean. (abi-perf-test changes ignored)"
+    git status -s
+    exit 2
+fi
+
+old_rev=${1-bad}
+new_rev=${2-bad}
+
+if [ "$new_rev" = "bad" ]; then
+    echo 1>&2 "Usage: $whoami old-rev new-rev"
+    exit 2
+fi
+
+old_rev_hash=$(git rev-parse $old_rev)
+new_rev_hash=$(git rev-parse $new_rev)
+
+cat <<EOF
+
+Checking ABI:
+* old revision: $old_rev = $old_rev_hash
+* new revision: $new_rev = $new_rev_hash
+
+EOF
+
+work=/tmp/check-abi
+if [ -d $work ]; then
+   if [ ! -f $work/.abi ]; then
+       echo 1>&2 "$work exists and is not ours"
+       exit 2
+   else
+       rm -rf $work
+   fi
+fi
+mkdir -p $work/{old,new}
+touch $work/.abi
+
+source=$PWD
+repo=file://$source/.git
+cd $work
+git clone $repo qpdf
+cd qpdf
+
+git tag abi-old $old_rev_hash
+git tag abi-new $new_rev_hash
+
+echo "** building old version **"
+
+git checkout abi-old
+cmake -S . -B build \
+  -DMAINTAINER_MODE=1 -DBUILD_STATIC_LIBS=0 -DBUILD_DOC=0 \
+  -DCMAKE_BUILD_TYPE=RelWithDebInfo
+cmake --build build -j$(nproc)
+
+echo "** saving old library and size information **"
+
+$source/check_abi check-sizes --lib build/libqpdf/libqpdf.so
+./build/qpdf/sizes >| $work/old/sizes
+cp build/libqpdf/libqpdf.so.*.* $work/old
+
+if [ "$SKIP_PERF" != "1" ]; then
+    echo "** writing performance details for old revision to $work/perf **"
+    $source/performance_check --dir $source/../performance-test-files | \
+        tee -a $work/perf
+fi
+
+echo "** building new version's library and sizes **"
+
+git checkout abi-new
+cmake -S . -B build \
+  -DMAINTAINER_MODE=1 -DBUILD_STATIC_LIBS=0 -DBUILD_DOC=0 \
+  -DCMAKE_BUILD_TYPE=RelWithDebInfo
+cmake --build build -j$(nproc) --target sizes
+
+echo "** saving new library and size information **"
+
+$source/check_abi check-sizes --lib build/libqpdf/libqpdf.so
+./build/qpdf/sizes >| $work/new/sizes
+cp build/libqpdf/libqpdf.so.*.* $work/new
+
+echo "** running ABI comparison **"
+
+$source/check_abi compare --old-lib $work/old/libqpdf.so.*.* \
+  --new-lib build/libqpdf/libqpdf.so \
+  --old-sizes $work/old/sizes --new-sizes $work/new/sizes
+
+if [ "$SKIP_TESTS" != "1" ]; then
+    # Switch back to the previous release and run tests. There may be
+    # some failures based on functionality change. We are looking for
+    # ABI breakage.
+    git checkout abi-old
+    set +e
+    (cd build; ctest --verbose)
+    if [ $? != 0 ]; then
+        cat <<EOF
+
+**********************************************************************
+There were some test failures; inspect to determine whether they are
+ABI-related.
+**********************************************************************
+
+EOF
+    fi
+    set -e
+fi
+
+git checkout abi-new
+
+if [ "$SKIP_PERF" != "1" ]; then
+    echo "** writing performance details for new revision to $work/perf **"
+
+    cmake -S . -B build \
+      -DMAINTAINER_MODE=1 -DBUILD_STATIC_LIBS=0 -DBUILD_DOC=0 \
+      -DCMAKE_BUILD_TYPE=RelWithDebInfo
+    cmake --build build -j$(nproc)
+    $source/performance_check --dir $source/../performance-test-files | \
+        tee -a $work/perf
+fi
+#!/usr/bin/env python3
+import os
+import sys
+import argparse
+import subprocess
+import re
+
+whoami = os.path.basename(sys.argv[0])
+whereami = os.path.dirname(os.path.realpath(__file__))
+
+
+def warn(*args, **kwargs):
+    print(*args, file=sys.stderr, **kwargs)
+
+
+class Main:
+    def main(self, args=sys.argv[1:], prog=whoami):
+        options = self.parse_args(args, prog)
+        if options.action == 'dump':
+            self.dump(options)
+        elif options.action == 'check-sizes':
+            self.check_sizes(options)
+        elif options.action == 'compare':
+            self.compare(options)
+        else:
+            exit(f'{whoami}: unknown action')
+
+    def parse_args(self, args, prog):
+        parser = argparse.ArgumentParser(
+            prog=prog,
+            # formatter_class=argparse.RawDescriptionHelpFormatter,
+            description='Check ABI for changes',
+        )
+
+        subparsers = parser.add_subparsers(
+            dest='action',
+            help='specify subcommand; run action with --help for details',
+            required=True)
+        lib_arg = ('--lib', {'help': 'library file', 'required': True})
+
+        p_dump = subparsers.add_parser(
+            'dump',
+            help='dump qpdf symbols in a library')
+        p_dump.add_argument(lib_arg[0], **lib_arg[1])
+
+        p_check_sizes = subparsers.add_parser(
+            'check-sizes',
+            help='check consistency between library and sizes.cc')
+        p_check_sizes.add_argument(lib_arg[0], **lib_arg[1])
+
+        p_compare = subparsers.add_parser(
+            'compare',
+            help='compare libraries and sizes')
+        p_compare.add_argument('--new-lib',
+                               help='new library file',
+                               required=True)
+        p_compare.add_argument('--old-lib',
+                               help='old library file',
+                               required=True)
+        p_compare.add_argument('--old-sizes',
+                               help='output of old sizes',
+                               required=True)
+        p_compare.add_argument('--new-sizes',
+                               help='output of new sizes',
+                               required=True)
+        return parser.parse_args(args)
+
+    def get_versions(self, path):
+        p = os.path.basename(os.path.realpath(path))
+        m = re.match(r'^libqpdf.so.(\d+).(\d+).(\d+)$', p)
+        if not m:
+            exit(f'{whoami}: {path} does end with libqpdf.so.x.y.z')
+        major = int(m.group(1))
+        minor = int(m.group(2))
+        patch = int(m.group(3))
+        return (major, minor, patch)
+
+    def get_symbols(self, path):
+        symbols = set()
+        p = subprocess.run(
+            ['nm', '-D', '--demangle', '--with-symbol-versions', path],
+            stdout=subprocess.PIPE)
+        if p.returncode:
+            exit(f'{whoami}: failed to get symbols from {path}')
+        for line in p.stdout.decode().split('\n'):
+            # The LIBQPDF_\d+ comes from the version tag in
+            # libqpdf.map.in.
+            m = re.match(r'^[0-9a-f]+ (.) (.+)@@LIBQPDF_\d+\s*$', line)
+            if not m:
+                continue
+            symbols.add(m.group(2))
+        return symbols
+
+    def dump(self, options):
+        # This is just for manual use to investigate surprises.
+        for i in sorted(self.get_symbols(options.lib)):
+            print(i)
+
+    def check_sizes(self, options):
+        # Make sure that every class with methods in the public API
+        # appears in sizes.cc either explicitly ignored or in a
+        # print_size call. This enables us to reliably test whether
+        # any object changed size by following the ABI checking
+        # procedures outlined in README-maintainer.
+
+        # To keep things up to date, whenever we add or remove
+        # objects, we have to update sizes.cc. The check-sizes option
+        # can be run at any time on an up-to-date build.
+
+        lib = self.get_symbols(options.lib)
+        classes = set()
+        for i in sorted(lib):
+            # Find a symbol that looks like a class method.
+            m = re.match(r'(((?:^\S*?::)?(?:[^:\s]+))::([^:\s]+))\(', i)
+            if m:
+                full = m.group(1)
+                clas = m.group(2)
+                method = m.group(3)
+                if full.startswith('std::') or method.startswith('~'):
+                    # Sometimes std:: template instantiations make it
+                    # into the library. Ignore those. Also ignore
+                    # classes whose only exported method is a
+                    # destructor.
+                    continue
+                # Otherwise, if the class exports a method, we
+                # potentially care about changes to its size, so add
+                # it.
+                classes.add(clas)
+        in_sizes = set()
+        # Read the sizes.cc to make sure everything's there.
+        with open(os.path.join(whereami, 'qpdf/sizes.cc'), 'r') as f:
+            for line in f.readlines():
+                m = re.search(r'^\s*(?:ignore_class|print_size)\((.*?)\)',
+                              line)
+                if m:
+                    in_sizes.add(m.group(1))
+        sizes_only = in_sizes - classes
+        classes_only = classes - in_sizes
+        if sizes_only or classes_only:
+            if sizes_only:
+                print("classes in sizes.cc but not in the library:")
+                for i in sorted(sizes_only):
+                    print(' ', i)
+            if classes_only:
+                print("classes in the library but not in sizes.cc:")
+                for i in sorted(classes_only):
+                    print(' ', i)
+            exit(f'{whoami}: mismatch between library and sizes.cc')
+        else:
+            print(f'{whoami}: sizes.cc is consistent with the library')
+
+    def read_sizes(self, filename):
+        sizes = {}
+        with open(filename, 'r') as f:
+            for line in f.readlines():
+                line = line.strip()
+                m = re.match(r'^(.*) (\d+)$', line)
+                if not m:
+                    exit(f'{filename}: bad sizes line: {line}')
+                sizes[m.group(1)] = m.group(2)
+        return sizes
+
+    def compare(self, options):
+        old_version = self.get_versions(options.old_lib)
+        new_version = self.get_versions(options.new_lib)
+        old = self.get_symbols(options.old_lib)
+        new = self.get_symbols(options.new_lib)
+        if old_version > new_version:
+            exit(f'{whoami}: old version is newer than new version')
+        allow_abi_change = new_version[0] > old_version[0]
+        allow_added = allow_abi_change or (new_version[1] > old_version[1])
+        removed = sorted(old - new)
+        added = sorted(new - old)
+        if removed:
+            print('INTERFACES REMOVED:')
+            for i in removed:
+                print(' ', i)
+        else:
+            print('No interfaces were removed')
+        if added:
+            print('INTERFACES ADDED')
+            for i in added:
+                print(' ', i)
+        else:
+            print('No interfaces were added')
+
+        if removed and not allow_abi_change:
+            exit(f'{whoami}: **ERROR**: major version must be bumped')
+        elif added and not allow_added:
+            exit(f'{whoami}: **ERROR**: minor version must be bumped')
+        else:
+            print(f'{whoami}: ABI check passed.')
+
+        old_sizes = self.read_sizes(options.old_sizes)
+        new_sizes = self.read_sizes(options.new_sizes)
+        size_errors = False
+        for k, v in old_sizes.items():
+            if k in new_sizes and v != new_sizes[k]:
+                size_errors = True
+                print(f'{k} changed size from {v} to {new_sizes[k]}')
+        if size_errors:
+            if not allow_abi_change:
+                exit(f'{whoami}:'
+                     'size changes detected; this is an ABI change.')
+        else:
+            print(f'{whoami}: no size changes detected')
+
+
+if __name__ == '__main__':
+    try:
+        Main().main()
+    except KeyboardInterrupt:
+        exit(130)
@@ -579,3 +579,13 @@ if(INSTALL_CMAKE_PACKAGE)
     ${CMAKE_CURRENT_BINARY_DIR}/qpdfConfig.cmake
     DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/qpdf)
 endif()
+
+if(CHECK_SIZES AND BUILD_SHARED_LIBS AND (CMAKE_SYSTEM_NAME STREQUAL "Linux"))
+  # We can only do this check on a system with ELF shared libraries.
+  # Since this is a maintainer-only option, testing for Linux is a
+  # close enough approximation.
+  add_test(
+    NAME check-sizes
+    COMMAND ${qpdf_SOURCE_DIR}/check_abi check-sizes
+    --lib $<TARGET_FILE:libqpdf>)
+endif()
@@ -239,6 +239,14 @@ QTEST_COLOR
 Options for Working on qpdf
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
  
+CHECK_SIZES
+  The source file :file:`qpdf/sizes.cc` is used to display the sizes
+  of all objects in the public API. Consistency of its output between
+  releases is used as part of the check against accidental breakage of
+  the binary interface (ABI). Turning this on causes a test to be run
+  that ensures an exact match between classes in ``sizes.cc`` and
+  classes in the library's public API. This option requires Python 3.
+
 GENERATE_AUTO_JOB
   Some qpdf source files are automatically generated from
   :file:`job.yml` and the CLI documentation. If you are adding new
@@ -277,6 +285,8 @@ MAINTAINER_MODE
  
   - ``BUILD_DOC``
  
+  - ``CHECK_SIZES``
+
   - ``GENERATE_AUTO_JOB``
  
   - ``WERROR``
@@ -2,6 +2,7 @@ set(MAIN_CXX_PROGRAMS
   qpdf
   fix-qdf
   pdf_from_scratch
+  sizes
   test_driver
   test_large_file
   test_parsedoffset
@@ -26,6 +27,7 @@ foreach(PROG ${MAIN_C_PROGRAMS})
   set_property(TARGET ${PROG} PROPERTY LINKER_LANGUAGE CXX)
 endforeach()
 target_include_directories(qpdf-ctest PRIVATE ${CMAKE_BINARY_DIR}/libqpdf)
+target_include_directories(sizes PRIVATE ${JPEG_INCLUDE})
  
 foreach(B qpdf test_unicode_filenames fix-qdf test_shell_glob)
   if(WINDOWS_WMAIN_COMPILE)
+// See "ABI checks" in README-maintainer and comments in check_abi.
+
+#include <iostream>
+
+#include <qpdf/Buffer.hh>
+#include <qpdf/BufferInputSource.hh>
+#include <qpdf/ClosedFileInputSource.hh>
+#include <qpdf/FileInputSource.hh>
+#include <qpdf/InputSource.hh>
+#include <qpdf/JSON.hh>
+#include <qpdf/PDFVersion.hh>
+#include <qpdf/Pipeline.hh>
+#include <qpdf/Pl_Buffer.hh>
+#include <qpdf/Pl_Concatenate.hh>
+#include <qpdf/Pl_Count.hh>
+#include <qpdf/Pl_DCT.hh>
+#include <qpdf/Pl_Discard.hh>
+#include <qpdf/Pl_Flate.hh>
+#include <qpdf/Pl_QPDFTokenizer.hh>
+#include <qpdf/Pl_RunLength.hh>
+#include <qpdf/Pl_StdioFile.hh>
+#include <qpdf/QPDF.hh>
+#include <qpdf/QPDFAcroFormDocumentHelper.hh>
+#include <qpdf/QPDFAnnotationObjectHelper.hh>
+#include <qpdf/QPDFCryptoProvider.hh>
+#include <qpdf/QPDFEFStreamObjectHelper.hh>
+#include <qpdf/QPDFEmbeddedFileDocumentHelper.hh>
+#include <qpdf/QPDFExc.hh>
+#include <qpdf/QPDFFileSpecObjectHelper.hh>
+#include <qpdf/QPDFFormFieldObjectHelper.hh>
+#include <qpdf/QPDFJob.hh>
+#include <qpdf/QPDFMatrix.hh>
+#include <qpdf/QPDFNameTreeObjectHelper.hh>
+#include <qpdf/QPDFNumberTreeObjectHelper.hh>
+#include <qpdf/QPDFObjGen.hh>
+#include <qpdf/QPDFObject.hh>
+#include <qpdf/QPDFObjectHandle.hh>
+#include <qpdf/QPDFOutlineDocumentHelper.hh>
+#include <qpdf/QPDFOutlineObjectHelper.hh>
+#include <qpdf/QPDFPageDocumentHelper.hh>
+#include <qpdf/QPDFPageLabelDocumentHelper.hh>
+#include <qpdf/QPDFPageObjectHelper.hh>
+#include <qpdf/QPDFStreamFilter.hh>
+#include <qpdf/QPDFSystemError.hh>
+#include <qpdf/QPDFTokenizer.hh>
+#include <qpdf/QPDFUsage.hh>
+#include <qpdf/QPDFWriter.hh>
+#include <qpdf/QPDFXRefEntry.hh>
+
+#define ignore_class(cls)
+#define print_size(cls) \
+    std::cout << #cls << " " << sizeof(cls) << std::endl
+
+// These classes are not really public.
+// ------
+ignore_class(BitStream);
+ignore_class(BitWriter);
+ignore_class(CryptoRandomDataProvider);
+ignore_class(InsecureRandomDataProvider);
+ignore_class(JSONHandler);
+ignore_class(MD5);
+ignore_class(Pl_AES_PDF);
+ignore_class(Pl_ASCII85Decoder);
+ignore_class(Pl_ASCIIHexDecoder);
+ignore_class(Pl_LZWDecoder);
+ignore_class(Pl_MD5);
+ignore_class(Pl_PNGFilter);
+ignore_class(Pl_RC4);
+ignore_class(Pl_SHA2);
+ignore_class(Pl_TIFFPredictor);
+ignore_class(QPDFArgParser);
+ignore_class(RC4);
+ignore_class(SecureRandomDataProvider);
+ignore_class(SparseOHArray);
+
+// This is public because of QPDF_DLL_CLASS on InputSource
+// -------
+ignore_class(InputSource::Members);
+
+// These are not classes
+// -------
+ignore_class(QUtil);
+ignore_class(QTC);
+
+int main()
+{
+    // Print the size of every class in the public API. This file is
+    // read by the check_abi script at the top of the repository as
+    // part of the binary compatibility checks performed before each
+    // release.
+    print_size(Buffer);
+    print_size(BufferInputSource);
+    print_size(ClosedFileInputSource);
+    print_size(FileInputSource);
+    print_size(InputSource);
+    print_size(JSON);
+    print_size(PDFVersion);
+    print_size(Pipeline);
+    print_size(Pl_Buffer);
+    print_size(Pl_Concatenate);
+    print_size(Pl_Count);
+    print_size(Pl_DCT);
+    print_size(Pl_Discard);
+    print_size(Pl_Flate);
+    print_size(Pl_QPDFTokenizer);
+    print_size(Pl_RunLength);
+    print_size(Pl_StdioFile);
+    print_size(QPDF);
+    print_size(QPDFAcroFormDocumentHelper);
+    print_size(QPDFAnnotationObjectHelper);
+    print_size(QPDFCryptoProvider);
+    print_size(QPDFEFStreamObjectHelper);
+    print_size(QPDFEmbeddedFileDocumentHelper);
+    print_size(QPDFExc);
+    print_size(QPDFFileSpecObjectHelper);
+    print_size(QPDFFormFieldObjectHelper);
+    print_size(QPDFJob);
+    print_size(QPDFJob::AttConfig);
+    print_size(QPDFJob::Config);
+    print_size(QPDFJob::CopyAttConfig);
+    print_size(QPDFJob::EncConfig);
+    print_size(QPDFJob::PagesConfig);
+    print_size(QPDFJob::UOConfig);
+    print_size(QPDFMatrix);
+    print_size(QPDFNameTreeObjectHelper);
+    print_size(QPDFNameTreeObjectHelper::iterator);
+    print_size(QPDFNumberTreeObjectHelper);
+    print_size(QPDFNumberTreeObjectHelper::iterator);
+    print_size(QPDFObjGen);
+    print_size(QPDFObject);
+    print_size(QPDFObjectHandle);
+    print_size(QPDFObjectHandle::ParserCallbacks);
+    print_size(QPDFObjectHandle::QPDFArrayItems);
+    print_size(QPDFObjectHandle::QPDFArrayItems::iterator);
+    print_size(QPDFObjectHandle::QPDFDictItems);
+    print_size(QPDFObjectHandle::QPDFDictItems::iterator);
+    print_size(QPDFObjectHandle::StreamDataProvider);
+    print_size(QPDFObjectHandle::TokenFilter);
+    print_size(QPDFOutlineDocumentHelper);
+    print_size(QPDFOutlineObjectHelper);
+    print_size(QPDFPageDocumentHelper);
+    print_size(QPDFPageLabelDocumentHelper);
+    print_size(QPDFPageObjectHelper);
+    print_size(QPDFStreamFilter);
+    print_size(QPDFSystemError);
+    print_size(QPDFTokenizer);
+    print_size(QPDFTokenizer::Token);
+    print_size(QPDFUsage);
+    print_size(QPDFWriter);
+    print_size(QPDFXRefEntry);
+    return 0;
+}