diff --git a/CMakeLists.txt b/CMakeLists.txt index 051b25ed..afca8255 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -38,9 +38,6 @@ 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.md b/README-maintainer.md index a8328ecd..50db5454 100644 --- a/README-maintainer.md +++ b/README-maintainer.md @@ -862,6 +862,8 @@ RelWithDebInfo when using external-libs. ## ABI checks +Note: the check_abi program requires [castxml](https://github.com/CastXML/CastXML) to be installed. + 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 @@ -891,9 +893,7 @@ still things that could potentially break ABI, such as 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. +steps. See comments in check_abi for additional notes. ## CODE FORMATTING diff --git a/abi-perf-test b/abi-perf-test index 3473412c..186ece12 100755 --- a/abi-perf-test +++ b/abi-perf-test @@ -59,7 +59,7 @@ cmake --build build -j$(nproc) echo "** saving old library and size information **" -./build/qpdf/sizes >| $work/old/sizes +$source/check_abi get-sizes --include include >| $work/old/sizes cp build/libqpdf/libqpdf.so.*.* $work/old if [ "$SKIP_PERF" != "1" ]; then @@ -74,12 +74,11 @@ 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 +cmake --build build -j$(nproc) --target libqpdf echo "** saving new library and size information **" -$source/check_abi check-sizes --lib build/libqpdf/libqpdf.so -./build/qpdf/sizes >| $work/new/sizes +$source/check_abi get-sizes --include include >| $work/new/sizes cp build/libqpdf/libqpdf.so.*.* $work/new echo "** running ABI comparison **" diff --git a/cSpell.json b/cSpell.json index 2e10fe75..441767fe 100644 --- a/cSpell.json +++ b/cSpell.json @@ -61,6 +61,7 @@ "bufsize", "buildrules", "calledgetallpages", + "castxml", "ccase", "ccitt", "cdef", diff --git a/check_abi b/check_abi index b2abe406..63e1f3f1 100755 --- a/check_abi +++ b/check_abi @@ -4,9 +4,10 @@ import sys import argparse import subprocess import re +import xml.etree.ElementTree as ET +from operator import itemgetter whoami = os.path.basename(sys.argv[0]) -whereami = os.path.dirname(os.path.realpath(__file__)) def warn(*args, **kwargs): @@ -18,8 +19,8 @@ class Main: 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 == 'get-sizes': + self.get_sizes(options) elif options.action == 'compare': self.compare(options) else: @@ -43,10 +44,12 @@ class Main: 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_get_sizes = subparsers.add_parser( + 'get-sizes', + help='dump sizes of all public classes') + p_get_sizes.add_argument('--include', + help='include path', + required=True) p_compare = subparsers.add_parser( 'compare', @@ -103,60 +106,73 @@ class Main: 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. + @staticmethod + def get_header_sizes(include, filename): + print(f'{filename}...', file=sys.stderr) + p = subprocess.run([ + 'castxml', '--castxml-output=1', f'-I{include}', + f'{include}/{filename}', '-o', '-', + ], stdout=subprocess.PIPE, check=True) + tree = ET.fromstring(p.stdout) + this_file = tree.find(f'.//File[@name="{include}/{filename}"]') + if this_file is None: + # This file doesn't define anything, e.g., DLL.h + return [] + this_file_id = this_file.attrib["id"] + wanted = [ + 'Namespace', + 'Struct', 'Union', 'Class', # records + ] - # 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. + by_id = {} + for elem in tree: + # Reference + # https://github.com/CastXML/CastXML/blob/master/doc/manual/castxml.xsd + if elem.tag not in wanted: + continue + is_record = elem.tag != 'Namespace' + record = { + 'is_record': is_record, + 'in_file': (elem.get('file') == this_file_id and + elem.get('incomplete') is None), + 'name': elem.get('name'), + 'access': elem.get('access'), + 'context': elem.get('context'), + 'size': elem.get('size'), + } + by_id[elem.attrib['id']] = record + classes = [] + for id_, record in by_id.items(): + cur = record + if not cur['in_file']: + continue + name = '' + private = False + while cur is not None: + if cur.get('access') == 'private': + private = True + parent = cur.get('context') + name = f'{cur["name"]}{name}' + if parent is None or parent == '_1': + break + name = f'::{name}' + cur = by_id.get(cur.get('context')) + if not private: + classes.append([name, record['size']]) + return classes - 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 get_sizes(self, options): + classes = [] + for f in os.listdir(f'{options.include}/qpdf'): + if f.startswith('auto_') or f == 'QPDFObject.hh': + continue + if f.endswith('.h') or f.endswith('.hh'): + classes.extend(self.get_header_sizes( + options.include, f"qpdf/{f}")) + + classes.sort(key=itemgetter(0)) + for c, s in classes: + print(c, s) def read_sizes(self, filename): sizes = {} @@ -216,6 +232,12 @@ class Main: if __name__ == '__main__': + try: + subprocess.run(['castxml', '--version'], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL) + except Exception: + exit('f{whoami}: castxml must be installed') try: Main().main() except KeyboardInterrupt: diff --git a/job.sums b/job.sums index 6b11e1db..10e39f11 100644 --- a/job.sums +++ b/job.sums @@ -1,5 +1,5 @@ # Generated by generate_auto_job -CMakeLists.txt f0819695e4867e4f4389d38b0c124e79aa3ec9ace50f16ad8c751ff7f1ec6690 +CMakeLists.txt 88e8974a8b14e10c941a4bb04ff078c3d3063b98af3ea056e02b1dcdff783d22 generate_auto_job f64733b79dcee5a0e3e8ccc6976448e8ddf0e8b6529987a66a7d3ab2ebc10a86 include/qpdf/auto_job_c_att.hh 4c2b171ea00531db54720bf49a43f8b34481586ae7fb6cbf225099ee42bc5bb4 include/qpdf/auto_job_c_copy_att.hh 50609012bff14fd82f0649185940d617d05d530cdc522185c7f3920a561ccb42 diff --git a/libqpdf/CMakeLists.txt b/libqpdf/CMakeLists.txt index ce60238d..af8034ac 100644 --- a/libqpdf/CMakeLists.txt +++ b/libqpdf/CMakeLists.txt @@ -599,13 +599,3 @@ if(INSTALL_CMAKE_PACKAGE) COMPONENT ${COMPONENT_DEV} 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 195dc52e..01e0808c 100644 --- a/manual/installation.rst +++ b/manual/installation.rst @@ -80,6 +80,13 @@ made because developers have to set the environment variable themselves now rather than setting it through the build. Either way, they are off by default. +Maintainer Dependencies +~~~~~~~~~~~~~~~~~~~~~~~ + +- To run ABI checks as a maintainer, you need `castxml + `__, which is used by + ``check_abi`` to generate sizes of all public classes. + Additional Requirements on Windows ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -301,14 +308,6 @@ ZOPFLI 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. - ENABLE_COVERAGE Compile with ``--coverage``. See README-maintainer.md for information about generating coverage reports. @@ -361,8 +360,6 @@ MAINTAINER_MODE - ``BUILD_DOC`` - - ``CHECK_SIZES`` - - ``ENABLE_QTC`` - ``GENERATE_AUTO_JOB`` diff --git a/manual/release-notes.rst b/manual/release-notes.rst index 64ddc7ef..2ffb3856 100644 --- a/manual/release-notes.rst +++ b/manual/release-notes.rst @@ -132,6 +132,10 @@ more detail. - There has also been significant refactoring of how qpdf internally iterates over arrays and dictionaries. + - The internal mechanism used to check object sizes for binary + compatibility between releases has been changed. As such, the + ``CHECK_SIZES`` maintainer-only build option has been removed. + 11.10.1: February 15, 2025 - Build fixes diff --git a/qpdf/CMakeLists.txt b/qpdf/CMakeLists.txt index ae3d07d5..ee90a7bb 100644 --- a/qpdf/CMakeLists.txt +++ b/qpdf/CMakeLists.txt @@ -2,7 +2,6 @@ set(MAIN_CXX_PROGRAMS qpdf fix-qdf pdf_from_scratch - sizes test_char_sign test_driver test_large_file @@ -28,7 +27,6 @@ foreach(PROG ${MAIN_C_PROGRAMS}) target_link_libraries(${PROG} libqpdf) set_property(TARGET ${PROG} PROPERTY LINKER_LANGUAGE CXX) endforeach() -target_include_directories(sizes PRIVATE ${JPEG_INCLUDE}) set(needs_private_headers test_large_file diff --git a/qpdf/sizes.cc b/qpdf/sizes.cc deleted file mode 100644 index 8ea4e6c3..00000000 --- a/qpdf/sizes.cc +++ /dev/null @@ -1,134 +0,0 @@ -// 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 -#include -#include -#include - -#define ignore_class(cls) -#define print_size(cls) std::cout << #cls << " " << sizeof(cls) << std::endl - -// 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_Function); - print_size(Pl_OStream); - print_size(Pl_QPDFTokenizer); - print_size(Pl_RunLength); - print_size(Pl_StdioFile); - print_size(Pl_String); - 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(QPDFJob::PageLabelsConfig); - print_size(QPDFLogger); - print_size(QPDFMatrix); - print_size(QPDFNameTreeObjectHelper); - print_size(QPDFNameTreeObjectHelper::iterator); - print_size(QPDFNumberTreeObjectHelper); - print_size(QPDFNumberTreeObjectHelper::iterator); - 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(QPDFObjectHelper); - 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(QPDFWriter::FunctionProgressReporter); - print_size(QPDFXRefEntry); - return 0; -}