diff options
-rw-r--r-- | CMakeLists.txt | 3 | ||||
-rw-r--r-- | README-maintainer | 67 | ||||
-rwxr-xr-x | abi-perf-test | 123 | ||||
-rwxr-xr-x | check_abi | 213 | ||||
-rw-r--r-- | libqpdf/CMakeLists.txt | 10 | ||||
-rw-r--r-- | manual/installation.rst | 10 | ||||
-rw-r--r-- | qpdf/CMakeLists.txt | 2 | ||||
-rw-r--r-- | qpdf/sizes.cc | 152 |
8 files changed, 560 insertions, 20 deletions
diff --git a/CMakeLists.txt b/CMakeLists.txt index d3284626..d58f76c9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -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( diff --git a/README-maintainer b/README-maintainer index 92c73528..fd787e6b 100644 --- a/README-maintainer +++ b/README-maintainer @@ -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. diff --git a/abi-perf-test b/abi-perf-test new file mode 100755 index 00000000..8393e283 --- /dev/null +++ b/abi-perf-test @@ -0,0 +1,123 @@ +#!/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 diff --git a/check_abi b/check_abi new file mode 100755 index 00000000..486b2c10 --- /dev/null +++ b/check_abi @@ -0,0 +1,213 @@ +#!/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) diff --git a/libqpdf/CMakeLists.txt b/libqpdf/CMakeLists.txt index 2d8c7b65..c5525db2 100644 --- a/libqpdf/CMakeLists.txt +++ b/libqpdf/CMakeLists.txt @@ -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() diff --git a/manual/installation.rst b/manual/installation.rst index 28d4ccd9..315b4bd8 100644 --- a/manual/installation.rst +++ b/manual/installation.rst @@ -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`` diff --git a/qpdf/CMakeLists.txt b/qpdf/CMakeLists.txt index 300174ce..94cf9e2d 100644 --- a/qpdf/CMakeLists.txt +++ b/qpdf/CMakeLists.txt @@ -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) diff --git a/qpdf/sizes.cc b/qpdf/sizes.cc new file mode 100644 index 00000000..bf7f5e9c --- /dev/null +++ b/qpdf/sizes.cc @@ -0,0 +1,152 @@ +// 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; +} |