Skip to content
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

MONGOCRYPT-535 Support libdfp as a Decimal128 abstraction #566

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

vector-of-bool
Copy link
Contributor

This changeset allows building using a system-installation of libdfp as an alternative to IntelDFP.

Change Summary

Package/CMake Changes

  • Setting MONGOCRYPT_DFP_DIR to USE-SYSTEM will search for both IntelDFP and libdfp. If IntelDFP is found, it will be used; otherwise if libdfp is found, it will be used; otherwise we generate an error.
  • CMake code has been cleaned up to say "dfp" instead of "intel_dfp" throughout.
  • The _mongocrypt::dfp is an INTERFACE target alike what was previously defined, and is used to pivot the selection of a DFP backend.
  • The _mongocrypt::system-dfp is an IMPORTED target that points to the system-installed DFP library. _mongocrypt::dfp links to this library iff USE-SYSTEM was specified.
  • At import-time, the _mongocrypt::system-dfp library needs to be redefined if we were built with USE-SYSTEM, and a minimum amount of find_library logic was inserted into mongocrypt-config.cmake to find the same library that was used for the build. This is similar to the prior version, but it now needs to consider whether we are built with IntelDFP or libdfp. We export the filename of the found library into the mongocrypt-config.cmake so that we can find the correct matching library later on.
  • The .pc for the shared library has the link to the DFP lib file removed since either: 1) We linked against a static DFP lib, and the mongocrypt.so already contains the required TUs, or 2) we built against a dynamic DFP lib, and the mongocrypt.so will already have the reference to the proper SONAME. This makes the explicit linking unecessary in either case. The explicit link may fail if the generated .pc contains the path to a dev-only symlink (this is the case on e.g. Fedora Rawhide where libdfp.so is a symilnk to libdfp.so.1, but the libdfp runtime package only includes libdfp.so.1). Alternative solution: Have libmongocrypt-dev depend on the libdfp-dev packages, but that feels like a heavier burden on users.
  • A few miscellaneous tweaks from uncovering some testing issues on some platforms (add_test can be finicky)
  • Test the RPM build using system's libdfp-devel instead of the bundled IntelDFP. Also tentatively: Use libdfp-dev instead of libintelrdfp-dev as the build-dep for the libmongocrypt deb. This can be rolled back if we don't want to make that switch.

Code Changes

  • We detect libdfp by #includeing math.h, and checking for _DFP_MATH_H. If found, it means that libdfp has intercepted the stdlib headers and the stdlib symbols may be replaced by the libdfp shims. This switches mc-dec128.h to use libdfp as the backend instead of IntelDFP.
  • If libdfp is not detected, continue to use IntelDFP.
  • Some dec128 classifying functions were renamed to use the stdlib-like names (e.g. is_infisinf).
  • The core of "implementation switching" is done with two function-like macros: MC_IF_LIBDFP(...), which expands its arguments if libdfp is in use, and MC_IF_IntelDFP(...) which expands its arguments if libdfp is not in use.
  • Significant: libdfp uses the C stdlib <fenv.h> facilities, which are awful and painful and Widely Regarded as a Bad Move. (Which is why the IntelDFP version opts for the explicit-env arguments for rounding and exceptions).
    • The fenv facilities are abstracted using MC_HOLD_FENV(Rounding, FlagsPtr), which is a control-statement-like function-macro that wraps a code block and saves/restores the floating-point environment, while also setting the appropriate rounding mode and firing SIGFPE in the case of unhandled div-by-zero.
  • The to/from string functions were moved out-of-line into a separate TU, since #include-order can affect the behavior of libdfp on the required functions, so we need to ensure it comes first in the TU.
  • The migration to libdfp uncovered some misbehaving test cases that were "just working" due to bad macro expansions. These cases have been fixed.

@kevinAlbs kevinAlbs changed the title Support libdfp as a Decimal128 abstraction MONGOCRYPT-535 Support libdfp as a Decimal128 abstraction Feb 10, 2023
@rcsanchez97
Copy link
Collaborator

Alternative solution: Have libmongocrypt-dev depend on the libdfp-dev packages, but that feels like a heavier burden on users.

This is actually the correct solution. For instance, in the C driver, the libmongoc-dev package expresses the following dependencies:

Depends: libmongoc-1.0-0 (= ${binary:Version}),
         libbson-dev (= ${binary:Version}),
         libmongocrypt-dev,
         libssl-dev,
         zlib1g-dev,
         libsnappy-dev,
         libsasl2-dev,
         libzstd-dev,

In general, we need that sort of thing to ensure access to a .so (the unversioned symlink), the .a (for libraries that support static linking), and also helper scripts (like .pc scripts for pkg-config and .cmake target files) to ensure appropriate compiler options/definitions are set and transitive link dependencies are resolved.

Copy link
Contributor

@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.

Code changes look reasonable.

I am concerned about the extra cost for supporting libdfp. Supporting both libdfp and IntelDFP makes two variants of libmongocrypt to test and maintain. IIUC libmongocrypt built with libdfp is not tested Evergreen. IMO the benefit of adding support for libdfp needs to outweigh that cost. On platforms where IntelDFP is not available as a package, users have the option to build libmongocrypt from source using the vendored IntelDFP.

I prefer proceeding with the "off-switch" to disable Decimal128 with Range Index support on platforms where it is unavailable. Then wait to see if there are requests to support Decimal128 with Range Index support on those platforms.

set (_lib NOTFOUND)
set (_inc_dir NOTFOUND)
if (_inteldfp_lib AND _inteldfp_bid_conf_h_dir)
# Use libdfp:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Use libdfp:
# Use IntelDFP:

@@ -300,7 +300,7 @@ deb-build:
ARG env=deb-unstable
FROM +env.$env
RUN __install git-buildpackage fakeroot debhelper cmake libbson-dev \
libintelrdfpmath-dev
libdfp-dev quilt
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can drop quilt from here. It is no longer necessary after the Debian packaging changes included in #567 .

@@ -8,7 +8,7 @@ Build-Depends: debhelper (>= 10),
cmake,
libssl-dev,
pkg-config,
libintelrdfpmath-dev (>= 2.0u2-6),
libdfp-dev (>= 1.0.16-1),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
libdfp-dev (>= 1.0.16-1),
libdfp-dev,

Let's drop the version, as it's not strictly necessary for libdfp-dev and including it will make backporting libmongocrypt more difficult.

@@ -20,7 +20,6 @@ Architecture: any
Multi-Arch: same
Depends: libbson-dev,
libmongocrypt0 (= ${binary:Version}),
libintelrdfpmath-dev (>= 2.0u2-6),
${misc:Depends}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
${misc:Depends}
libdfp-dev,
${misc:Depends}

We should specify libdfp-dev here, as things like headers, static libraries, and configure scripts could be needed by consumers of libmongocrypt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants