marzer/tomlplusplus

Cannot build with Intel MKL include dirs.

iago-lito opened this issue · 9 comments

Environment

toml++ version and/or commit hash:

Submodule : TOML++ v3.3.0 (c635f21)

Compiler:

$ c++ --version
c++ (GCC) 13.2.1 20230801

C++ standard mode:

set(CMAKE_CXX_STANDARD 17)

Target arch:

$ uname -a
Linux copak 6.4.12-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 24 Aug 2023 00:38:14 +0000 x86_64 GNU/Linux

Library configuration overrides:
none

Relevant compilation flags:
After having installed intel-oneapi-mkl:

-I/opt/intel/oneapi/compiler/latest/linux/compiler/include

Describe the bug

The library compiles fine, until I add the above flag (because the project otherwise uses MKL), then I get:

In file included from ./extern/toml++/include/toml++/toml.h:36,
                 from ./main.cpp:1:
./extern/toml++/include/toml++/impl/forward_declarations.h:38:15: error: ‘FLT_RADIX’ was not declared in this scope
   38 | static_assert(FLT_RADIX == 2, TOML_ENV_MESSAGE);
      |               ^~~~~~~~~

Steps to reproduce (or a small repro code sample)

main.cpp

#include <toml++/toml.h>
int main(){};

CMakeLists.txt

cmake_minimum_required(VERSION 3.20..3.22)

project(TomlAndMkl LANGUAGES CXX)

set(CMAKE_CXX_STANDARD 17)

add_library(toml++ INTERFACE)
target_include_directories(toml++ INTERFACE "./extern/toml++/include")

add_executable(main main.cpp)

#-------------------------------------------------------------------------------
# Works if I remove this snippet.
find_package(PkgConfig REQUIRED)
pkg_search_module(MKL mkl-dynamic-lp64-iomp)
target_include_directories(main PRIVATE ${MKL_INCLUDE_DIRS})
#-------------------------------------------------------------------------------

target_link_libraries(main PRIVATE toml++) 

I'm not exactly sure what the error deeply means here. FWIU the FLT_RADIX macro is (re-)defined within intel/oneapi/compiler/latest/linux/compiler/include/float.h:75 to:

#undef  FLT_RADIX
#define FLT_RADIX   2

but I'm not confident what it means or why this would result in "undeclared FLT_RADIX". I'm happy to learn more ^ ^"

marzer commented

Hmnmn, that is weird! If float.h is undefining and redefining FLT_RADIX immediately, nothing should be amiss from toml++'s perspective. I suppose the MKL package does something weird with FLT_RADIX elsewhere and there's some include-order shenanigans going on with the CMake config. I have no idea what to do about it, though. This seems like a bug in the MKL headers.

marzer commented

Can you try replacing toml++'s use of FLT_RADIX with std::numeric_limits<double>::radix? i.e. change this line in toml++/impl/forward_declarations.hpp:

static_assert(FLT_RADIX == 2, TOML_ENV_MESSAGE);

to this:

static_assert(std::numeric_limits<double>::radix == 2, TOML_ENV_MESSAGE);
marzer commented

If that doesn't work, you can #define TOML_DISABLE_ENVIRONMENT_CHECKS somewhere before you include toml++ and it should work OK. The float radix is only used to check that the local environment is compatible with the TOML spec, and isn't actually used anywhere else.

Replacing the macro invocation by std::*::radix does fix the problem. And indeed I should maybe report this to intel-mkl. It seems that they broke something ^ ^" Thank you for swift support, and feel free to close then. I'll just need to figure out where to find intel now ^ ^"

you can #define TOML_DISABLE_ENVIRONMENT_CHECKS

I think I'll go for some more targetted hack instead:

#ifndef FLT_RADIX
#define FLT_RADIX std::numeric_limits<float>::radix
#endif

.. but I just want to get rid of one confusion first: do you expect FLT_RADIX to be std::numeric_limits<float>::radix or std::numeric_limits<double>::radix ? (because it seems that DBL_RADIX also exists)

marzer commented

I think I'll go for some more targetted hack instead [...]

I'd caution against that solution; the standard specifies std::numeric_limits<(float|double|long double)>::radix in terms of FLT_RADIX (ref), so you might end up causing compiler errors elsewhere. I'll add a workaround to the toml++ code to detect FLT_RADIX shenanigans to avoid this in future.

but I just want to get rid of one confusion first [...]

Either would be fine since they should be the same, but floats in TOML++ are (at least) 64-bit so it uses double everywhere internally. (double isn't necessarily going to be 64-bit on all platforms, of course, but the other static asserts check for that.)

Wop, thank you for spotting the snag! ^ ^"

FWIW it seems that the error does not occur.. provided you opt into using Intel's proprietary compiler icpx :\

marzer commented

Ah, right, so I guess MKL assumes you're using that compiler and does #undef FLT_RADIX because it knows that the compiler has some special behaviour in that area.

That's... unfortunate 😅