Skip to content

Apply __cdecl to all exported functions (CXX-3092, CXX-3093) #1202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 29 commits into from
Sep 12, 2024

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Sep 3, 2024

Summary

Resolves CXX-3092 and CXX-3093, related to CXX-1569. Verified by this patch.

Refactors and redefines export macros to ensure proper inclusion of __cdecl in declarations of all exported functions. Additionally extends __cdecl test coverage by passing /Gv with MSVC to set the default calling convention to __vectorcall for all test executables, not just examples executables.

Important

This PR contains ABI breaking changes.

Export Macros

These changes are cherry-picked from changes related to CXX-2745 (v1 namespace migration) and edited for use by the current v_noabi headers.

The export macros are given the BSONCXX_ABI_* prefix to avoid confusing the symbol export mechanism with API accessibility, as whether an entity is exported or not (property of the ABI) is orthogonal to whether the entity is public or private (property of the API); see API and ABI Versioning. A note was added to existing export macros in 1baf35c#diff-28421749827a0253d8047c5d4d1d2bb4079a47fd64869369240454379c632538R205 (#1173) to also clarify this for users who may be used to seeing these macros.

Accordingly, the existing export macros have been redefined as follows (behavior is unchanged):

  • BSONCXX_API -> BSONCXX_ABI_EXPORT
  • BSONCXX_PRIVATE -> BSONCXX_ABI_NO_EXPORT
  • BSONCXX_CALL -> BSONCXX_ABI_CDECL
  • BSONCXX_INLINE -> inline BSONCXX_ABI_NO_EXPORT

Note

Export control via BSONCXX_ABI_NO_EXPORT is not necessary for inline functions per CXX_VISIBILITY_PRESET=hidden + VISIBILITY_INLINES_HIDDEN=ON. This includes member functions defined within the class definition as well as function template instantiations.

The existing BSONCXX_DEPRECATED_EXPORT and BSONCXX_DEPRECATED_NO_EXPORT macros (which are generated alongside BSONCXX_DEPRECATED) are not expected to be used, as they unnecessarily ties together symbol export with deprecation. We can consider their removal (via undef) in an upcoming major release.

Class Export -> Member Function Export

This PR removes use of export macros on all classes except those containing virtual functions (aka polymorphic classes). This ensures a (nearly) one-to-one correspondance between exported symbols and use of export macros.

Polymorphic Classes

Polymorphic classes must still be exported at the class level in order to export its corresponding vtable and typeinfo. Defining a "key function" out-of-line (as is now done for all polymorphic classes) seems to be insufficient to reliably and consistently trigger export of the vtable or associated typeinfo. Nevertheless, this PR ensures all polymorphic classes declared in a header file define a key function in a source file in order to address -Wweak-vtables warnings (and reduce the likelihood of vtable mismatches across shared library boundaries).

When no appropriate virtual member function already exists to serve as a key function, the virtual destructor is used instead. The sole exception is bsoncxx::v_noabi::stdx::bad_optional_access, as we do not want to include polyfill symbols in the ABI.

Important

All special member functions must be explicitly defined/defaulted/deleted given a user-declared destructor. This prevents special move members from being implicitly not-declared and also avoids depending on the implicit defaulting of copy special member functions (deprecated behavior).

This change results in 9 new exported symbols:

  • bsoncxx::v_noabi::exception::~exception()
  • mongocxx::v_noabi::authentication_exception::~authentication_exception()
  • mongocxx::v_noabi::bulk_write_exception::~bulk_write_exception()
  • mongocxx::v_noabi::exception::~exception()
  • mongocxx::v_noabi::gridfs_exception::~gridfs_exception()
  • mongocxx::v_noabi::operation_exception::~operation_exception()
  • mongocxx::v_noabi::query_exception::~query_exception()
  • mongocxx::v_noabi::write_exception::~write_exception()

Private Member Functions

Use of (BSONCXX|MONGOCXX)_ABI_NO_EXPORT should only be needed for private member functions of exported classes. So far, there is only one such instance of this, which is mongocxx::v_noabi::options::index::wiredtiger_storage_options::type() const.

