-
Notifications
You must be signed in to change notification settings - Fork 543
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
Conversation
Applied rebase on top of latest commits to avoid merge conflicts. |
Rebase failed to apply conflict resolutions that preserve changes in 0d649bf. Give me a moment to fix this properly... |
Fixed. |
This reverts commit f19b89e.
Reverted the calc_release_version.py fix attempt: filed CXX-3099 to resolve separately from this PR. |
There was a problem hiding this 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.
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 perCXX_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
andBSONCXX_DEPRECATED_NO_EXPORT
macros (which are generated alongsideBSONCXX_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 ismongocxx::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 onbsoncxx::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 ofmain()
in examples.std::function<T>
This additionally required the addition of
__cdecl
in the function typeT
ofstd::function<T>
as used in the parameters of exported symbols. Even thoughstd::function<T>
itself is capable of handling multiple calling conventions via type erasure, function whose parameter type containsstd::function<T>
is not. The symbol name forfoo(std::function<void()> fn)
!= the symbol name forfoo(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 withmongocxx::test_util::mock<T>
to account for lack of__cdecl
in themongoc_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 setsSTRICT_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.