Skip to content

Commit

Permalink
build: Bump fmt minimum 7.0, fix fmt+gcc bugs (#3973)
Browse files Browse the repository at this point in the history
Bump fmt minimum to 7.0 for OIIO 2.5. fmt 7.0 is 3 years old, so let's
stop doing extra work to support 6.x. It's not that we depend on newer
features per se, but I'd like to no longer need separate reference
output for some tests whose formatting behavior changed slightly between
6.x and 7.0.

Rig CI to properly test "latest" and "bleeding edge" fmt. It turns out
we had not yet been testing 10.1 or keeping up with their master.

In the process, I discovered that the combo of fmt >= 10.1 and gcc >= 11
results in a mangled AVX math heisenbug, symptomatic in our simd_test. I
struggled with this on and off for many days, having great difficulty
reproducing (though it does fail every time and deterministically, I
just can't seem to narrow it to a smaller example). It only happens with
gcc, only gcc >= 11, only when we use FMT_EXCEPTIONS=0 to disable true
exceptions in fmt. The change happens specifically at fmt commit 9a034b0
(midway between 10.0 and 10.1), which changes the definition of
FMT_THROW when exceptions are disabled. Why this should affect SIMD math
is a total mystery, and currently I am suspecting either a gcc compiler
error or that it's exposing undefined behavior. I found that redefining
FMT_THROW on our side to something innocuous is a good workaround. If I
no longer disable fmt exceptions, that also makes it work, but I don't
know what that might break for us or downstream.

My report of this to the fmt project is
[here](fmtlib/fmt#3620), maybe they'll uncover
the true cause.

Signed-off-by: Larry Gritz <[email protected]>
  • Loading branch information
lgritz authored Sep 5, 2023
1 parent 4da1679 commit 4c36a07
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 16 deletions.
14 changes: 7 additions & 7 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ jobs:
openexr_ver: v2.4.3
python_ver: 2.7
simd: sse4.2
fmt_ver: 6.1.2
fmt_ver: 7.0.1
pybind11_ver: v2.4.2
setenvs: export PUGIXML_VERSION=v1.9 CMAKE_VERSION=3.15.5
- desc: gcc6/C++14 py3.7 boost1.70 exr2.4 ocio1.1
Expand Down Expand Up @@ -96,7 +96,7 @@ jobs:
cxx_std: 17
python_ver: 3.9
simd: "avx2,f16c"
fmt_ver: 8.1.1
fmt_ver: 9.1.0
pybind11_ver: v2.8.1
- desc: icc/C++17 py3.9 boost1.76 exr3.1 ocio2.1 qt5.15
nametag: linux-vfx2022-icc
Expand Down Expand Up @@ -147,7 +147,7 @@ jobs:
cxx_std: 17
python_ver: "3.10"
simd: "avx2,f16c"
fmt_ver: 9.1.0
fmt_ver: 10.1.1
pybind11_ver: v2.10.0
- desc: oldest/hobbled gcc6.3/C++14 py2.7 boost-1.66 exr-2.4 no-sse no-ocio
# Oldest versions of the dependencies that we can muster, and various
Expand All @@ -157,7 +157,7 @@ jobs:
container: aswf/ci-osl:2019
vfxyear: 2019
cxx_std: 14
fmt_ver: 6.1.2
fmt_ver: 7.0.1
openexr_ver: v2.4.0
pybind11_ver: v2.4.2
python_ver: 2.7
Expand Down Expand Up @@ -255,7 +255,7 @@ jobs:
cc_compiler: gcc-11
cxx_compiler: g++-11
cxx_std: 17
fmt_ver: 9.1.0
fmt_ver: 10.1.1
openexr_ver: v3.2.0
pybind11_ver: v2.11.1
python_ver: "3.10"
Expand All @@ -275,7 +275,7 @@ jobs:
cc_compiler: gcc-12
cxx_compiler: g++-12
cxx_std: 20
fmt_ver: 10.0.0
fmt_ver: master
openexr_ver: main
pybind11_ver: master
python_ver: "3.10"
Expand All @@ -296,7 +296,7 @@ jobs:
cxx_compiler: clang++
cc_compiler: clang
cxx_std: 20
fmt_ver: 9.0.0
fmt_ver: 10.1.1
openexr_ver: v3.1.11
pybind11_ver: v2.9.2
python_ver: 3.8
Expand Down
6 changes: 3 additions & 3 deletions INSTALL.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ NEW or CHANGED MINIMUM dependencies since the last major release are **bold**.
* libjpeg >= 8, or libjpeg-turbo >= 1.1 (tested through jpeg9d and jpeg-turbo
2.1)
* Boost >= 1.53 (recommended: at least 1.66; tested through 1.81)
* [fmtlib](https://github.com/fmtlib/fmt) >= 6.1.2 (tested through 10.0). If
not found at build time, this will be automatically downloaded unless the
build sets `-DBUILD_MISSING_FMT=OFF`.
* **[fmtlib](https://github.com/fmtlib/fmt) >= 7.0** (tested through 10.1).
If not found at build time, this will be automatically downloaded unless
the build sets `-DBUILD_MISSING_FMT=OFF`.

### Optional dependencies -- features may be disabled if not found
* If you are building the `iv` viewer (which will be disabled if any of
Expand Down
5 changes: 3 additions & 2 deletions src/cmake/externalpackages.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ endmacro()
option (BUILD_FMT_FORCE "Force local download/build of fmt even if installed" OFF)
option (BUILD_MISSING_FMT "Local download/build of fmt if not installed" ON)
option (INTERNALIZE_FMT "Copy fmt headers into <install>/include/OpenImageIO/detail/fmt" ON)
set (BUILD_FMT_VERSION "9.0.0" CACHE STRING "Preferred fmtlib/fmt version, when downloading/building our own")
set (BUILD_FMT_VERSION "10.0.0" CACHE STRING "Preferred fmtlib/fmt version, when downloading/building our own")

macro (find_or_download_fmt)
# If we weren't told to force our own download/build of fmt, look
Expand Down Expand Up @@ -350,7 +350,8 @@ macro (find_or_download_fmt)
else ()
set (OIIO_USING_FMT_LOCAL FALSE)
endif ()
checked_find_package (fmt REQUIRED)
checked_find_package (fmt REQUIRED
VERSION_MIN 7.0)
endmacro()

find_or_download_fmt()
Expand Down
9 changes: 9 additions & 0 deletions src/include/OpenImageIO/detail/fmt.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#pragma once
#define OIIO_FMT_H

#include <OpenImageIO/dassert.h>
#include <OpenImageIO/platform.h>
#include <OpenImageIO/type_traits.h>

Expand All @@ -18,6 +19,14 @@
# define FMT_EXCEPTIONS 0
#endif

// Redefining FMT_THROW to something benign seems to avoid some UB or possibly
// gcc 11+ compiler bug triggered by the definition of FMT_THROW in fmt 10.1+
// when FMT_EXCEPTIONS=0, which results in mangling SIMD math. This nugget
// below works around the problems for hard to understand reasons.
#if !defined(FMT_THROW) && OIIO_GNUC_VERSION >= 110000
# define FMT_THROW(x) OIIO_ASSERT_MSG(0, "fmt exception: %s", (x).what())
#endif

// Use the grisu fast floating point formatting for old fmt versions
// (irrelevant for >= 7.1).
#ifndef FMT_USE_GRISU
Expand Down
4 changes: 0 additions & 4 deletions src/include/OpenImageIO/strutil.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,7 @@ template<typename Str, typename... Args>
OIIO_NODISCARD
inline std::string format(const Str& fmt, Args&&... args)
{
#if FMT_VERSION >= 70000
return ::fmt::vformat(fmt, ::fmt::make_format_args(args...));
#else
return ::fmt::format(fmt, args...);
#endif
}
} // namespace fmt

Expand Down

0 comments on commit 4c36a07

Please sign in to comment.