diff --git a/CMakeLists.txt b/CMakeLists.txt index d3284626..d58f76c9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -35,6 +35,9 @@ CMAKE_DEPENDENT_OPTION( 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) 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- @ + + 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 <&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 < 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 $) +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 + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#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; +}