This PR removes 7 private member functions from the ABI:

  • bsoncxx::v_noabi::types::bson_value::value::value(const uint8_t*, uint32_t, uint32_t, uint32_t)
  • bsoncxx::v_noabi::types::bson_value::view::_init(void*)
  • bsoncxx::v_noabi::types::bson_value::view::view(const uint8_t*, uint32_t, uint32_t, uint32_t)
  • bsoncxx::v_noabi::types::bson_value::view::view(void*)
  • mongocxx::v_noabi::options::aggregate::append(bsoncxx::v_noabi::builder::basic::document&)
  • mongocxx::v_noabi::options::change_stream::as_bson()
  • mongocxx::v_noabi::options::index::storage_options()

Despite being ABI breaking changes, these removals are not expected to break ABI compatibility in practice due to their corresponding API being private: only internal library code should be depending on these functions.

bsoncxx::v_noabi::types::bson_value::value::value(void* internal_value) appears to be the only case of a private member function that must be exported due to mongocxx depending on bsoncxx::v_noabi::types::bson_value::make_owning_bson(void* internal_value) in internal code. This case also demonstrates the motivation behind the export macro renaming described above (public vs. private != export vs. no export).

Calling Conventions and __cdecl

The export macro refactor and per-symbol export pattern also better ensures that no export function symbol is missing a corresponding __cdecl specifier in its declaration.

main()

The main function must be __cdecl.

As followup to #1200, a custom Catch2 main function is used in place of Catch2::Catch2WithMain for test executables. (Catch2's handling of the main function on Windows does not seem to satisfy our build requirements.) This finally allows building test executables to be compiled with /Gv to extend __cdecl test coverage to the test suite.

To keep things simple, an EXAMPLES_CDECL macro is applied to all instances of main() in examples.

std::function<T>

This additionally required the addition of __cdecl in the function type T of std::function<T> as used in the parameters of exported symbols. Even though std::function<T> itself is capable of handling multiple calling conventions via type erasure, function whose parameter type contains std::function<T> is not. The symbol name for foo(std::function<void()> fn) != the symbol name for foo(std::function<void __cdecl()> fn) when the default calling convention is not __cdecl. This primarily affects APM callback function interfaces.

mongoc_log_func_t

The mongoc_log_set_handler function is manually registered with mongocxx::test_util::mock<T> to account for lack of __cdecl in the mongoc_log_func_t typedef. See CDRIVER-5678 for details.

Warnings for Exported Classes (C4251, C4275)

This PR adds manual warning-disables for C4251 and C4275 for exported classes (primarily exception classes) containing data members or inheriting from non-exported class types. These cases are not expected to cause ABI stability issues in practice.

These warnings have been disabled for a very long time but C4275 seems to have reappeared due to a type in 3bbdd8f#diff-033d4100762685caa68f9ea8d51b4ea33216b3eb35c2c97cdeefa4c245ea030bR43. This PR moves the warning-disables from the config header into the component headers themselves to minimally-reduce their scope of effect and to reduce the complexity of config header inclusion order inter-dependencies (push in config.hpp, pop in postlude.hpp, which does not account for CXX-2770).

Doxygen

As drive-by improvements, this PR sets ALWAYS_DETAILED_SEC = YES to ensure consistent formatting of documented entities such that there is always a clickable "detailed" section rather than an unclickable brief-only section (which has an ID anchor but no easy way to obtain it via interactable page elements). This PR also sets STRICT_PROTO_MATCHING = YES to better validate references to functions in documentation.

Macro Guard Tests

This PR restricts the scope of macro guard tests to public headers only, which is where it is most relevant. Although support for internal macro guards exists (via private macro guard headers), in practice this seems to be mostly unnecessary/unused.

@eramongodb eramongodb requested a review from kevinAlbs September 3, 2024 18:52
@eramongodb eramongodb self-assigned this Sep 3, 2024
@kevinAlbs kevinAlbs requested a review from adriandole September 3, 2024 18:58
@eramongodb
Copy link
Contributor Author

eramongodb commented Sep 3, 2024

Applied rebase on top of latest commits to avoid merge conflicts.

@eramongodb
Copy link
Contributor Author

Rebase failed to apply conflict resolutions that preserve changes in 0d649bf. Give me a moment to fix this properly...

@eramongodb
Copy link
Contributor Author

Fixed.

@eramongodb
Copy link
Contributor Author

Reverted the calc_release_version.py fix attempt: filed CXX-3099 to resolve separately from this PR.

Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for delayed review. The new export macro names seem much clearer about the intended effect.

@eramongodb eramongodb merged commit 591e46d into mongodb:master Sep 12, 2024
71 of 79 checks passed
@eramongodb eramongodb deleted the cxx-3092 branch September 12, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants